Bug 188389 – Avoid timeout resulted from calling waitUntilDone when test is not running

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

If the test is not running(already done), we should not set waitUntilDone flag, because this could make subsequent tests timeout.

[Sihui Liu]

Created attachment 346727 [details]
Patch

[Chris Dumez]

Comment on attachment 346727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346727&action=review

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> +    if (!injectedBundle.isTestRunning())

Could this be an assertion? Presumably, it is the test that is written badly in this case.

[Alexey Proskuryakov]

Comment on attachment 346727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346727&action=review

>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
>> +    if (!injectedBundle.isTestRunning())
> 
> Could this be an assertion? Presumably, it is the test that is written badly in this case.

Yes, I think that a release assert would be appropriate.

[Sihui Liu]

Created attachment 346750 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 346727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346727&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> > +    if (!injectedBundle.isTestRunning())
> 
> Could this be an assertion? Presumably, it is the test that is written badly
> in this case.

Done.

[Chris Dumez]

Comment on attachment 346750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346750&action=review

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> +    ASSERT(!injectedBundle.isTestRunning());

This assertion looks wrong / resersed. Please run layout tests in debug locally to make sure your assertion does not hit as we do not have wk2 debug configuration on EWS.

[Sihui Liu]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 346750 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346750&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> > +    ASSERT(!injectedBundle.isTestRunning());
> 
> This assertion looks wrong / resersed. Please run layout tests in debug
> locally to make sure your assertion does not hit as we do not have wk2 debug
> configuration on EWS.

Oops..it is reversed. I am running the layout tests locally, will upload patch after verification. Thanks!

[Alexey Proskuryakov]

Comment on attachment 346750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346750&action=review

>>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
>>> +    ASSERT(!injectedBundle.isTestRunning());
>> 
>> This assertion looks wrong / resersed. Please run layout tests in debug locally to make sure your assertion does not hit as we do not have wk2 debug configuration on EWS.
> 
> Oops..it is reversed. I am running the layout tests locally, will upload patch after verification. Thanks!

But also, please make it a *release* assert. There is no reason why we would only want to catch this situation in debug builds.

[Sihui Liu]

Created attachment 346850 [details]
Patch

[Chris Dumez]

Comment on attachment 346850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346850&action=review

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> +    ASSERT(injectedBundle.isTestRunning());

Alexey asked for a release assertion.

[Sihui Liu]

Created attachment 346854 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 346850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346850&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> > +    ASSERT(injectedBundle.isTestRunning());
> 
> Alexey asked for a release assertion.


Forgot to add that after local layout tests done. Uploading again...

[Chris Dumez]

Comment on attachment 346854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346854&action=review

r=me if Mac-wk2 EWS is green.

> Tools/ChangeLog:6
> +        If test is not running, we should not set the waitUntilDone flag, or it may cause subsequent tests timeout.

This should come *after* the Reviewed by line.

[EWS Watchlist]

Comment on attachment 346854 [details]
Patch

Attachment 346854 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8811097

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

[EWS Watchlist]

Created attachment 346856 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Sihui Liu]

Created attachment 347039 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 347039 [details]
Patch for landing

Clearing flags on attachment: 347039

Committed r234819: <https://trac.webkit.org/changeset/234819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43255316>