Skip to content

Set all_loaded flag synchronously#37549

Open
jgraham wants to merge 1 commit intomasterfrom
testharness_onload_fix
Open

Set all_loaded flag synchronously#37549
jgraham wants to merge 1 commit intomasterfrom
testharness_onload_fix

Conversation

@jgraham
Copy link
Contributor

@jgraham jgraham commented Dec 16, 2022

This should fix the case where done() is called in the load handler, but still allows new tests to be defined inside the handler in the case that you don't explicitly call done()

@jgraham
Set all_loaded flag synchronously
ede47d5 
This should fix the case where done() is called in the load handler,
but still allows new tests to be defined inside the handler in the case
that you don't explicitly call done()
@wpt-pr-bot
Copy link
Collaborator

wpt-pr-bot commented Dec 16, 2022

There are no reviewers for this pull request besides its author. Please reach out on the chat room to get help with this. Thank you!

@WeizhongX WeizhongX mentioned this pull request Dec 16, 2022
Closed
@foolip
Copy link
Member

foolip commented Dec 20, 2022

@jgraham will this have any impact on tests that are defined in a window load event handler if done() isn't called? From a search of the code I think i doesn't, but then I'm not sure what the effect of this is. Can you add a new test that would fail without this change?

@jgraham
Copy link
Contributor Author

jgraham commented Dec 20, 2022

Well there's already a test that fails without this change; it's currently blocking all CI that runs the resources tests. And I think the original regression added an infrastructure test that defines tests in onload without calling done. But I can add a resources test that does that to validate that it works.

@foolip
Copy link
Member

foolip commented Dec 21, 2022

Oh, I didn't connect the dots, this fixes the pytest failures reported in #37618.

I think we should revert the PR that introduced the failures and then we can take our time figuring out what tests are needed.

@foolip
Copy link
Member

foolip commented Dec 21, 2022

#37619 is that revert, but I'm trying to revert it via https://chromium-review.googlesource.com/c/chromium/src/+/4111318 to also revert related changes in Chromium.

@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Dec 21, 2022
Merged
chromium-wpt-export-bot pushed a commit that referenced this pull request Dec 21, 2022
@foolip @chromium-wpt-export-bot
Revert "WPT: Allow window.onload to contain multiple test()s"
b4be5bd 
This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
#37623

There was an attempted fix for this:
#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
chromium-wpt-export-bot pushed a commit that referenced this pull request Dec 21, 2022
@foolip @chromium-wpt-export-bot
Revert "WPT: Allow window.onload to contain multiple test()s"
494bd5b 
This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
#37623

There was an attempted fix for this:
#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 21, 2022
@foolip
Revert "WPT: Allow window.onload to contain multiple test()s"
7ad2fe6 
This reverts commit 4a03c6c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
web-platform-tests/wpt#37623

There was an attempted fix for this:
web-platform-tests/wpt#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}
chromium-wpt-export-bot pushed a commit that referenced this pull request Dec 21, 2022
@foolip @chromium-wpt-export-bot
Revert "WPT: Allow window.onload to contain multiple test()s"
9e5577b 
This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
#37623

There was an attempted fix for this:
#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}
chromium-wpt-export-bot pushed a commit that referenced this pull request Dec 21, 2022
@foolip @chromium-wpt-export-bot
Revert "WPT: Allow window.onload to contain multiple test()s"
5c571b8 
This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
#37623

There was an attempted fix for this:
#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}
@mfreed7
Copy link
Contributor

mfreed7 commented Jan 3, 2023

Any update on this issue/fix? It causes tests to be silently skipped, so I'd really like to see what we can do to fix it.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 4, 2023
@foolip @moz-wptsync-bot
Bug 1806808 [wpt PR 37624] - Revert "WPT: Allow window.onload to co…
4b29f46 
…ntain multiple `test()`s", a=testonly

Automatic update from web-platform-tests
Revert "WPT: Allow `window.onload` to contain multiple `test()`s"

This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
web-platform-tests/wpt#37623

There was an attempted fix for this:
web-platform-tests/wpt#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}

--

wpt-commits: 5c571b86bc98819157b584132bca1b80ca4503cb
wpt-pr: 37624
@mfreed7
Copy link
Contributor

mfreed7 commented Jan 4, 2023

Any update on this issue/fix? It causes tests to be silently skipped, so I'd really like to see what we can do to fix it.

Perhaps we could just remove the test/tests/unit/late-test.html test, which attempts to make sure late-added tests are ignored completely? Then we could just re-land #37299?

I'd rather have late-added tests potentially cause flakiness (and therefore hopefully get discovered and fixed) rather than have not-exactly-late-added tests getting silently ignored. In fact, silently ignoring any tests feels bad no matter what the source of the test, right?

@foolip

@foolip
Copy link
Member

foolip commented Jan 5, 2023

@jcscottiii can you take over helping @mfreed7 reland #37299 one way or another? #37631 is my draft PR to reland. Next steps are to do a full run with the changes to see which tests results would change.

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jan 13, 2023
@foolip @moz-wptsync-bot
Bug 1806808 [wpt PR 37624] - Revert "WPT: Allow window.onload to co…
c2dd06e 
…ntain multiple `test()`s", a=testonly

Automatic update from web-platform-tests
Revert "WPT: Allow `window.onload` to contain multiple `test()`s"

This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
web-platform-tests/wpt#37623

There was an attempted fix for this:
web-platform-tests/wpt#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}

--

wpt-commits: 5c571b86bc98819157b584132bca1b80ca4503cb
wpt-pr: 37624
jwidar pushed a commit to jwidar/LatencyZeroGithub that referenced this pull request Sep 16, 2025
@foolip @moz-wptsync-bot
Bug 1806808 [wpt PR 37624] - Revert "WPT: Allow window.onload to co…
f1991d4 
…ntain multiple `test()`s", a=testonly

Automatic update from web-platform-tests
Revert "WPT: Allow `window.onload` to contain multiple `test()`s"

This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
web-platform-tests/wpt#37623

There was an attempted fix for this:
web-platform-tests/wpt#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** 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}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}

--

wpt-commits: 5c571b86bc98819157b584132bca1b80ca4503cb
wpt-pr: 37624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

infra status:needs-reviewers testharness.js

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jgraham @wpt-pr-bot @foolip @mfreed7