Skip to content
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

WPT: Allow window.onload to contain multiple test()s #37299

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 2, 2022


*** SHERIFFS: please don't revert this CL if it causes web_tests
to flake/fail. If that happens, the cause is a bad
test. Please mark that test as flaky/fail in
TestExpectations, with a new crbug. Please block the
new bug against crbug.com/1395228. Thanks!


Prior to this CL, a test like this:

\<script>
window.onload = () => {
  test((t) => { ... }, 'test 1');
  test((t) => { ... }, 'test 2');
  test((t) => { ... }, 'test 3');
};
\</script>

would not run anything after test #1. The issue is that the testharness
immediately adds a window load handler that marks all_loaded = true,
and that ends the tests as soon as the first result from the first test
is processed. (The test runner waits for the first test because
Tests.prototype.all_done() also waits until this.tests.length > 0.)
There were various mitigating corner cases, such as if you started
the list of tests with a promise_test(), that would increment a
counter that kept the rest of the tests alive. Etc.

With this CL, the testharness-added window.onload handler runs a
setTimeout(0), so that all_loaded is only set to true after all of
the tests are loaded by any window.onload handler.

This exposed a few tests that should have been failing but were
masked by the lack of test coverage - bugs have been filed for
those. Also, several tests that were working around this via various
means are also cleaned up in this CL. I'm sure there are more of
those.

Bug: 1395228,1395226,1307772
Change-Id: I6f12b5922186af4e1e06808ad23b47ceac68559c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4074305
Reviewed-by: Weizhong Xia <weizhong@google.com>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1081558}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title Allow window.onload to contain WPT test() tests WPT: Allow window.onload to contain multiple test()s Dec 2, 2022
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-4074305 branch 3 times, most recently from 4971833 to f886c6a Compare December 7, 2022 22:52
WeizhongX added a commit that referenced this pull request Dec 7, 2022
WeizhongX added a commit that referenced this pull request Dec 8, 2022
* Add test expectation for window-onload-test.html

To unblock #37299
@WeizhongX WeizhongX closed this Dec 8, 2022
@WeizhongX WeizhongX reopened this Dec 8, 2022
******************************************************************
*** SHERIFFS: please don't revert this CL if it causes web_tests
              to flake/fail. If that happens, the cause is a bad
              test. Please mark that test as flaky/fail in
              TestExpectations, with a new crbug. Please block the
              new bug against crbug.com/1395228. Thanks!
******************************************************************

Prior to this CL, a test like this:

```
<script>
window.onload = () => {
  test((t) => { ... }, 'test 1');
  test((t) => { ... }, 'test 2');
  test((t) => { ... }, 'test 3');
};
</script>
```

would not run anything after test #1. The issue is that the testharness
immediately adds a window load handler that marks `all_loaded = true`,
and that ends the tests as soon as the first result from the first test
is processed. (The test runner waits for the first test because
`Tests.prototype.all_done()` also waits until `this.tests.length > 0`.)
There were various mitigating corner cases, such as if you started
the list of tests with a promise_test(), that would increment a
counter that kept the rest of the tests alive. Etc.

With this CL, the testharness-added window.onload handler runs a
setTimeout(0), so that `all_loaded` is only set to true after all of
the tests are loaded by any window.onload handler.

This exposed a few tests that should have been failing but were
masked by the lack of test coverage - bugs have been filed for
those. Also, several tests that were working around this via various
means are also cleaned up in this CL. I'm sure there are more of
those.

Bug: 1395228,1395226,1307772
Change-Id: I6f12b5922186af4e1e06808ad23b47ceac68559c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4074305
Reviewed-by: Weizhong Xia <weizhong@google.com>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1081558}
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit e085ff1 into master Dec 9, 2022
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-4074305 branch December 9, 2022 19:04
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 12, 2022
…est.html, a=testonly

Automatic update from web-platform-tests
Add test expectation for window-onload-test.html (#37390)

* Add test expectation for window-onload-test.html

To unblock web-platform-tests/wpt#37299
--

wpt-commits: 466c3b35543d6c2d8382eb0fd408d58d88af550c
wpt-pr: 37390
BruceDai pushed a commit to BruceDai/wpt that referenced this pull request Dec 13, 2022
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Dec 14, 2022
…est.html, a=testonly

Automatic update from web-platform-tests
Add test expectation for window-onload-test.html (#37390)

* Add test expectation for window-onload-test.html

To unblock web-platform-tests/wpt#37299
--

wpt-commits: 466c3b35543d6c2d8382eb0fd408d58d88af550c
wpt-pr: 37390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants