[Piglit] [PATCH] framework: add a generic timeout mechanism
Dylan Baker
baker.dylan.c at gmail.com
Tue Sep 9 12:07:24 PDT 2014
Hi Thomas. Sorry that I broke igt.
I have a few comments, some are stylistic comments, but some are
interface changes I'd like to see.
Thanks for working on this.
Dylan
On Tuesday, September 09, 2014 04:15:12 PM Thomas Wood wrote:
> The timeout mechanism within igt.py can no longer be used since
> get_command_result was renamed and made private in commit 5e9dd69
> (exectest.py: rename get_command_result to __run_command). Therefore,
> move the timeout mechanism into exectest.py, allowing all test profiles
> to use it if needed and also avoiding code duplication between igt.py
> and exectest.py.
>
> Cc: Dylan Baker <baker.dylan.c at gmail.com>
> Cc: Chad Versace <chad.versace at linux.intel.com>
> Signed-off-by: Thomas Wood <thomas.wood at intel.com>
> ---
> framework/exectest.py | 67 +++++++++++++++++++++++++++++++++++--
> framework/profile.py | 3 +-
> tests/igt.py | 93 +--------------------------------------------------
> 3 files changed, 68 insertions(+), 95 deletions(-)
>
> diff --git a/framework/exectest.py b/framework/exectest.py
> index 3ed9b6f..90920c6 100644
> --- a/framework/exectest.py
> +++ b/framework/exectest.py
> @@ -29,6 +29,9 @@ import shlex
> import time
> import sys
> import traceback
> +from datetime import datetime
> +import threading
> +import signal
> import itertools
> import abc
> try:
> @@ -50,6 +53,45 @@ else:
> TEST_BIN_DIR = os.path.normpath(os.path.join(os.path.dirname(__file__),
> '../bin'))
>
Two spaces between each toplevel function and class, per pep8
> +# This class is for timing out tests that hang. Create an instance by passing
> +# it a timeout in seconds and the Popen object that is running your test. Then
> +# call the start method (inherited from Thread) to start the timer. The Popen
> +# object is killed if the timeout is reached and it has not completed. Wait for
> +# the outcome by calling the join() method from the parent.
This should be a docstring for the ProcessTimeout class
> +
> +class ProcessTimeout(threading.Thread):
> + def __init__(self, timeout, proc):
> + threading.Thread.__init__(self)
> + self.proc = proc
> + self.timeout = timeout
> + self.status = 0
> +
> + def run(self):
> + start_time = datetime.now()
> + delta = 0
> + while (delta < self.timeout) and (self.proc.poll() is None):
you could drop the parens if you wanted, they're not necessary here.
Also:
while delta < self.timeout and not self.proc.poll()
> + time.sleep(1)
> + delta = (datetime.now() - start_time).total_seconds()
> +
> + # if the test is not finished after timeout, first try to terminate it
> + # and if that fails, send SIGKILL to all processes in the test's
> + # process group
> +
> + if self.proc.poll() is None:
if not self.proc.poll()
> + self.status = 1
> + self.proc.terminate()
> + time.sleep(5)
> +
> + if self.proc.poll() is None:
The same as above
> + self.status = 2
> + if hasattr(os, 'killpg'):
> + os.killpg(self.proc.pid, signal.SIGKILL)
> + self.proc.wait()
Only one newline here
> +
> +
> + def join(self):
> + threading.Thread.join(self)
> + return self.status
Two newlines
>
> class Test(object):
> """ Abstract base class for Test classes
> @@ -82,12 +124,14 @@ class Test(object):
> self.env = {}
> self.result = TestResult({'result': 'fail'})
> self.cwd = None
> + self.timeout = 0
Make timeout a class attribute rather than an instance attribute, so
that classes of tests can define a sane default for themselves
> + self.__proc_timeout = None
You will need to add any new instance attributes to the __slots__ list.
You do not need to add class methods to the __slots__ list.
>
> # This is a hook for doing some testing on execute right before
> # self.run is called.
> self._test_hook_execute_run = lambda: None
>
> - def execute(self, path, log, dmesg):
> + def execute(self, path, log, dmesg, timeout):
> """ Run a test
>
> Run a test, but with features. This times the test, uses dmesg checking
> @@ -99,6 +143,7 @@ class Test(object):
> dmesg -- a dmesg.BaseDmesg derived class
>
> """
> + self.timeout = timeout
I don't like the idea of passing the timeout into Test.execute(). What
I'd rather see is a system where each Test derived class sets a sane
timeout as a class attribute (Test should set 0 so that by default
classes don't get a timeout), and that individual tests can override
that in the profile.
Here's why: For GL we rely on 4 different test classes, GleanTest,
PiglitTest, ShaderTest, and GLSLParserTest. While the last 3 might share
a sane default (they are all derived from PiglitTest), GleanTest
definitely needs a longer timeout than the other 3. I know that the CL
guys also use multiple test classes, and they probably want to be able
to set different timeouts between those classes as well.
> log_current = log.pre_log(path if self.OPTS.verbose else None)
>
> # Run the test
> @@ -199,6 +244,11 @@ class Test(object):
> # Test passed but has valgrind errors.
> self.result['result'] = 'fail'
>
> + if self.timeout > 0:
> + # check for timeout
> + if self.__proc_timeout.join() > 0:
You can combine the two ifs with an and
> + self.result['result'] = 'timeout'
> +
> def is_skip(self):
> """ Application specific check for skip
>
> @@ -208,6 +258,10 @@ class Test(object):
> """
> return False
>
> + def __set_process_group(self):
> + if hasattr(os, 'setpgrp'):
> + os.setpgrp()
> +
> def __run_command(self):
> """ Run the test command and get the result
>
> @@ -240,7 +294,16 @@ class Test(object):
> stderr=subprocess.PIPE,
> cwd=self.cwd,
> env=fullenv,
> - universal_newlines=True)
> + universal_newlines=True,
> + preexec_fn=self.__set_process_group)
> + # create a ProcessTimeout object to watch out for test hang if the
> + # process is still going after the timeout, then it will be killed
> + # forcing the communicate function (which is a blocking call) to
> + # return
> + if self.timeout > 0:
> + self.__proc_timeout = ProcessTimeout(self.timeout, proc)
> + self.__proc_timeout.start()
> +
> out, err = proc.communicate()
> returncode = proc.returncode
> except OSError as e:
> diff --git a/framework/profile.py b/framework/profile.py
> index b801147..f810096 100644
> --- a/framework/profile.py
> +++ b/framework/profile.py
> @@ -71,6 +71,7 @@ class TestProfile(object):
> self._dmesg = None
> self.dmesg = False
> self.results_dir = None
> + self.timeout = 0
See my previous comment, but I think its better to set timeouts by test
class than profile.
>
> @property
> def dmesg(self):
> @@ -202,7 +203,7 @@ class TestProfile(object):
>
> """
> name, test = pair
> - test.execute(name, log, self.dmesg)
> + test.execute(name, log, self.dmesg, self.timeout)
> backend.write_test(name, test.result)
>
> def run_threads(pool, testlist):
> diff --git a/tests/igt.py b/tests/igt.py
> index 22250ce..e7ee518 100644
> --- a/tests/igt.py
> +++ b/tests/igt.py
> @@ -84,47 +84,6 @@ igtEnvironmentOk = checkEnvironment()
>
> profile = TestProfile()
>
> -# This class is for timing out tests that hang. Create an instance by passing
> -# it a timeout in seconds and the Popen object that is running your test. Then
> -# call the start method (inherited from Thread) to start the timer. The Popen
> -# object is killed if the timeout is reached and it has not completed. Wait for
> -# the outcome by calling the join() method from the parent.
> -
> -class ProcessTimeout(threading.Thread):
> - def __init__(self, timeout, proc):
> - threading.Thread.__init__(self)
> - self.proc = proc
> - self.timeout = timeout
> - self.status = 0
> -
> - def run(self):
> - start_time = datetime.now()
> - delta = 0
> - while (delta < self.timeout) and (self.proc.poll() is None):
> - time.sleep(1)
> - delta = (datetime.now() - start_time).total_seconds()
> -
> - # if the test is not finished after timeout, first try to terminate it
> - # and if that fails, send SIGKILL to all processes in the test's
> - # process group
> -
> - if self.proc.poll() is None:
> - self.status = 1
> - self.proc.terminate()
> - time.sleep(5)
> -
> - if self.proc.poll() is None:
> - self.status = 2
> - os.killpg(self.proc.pid, signal.SIGKILL)
> - self.proc.wait()
> -
> -
> - def join(self):
> - threading.Thread.join(self)
> - return self.status
> -
> -
> -
> class IGTTest(Test):
> def __init__(self, binary, arguments=None):
> if arguments is None:
> @@ -153,57 +112,6 @@ class IGTTest(Test):
>
> super(IGTTest, self).run()
>
> -
> - def get_command_result(self):
> - out = ""
> - err = ""
> - returncode = 0
> - fullenv = os.environ.copy()
> - for key, value in self.env.iteritems():
> - fullenv[key] = str(value)
> -
> - try:
> - proc = subprocess.Popen(self.command,
> - stdout=subprocess.PIPE,
> - stderr=subprocess.PIPE,
> - env=fullenv,
> - universal_newlines=True,
> - preexec_fn=os.setpgrp)
> -
> - # create a ProcessTimeout object to watch out for test hang if the
> - # process is still going after the timeout, then it will be killed
> - # forcing the communicate function (which is a blocking call) to
> - # return
> - timeout = ProcessTimeout(600, proc)
> - timeout.start()
> - out, err = proc.communicate()
> -
> - # check for pass or skip first
> - if proc.returncode in (0, 77):
> - returncode = proc.returncode
> - elif timeout.join() > 0:
> - returncode = 78
> - else:
> - returncode = proc.returncode
> -
> - except OSError as e:
> - # Different sets of tests get built under
> - # different build configurations. If
> - # a developer chooses to not build a test,
> - # Piglit should not report that test as having
> - # failed.
> - if e.errno == errno.ENOENT:
> - out = "PIGLIT: {'result': 'skip'}\n" \
> - + "Test executable not found.\n"
> - err = ""
> - returncode = None
> - else:
> - raise e
> - self.result['out'] = out.decode('utf-8', 'replace')
> - self.result['err'] = err.decode('utf-8', 'replace')
> - self.result['returncode'] = returncode
> -
> -
> def listTests(listname):
> with open(path.join(igtTestRoot, listname + '.txt'), 'r') as f:
> lines = (line.rstrip() for line in f.readlines())
> @@ -255,6 +163,7 @@ for test in tests:
> addSubTestCases(test)
>
> profile.dmesg = True
> +profile.timeout = 600
>
> # the dmesg property of TestProfile returns a Dmesg object
> profile.dmesg.regex = re.compile(r"(\[drm:|drm_|intel_|i915_)")
> --
> 1.9.3
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140909/ad4d3209/attachment.sig>
More information about the Piglit
mailing list