Support reftest-wait in web-platform-tests

Support reftest-wait in web-platform-tests

The web-platform-tests (wpt) suite allows tests to control when the screenshot is
taken for reftests, based on the presence (and then absence) of a 'reftest-wait'
class on the root element.

To support this mechanism a JavaScript shim is inserted into wpt tests, which uses
existing TestRunner methods (waitUntilDone, notifyDone), combined with a
MutationObserver on the root element.

BUG=698256
Review-Url: https://codereview.chromium.org/2729863004
Cr-Commit-Position: refs/heads/master@{#455009}
Committed: https://chromium.googlesource.com/chromium/src/+/255d25e6d389106b75c90110b4e00b022e472cc5

[smcgruer, 2017-03-03 17:33:01]

smcgruer@chromium.org changed reviewers:
+ qyearsley@chromium.org

[smcgruer, 2017-03-03 17:33:02]

As per crbug.com/698256, this is just a proof of concept of an idea I had.  Very
open to being told it's the wrong idea :)

[qyearsley, 2017-03-03 23:35:57]

+szager, tkent as they have experience with this code and could likely give good
feedback

Alright!

So, this approach is to inject some JavaScript when running web platform tests,
which executes when the load event is fired, which then uses JS to call
testRunner.notifyDone when the reftest-wait class is removed from the document
element?

I suspect the right solution might be add this event listener in
testharnessreport.js
(https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...)

testharnessreport.js is included in all testharness.js tests, after
testharness.js, with the purpose of allowing different test running frameworks
to add extra stuff like this :-)

[smcgruer, 2017-03-03 23:58:48]

On 2017/03/03 23:35:57, qyearsley wrote:
> testharnessreport.js is included in all testharness.js tests, after
> testharness.js, with the purpose of allowing different test running frameworks
> to add extra stuff like this :-)

But testharness.js isn't used for reftests, is it? (Based on
http://web-platform-tests.org/writing-tests/index.html)

[qyearsley, 2017-03-04 00:05:27]

On 2017/03/03 at 23:58:48, smcgruer wrote:
> On 2017/03/03 23:35:57, qyearsley wrote:
> > testharnessreport.js is included in all testharness.js tests, after
> > testharness.js, with the purpose of allowing different test running
frameworks
> > to add extra stuff like this :-)
> 
> But testharness.js isn't used for reftests, is it? (Based on
http://web-platform-tests.org/writing-tests/index.html)

