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

Change Tests.prototype.all_done to require either tests or pending remotes #29970

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

andreubotella
Copy link
Member

@andreubotella andreubotella commented Aug 10, 2021

Tests.prototype.all_done used to treat the presence of at least one test as a necessary condition for all tests to be done. This would be false in cases where an exception is thrown inside the setup function, but the code path that runs in that case doesn't use the all_done() function.

However, if an exception is thrown inside the setup function in a worker test, tests.all_done() is called in that code path, which results in the completion message from the worker being ignored. This change fixes this by changing the condition to require the presence of either tests or remote contexts.

For service worker tests, however, that is not enough, since service worker registration is asynchronous and tests.all_done() will be called before the SW remote context is set up. This change lets the fetch_tests_from_worker function take a promise or an async function that returns the worker, and it guarantees that the test won't be considered complete until the promise is resolved. The HTML boilerplate for service worker tests is also changed in turn.

Closes #29777.


For historical context, this PR was initially a draft fix for #29777 that removed the condition that this.tests.length > 0. This turned out to break many tests (#29777 (comment)), and this PR was repurposed to solve this in a different way.

@andreubotella
Copy link
Member Author

Closing because the amount of test breakage this change introduces (see #29777 (comment)) means it's probably best to try other options to solve #29777.

@andreubotella andreubotella changed the title WIP: Don't check whether any tests exist in Tests.prototype.all_done WIP: Attempts to fix #29777 Aug 11, 2021
@andreubotella
Copy link
Member Author

Reopening because replacing this.tests.length > 0 with (this.tests.length > 0 || this.pending_remotes.length > 0) might just do the trick.

@andreubotella andreubotella reopened this Aug 11, 2021
@andreubotella andreubotella force-pushed the andreubotella/all-done-without-test-length branch from 50e0583 to 1d6d1e6 Compare August 11, 2021 20:52
@andreubotella andreubotella changed the title WIP: Attempts to fix #29777 Change Tests.prototype.all_done to require either tests or remote contexts Aug 11, 2021
@andreubotella
Copy link
Member Author

PR rebased, squashed, renamed and marked as ready for review.

@andreubotella andreubotella marked this pull request as ready for review August 11, 2021 20:56
@andreubotella andreubotella requested a review from jgraham August 11, 2021 20:56
@andreubotella andreubotella requested a review from foolip August 11, 2021 20:56
@andreubotella andreubotella changed the title Change Tests.prototype.all_done to require either tests or remote contexts Change Tests.prototype.all_done to require either tests or pending remotes Aug 11, 2021
@andreubotella andreubotella force-pushed the andreubotella/all-done-without-test-length branch from 1d6d1e6 to 8b3feba Compare August 11, 2021 20:57
@andreubotella andreubotella force-pushed the andreubotella/all-done-without-test-length branch from 8b3feba to 3e2af2c Compare August 31, 2021 10:25
@andreubotella
Copy link
Member Author

I've checked the diffs of this PR with the corresponding commit in master (7df27fb) for Chrome, and every difference seems to be due to a fluke that also affects other recent runs. The one exception is storage-access-api/hasStorageAccess.sub.window.html, but while debugging that I found bugs with the test itself (see #30320), and after fixing those bugs, the results before and after this change are identical (both for wpt serve and wpt run). So this PR by itself doesn't seem to be causing any unexpected failures or successes.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

No substantive concerns, but I think we need a docs update, and either an RFC or some agreement that this is a small enough change to skip an RFC.

resources/testharness.js Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot added the docs label Sep 8, 2021
@sideshowbarker sideshowbarker removed their request for review September 9, 2021 02:06
@andreubotella
Copy link
Member Author

I just realized that the change to make fetch_tests_from_worker take a promise isn't really needed anymore. I added it back when this PR removed the this.tests.length > 0 condition, since it made SW tests be marked as done before the registration promise resolved. But when I changed the condition to (this.tests.length > 0 || this.pending_remotes.length > 0), now a test file that only fetches tests from a SW wouldn't complete before the remote context is set, so there's no longer any reason to add a dummy pending remote while the promise is pending.

…remotes

`Tests.prototype.all_done` used to treat the presence of at least one
test as a necessary condition for all tests to be done. This would be
false in cases where an exception is thrown inside the `setup` function,
but the code path that runs in that case doesn't use the `all_done()`
function.

However, if an exception is thrown inside the `setup` function in a
worker test, `tests.all_done()` is called in that code path, which
results in the completion message from the worker being ignored. This
change fixes this by changing the condition to require the presence of
either tests or remote contexts.

Closes #29777.
@andreubotella andreubotella force-pushed the andreubotella/all-done-without-test-length branch from af5a0a5 to ba417e3 Compare September 10, 2021 09:57
@andreubotella
Copy link
Member Author

andreubotella commented Sep 10, 2021

Checked the diffs against master again, and there are no regressions that can't be explained as flaky tests.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this. I know it's hard to get even a one line change to land :)

@jgraham jgraham merged commit c563d3a into master Sep 13, 2021
@jgraham jgraham deleted the andreubotella/all-done-without-test-length branch September 13, 2021 10:21
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.

Exceptions thrown in a worker's setup function don't fail the test until it times out
5 participants