Changing resources/testharness.js doesn't trigger resources/ tests #37623
Labels
Comments
|
I think this is where we'd put the logic to ignore these files for finding affected tests: Lines 804 to 805 in 0467ad1 |
chromium-wpt-export-bot
pushed a commit
that referenced
this issue
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 <[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
chromium-wpt-export-bot
pushed a commit
that referenced
this issue
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 <[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
aarongable
pushed a commit
to chromium/chromium
that referenced
this issue
Dec 21, 2022
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}
chromium-wpt-export-bot
pushed a commit
that referenced
this issue
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 <[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}
chromium-wpt-export-bot
pushed a commit
that referenced
this issue
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 <[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}
|
Thanks @jgraham! |
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this issue
Jan 4, 2023
…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
jamienicol
pushed a commit
to jamienicol/gecko
that referenced
this issue
Jan 13, 2023
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This can be observed by making any edit to resources/testharness.js, committing that, and then running
./wpt test-jobs abc..123whereabcis the parent commit and123is the new commit. (This is how the command is invoked in CI.) That will currently show:The reason that resources/ tests aren't run is because changes to resources/testharness.js are ignored by default:
wpt/tools/wpt/testfiles.py
Line 38 in cae0fb2
And that's because this same code is used to detect affected tests, and almost all tests would be affected by changes to testharness.js. The logic was most recently updated in #15485.
To solve this, we need to stop ignoring resources/* files when deciding which test jobs to run, but keep ignoring it for the affected tests heuristic.
The text was updated successfully, but these errors were encountered: