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_runner: fix test runner concurrency #47675

Closed
wants to merge 2 commits into from

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Apr 22, 2023

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 MoLow requested a review from aduh95 April 22, 2023 20:43
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 22, 2023
@MoLow
Copy link
Member Author

MoLow commented Apr 22, 2023

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

@MoLow 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
Copy link
Member

SafePromiseAllSettledReturnVoid runs sequentially before this fix

Is this the relevant invocation?

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
Copy link
Member Author

MoLow commented Apr 22, 2023

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
Copy link
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?

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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 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
Copy link
Member Author

MoLow commented Apr 22, 2023

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

@@ -62,3 +69,14 @@ describe(
it('should run after other suites', expectedTestTree);
});
}

test('--test multiple files', { skip: os.availableParallelism() < 3 }, async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig please confirm this skip makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@MoLow 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 pending requests for changes, and a CI started. labels Apr 23, 2023
@github-actions 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
Copy link
Collaborator

@MoLow MoLow force-pushed the fix-test-runner-concurrency branch from e1dbbd2 to bbc6eaf Compare April 24, 2023 18:26
@MoLow
Copy link
Member Author

MoLow commented Apr 24, 2023

@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
Copy link
Member Author

MoLow commented Apr 24, 2023

@benjamingr @mcollina PTAL

@MoLow 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
lib/internal/test_runner/tests_stream.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
@@ -237,22 +233,36 @@ class FileTest extends Test {
break;
}
}
#accumelateReportItem({ kind, node, comments, nesting = 0 }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@nodejs-github-bot
Copy link
Collaborator

@MoLow
Copy link
Member Author

MoLow commented Apr 24, 2023

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2023
@nodejs-github-bot 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
Copy link
Collaborator

Landed in 8becacb...dccd25e

nodejs-github-bot pushed a commit that referenced this pull request Apr 24, 2023
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
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 MoLow deleted the fix-test-runner-concurrency branch April 24, 2023 22:33
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
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>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
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>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
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>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
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>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
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>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
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>
targos pushed a commit that referenced this pull request May 2, 2023
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
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 targos mentioned this pull request May 2, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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
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
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
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>
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. 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
Development

Successfully merging this pull request may close these issues.

nested test counting is wrong concurrent tests are slow
6 participants