| Summary: | [Qt] run-qtwebkit-tests needs timeout | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||
| Component: | Tools / Tests | Assignee: | Jędrzej Nowacki <jedrzej.nowacki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Blocker | CC: | commit-queue, galpeter, jedrzej.nowacki, rgabor, wsiegrist | ||||||
| Priority: | P1 | Keywords: | Qt, QtTriaged | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Csaba Osztrogonác
2011-01-04 14:03:54 PST
Sure, the script should be able to kill a test, but in the first place the test should not hang, it have to fail ASAP ;-). I'm going to add the new functionality. Just to be precise, you mean kill a bad testsuite, not a testcase, right? I would describe them as: Testsuite is something like tst_painting Testcase is something like tst_painting::paint() QTestLib doesn't provide timeout functionality. I may change run-qtwebkit-tests script so it can kill a subprocess (which is a testsuite) after specified timeout. But that solution is essentially similar to kill the script by buildbot. If you want to kill a testcase it would be much more complex, as the script have to run each testcase in a separate process. That may introduce an issue; results of running tests manually and by the script could be different, because of possible dependency between testcases. (In reply to comment #2) > Just to be precise, you mean kill a bad testsuite, not a testcase, right? > > I would describe them as: > Testsuite is something like tst_painting > Testcase is something like tst_painting::paint() I was a little ambiguous, I mean killing the bad testsuite, of course. > QTestLib doesn't provide timeout functionality. I may change > run-qtwebkit-tests script so it can kill a subprocess (which > is a testsuite) after specified timeout. But that solution is > essentially similar to kill the script by buildbot. It would be absolutely correct solution. ;) I only would like to ensure that run-qtwebkit-tests won't block a buildslave for 20 minutes if something goes wrong in a testsuite. Now the runtime in parallel mode is ~14-15 seconds and ~38-40 seconds with single thread. So I think a 30 seconds timeout would be more than enough for each testsuite. Otherwise I supsect a network error/timeout might cause this problem, because the occurrences aren't random, but constant in a random (max. 1-2 hours long) time interval. Created attachment 78120 [details]
patch
I do not set cq to '?' as some configuration changes may by applied on the buildbot side.
Comment on attachment 78120 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=78120&action=review I got many errors like this: Exception in thread Thread-1: Traceback (most recent call last): File "/usr/lib/python2.5/threading.py", line 486, in __bootstrap_inner self.run() File "/usr/lib/python2.5/threading.py", line 663, in run self.function(*self.args, **self.kwargs) File "Tools/Scripts/run-qtwebkit-tests", line 133, in process_killer tst.terminate() AttributeError: 'Popen' object has no attribute 'terminate' > Tools/Scripts/run-qtwebkit-tests:72 > + opt.add_option("-t", "--timeout", action="store", type="int", > + dest="timeout", default=0, > + help="Timeout in seconds for each testsuite. Zero value means that there is not timeout. Default: %default.") I prefer 30 seconds as default value. Just some minor things: > Tools/Scripts/run-qtwebkit-tests:125 > + tst = Popen([test_suite.test_file_name(), ] + options.split(), stdout=PIPE, stderr=None, shell=False) The comma inside the brackets isn't required, it will be a list without it too. Also the shell=False is unnecessary, that is the default behavior for Popen. > Tools/Scripts/run-qtwebkit-tests:127 > + tst = Popen([test_suite.test_file_name(), ] + options.split(), stdout=None, stderr=STDOUT, shell=False) ditto. (In reply to comment #5) > (From update of attachment 78120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78120&action=review > > I got many errors like this: > > Exception in thread Thread-1: > Traceback (most recent call last): > File "/usr/lib/python2.5/threading.py", line 486, in __bootstrap_inner > self.run() > File "/usr/lib/python2.5/threading.py", line 663, in run > self.function(*self.args, **self.kwargs) > File "Tools/Scripts/run-qtwebkit-tests", line 133, in process_killer > tst.terminate() > AttributeError: 'Popen' object has no attribute 'terminate' Popen's terminate() method is only available from python 2.6 > Tools/Scripts/run-qtwebkit-tests:133
> + log.debug("Setting timeout timer %i sec on %s (process %s)", timeout, test_suite.test_file_name(), tst.pid)
> + tst.terminate()
maybe 'os.kill(tst.pid, signal.SIGTERM)' could resolve the problem for python 2.5
Comment on attachment 78120 [details] patch Please fix issues mentioned in Comment #6 and Comment #8 and add 30 seconds default timeout and I'll give an r+. (In reply to comment #5) > I prefer 30 seconds as default value. I don't think that it is a good idea. 30 sec is good for buildbot, but is not good enough for others. For example I need 2 min to run tests, more in debug mode and It takes only a few "weeks" on my old laptop :-). Personally I prefer to be not bothered by timeouts. (In reply to comment #6) > The comma inside the brackets isn't required, it will be a list without it too. Also the shell=False is unnecessary, that is the default behavior for Popen. True. (In reply to comment #8) > maybe 'os.kill(tst.pid, signal.SIGTERM)' could resolve the problem for python 2.5 It won't work on windows until python 2.7 :/. Anyway I'm not aware of any Windows user who is using the script. Probably one of http://code.activestate.com/recipes/347462-terminating-a-subprocess-on-windows/ solutions should be used instead, but I don't have environment to check. (In reply to comment #10) > (In reply to comment #8) > > maybe 'os.kill(tst.pid, signal.SIGTERM)' could resolve the problem for python 2.5 > It won't work on windows until python 2.7 :/. Anyway I'm not aware of any Windows user who is using the script. Probably one of http://code.activestate.com/recipes/347462-terminating-a-subprocess-on-windows/ solutions should be used instead, but I don't have environment to check. I would write something like that: try: tst.terminate() except AttributeError: # python version < 2.6 os.kill(tst.pid, signal.SIGTERM) # FIXME: Windows support what do you think? Is it enough? (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #8) > > > maybe 'os.kill(tst.pid, signal.SIGTERM)' could resolve the problem for python 2.5 > > It won't work on windows until python 2.7 :/. Anyway I'm not aware of any Windows user who is using the script. Probably one of http://code.activestate.com/recipes/347462-terminating-a-subprocess-on-windows/ solutions should be used instead, but I don't have environment to check. > > I would write something like that: > > try: > tst.terminate() > except AttributeError: > # python version < 2.6 > os.kill(tst.pid, signal.SIGTERM) > # FIXME: Windows support > > what do you think? Is it enough? What if we check the python version? ( http://docs.python.org/library/platform.html#platform.python_version_tuple ) I personally don't like exceptions if I can found an other way for it. so maybe like this: ver = platform.python_version_tuple() if ver[0] == 2 and ver[1] < 6: if platform.system == "win32": # win .. else: os.kill(tst.pid, signal.SIGTERM) else: tst.terminate() (In reply to comment #10) > I don't think that it is a good idea. 30 sec is good for buildbot, but is not good enough for others. For example I need 2 min to run tests, more in debug mode and It takes only a few "weeks" on my old laptop :-). Personally I prefer to be not bothered by timeouts. OK, you're right. Please add "--timeout 30" to Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg instead of the default timeout to make buildbots happy. (It must be enough, because the maximum runtime of a test suite is 10-12 seconds on the bot.) (In reply to comment #12) > What if we check the python version? ( http://docs.python.org/library/platform.html#platform.python_version_tuple ) > I personally don't like exceptions if I can found an other way for it. > > so maybe like this: > > ver = platform.python_version_tuple() > if ver[0] == 2 and ver[1] < 6: > if platform.system == "win32": > # win > .. > else: > os.kill(tst.pid, signal.SIGTERM) > else: > tst.terminate() It looks good to me with one of the windows implementation mentioned in comment #10 . (In reply to comment #12) > What if we check the python version? ( http://docs.python.org/library/platform.html#platform.python_version_tuple ) > I personally don't like exceptions if I can found an other way for it. You shouldn't avoid exceptions, it is nice feature that simplify your code ;-). Python encourages usage of exceptions. Please check http://docs.python.org/glossary.html#term-eafp Created attachment 78220 [details]
Patch
I haven't tested that on Win, but I checked docs and it should work(TM) :-)
Comment on attachment 78220 [details] Patch Clearing flags on attachment: 78220 Committed r75240: <http://trac.webkit.org/changeset/75240> All reviewed patches have been landed. Closing bug. Bill, could you update the master to make our bot happier, please? Buildbot updated. |