-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
test: refactor WPTRunner and enable parallel WPT execution #47635
Conversation
dc0c660
to
514dd0c
Compare
514dd0c
to
505b768
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
CI: https://ci.nodejs.org/job/node-test-pull-request/51380/ note to look into https://ci.nodejs.org/job/node-test-commit-osx/51831/nodes=osx11-x64/
edit3: I'll move these the few WPTs from wpt/test-timers to test/sequential in a separate PR. #47657 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Landed in a5ce18f |
PR-URL: nodejs#47635 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#47635 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#47635 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #47635 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@panva would you be able to backport this to v18.x-staging? |
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 onmain
(and in record time).Screenshot
I've refactored the runner as follows:
WPTRunner.prototype.runJsTests()
StatusLoader.prototype.load()
and each variant gets it's ownWPTTestSpec
instance.WPTTestSpec
instance insteadreferencingWPTTestSpec
instances in maps is no longer done with their filename but instead a unique SymbolSet
s are used instead.With this in place then
a unique Symbol pertheWPTTestSpec
is now usedWPTTestSpec
is now used instead of string/symbol references.WPTTestSpec
's filename continues to be usedWPTTestSpec
's filename + variant continues to be usedAdditionally:
ResourceLoader.prototype.read
got split intoread
andreadAsFetch
, the former is used within the WPTRunner, the latter only from the spawned workersI will applyPRs that should not land on the v20.x-staging branch and should not be released in v20.x.
dont-land-*
labels to observe the behaviour for a while before eventually landing this on Current by removing dont-land-on-v20.x