Bug 186045 – webkit-test-runner: Add support for the reftest-wait class name

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 341479 [details]
Patch (move reftest_wait_0 to LayoutTests root)

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 342053 [details]
Patch (WIP)

[Frédéric Wang (:fredw)]

Created attachment 342129 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 342141 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 342154 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 342158 [details]
Patch

[Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

[Frédéric Wang (:fredw)]

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

[youenn fablet]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[Carlos Alberto Lopez Perez]

I have been hit by this on bug 202783. 

This is affecting a lot of tests already, many of them incorrectly marked.

$ grep -rl reftest-wait LayoutTests/|grep html$|wc -l
118

[Antti Koivisto]

Would be nice to get this landed. I'm running into reftest-wait tests I'd like to enable.

[Simon Fraser (smfr)]

Ditto. I'd like imported/w3c/web-platform-tests/css/css-display/display-none-inline-img.html to work.

[Frédéric Wang (:fredw)]

@Simon @Antti @Carlos Just for the record, I'm not working on this as we didn't have an agreement on the way to proceed. I think we should restart discussion on blink-dev. The status is basically what I said in comment 42.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #46)
> discussion on blink-dev.

webkit-dev sorry -- multi-browser work :-)

[Radar WebKit Bug Importer]

<rdar://problem/59558132>

[Hector Lopez]

imported/w3c/web-platform-tests/css/css-display/display-none-inline-img.html

Test expectation changed to ImageOnlyFailure

Test expectation changed:

https://trac.webkit.org/changeset/266065/webkit

[Darin Adler]

This test is marked as failing with this bug number:

imported/w3c/web-platform-tests/css/css-animations/animation-delay-010.html

But it’s passing for me. What’s the actual situation?

[Alexey Proskuryakov]

Looks like it's failing pretty consistently on macOS, and flaky on iOS:

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-animations%2Fanimation-delay-010.html

[Darin Adler]

Passed for me on Big Sur.

[Sam Sneddon [:gsnedders]]

