FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: fix test runner concurrency by MoLow · Pull Request #47675 · nodejs/node · GitHub

/ node Public

test_runner: fix test runner concurrency - #47675

Closed
MoLow wants to merge 2 commits into
nodejs:mainfrom
MoLow:fix-test-runner-concurrency
ConversationCommits (2)ChecksFiles changed
Closed

test_runner: fix test runner concurrency#47675
MoLow wants to merge 2 commits into
nodejs:mainfrom
MoLow:fix-test-runner-concurrency

Conversation

MoLow commented Apr 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #47365
Fixes: #47696

SafePromiseAllSettledReturnVoid runs sequentially before this fix :(
this makes me wonder if we should add a benchmark for the test runner

MoLow requested a review from aduh95 April 22, 2023 20:43
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 22, 2023

MoLow commented Apr 22, 2023

Copy link
Copy Markdown
Member Author

CC @nodejs/performance Who can help me with creating a benchmark? I never have before

MoLow added performance Issues and PRs related to the performance of Node.js. test_runner Issues and PRs related to the test runner subsystem. labels Apr 22, 2023

tniessen commented Apr 22, 2023

Copy link
Copy Markdown
Member

SafePromiseAllSettledReturnVoid runs sequentially before this fix

Is this the relevant invocation?

node/lib/internal/test_runner/runner.js

Lines 390 to 397 in 0c5f253

const runFiles = () => {
root.harness.bootstrapComplete = true;
return SafePromiseAllSettledReturnVoid(testFiles, (path) => {
const subtest = runTestFile(path, root, inspectPort, filesWatcher);
runningSubtests.set(path, subtest);
return subtest;
});
};

this makes me wonder if we should add a benchmark for the test runner

If it is expected that test files are executed in parallel, then that should be covered by a test before any benchmark. Benchmarks are good for performance comparisons, but this is about a difference between design and implementation, i.e., functional correctness.

MoLow commented Apr 22, 2023

Copy link
Copy Markdown
Member Author

Is this the relevant invocation?

yes

If it is expected that test files are executed in parallel, then that should be covered by a test before any benchmark. Benchmarks are good for performance comparisons, but this is about a difference between design and implementation, i.e., functional correctness.

I will add a test

tniessen commented Apr 22, 2023

Copy link
Copy Markdown
Member

@aduh95 In #45175, was the main motivation behind SafePromiseAllSettledReturnVoid not having to allocate an array for results, and so that the caller automatically obtains a Promise<void>? If so, this appears to be the only call site, so maybe we can just use SafePromiseAllSettled instead and return the Promise<void> from the test runner instead?

tniessen reviewed Apr 22, 2023
View reviewed changes

tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is the number of concurrent tests guaranteed to be at least two? I thought it depended on os.availableParallelism(), in which case it looks like this test might fail when only one or two CPU cores are assigned to the process group.

MoLow 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 22, 2023

MoLow commented Apr 22, 2023

Copy link
Copy Markdown
Member Author

this fix introduced a race condition in tests outputs being reported out of order, so I fixed that as well

MoLow commented Apr 22, 2023
View reviewed changes
Comment thread test/parallel/test-runner-concurrency.js Outdated

MoLow Apr 22, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@cjihrig please confirm this skip makes sense

cjihrig Apr 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it makes sense. I think you could verify by replacing the use of os.availableParallelism() in the test runner code with 1 or 2 and seeing if the test hangs.

mcollina approved these changes Apr 23, 2023
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Apr 23, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2023

nodejs-github-bot commented Apr 23, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Apr 24, 2023
Open
39 tasks
MoLow mentioned this pull request Apr 24, 2023
Merged
MoLow force-pushed the fix-test-runner-concurrency branch from e1dbbd2 to bbc6eaf Compare April 24, 2023 18:26

MoLow commented Apr 24, 2023

Copy link
Copy Markdown
Member Author

@cjihrig can you take a look? I've had to fix multiple race conditions in the test runner.
this also depends on #47699 to accurately count tests

MoLow commented Apr 24, 2023

Copy link
Copy Markdown
Member Author

@benjamingr @mcollina PTAL

MoLow added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 24, 2023
cjihrig reviewed Apr 24, 2023
View reviewed changes
Comment thread lib/internal/test_runner/tests_stream.js Outdated
Comment thread lib/internal/test_runner/runner.js Outdated
Comment thread lib/internal/test_runner/runner.js Outdated
Comment thread lib/internal/test_runner/runner.js Outdated

cjihrig Apr 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The logic in this function (and even the function signature) seems like it should be part of #handleReportItem(). Is it possible to handle this there?

MoLow Apr 24, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

handleReportItem happens only in order of reporting (using this.isClearToSend()), however, #skipReporting returns wrong values if it is called after the child process has completed but the test has not yet been reported

MoLow force-pushed the fix-test-runner-concurrency branch from bbc6eaf to 17e3338 Compare April 24, 2023 19:25
MoLow mentioned this pull request Apr 24, 2023
Closed

nodejs-github-bot commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow commented Apr 24, 2023

Copy link
Copy Markdown
Member Author

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

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2023

nodejs-github-bot commented Apr 24, 2023

Copy link
Copy Markdown
Collaborator

Landed in 8becacb...dccd25e

nodejs-github-bot pushed a commit that referenced this pull request Apr 24, 2023
test_runner: fix test counting
dac0263 
PR-URL: #47675
Fixes: #47365
Fixes: #47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Apr 24, 2023
test_runner: fix test runner concurrency
dccd25e 
PR-URL: #47675
Fixes: #47365
Fixes: #47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MoLow deleted the fix-test-runner-concurrency branch April 24, 2023 22:33
This was referenced Apr 25, 2023
Open
Open
tniessen mentioned this pull request Apr 26, 2023
Closed
This was referenced Apr 27, 2023
Open
Open
github-actions Bot mentioned this pull request Apr 29, 2023
Open
32 tasks
github-actions Bot mentioned this pull request Apr 30, 2023
Open
33 tasks
targos pushed a commit that referenced this pull request May 2, 2023
test_runner: fix test counting
2d7cac0 
PR-URL: #47675
Fixes: #47365
Fixes: #47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request May 2, 2023
test_runner: fix test runner concurrency
40b3879 
PR-URL: #47675
Fixes: #47365
Fixes: #47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos mentioned this pull request May 2, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
test_runner: fix test counting
003b2d4 
PR-URL: #47675
Fixes: #47365
Fixes: #47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
test_runner: fix test runner concurrency
85edbc3 
PR-URL: #47675
Fixes: #47365
Fixes: #47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
test_runner: fix test counting
3bbb1fc 
PR-URL: nodejs#47675
Fixes: nodejs#47365
Fixes: nodejs#47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
test_runner: fix test runner concurrency
323881f 
PR-URL: nodejs#47675
Fixes: nodejs#47365
Fixes: nodejs#47696
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams mentioned this pull request Jul 10, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen left review comments
mcollina mcollina approved these changes
benjamingr benjamingr approved these changes
cjihrig cjihrig approved these changes
aduh95 Awaiting requested review from aduh95

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

nested test counting is wrong concurrent tests are slow

6 participants

Back | FazBrowse Home | New Git URL