Skip to content

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

Merged
chromium-wpt-export-bot merged 1 commit intomasterfrom
chromium-export-cl-4111318
Dec 21, 2022
Merged

Revert "WPT: Allow window.onload to contain multiple test()s"#37624
chromium-wpt-export-bot merged 1 commit intomasterfrom
chromium-export-cl-4111318

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

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

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}

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.

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

3 participants