Set all_loaded flag synchronously #37549

jgraham · 2022-12-16T12:15:33Z

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 · 2022-12-16T12:15:42Z

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!

foolip · 2022-12-20T15:57:12Z

@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 · 2022-12-20T20:54:41Z

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 · 2022-12-21T08:47:38Z

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 · 2022-12-21T11:34:00Z

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

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 <[email protected]> > Auto-Submit: Mason Freed <[email protected]> > Reviewed-by: Mason Freed <[email protected]> > Commit-Queue: Mason Freed <[email protected]> > Reviewed-by: Xianzhu Wang <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1081558} Bug: 1395228,1395226,1307772 Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2

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 <[email protected]> > Auto-Submit: Mason Freed <[email protected]> > Reviewed-by: Mason Freed <[email protected]> > Commit-Queue: Mason Freed <[email protected]> > Reviewed-by: Xianzhu Wang <[email protected]> > 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 <[email protected]> Commit-Queue: Philip Jägenstedt <[email protected]> Cr-Commit-Position: refs/heads/main@{#1085925}

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 <[email protected]> > Auto-Submit: Mason Freed <[email protected]> > Reviewed-by: Mason Freed <[email protected]> > Commit-Queue: Mason Freed <[email protected]> > Reviewed-by: Xianzhu Wang <[email protected]> > 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 <[email protected]> Commit-Queue: Philip Jägenstedt <[email protected]> Cr-Commit-Position: refs/heads/main@{#1085925}

mfreed7 · 2023-01-03T21:00:19Z

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.

…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 <[email protected]> > Auto-Submit: Mason Freed <[email protected]> > Reviewed-by: Mason Freed <[email protected]> > Commit-Queue: Mason Freed <[email protected]> > Reviewed-by: Xianzhu Wang <[email protected]> > 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 <[email protected]> Commit-Queue: Philip Jägenstedt <[email protected]> Cr-Commit-Position: refs/heads/main@{#1085925} -- wpt-commits: 5c571b86bc98819157b584132bca1b80ca4503cb wpt-pr: 37624

mfreed7 · 2023-01-04T17:43:56Z

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 · 2023-01-05T05:24:27Z

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

…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 <[email protected]> > Auto-Submit: Mason Freed <[email protected]> > Reviewed-by: Mason Freed <[email protected]> > Commit-Queue: Mason Freed <[email protected]> > Reviewed-by: Xianzhu Wang <[email protected]> > 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 <[email protected]> Commit-Queue: Philip Jägenstedt <[email protected]> Cr-Commit-Position: refs/heads/main@{#1085925} -- wpt-commits: 5c571b86bc98819157b584132bca1b80ca4503cb wpt-pr: 37624

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 added infra testharness.js status:needs-reviewers labels Dec 16, 2022

WeizhongX mentioned this pull request Dec 16, 2022

Fix pytest failure at upstream CI #37538

Closed

chromium-wpt-export-bot mentioned this pull request Dec 21, 2022

Revert "WPT: Allow window.onload to contain multiple test()s" #37624

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set all_loaded flag synchronously #37549

Set all_loaded flag synchronously #37549

jgraham commented Dec 16, 2022

wpt-pr-bot commented Dec 16, 2022

foolip commented Dec 20, 2022

jgraham commented Dec 20, 2022

foolip commented Dec 21, 2022

foolip commented Dec 21, 2022

mfreed7 commented Jan 3, 2023

mfreed7 commented Jan 4, 2023

foolip commented Jan 5, 2023

Set all_loaded flag synchronously #37549

Are you sure you want to change the base?

Set all_loaded flag synchronously #37549

Conversation

jgraham commented Dec 16, 2022

wpt-pr-bot commented Dec 16, 2022

foolip commented Dec 20, 2022

jgraham commented Dec 20, 2022

foolip commented Dec 21, 2022

foolip commented Dec 21, 2022

mfreed7 commented Jan 3, 2023

mfreed7 commented Jan 4, 2023

foolip commented Jan 5, 2023