Bug 186045 - webkit-test-runner: Add support for the reftest-wait class name
Summary: webkit-test-runner: Add support for the reftest-wait class name
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: https://web-platform-tests.org/writin...
Keywords:
Depends on: 187095
Blocks: 186127 170784
  Show dependency treegraph
 
Reported: 2018-05-28 23:34 PDT by Frédéric Wang (:fredw)
Modified: 2018-07-11 06:00 PDT (History)
10 users (show)

See Also:


Attachments
Patch (move reftest_wait_0 to LayoutTests root) (1.31 KB, patch)
2018-05-28 23:35 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (proof of concept) (2.14 KB, patch)
2018-05-30 05:02 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (WIP) (8.62 KB, patch)
2018-06-06 09:46 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (19.64 KB, patch)
2018-06-06 23:45 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (627.42 KB, application/zip)
2018-06-07 00:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.98 MB, application/zip)
2018-06-07 00:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (369.65 KB, application/zip)
2018-06-07 00:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (269.10 KB, application/zip)
2018-06-07 01:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (9.82 MB, application/zip)
2018-06-07 01:28 PDT, Build Bot
no flags Details
Patch (20.55 KB, patch)
2018-06-07 01:30 PDT, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.46 MB, application/zip)
2018-06-07 02:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (2.92 MB, application/zip)
2018-06-07 02:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.59 MB, application/zip)
2018-06-07 02:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.84 MB, application/zip)
2018-06-07 03:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (3.04 MB, application/zip)
2018-06-07 03:09 PDT, Build Bot
no flags Details
Patch (20.85 KB, patch)
2018-06-07 06:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (22.57 KB, patch)
2018-06-07 07:25 PDT, Frédéric Wang (:fredw)
rniwa: review-
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.73 MB, application/zip)
2018-06-07 11:22 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-05-28 23:34:16 PDT
From the WPT doc, reftest-wait is a mechanism similar to testRunner's waitUntilDone / notifyDone():

"Controlling When Comparison Occurs

By default reftest screenshots are taken after the load event has fired, and web fonts (if any) are loaded. In some cases it is necessary to delay the screenshot later than this, for example because some DOM manipulation is required to set up the desired test conditions. To enable this, the test may have a class="reftest-wait" attribute specified on the root element. This will cause the screenshot to be delayed until the load event has fired and the reftest-wait class has been removed from the root element. Note that in neither case is exact timing of the screenshot guaranteed: it is only guaranteed to be after those events."

The reftest-wait attributed is already used by the following tests imported into WebKit:

./FileAPI/url/url_xmlhttprequest_img.html
./css/css-grid/abspos/absolute-positioning-changing-containing-block-001.html
./css/css-grid/abspos/grid-item-absolute-positioning-dynamic-001.html
./css/css-grid/abspos/grid-positioned-item-dynamic-change-001.html
./css/css-ui/text-overflow-005.html
./css/css-ui/text-overflow-017.html
./css/css-ui/text-overflow-021.html
./css/mediaqueries/min-width-tables-001.html
./css/selectors/focus-within-001.html
./css/selectors/focus-within-002.html
./css/selectors/focus-within-003.html
./css/selectors/focus-within-004.html
./css/selectors/focus-within-005.html
./css/selectors/focus-within-006-expected.html
./css/selectors/focus-within-006.html
./css/selectors/focus-within-007.html
./css/selectors/focus-within-008.html
./css/selectors/focus-within-010.html
./css/selectors/focus-within-shadow-001.html
./css/selectors/focus-within-shadow-002.html
./css/selectors/focus-within-shadow-003.html
./css/selectors/focus-within-shadow-004.html
./css/selectors/focus-within-shadow-005.html
./css/selectors/focus-within-shadow-006.html
./css/selectors/selector-placeholder-shown-type-change-001.html
./css/selectors/selector-placeholder-shown-type-change-002.html
./css/selectors/selector-placeholder-shown-type-change-003.html
./css/selectors/selector-read-write-type-change-002.html
./css/selectors/selector-required-type-change-002.html
./mathml/relations/html5-tree/href-click-1.html
./mathml/relations/html5-tree/href-click-2.html

Probably, most of them pass because the expected final rendering happens fast enough. However, this is a bit fragile and might lead to flacky tests. Attached is a patch that moves WPT's reftest_wait_0 test so that one can run it with Tools/Scripts/run-webkit-tests reftest_wait_0.html. As indicated in the test description, it is expected to fail since: 

