GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: refactor WPTRunner and enable parallel WPT execution by panva · Pull Request #47635 · nodejs/node

Conversation

@panva

@panva panva commented Apr 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 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
All reactions
  • 👍 1 reaction
@panva panva added test Issues and PRs related to Node.js core tests and test infrastructure. tools Issues and PRs related to the tools directory. dont-land-on-v14.x labels Apr 20, 2023
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Apr 20, 2023
@panva panva mentioned this pull request Apr 20, 2023
test: revert parallel wpt commits #47627
Closed
@panva
panva force-pushed the wpt-runner-refactor branch 2 times, most recently from dc0c660 to 514dd0c Compare April 20, 2023 10:45
@panva
test: refactor WPTRunner and enable parallel WPT execution
505b768
@panva
panva force-pushed the wpt-runner-refactor branch from 514dd0c to 505b768 Compare April 20, 2023 11:13
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Apr 20, 2023 •
edited by panva
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
anonrig
anonrig approved these changes Apr 22, 2023
View reviewed changes
@panva panva added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Apr 22, 2023
This was referenced Apr 24, 2023
CI Reliability 2023-04-24 nodejs/reliability#545
Open
CI Reliability 2023-04-25 nodejs/reliability#546
Open
aduh95
aduh95 reviewed Apr 25, 2023
View reviewed changes
Comment thread test/common/wpt.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@panva
fixup! test: refactor WPTRunner and enable parallel WPT execution
0661e23
@panva panva added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Apr 25, 2023
panva added 2 commits April 25, 2023 10:46
@panva
fixup! test: refactor WPTRunner and enable parallel WPT execution
5b7ff31
@panva
fixup! test: refactor WPTRunner and enable parallel WPT execution
34c4ffe
aduh95
aduh95 approved these changes Apr 25, 2023
View reviewed changes
Comment thread test/common/wpt.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@panva
fixup! test: refactor WPTRunner and enable parallel WPT execution
ecd22cb
aduh95
aduh95 approved these changes Apr 25, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51461/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51462/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 25, 2023
@nodejs-github-bot
nodejs-github-bot merged commit a5ce18f into nodejs:main Apr 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2023

Copy link
Copy Markdown
Collaborator

Landed in a5ce18f

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@panva
panva deleted the wpt-runner-refactor branch April 25, 2023 14:33
This was referenced Apr 26, 2023
CI Reliability 2023-04-26 nodejs/reliability#547
Open
CI Reliability 2023-04-27 nodejs/reliability#548
Open
@panva panva removed dont-land-on-v20.x labels Apr 27, 2023
@github-actions github-actions Bot mentioned this pull request Apr 28, 2023
CI Reliability 2023-04-28 nodejs/reliability#549
Open
29 tasks
@github-actions github-actions Bot mentioned this pull request Apr 29, 2023
CI Reliability 2023-04-29 nodejs/reliability#550
Open
32 tasks
This was referenced Apr 30, 2023
CI Reliability 2023-04-30 nodejs/reliability#551
Open
CI Reliability 2023-05-01 nodejs/reliability#552
Open
targos pushed a commit that referenced this pull request May 2, 2023
@panva @targos
test: refactor WPTRunner and enable parallel WPT execution
84fa9fd
PR-URL: #47635
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos mentioned this pull request May 2, 2023
v20.1.0 release proposal #47820
Merged
@atlowChemi atlowChemi mentioned this pull request May 2, 2023
lib: implement AbortSignal.any() #47821
Merged
@danielleadams danielleadams added the backport-requested-v18.x label Jul 4, 2023
@danielleadams danielleadams mentioned this pull request Jul 4, 2023
test: reduce WPT concurrency #47834
Merged
@aduh95

aduh95 commented Jul 4, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@panva

This comment was marked as outdated.

Sign in to view
@panva

panva commented Jul 4, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@aduh95 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.

Projects

None yet

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.

5 participants

@panva @nodejs-github-bot @aduh95 @anonrig @danielleadams
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.