Oh, right - of course. Right, so I can't think of any better approach. Around
line 333 there it might be helpful to add some comments explaining why this is
done (maybe just a note that it's for wpt ref tests, with a link to
http://web-platform-tests.org/writing-tests/reftests.html).

[smcgruer, 2017-03-04 03:04:12]

On 2017/03/04 00:05:27, qyearsley wrote:
> Oh, right - of course. Right, so I can't think of any better approach. Around
> line 333 there it might be helpful to add some comments explaining why this is
> done (maybe just a note that it's for wpt ref tests, with a link to
> http://web-platform-tests.org/writing-tests/reftests.html).

Done, and made it a string literal for some readability improvements.

Also added a LayoutTest, or rather an upstream infrastructure test - hopefully
the wpt folks will like it.

[qyearsley, 2017-03-06 16:51:43]

LGTM, I think the test is good and the injected JS is a bit easier to read now.

Before submitting I think you'll want to change the title to something like
"Support reftest-wait in web-platform-tests."

Note, I suspect that this may cause some imported tests to behave differently,
and we may even be able to remove some failure expectations from
TestExpectations. I just started some try jobs, and if we get unexpected passes,
the try jobs will still pass, I think; changing TestExpectations in this CL is
optional.

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
content/shell/test_runner/test_runner.cc:336: // javascript that implements the
same behavior using TestRunner.
Nit: Some people write javascript as JavaScript.

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
content/shell/test_runner/test_runner.cc:339: // details on web-platform-tests
expectations on reference tests.
Possible rephrasing: "...details about reference tests in the web-platform-tests
suite."

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
content/shell/test_runner/test_runner.cc:356: const config = { attributes: true
};
Style nit: No need for spaces after "{" or before "}".

Reference:
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...
says to defer to 
https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatti...,
which suggests no spaces.

[smcgruer, 2017-03-06 17:03:39]

Description was changed from

==========
Proof of concept support for reftest-wait

BUG=698256
==========

to

==========
Support reftest-wait in web-platform-tests

The web-platform-tests (wpt) suite allows tests to control when the screenshot
is
taken for reftests, based on the presence (and then absence) of a 'reftest-wait'
class on the root element.

To support this mechanism a JavaScript shim is inserted into wpt tests, which
uses
existing TestRunner methods (waitUntilDone, notifyDone), combined with a
MutationObserver on the root element.

BUG=698256
==========

[smcgruer, 2017-03-06 17:35:32]

On 2017/03/06 16:51:43, qyearsley wrote:
> Before submitting I think you'll want to change the title to something like
> "Support reftest-wait in web-platform-tests."

Done.

> I just started some try jobs, and if we get unexpected passes,
> the try jobs will still pass, I think; changing TestExpectations in this CL is
> optional.

Haha, and instead they failed because my javascript doesn't handle a null
target. Will fix.

[smcgruer, 2017-03-06 17:35:45]

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
content/shell/test_runner/test_runner.cc:336: // javascript that implements the
same behavior using TestRunner.
On 2017/03/06 16:51:42, qyearsley wrote:
> Nit: Some people write javascript as JavaScript.

Done.

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
content/shell/test_runner/test_runner.cc:339: // details on web-platform-tests
expectations on reference tests.
On 2017/03/06 16:51:42, qyearsley wrote:
> Possible rephrasing: "...details about reference tests in the
web-platform-tests
> suite."

Done.

https://codereview.chromium.org/2729863004/diff/60001/content/shell/test_runn...
content/shell/test_runner/test_runner.cc:356: const config = { attributes: true
};
On 2017/03/06 16:51:42, qyearsley wrote:
> Style nit: No need for spaces after "{" or before "}".
> 
> Reference:
>
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...
> says to defer to 
>
https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatti...,
> which suggests no spaces.

Done.

[qyearsley, 2017-03-06 18:36:11]

Still LGTM, not sure if szager@ or tkent@ have any thoughts though?

[smcgruer, 2017-03-06 18:46:42]

Added tkent as reviewer for OWNERS (still welcome comments by szager as well!)

Fixed the js for the case where there is no documentElement.

From a local run on my machine, I get:

$ python third_party/WebKit/Tools/Scripts/run-webkit-tests -t Release
external/wpt/
...                                                                             
   
4308 tests ran as expected (3922 passed, 386 didn't), 20 didn't:


Expected to timeout, but passed: (15)
 
external/wpt/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/allow-scripts-flag-changing-2.html
 
external/wpt/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/moving-documents.html
  external/wpt/referrer-policy/generic/subresource-test/image-decoding.html
  external/wpt/streams/count-queuing-strategy.html
  external/wpt/streams/readable-streams/bad-strategies.html
  external/wpt/streams/readable-streams/bad-underlying-sources.html
  external/wpt/streams/readable-streams/brand-checks.html
  external/wpt/streams/readable-streams/cancel.html
  external/wpt/streams/readable-streams/count-queuing-strategy-integration.html
  external/wpt/streams/readable-streams/garbage-collection.html
  external/wpt/streams/readable-streams/pipe-through.html
  external/wpt/streams/readable-streams/readable-stream-reader.html
  external/wpt/streams/readable-streams/tee.html
  external/wpt/streams/readable-streams/templated.html
  external/wpt/workers/Worker_terminate_event_queue.htm

Expected to fail, but passed: (4)
  external/wpt/html/browsers/browsing-the-web/read-media/pageload-image.html
 
external/wpt/html/webappapis/scripting/processing-model-2/compile-error-data-url.html
 
external/wpt/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/keep-scheme-redirect/opt-in-blocks.https.html
 
external/wpt/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/swap-scheme-redirect/opt-in-blocks.https.html


Haven't checked yet if any of those new successes would be due to reftest-wait
or just due to flake :)

[smcgruer, 2017-03-06 18:46:59]

smcgruer@chromium.org changed reviewers:
+ tkent@chromium.org

[smcgruer, 2017-03-06 18:47:00]

(Actually +tkent ... )

[smcgruer, 2017-03-06 20:06:29]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 20:07:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 21:18:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 21:18:22]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[smcgruer, 2017-03-06 22:20:44]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 22:21:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-03-06 22:30:21]

lgtm.

external/wpt/webvtt/rendering/cues-with-video/processing-model/dom_override_remove_cue_while_paused.html
 fails on Windows. Add it to TestExpectations?

[smcgruer, 2017-03-06 23:37:50]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-03-06 23:37:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2729863004/#ps110001
(title: "Add TestExpectations entry")

[commit-bot: I haz the power, 2017-03-06 23:38:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 00:56:48]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1488843470950690,
"parent_rev": "2344921f94b85be9884ed847d9a041a5e2575946", "commit_rev":
"255d25e6d389106b75c90110b4e00b022e472cc5"}

[commit-bot: I haz the power, 2017-03-07 00:57:32]

Description was changed from

==========
Support reftest-wait in web-platform-tests

The web-platform-tests (wpt) suite allows tests to control when the screenshot
is
taken for reftests, based on the presence (and then absence) of a 'reftest-wait'
class on the root element.

To support this mechanism a JavaScript shim is inserted into wpt tests, which
uses
existing TestRunner methods (waitUntilDone, notifyDone), combined with a
MutationObserver on the root element.

BUG=698256
==========

to

==========
Support reftest-wait in web-platform-tests

The web-platform-tests (wpt) suite allows tests to control when the screenshot
is
taken for reftests, based on the presence (and then absence) of a 'reftest-wait'
class on the root element.

To support this mechanism a JavaScript shim is inserted into wpt tests, which
uses
existing TestRunner methods (waitUntilDone, notifyDone), combined with a
MutationObserver on the root element.

BUG=698256

Review-Url: https://codereview.chromium.org/2729863004
Cr-Commit-Position: refs/heads/master@{#455009}
Committed:
https://chromium.googlesource.com/chromium/src/+/255d25e6d389106b75c90110b4e0...
==========

[commit-bot: I haz the power, 2017-03-07 00:57:33]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/255d25e6d389106b75c90110b4e0...