- reftest_wait_0.html is a doc with a red background but after 2 seconds the background color is changed to green (The end of the test is controlled via the reftest-wait attribute).
- reftest_wait_0-expected.html is a doc with a red background.

However, because webkit-test-runner does not support reftest-wait, the screenshot of reftest_wait_0.html is actually taken before the background color is turned into green.
Comment 1 Frédéric Wang (:fredw) 2018-05-28 23:35:11 PDT
Created attachment 341479 [details]
Patch (move reftest_wait_0 to LayoutTests root)
Comment 2 Frédéric Wang (:fredw) 2018-05-30 05:02:41 PDT
Created attachment 341562 [details]
Patch (proof of concept)

This patch shows how to emulate reftest-wait with some helper JS script that relies on testRunner's waitUntiDone/notifyDone. I wonder if we should make the test runner script inject such JS code or if class="reftest-wait" should be handled in any other way...
Comment 3 Frédéric Wang (:fredw) 2018-06-06 09:46:48 PDT
Created attachment 342053 [details]
Patch (WIP)
Comment 4 Frédéric Wang (:fredw) 2018-06-06 23:45:33 PDT
Created attachment 342129 [details]
Patch
Comment 5 Build Bot 2018-06-07 00:11:31 PDT
Comment on attachment 342129 [details]
Patch

Attachment 342129 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/8048035

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2018-06-07 00:11:32 PDT
Created attachment 342130 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2018-06-07 00:37:31 PDT
Comment on attachment 342129 [details]
Patch

Attachment 342129 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8048232

New failing tests:
compositing/debug-borders-dynamic.html
Comment 8 Build Bot 2018-06-07 00:37:32 PDT
Created attachment 342134 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2018-06-07 00:38:42 PDT
Comment on attachment 342129 [details]
Patch

Attachment 342129 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8048159

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2018-06-07 00:38:43 PDT
Created attachment 342135 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Build Bot 2018-06-07 01:08:15 PDT
Comment on attachment 342129 [details]
Patch

Attachment 342129 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8049058

Number of test failures exceeded the failure limit.
Comment 12 Build Bot 2018-06-07 01:08:17 PDT
Created attachment 342137 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 13 Build Bot 2018-06-07 01:28:57 PDT
Comment on attachment 342129 [details]
Patch

Attachment 342129 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8048826

New failing tests:
http/tests/webfont/font-loading-system-fallback-visibility.html
http/tests/webfont/font-loading-system-fallback-visibility-FontRanges.html
Comment 14 Build Bot 2018-06-07 01:28:59 PDT
Created attachment 342140 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 15 Frédéric Wang (:fredw) 2018-06-07 01:30:28 PDT
Created attachment 342141 [details]
Patch
Comment 16 Build Bot 2018-06-07 02:18:01 PDT
Comment on attachment 342141 [details]
Patch

Attachment 342141 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/8050515

New failing tests:
testing/reftest-wait-long.html
testing/reftest-wait-other-class-names.html
Comment 17 Build Bot 2018-06-07 02:18:03 PDT
Created attachment 342142 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 18 Build Bot 2018-06-07 02:21:17 PDT
Comment on attachment 342141 [details]
Patch

Attachment 342141 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8050556

New failing tests:
compositing/debug-borders-dynamic.html
Comment 19 Build Bot 2018-06-07 02:21:19 PDT
Created attachment 342143 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 20 Build Bot 2018-06-07 02:54:16 PDT
Comment on attachment 342141 [details]
Patch

Attachment 342141 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8050675

New failing tests:
http/tests/webfont/font-loading-system-fallback-visibility.html
http/tests/webfont/font-loading-system-fallback-visibility-FontRanges.html
Comment 21 Build Bot 2018-06-07 02:54:18 PDT
Created attachment 342144 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 22 Build Bot 2018-06-07 03:08:51 PDT
Comment on attachment 342141 [details]
Patch

Attachment 342141 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8051148

New failing tests:
testing/reftest-wait-long.html
testing/reftest-wait-other-class-names.html
Comment 23 Build Bot 2018-06-07 03:09:02 PDT
Created attachment 342145 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 24 Build Bot 2018-06-07 03:09:51 PDT
Comment on attachment 342141 [details]
Patch

Attachment 342141 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8050924

