[Piglit] [PATCH] framework: add a generic timeout mechanism
Thomas Wood
thomas.wood at intel.com
Wed Sep 17 07:16:22 PDT 2014
On 9 September 2014 20:07, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> 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()
Popen.poll() returns the returncode attribute, which is either None if
the process hasn't terminated, or the return code of the child
process. Since the return code can be 0, this needs to explicitly
check for None.
>
>> + 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
>>
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list