GeezerVison is showing github.com — test: refactor WPTRunner and enable parallel WPT execution by panva · Pull Request #47635 · nodejs/node
Conversation
panva
commented
Apr 20, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
This PR reintroduces #47283 but with more refactoring done to the runner to get around the issue that caused it to get reverted in #47627.
The issue at hand was that upon test completion the worker gets intentionally terminated, the worker got terminated based on a Map<string, Worker> where the key is the filename. That was not a problem until concurrency got introduced which meant that the same file but with different query string variants which didn't affect the key were added to the Map. This has lead to some multi variant test workers to get terminated. I've fixed this issue with the refactors explained below and confirmed that it's fixed by comparing the number of individual executed tests to be the same as on main (and in record time).
Screenshot

I've refactored the runner as follows:
- variants are no longer a second iteration within
WPTRunner.prototype.runJsTests()
- variants are instead detected during
StatusLoader.prototype.load() and each variant gets it's own WPTTestSpec instance.
- methods where filename was used as an argument get a
WPTTestSpec instance instead
referencing WPTTestSpec instances in maps is no longer done with their filename but instead a unique Symbol Sets are used instead.
- this means when a spec completes the worker that gets terminated is always the correct one
- more type annotations were added
With this in place then
- When it comes to worker management
a unique Symbol per WPTTestSpec is now used the WPTTestSpec is now used instead of string/symbol references.
- (no change) When it comes to expectation files
WPTTestSpec's filename continues to be used
- (no change) When it comes to the WPT Report
WPTTestSpec's filename + variant continues to be used
Additionally:
- it is now possible to run a single test with a variant like so
./node ./test/wpt/test-webcrypto.js 'generateKey/successes_RSA-PSS.https.any.js?11-20'
- failed test commands get printed using the above syntax when the failure is within a variant test
- the logs got cleaned up a bit
ResourceLoader.prototype.read got split into read and readAsFetch, the former is used within the WPTRunner, the latter only from the spawned workers
I will apply dont-land-* labels to observe the behaviour for a while before eventually landing this on Current by removing https://github.com/nodejs/node/labels/dont-land-on-v20%2Ex
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced Apr 24, 2023
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Apr 26, 2023
This was referenced Apr 30, 2023
|
@panva would you be able to backport this to v18.x-staging?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
|
@aduh95 I think it's best to leave these changes out of v18.x and believe we can backport #47821 more easily instead.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
anonrig
anonrig approved these changes
aduh95
aduh95 approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
author ready
PRs with CI started, the required approvals, and no outstanding review comments.
build
Issues and PRs related to Node.js builds or CI infrastructure.
commit-queue-squash
PRs the Commit Queue should land as one squashed commit.
needs-ci
PRs that need a full CI run.
test
Issues and PRs related to Node.js core tests and test infrastructure.
tools
Issues and PRs related to the tools directory.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|