New failing tests:
testing/reftest-wait-long.html
testing/reftest-wait-other-class-names.html
Comment 25 Build Bot 2018-06-07 03:09:52 PDT
Created attachment 342146 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 26 Frédéric Wang (:fredw) 2018-06-07 06:34:39 PDT
Created attachment 342154 [details]
Patch
Comment 27 Frédéric Wang (:fredw) 2018-06-07 07:25:41 PDT
Created attachment 342158 [details]
Patch
Comment 28 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-06-07 07:55:18 PDT
Comment on attachment 342158 [details]
Patch

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

> Tools/Scripts/webkitpy/port/driver.py:510
> +                command += "'--check-reftest-wait"

Something seems to be off about the quotes at the start here.
Comment 29 Frédéric Wang (:fredw) 2018-06-07 09:37:09 PDT
Comment on attachment 342158 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/driver.py:510
>> +                command += "'--check-reftest-wait"
> 
> Something seems to be off about the quotes at the start here.

The single quote is just a separator between arguments, it does not need to be closed (see the rest of the _command_from_driver_input function for more context).
Comment 30 youenn fablet 2018-06-07 10:29:12 PDT
Do you know how Chrome is handling ref-test?

If we have to handle various classes of documents, maybe it would be better to handle them in JS directly.
Like updating testharnessreport.js to handle these classes, or maybe a script dedicated to handle these classes.
This would require updating the tests to include that script (which would be specialized for each browser) so we would need to discuss this on WPT GitHub first.
Comment 31 Frédéric Wang (:fredw) 2018-06-07 10:45:01 PDT
(In reply to youenn fablet from comment #30)
> Do you know how Chrome is handling ref-test?
>

No, I have not checked. I would need to investigate that...

> If we have to handle various classes of documents, maybe it would be better
> to handle them in JS directly.
> Like updating testharnessreport.js to handle these classes, or maybe a
> script dedicated to handle these classes.
> This would require updating the tests to include that script (which would be
> specialized for each browser) so we would need to discuss this on WPT GitHub
> first.

Yeah, I had initially tried testharnessreport.js, however reftests generally don't include such scripts. Injecting the script in the TestRunner is maybe not the best approach but I'm not sure it will be better to have the import script insert a script or to have to modify all the WPT reftests that use reftest-wait...
Comment 32 Frédéric Wang (:fredw) 2018-06-07 11:06:50 PDT
(In reply to Frédéric Wang (:fredw) from comment #31)
> (In reply to youenn fablet from comment #30)
> > Do you know how Chrome is handling ref-test?
> >
> 
> No, I have not checked. I would need to investigate that...

Apparently, Chromium does a similar thing as my patch and injects a script to emulate reftest-wait support:

https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?rcl=383ea4e718310f7d214061c8081202cc2f2937c2&l=314

> > If we have to handle various classes of documents, maybe it would be better
> > to handle them in JS directly.

Just to clarify, I think "reftest-wait" is the only document class with a special semantics in WPT tests.
Comment 33 Build Bot 2018-06-07 11:22:38 PDT
Comment on attachment 342158 [details]
Patch

Attachment 342158 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8059600

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
http/tests/preload/onload_event.html
Comment 34 Build Bot 2018-06-07 11:22:49 PDT
Created attachment 342193 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 35 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-06-08 03:00:27 PDT
We're definitely not requiring upstream that reftests must contain a script, so something along the lines of what's proposed here makes sense to me.
Comment 36 Frédéric Wang (:fredw) 2018-06-11 00:55:00 PDT
Comment on attachment 342158 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:426
> +    "})();";

I think Chromium's injected script has a few improvements over the one I proposed in attachment 342158 [details], so I think we can take it: https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?rcl=383ea4e718310f7d214061c8081202cc2f2937c2&l=314

In particular, Chromium and upstream WPT also wait for web fonts to be loaded (Chromium does not do it when reftest-wait is absent, see https://bugs.chromium.org/p/chromium/issues/detail?id=848804) so "checkReftestWait" is maybe not a good name. Note that document.font.ready is buggy in WebKit see bug 174030 and https://github.com/web-platform-tests/wpt/pull/10025
Comment 37 Frédéric Wang (:fredw) 2018-06-27 01:47:26 PDT
Comment on attachment 342158 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * http/wpt/reftest-wait/reftest-wait-events-expected.html: Added.

I noticed that WPT actually has tests for reftest-wait support, so probably we should import infrastructure/.
Comment 38 Ryosuke Niwa 2018-06-27 15:52:56 PDT
Ugh... this is so ugly. Why can't WPT define a specific JS function helper for browsers to hook in instead of relying on having some browser specific detection of the class name?
Comment 39 Ryosuke Niwa 2018-06-27 15:54:46 PDT
(In reply to Ms2ger from comment #35)
> We're definitely not requiring upstream that reftests must contain a script,
> so something along the lines of what's proposed here makes sense to me.

Why not? If the way the tests identify the time to record the actual result is removing the class name from the document element, why can't that code simply invoke a function instead? Given the amount of boilerplate WPT requires elsewhere, that seems to have a low cost.

I'm not at all excited to hard-code this insane ad-hoc convention WPT has into WTR or DRT.
Comment 40 Ryosuke Niwa 2018-06-27 16:12:24 PDT
Comment on attachment 342158 [details]
Patch

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

>>> Tools/Scripts/webkitpy/port/driver.py:510
>>> +                command += "'--check-reftest-wait"
>> 
>> Something seems to be off about the quotes at the start here.
> 
> The single quote is just a separator between arguments, it does not need to be closed (see the rest of the _command_from_driver_input function for more context).

Why do we need to have a runtime option for this? Why can't we always check for this condition.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:404
> +static const char* checkReftestWaitScript =

I don't think it makes sense to implement a logic like this in JS if we're modifying WTR
since we already have a code to wait for sub-resources to load.
If we're going with the injected script route, we're much better off adding a generic mechanism to inject arbitrary JS into the test.
I really don't want WPT specific logic creeping into WTR or DRT. r- because of this.
Comment 41 Frédéric Wang (:fredw) 2018-06-28 01:29:00 PDT
Comment on attachment 342158 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/port/driver.py:510
>>>> +                command += "'--check-reftest-wait"
>>> 
>>> Something seems to be off about the quotes at the start here.
>> 
>> The single quote is just a separator between arguments, it does not need to be closed (see the rest of the _command_from_driver_input function for more context).
> 
> Why do we need to have a runtime option for this? Why can't we always check for this condition.

Not sure I understand your question, but this --check-reftest-wait command is passed to the WTR and DRT binaries. I'm not sure they know whether we are executing a reftest or whether a test is a WPT, unless you suggest to copy the logic into WTR and DRT too.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:404
>> +static const char* checkReftestWaitScript =
> 
> I don't think it makes sense to implement a logic like this in JS if we're modifying WTR
> since we already have a code to wait for sub-resources to load.
> If we're going with the injected script route, we're much better off adding a generic mechanism to inject arbitrary JS into the test.
> I really don't want WPT specific logic creeping into WTR or DRT. r- because of this.

I also didn't like this approach, it was mostly a first attempt to open discussion however I was surprised that Chromium people actually did the same thing and supported this approach in a mail exchanges Youenn and I had with them. One argument in favor of this is that the sync logic is nontrivial, apparently one needs to properly check that web fonts are ready before taking the screenshot. This is needed even for tests without reftest-wait class, so that is one more argument in favor of a generic injection mechanism (another argument is that the same code would probably need to be injected in DRT too).
Comment 42 Frédéric Wang (:fredw) 2018-06-28 01:49:09 PDT
(In reply to Ryosuke Niwa from comment #38)
> Ugh... this is so ugly. Why can't WPT define a specific JS function helper
> for browsers to hook in instead of relying on having some browser specific
> detection of the class name?

(In reply to Ryosuke Niwa from comment #39)
> (In reply to Ms2ger from comment #35)
> > We're definitely not requiring upstream that reftests must contain a script,
> > so something along the lines of what's proposed here makes sense to me.
> 
> Why not? If the way the tests identify the time to record the actual result
> is removing the class name from the document element, why can't that code
> simply invoke a function instead? Given the amount of boilerplate WPT
> requires elsewhere, that seems to have a low cost.

Regarding this reftest-wait class, I see tree options:
1) Implement support in WebKit
2) Implement a conversion mechanism in the script importer/exporter (reftest-wait <-> wait/done) 
3) Convince upstream WPT to implement a new wait/done mechanism (and update tests)

(2) seems a bit fragile. We already have import/export issue with reftests, so I don't really want to make that worse.

reftest-wait is already a well-established convention inherited from initial 2007 implementation in Mozilla and is used everywhere in upstream WPT, so I doubt (3) will be easy, but we can try.

(1) is what I tried here and that's what Chromium people decided too and they seem to be happy with that.

We've already discussed this by private emails with Youenn and some Google/WPT people. Maybe we should open a public discussion with the rest of the browser vendors and WPT authors...