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

test: refactor WPTRunner and enable parallel WPT execution #47635

Merged
merged 5 commits into from
Apr 25, 2023

Conversation

panva
Copy link
Member

@panva panva commented Apr 20, 2023

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

Screenshot 2023-04-20 at 12 22 42

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 dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x.

@panva panva added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Apr 20, 2023
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Apr 20, 2023
@panva panva force-pushed the wpt-runner-refactor branch 2 times, most recently from dc0c660 to 514dd0c Compare April 20, 2023 10:45
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 20, 2023

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/

edit: I've stress tested wpt/test-timers in CI as well as locally and couldn't get a new hit on this failure. I'm not going to mark it as flaky now but if it resurfaces, since it's timers related marking it flaky in WPT would be the right course of action.

edit2: Having had a look at the reliability reports, i'm going to open a separate PR to skip the flaky test (#47646)

edit3: I'll move these the few WPTs from wpt/test-timers to test/sequential in a separate PR. #47657

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 22, 2023
test/common/wpt.js Outdated Show resolved Hide resolved
@panva panva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 25, 2023
test/common/wpt.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2023
@nodejs-github-bot nodejs-github-bot merged commit a5ce18f into nodejs:main Apr 25, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in a5ce18f

@panva panva deleted the wpt-runner-refactor branch April 25, 2023 14:33
@panva panva removed dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v19.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Apr 27, 2023
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
PR-URL: nodejs#47635
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
PR-URL: nodejs#47635
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
PR-URL: nodejs#47635
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request May 2, 2023
PR-URL: #47635
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request May 2, 2023
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 4, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jul 4, 2023

@panva would you be able to backport this to v18.x-staging?

@panva

This comment was marked as outdated.

@panva
Copy link
Member Author

panva commented Jul 4, 2023

@aduh95 I think it's best to leave these changes out of v18.x and believe we can backport #47821 more easily instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants