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 <[email protected]> > Cc: Chad Versace <[email protected]> > Signed-off-by: Thomas Wood <[email protected]> > --- > 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 >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
