New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[html] Refactor tests #19918
[html] Refactor tests #19918
Conversation
Diffs look good: The only differences are MISSING → NOTRUN for two subtests, which looks correct: @Hexcles this is a good example of needed to do quite a bit of work to understand the overall effect of a PR. |
This is an alternative to adding `setup({ single_test: true })` in #19918
This is an alternative to adding `setup({ single_test: true })` in #19918.
This is an alternative to adding `setup({ single_test: true })` in #19918.
This is an alternative to adding `setup({ single_test: true })` in #19918.
This is an alternative to adding `setup({ single_test: true })` in #19918.
@@ -12,6 +12,7 @@ | |||
|
|||
<script> | |||
"use strict"; | |||
setup({ single_test: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer test(() => { ... })
wrapping here and for the other synchronous tests in this PR. (This is the first one.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the change you requested. Many of these tests (and still more in the related pull requests) could be expressed with a single async_test
by swapping out done()
for t.done()
. Do you think that's preferable, too?
@@ -6,6 +6,8 @@ | |||
<script src="/resources/testdriver.js"></script> | |||
<script src="/resources/testdriver-vendor.js"></script> | |||
<script> | |||
setup({ single_test: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer #19926 to this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I've merged your pull request and removed all modifications from this changeset so it will merge cleanly.
This is an alternative to adding `setup({ single_test: true })` in #19918.
0c431e0
to
d81f8b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jugglinmike this looks good now, leaving merging to you in case you want to tweak the commit message
testharness.js was recently extended with an API to explicitly opt-in to the "single page test" feature [1]. As per WPT RFC 28 [2], tests which do not use this API and which do not declare any subtests will soon be reported as a harness error. Update some tests which previously opted in implicitly to use the new API. Update others to instead declare a single subtest (so that they are no longer single-page tests). [1] web-platform-tests#19449 [2] https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md
d81f8b9
to
aafbcad
Compare
Thanks! I've rebased and reworded the commit and updated the title/description of this pull request accordingly. The original version of this branch is available here. The latest version just passed in CI, so I'll do the honors. |
…o promise_test, a=testonly Automatic update from web-platform-tests [html] Convert srcdoc_change_hash.html to promise_test This is an alternative to adding `setup({ single_test: true })` in web-platform-tests/wpt#19918. -- wpt-commits: 2127be728a1d785b3443eaec5955070c0908aa62 wpt-pr: 19926 Differential Revision: https://phabricator.services.mozilla.com/D53517
…o promise_test, a=testonly Automatic update from web-platform-tests [html] Convert srcdoc_change_hash.html to promise_test This is an alternative to adding `setup({ single_test: true })` in web-platform-tests/wpt#19918. -- wpt-commits: 2127be728a1d785b3443eaec5955070c0908aa62 wpt-pr: 19926 Differential Revision: https://phabricator.services.mozilla.com/D53517
…o promise_test, a=testonly Automatic update from web-platform-tests [html] Convert srcdoc_change_hash.html to promise_test This is an alternative to adding `setup({ single_test: true })` in web-platform-tests/wpt#19918. -- wpt-commits: 2127be728a1d785b3443eaec5955070c0908aa62 wpt-pr: 19926 Differential Revision: https://phabricator.services.mozilla.com/D53517 UltraBlame original commit: c1d0439f81cad16038d0c7d8fa991c93bcb807de
…o promise_test, a=testonly Automatic update from web-platform-tests [html] Convert srcdoc_change_hash.html to promise_test This is an alternative to adding `setup({ single_test: true })` in web-platform-tests/wpt#19918. -- wpt-commits: 2127be728a1d785b3443eaec5955070c0908aa62 wpt-pr: 19926 Differential Revision: https://phabricator.services.mozilla.com/D53517 UltraBlame original commit: c1d0439f81cad16038d0c7d8fa991c93bcb807de
…o promise_test, a=testonly Automatic update from web-platform-tests [html] Convert srcdoc_change_hash.html to promise_test This is an alternative to adding `setup({ single_test: true })` in web-platform-tests/wpt#19918. -- wpt-commits: 2127be728a1d785b3443eaec5955070c0908aa62 wpt-pr: 19926 Differential Revision: https://phabricator.services.mozilla.com/D53517 UltraBlame original commit: c1d0439f81cad16038d0c7d8fa991c93bcb807de
testharness.js was recently extended with an API to explicitly opt-in to
the "single page test" feature [1]. As per WPT RFC 28 [2], tests which
do not use this API and which do not declare any subtests will soon be
reported as a harness error.
Update some tests which previously opted in implicitly to use the new
API. Update others to instead declare a single subtest (so that they are
no longer single-page tests).
[1] #19449
[2] https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md