Bug 90968 – [NRWT] Pass --timeout to DRT/WTR if a test is marked as SLOW

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

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?

[Dirk Pranke]

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.

[János Badics]

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?

[WebKit Review Bot]

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.

[Peter Gal]

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?

[Peter Gal]

(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?

[János Badics]

Thanks for the hints, I'm on it.

[János Badics]

> 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.

[János Badics]

Created attachment 163643 [details]
draft patch

Ohh, and this is the current patch.

[Dirk Pranke]

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.

[János Badics]

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.

[Peter Gal]

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

[Dirk Pranke]

Comment on attachment 164136 [details]
draft patch

looks fine w/ peter's comments addressed.

[János Badics]

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.

[Peter Gal]

(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 :)

[János Badics]

(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.

[János Badics]

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?

[Dirk Pranke]

Comment on attachment 168204 [details]
proposed patch

python-side changes look fine to me.

[János Badics]

Created attachment 172558 [details]
proposed patch update

updated patch because last one became obsolete

[Peter Beverloo (cr-android ews)]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

Comment on attachment 172558 [details]
proposed patch update

Attachment 172558 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14721986

[Csaba Osztrogonác]

Comment on attachment 172558 [details]
proposed patch update

r- now based on build failures

[Peter Gal]

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?

[János Badics]

(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.

[János Badics]

Created attachment 172596 [details]
updated patch

Corrected updating of patch

[Peter Gal]

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.

[Peter Gal]

(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?

[János Badics]

(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.

[János Badics]

Created attachment 175201 [details]
updated patch

Updated and corrected patch according to suggestions of Péter Gál.

[Peter Gal]

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.

[János Badics]

Created attachment 175356 [details]
patch

[Peter Gal]

(In reply to comment #33)
> Created an attachment (id=175356) [details]
> patch

I thinks this is ok now.

[János Badics]

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.

[Eric Seidel (no email)]

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.

[Csaba Osztrogonác]

Comment on attachment 176956 [details]
patch

r=me

[WebKit Review Bot]

Comment on attachment 176956 [details]
patch

Clearing flags on attachment: 176956

Committed r139194: <http://trac.webkit.org/changeset/139194>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.