If a test is marked as SLOW, NRWT let it run for a long time, but DRT/WTR can kill it with notifyDone timeout (30 seconds by default) We should pass the --timeout to DRT/WTR too.
It seems now only Qt port supports --timeout option. :-/ I don't know too much about other port's DRT, so I have no idea what will happen if we pass --timeout despite they don't support it. Should we implement --timeout for all port?
One more problem: The SLOW modifier is a per test modifier, but --timeout is a per driver (DRT/WTR) option. I have no idea now, how can we solve this problem ... Any idea?
the chromium port (in --test-shell mode) used to support a per-test timeout value that was passed in as another argument after the test url, not that that's particularly helpful. It has been my plan/hope for some time to modify DRT/WTR to support per-test arguments. However, in the mean time we use the DriverProxy class as a workaround for this and run one DRT per set of desired command line arguments. It would only be a slight modification to that class to include the timeout and run one DRT for normal tests and one for slow tests. This doesn't scale very well, obviously, but it might do as a stopgap.
Created attachment 163359 [details] draft patch I have created a draft patch to resolve the issue on Qt port. There is a stub in DumpRenderTree.h for other platforms to implement. Now --timeout is passed to DRT, containing the number given in --time-out-ms or the default value (that is 5 times the default value if the current test is marked as SLOW in TestExpectations). What is your opinion about the draft? Could it be a viable solution if polished a bit?
Attachment 163359 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/DumpRenderTree/DumpRenderTree.h', u'..." exit_code: 1 Tools/DumpRenderTree/DumpRenderTree.h:75: timeout_ms is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:689: Place brace on its own line for function definitions. [whitespace/braces] [4] Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:689: Missing space before { [whitespace/braces] [5] Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:694: One line control clauses should not use braces. [whitespace/braces] [4] Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:72: Missing space before { [whitespace/braces] [5] Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:74: Missing space before { [whitespace/braces] [5] Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:81: Else clause should never be on same line as else (use 2 lines) [whitespace/newline] [4] Total errors found: 7 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
View in context: https://bugs.webkit.org/attachment.cgi?id=163359&action=review > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:76 > + std::string k = tokenizer.next(); > + result.timeout_ms = (atoi(k.c_str())); What does the 'k' mean? Also there is an extra () around the atoi. Hmmm... maybe we should check if there is a next token at all, like in the -p case. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:729 > + DumpRenderTree::readTimeout(input); Why is this required? In the readTimeout you'll parse the input again whit the parseInputLine method, like at the start of this method. Processing the 'command' could be performed here or not? (maybe I'm missing something?) > Tools/Scripts/webkitpy/layout_tests/port/driver.py:342 > + command += "'" + self._port._options.time_out_ms Accessing members marked private (_options) isn't really a good idea. Maybe there is a getter for it?
(In reply to comment #4) > Created an attachment (id=163359) [details] > draft patch > > I have created a draft patch to resolve the issue on Qt port. There is a stub in DumpRenderTree.h for other platforms to implement. Now --timeout is passed to DRT, containing the number given in --time-out-ms or the default value (that is 5 times the default value if the current test is marked as SLOW in TestExpectations). > > What is your opinion about the draft? Could it be a viable solution if polished a bit? This'll always pass the --timeout every case, is this intended? Or there will be more work on top of this?
Thanks for the hints, I'm on it.
> This'll always pass the --timeout every case, is this intended? Or there will be more work on top of this? We can test if it makes some tests fail. I suppose it shouldn't, but thank you for drawing my attention. I will check it tomorrow. It wasn't really intended, it was just a starting point. > What does the 'k' mean? Also there is an extra () around the atoi. Hmmm... maybe we should check if there is a next token at all, like in the -p case. Sorry, I corrected it. At first I checked for another token, but now it is given every time by NRWT, so I skipped it. Do you think it would be a better idea to check for another token, considering better future use? > Accessing members marked private (_options) isn't really a good idea. Maybe there is a getter for it? I have corrected it. Now it uses get_option. > Why is this required? In the readTimeout you'll parse the input again whit the parseInputLine method, like at the start of this method. Processing the 'command' could be performed here or not? (maybe I'm missing something?) It has also been corrected. parseInputLine is called only once now. I also made some style corrections. What is your opinion about the current state? Thank you for any help in advance.
Created attachment 163643 [details] draft patch Ohh, and this is the current patch.
Comment on attachment 163643 [details] draft patch View in context: https://bugs.webkit.org/attachment.cgi?id=163643&action=review > Tools/Scripts/webkitpy/layout_tests/port/driver.py:352 > + command += "'" + self._port.get_option("time_out_ms") Perhaps this should only be passed if we know that the driver supports this argument? What will other ports do if they see this? Also, self._port.get_option('time_out_ms') will use the global timeout setting, not the per-test setting, meaning that you won't have two different values for normal tests and slow tests.
Created attachment 164136 [details] draft patch Made modifications according to recommendations of Dirk Pranke. Added a function to base.py to check whether the actual port's DRT supports per test timeout.
View in context: https://bugs.webkit.org/attachment.cgi?id=164136&action=review > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:726 > + DumpRenderTree::readTimeout(command.timeout); I think this name (readTimeout) is a bit misleading. > Tools/Scripts/webkitpy/layout_tests/port/driver.py:356 > + command += "'--timeout" > + command += "'" + str(driver_input.timeout) IMHO: this should be written like this: command += "'--timeout'%d" % driver_input.timeout
Comment on attachment 164136 [details] draft patch looks fine w/ peter's comments addressed.
Created attachment 165331 [details] draft patch Made modifications suggested by Peter Gal. But while correcting, I noticed that it only solves the pboblem with DRT; WTR still stops testing after 30 seconds, even though WebKitTestRunner/qt/TestControllerQt.cpp::platformRunUntil uses 60 seconds timeout in its while loop. The counter never reaches that 60000 because something stops the testing after 30 seconds. At the moment, I couldn't tell what causes the stop, but I'm on it. Any help is appreciated.
(In reply to comment #15) > Created an attachment (id=165331) [details] > draft patch > > Made modifications suggested by Peter Gal. > But while correcting, I noticed that it only solves the pboblem with DRT; WTR still stops testing after 30 seconds, even though WebKitTestRunner/qt/TestControllerQt.cpp::platformRunUntil uses 60 seconds timeout in its while loop. The counter never reaches that 60000 because something stops the testing after 30 seconds. At the moment, I couldn't tell what causes the stop, but I'm on it. Any help is appreciated. I see you parse the --timeout in WTR, but where do you set the timeout in there? Also this really needs a changelog :)
(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=165331) [details] [details] > > draft patch > > > > Made modifications suggested by Peter Gal. > > But while correcting, I noticed that it only solves the pboblem with DRT; WTR still stops testing after 30 seconds, even though WebKitTestRunner/qt/TestControllerQt.cpp::platformRunUntil uses 60 seconds timeout in its while loop. The counter never reaches that 60000 because something stops the testing after 30 seconds. At the moment, I couldn't tell what causes the stop, but I'm on it. Any help is appreciated. > > I see you parse the --timeout in WTR, but where do you set the timeout in there? > > Also this really needs a changelog :) For the first step, I just would like to allow WTR to use the default 60 sec longTimeout - but it stops at 30 sec. Of course I would like to implement setting timeout later, but I can't while this problem is present.
Created attachment 168204 [details] proposed patch I've completed the previous patch. Now DRT and WTR use the exact timeout given by time-out-ms in NRWT. Timeout values can be passed to DRT/WTR by --timeout option. Any ideas/suggestions, maybe?
Comment on attachment 168204 [details] proposed patch python-side changes look fine to me.
Created attachment 172558 [details] proposed patch update updated patch because last one became obsolete
Comment on attachment 172558 [details] proposed patch update Attachment 172558 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14730231
Comment on attachment 172558 [details] proposed patch update Attachment 172558 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14747408
Comment on attachment 172558 [details] proposed patch update Attachment 172558 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14721986
Comment on attachment 172558 [details] proposed patch update r- now based on build failures
View in context: https://bugs.webkit.org/attachment.cgi?id=172558&action=review > Tools/DumpRenderTree/DumpRenderTree.h:70 > + TestCommand() : shouldDumpPixels(false), timeout(30000) { } where is the timeout in the class? > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:68 > + result.pathOrURL = arg; where does the 'arg' comes from?
(In reply to comment #25) > View in context: https://bugs.webkit.org/attachment.cgi?id=172558&action=review > > where is the timeout in the class? > where does the 'arg' comes from? (In reply to comment #25) > View in context: https://bugs.webkit.org/attachment.cgi?id=172558&action=review > > > Tools/DumpRenderTree/DumpRenderTree.h:70 > > + TestCommand() : shouldDumpPixels(false), timeout(30000) { } > > where is the timeout in the class? > > > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:68 > > + result.pathOrURL = arg; > > where does the 'arg' comes from? Sorry, I had to update manually and missed some code. Checked everything but build-webkit :). I will double-check and upload a correct update soon.
Created attachment 172596 [details] updated patch Corrected updating of patch
View in context: https://bugs.webkit.org/attachment.cgi?id=172596&action=review > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:696 > +void DumpRenderTree::setArgTimeout(double timeout) > +{ > + if (timeout > 0) > + DumpRenderTree::setTimeout(timeout); > +} > + Why do we need this at all? Couldn't be this in the processLine method? (Also a simple setTimout(timeout) call should be enough I think.) > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:730 > + DumpRenderTree::setArgTimeout(command.timeout); no need for the 'DumpRenderTree::' if I'm correct. Also shouldn't this be before the if case? there is a return inside the else case.
(In reply to comment #28) > View in context: https://bugs.webkit.org/attachment.cgi?id=172596&action=review > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:696 > > +void DumpRenderTree::setArgTimeout(double timeout) > > +{ > > + if (timeout > 0) > > + DumpRenderTree::setTimeout(timeout); > > +} > > + > > Why do we need this at all? Couldn't be this in the processLine method? (Also a simple setTimout(timeout) call should be enough I think.) > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:730 > > + DumpRenderTree::setArgTimeout(command.timeout); > > no need for the 'DumpRenderTree::' if I'm correct. Also shouldn't this be before the if case? there is a return inside the else case. any comment on these?
(In reply to comment #29) > (In reply to comment #28) > > any comment on these? You're absolutely right. I will upload the corrected version in a jiffy.
Created attachment 175201 [details] updated patch Updated and corrected patch according to suggestions of Péter Gál.
View in context: https://bugs.webkit.org/attachment.cgi?id=175201&action=review > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:75 > + result.timeout = atoi(timeoutToken.c_str()); atoi return int here, but in the WRT atof is used. So should be use int or float/double. I see no reason to use float/double since int was used before and also who would give 3.5 (in ms) as a timeout value. (I should've seen this before) > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:724 > + setTimeout(command.timeout); You removed the 'if (timeout > 0)' check.
Created attachment 175356 [details] patch
(In reply to comment #33) > Created an attachment (id=175356) [details] > patch I thinks this is ok now.
Created attachment 176956 [details] patch The latest patch has been outdated by http://trac.webkit.org/changeset/135496. I have fixed the changes and added the ability to WTR to use timeout values higher than LongTimeout.
Comment on attachment 164136 [details] draft patch Cleared Dirk Pranke's review+ from obsolete attachment 164136 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 176956 [details] patch r=me
Comment on attachment 176956 [details] patch Clearing flags on attachment: 176956 Committed r139194: <http://trac.webkit.org/changeset/139194>
All reviewed patches have been landed. Closing bug.