(In reply to Darin Adler from comment #50)
> This test is marked as failing with this bug number:
> 
> imported/w3c/web-platform-tests/css/css-animations/animation-delay-010.html
> 
> But it’s passing for me. What’s the actual situation?

On the whole we should expect anything affected by this to be flaky.

[Darin Adler]

If true, then there should be a flakiness expectation rather than a failure expectation.

[Darin Adler]

Planning to work on this today.

[Darin Adler]

Created attachment 408200 [details]
Patch

[Darin Adler]

Created attachment 408201 [details]
Patch

[Antti Koivisto]

Comment on attachment 408201 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1995
> -    if (injectedBundle.pageCount())
> -        injectedBundle.page()->dump();
> -    else
> +    if (!injectedBundle.pageCount()) {
>          injectedBundle.done();
> +        return;
> +    }
> +
> +    pollForRefTestWaitAttributeRemoval();

This reads poorly. Dumping is the important thing, reftest-wait is a minor feature required by a few tests.

Maybe call it 'dumpRespectingRefTestWait' or similar? (same for the other runners)

> LayoutTests/ChangeLog:9
> +        * TestExpectations: Expect a pass on the test that directly tests this feature.
> +        There are multiple other tests that should now be passing.

What's the plan for enabling them?

[Alexey Proskuryakov]

> What's the plan for enabling them?

Someone needs to find potentially affected tests by grepping for reftest-wait, and to check result history a day or two after this lands. Perhaps bot watchers could?

[Darin Adler]

Comment on attachment 408201 [details]
Patch

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

Currently have a power outage at my house so I can’t do a new version of the patch yet. Will figure out how this is affecting the tests that are failing on the bots and upload a new version that fixes the tests (more likely) or the machinery (less likely).

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1995
>> +    pollForRefTestWaitAttributeRemoval();
> 
> This reads poorly. Dumping is the important thing, reftest-wait is a minor feature required by a few tests.
> 
> Maybe call it 'dumpRespectingRefTestWait' or similar? (same for the other runners)

I’ll rename.

The tricky thing about this naming is that the function is both the thing you call to start polling, and the function used by the polling cycle. So one name ends up serving for both. I emphasized the "poll" half because it’s what explains the strange arguments and function type signature.

>> LayoutTests/ChangeLog:9
>> +        There are multiple other tests that should now be passing.
> 
> What's the plan for enabling them?

I think Alexey’s plan makes sense. It’s easy to grep for the affected tests.

[Antti Koivisto]

> The tricky thing about this naming is that the function is both the thing
> you call to start polling, and the function used by the polling cycle. So
> one name ends up serving for both. I emphasized the "poll" half because it’s
> what explains the strange arguments and function type signature.

One option is to keep the common case dump() in here and call pollForRefTestWaitAttributeRemoval() only when the attribute is present. Slightly more complicated than needed but also very readable.

[Darin Adler]

(In reply to Antti Koivisto from comment #61)
> > The tricky thing about this naming is that the function is both the thing
> > you call to start polling, and the function used by the polling cycle. So
> > one name ends up serving for both. I emphasized the "poll" half because it’s
> > what explains the strange arguments and function type signature.
> 
> One option is to keep the common case dump() in here and call
> pollForRefTestWaitAttributeRemoval() only when the attribute is present.
> Slightly more complicated than needed but also very readable.

With threads it would be:

    waitUntilRefTestWaitIsRemoved();
    dump();

I’d like to write code like that. Maybe:

    afterRefTestWaitIsRemoved([] {
        dump();
    });

[Darin Adler]

Renamed it to dumpAfterWaitAttributeIsRemoved.

[Darin Adler]

Created attachment 408361 [details]
Patch for landing

[Darin Adler]

Created attachment 408385 [details]
Patch for landing

[EWS]

Committed r266817: <https://trac.webkit.org/changeset/266817>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408385 [details].

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/266817/webkit

have broken 5 tests for all Mac:
  imported/w3c/web-platform-tests/css/css-ui/text-overflow-017.html [ Timeout ]
  imported/w3c/web-platform-tests/css/css-values/vh_not_refreshing_on_chrome.html [ Timeout ]
  imported/w3c/web-platform-tests/html/rendering/replaced-elements/embedded-content/cross-domain-iframe.sub.html [ Timeout ]
  imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/dynamic-content-change-rendering.html [ Timeout ]
  imported/w3c/web-platform-tests/html/semantics/forms/the-select-element/reset-algorithm-rendering.html [ Timeout ]

and 1 test for Mac wk1:
  imported/w3c/web-platform-tests/css/css-position/fixed-z-index-blend.html [ ImageOnlyFailure ]

Diff:
https://build.webkit.org/results/Apple-Catalina-Release-WK1-Tests/r266817%20(8910)/imported/w3c/web-platform-tests/css/css-position/fixed-z-index-blend-diffs.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-position%2Ffixed-z-index-blend.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-ui%2Ftext-overflow-017.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-values%2Fvh_not_refreshing_on_chrome.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Frendering%2Freplaced-elements%2Fembedded-content%2Fcross-domain-iframe.sub.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fforms%2Fthe-option-element%2Fdynamic-content-change-rendering.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fforms%2Fthe-select-element%2Freset-algorithm-rendering.html

The timeouts appear to have been constantly failing tests before this change.

[Alexey Proskuryakov]

This is likely OK, and needs expectations update (skipping for timeouts). I looked at text-overflow-017.html, and it appears to be a bad test that requires a manual click - unless there is some other magical machinery that clicks elements automagically.

EWS has the design choice that it doesn't run tests with failure expectations at all, so it cannot catch it when something changes from failure to timeout or to crash.

[Sam Sneddon [:gsnedders]]

(In reply to Alexey Proskuryakov from comment #68)
> This is likely OK, and needs expectations update (skipping for timeouts). I
> looked at text-overflow-017.html, and it appears to be a bad test that
> requires a manual click - unless there is some other magical machinery that
> clicks elements automagically.
> 
> EWS has the design choice that it doesn't run tests with failure
> expectations at all, so it cannot catch it when something changes from
> failure to timeout or to crash.

I too only looked at text-overflow-017.html, but I'll note it has a <meta name=flags content=interact> (see https://web-platform-tests.org/writing-tests/css-metadata.html?highlight=interact) so ideally we'd be treating it as a manual test.

We (read: me) should really look into https://github.com/web-platform-tests/wpt/issues/5381 again now; commented there that we should probably do something soon which means we can then more easily filter out tests that WPT considers manual.

[Truitt Savell]

Skipped the 5 Timeouts in https://trac.webkit.org/changeset/266831/webkit

[Alexey Proskuryakov]

I looked at another test that turned into a timeout, and it's interesting.

imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/dynamic-content-change-rendering.html 

It completes after a TestRendered event fires, but there is nothing in the test that does that. It's fired by web-platform-tests/tools/wptrunner/wptrunner/executors/test-wait.js.

Which I guess means that for a complete reftest-wait implementation, we need that in DRT/WKTR?

[Alexey Proskuryakov]

Okay, why not look at all of them.

Most tests that had to be skipped use TestRendered. One appears to fail because we don't/cannot implement multiple test domains the way WPT wants to, and one appears to be just a failure.

[Darin Adler]

TestRendered event? Well it’s easy to fire an event; happy to add the code for that. Where is that documented?

[Simon Fraser (smfr)]

https://web-platform-tests.org/writing-tests/reftests.html

[Darin Adler]

(In reply to Alexey Proskuryakov from comment #72)
> Most tests that had to be skipped use TestRendered. One appears to fail
> because we don't/cannot implement multiple test domains the way WPT wants
> to, and one appears to be just a failure.

I only see 3 tests in the tree that are using TestRendered. Are there more than 3?

[Alexey Proskuryakov]

I think that's it as far as ones that we imported go. I think that all three are in the list that needed to be skipped (either macOS, or iOS, or both).

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #74)
> https://web-platform-tests.org/writing-tests/reftests.html

Apparently crashtests have a similar attribute but it's called test-wait:

https://web-platform-tests.org/writing-tests/crashtest.html

[Darin Adler]

While I am. irritated to hear that, sounds like a "too many cooks" situation, it’s trivial to make the tools check for both.

I don’t want a mess of modes and options, so I’d prefer to just implement both unconditionally unless there is an obvious reason we can’t. For the same reason, I made the code always send the TestRendered event, not conditionally.

Are we running crash tests in LayoutTests right now? Is there one that is crashing or flaky because of the lack of "test-wait"? Could someone file a new bug with the details; it is so easy to fix this. (Sadly need to rename source files that I named ReftestFunctions.h/cpp.)

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #78)
> While I am. irritated to hear that, sounds like a "too many cooks"
> situation, it’s trivial to make the tools check for both.
> 
> I don’t want a mess of modes and options, so I’d prefer to just implement
> both unconditionally unless there is an obvious reason we can’t. For the
> same reason, I made the code always send the TestRendered event, not
> conditionally.
> 
> Are we running crash tests in LayoutTests right now? Is there one that is
> crashing or flaky because of the lack of "test-wait"? Could someone file a
> new bug with the details; it is so easy to fix this. (Sadly need to rename
> source files that I named ReftestFunctions.h/cpp.)

AFAIK, it's a new feature. I just learned about it because I was writing my first WPT crashtest today in Chromium... Maybe someone who is more up-to-date about WPT development can give more explanation (Sam, Youenn?). Also cc'in Emilio who presented this at the last year web engines hackfest.

I don't know if we import/run crashtests in WebKit, but my understanding is that ignoring class="test-wait" would just make the crashtest exit early and so we could potential miss crashes.

[Alexey Proskuryakov]

We don't have support for tests crashing without causing bad effects (instability due to system load; trouble collecting crash logs).

[Hector Lopez]

imported/w3c/web-platform-tests/css/css-ui/text-overflow-017.html
imported/w3c/web-platform-tests/css/css-values/vh_not_refreshing_on_chrome.html

Test were noted on this bug as a constant timeouts. Test expectations to skip were made to Mac but also a constant timeout on iOS.

History:

https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fforms%2Fthe-option-element%2Fdynamic-content-change-rendering.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fforms%2Fthe-select-element%2Freset-algorithm-rendering.html

Test expectations skipped for iOS:

https://trac.webkit.org/changeset/267061/webkit

[Emilio Cobos Álvarez (:emilio)]

(In reply to Darin Adler from comment #78)
> While I am. irritated to hear that, sounds like a "too many cooks"
> situation, it’s trivial to make the tools check for both.
> 
> I don’t want a mess of modes and options, so I’d prefer to just implement
> both unconditionally unless there is an obvious reason we can’t. For the
> same reason, I made the code always send the TestRendered event, not
> conditionally.

Yes, that seems reasonable. I'm not aware of the details of why the chosen class is different (reftest-wait predates crashtests, and my guess is that someone complained that crashtests aren't _technically_ reftests so the new class was born, but...).

(In reply to Alexey Proskuryakov from comment #80)
> We don't have support for tests crashing without causing bad effects
> (instability due to system load; trouble collecting crash logs).

Well, the point is that those tests shouldn't crash, if they do it's a bug to fix in WebKit :). I guess skipping them until the bug is fixed if it causes problems and enabling them when the problem is fixed seems the easiest path forward there.

[Darin Adler]

Lets stop discussing "test-wait" here. Someone should file another bug to track adding it. It’s very easy to add support, but I prefer not to do it until we have at least one use of it in a test.

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #83)
> Lets stop discussing "test-wait" here. Someone should file another bug to
> track adding it. It’s very easy to add support, but I prefer not to do it
> until we have at least one use of it in a test.

OK done in https://bugs.webkit.org/show_bug.cgi?id=216921

[Brent Fulgham]

WPT has removed the following tests as of 8f71659306c53f3cabad054ed02ac7c4d14de693:

imported/w3c/web-platform-tests/css/css-backgrounds/background-clip/clip-text-dynamic-2.html
imported/w3c/web-platform-tests/css/css-backgrounds/background-color-body-propagation-006.html
imported/w3c/web-platform-tests/css/css-backgrounds/background-color-root-propagation-002.html 
imported/w3c/web-platform-tests/css/css-backgrounds/border-radius-dynamic-from-no-radius.html
imported/w3c/web-platform-tests/css/css-backgrounds/child-move-reveals-parent-background.html