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, cli: add --test-concurrency flag #49996

Merged
merged 1 commit into from
Oct 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1654,6 +1654,15 @@ Starts the Node.js command line test runner. This flag cannot be combined with
See the documentation on [running tests from the command line][]
for more details.

### `--test-concurrency`

<!-- YAML
added: REPLACEME
-->

The maximum number of test files that the test runner CLI will execute
concurrently. The default value is `os.availableParallelism() - 1`.

### `--test-name-pattern`

<!-- YAML
Expand Down
12 changes: 7 additions & 5 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,12 @@ in the [test runner execution model][] section.

### Test runner execution model

Each matching test file is executed in a separate child process. If the child
process finishes with an exit code of 0, the test is considered passing.
Otherwise, the test is considered to be a failure. Test files must be
executable by Node.js, but are not required to use the `node:test` module
internally.
Each matching test file is executed in a separate child process. The maximum
number of child processes running at any time is controlled by the
[`--test-concurrency`][] flag. If the child process finishes with an exit code
of 0, the test is considered passing. Otherwise, the test is considered to be a
failure. Test files must be executable by Node.js, but are not required to use
the `node:test` module internally.

Each test file is executed as if it was a regular script. That is, if the test
file itself uses `node:test` to define tests, all of those tests will be
Expand Down Expand Up @@ -2551,6 +2552,7 @@ added:
[TTY]: tty.md
[`--experimental-test-coverage`]: cli.md#--experimental-test-coverage
[`--import`]: cli.md#--importmodule
[`--test-concurrency`]: cli.md#--test-concurrency
[`--test-name-pattern`]: cli.md#--test-name-pattern
[`--test-only`]: cli.md#--test-only
[`--test-reporter-destination`]: cli.md#--test-reporter-destination
Expand Down
4 changes: 4 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,10 @@ Specify the minimum allocation from the OpenSSL secure heap. The default is 2. T
.It Fl -test
Starts the Node.js command line test runner.
.
.It Fl -test-concurrency
The maximum number of test files that the test runner CLI will execute
concurrently.
.
.It Fl -test-name-pattern
A regular expression that configures the test runner to only execute tests
whose name matches the provided pattern.
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const {
prepareMainThreadExecution(false);
markBootstrapComplete();

let concurrency = true;
let concurrency = getOptionValue('--test-concurrency') || true;
let inspectPort;

if (isUsingInspector()) {
Expand Down
3 changes: 3 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddOption("--test",
"launch test runner on startup",
&EnvironmentOptions::test_runner);
AddOption("--test-concurrency",
"specify test runner concurrency",
&EnvironmentOptions::test_runner_concurrency);
AddOption("--experimental-test-coverage",
"enable code coverage in the test runner",
&EnvironmentOptions::test_runner_coverage);
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ class EnvironmentOptions : public Options {
std::string env_file;
bool has_env_file_string = false;
bool test_runner = false;
uint64_t test_runner_concurrency = 0;
bool test_runner_coverage = false;
std::vector<std::string> test_name_pattern;
std::vector<std::string> test_reporter;
Expand Down
45 changes: 45 additions & 0 deletions test/parallel/test-runner-cli-concurrency.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';
const common = require('../common');
const tmpdir = require('../common/tmpdir');
const { deepStrictEqual, strictEqual } = require('node:assert');
const { spawnSync } = require('node:child_process');
const { readdirSync, writeFileSync } = require('node:fs');
const { join } = require('node:path');
const { beforeEach, test } = require('node:test');

function createTestFile(name) {
writeFileSync(join(tmpdir.path, name), `
const fs = require('node:fs');

fs.unlinkSync(__filename);
setTimeout(() => {}, 1_000_000_000);
`);
}

beforeEach(() => {
tmpdir.refresh();
createTestFile('test-1.js');
createTestFile('test-2.js');
});

test('concurrency of one', () => {
const cp = spawnSync(process.execPath, ['--test', '--test-concurrency=1'], {
cwd: tmpdir.path,
timeout: common.platformTimeout(1000),
});

strictEqual(cp.stderr.toString(), '');
strictEqual(cp.error.code, 'ETIMEDOUT');
deepStrictEqual(readdirSync(tmpdir.path), ['test-2.js']);
});

test('concurrency of two', () => {
const cp = spawnSync(process.execPath, ['--test', '--test-concurrency=2'], {
Copy link
Member

Choose a reason for hiding this comment

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

how does this test if the files really run concurrently? We have fixtures that create a mutex, can we use them?

test('--test multiple files', { skip: os.availableParallelism() < 3 }, async () => {
await fs.writeFile(tmpdir.resolve('test-runner-concurrency'), '');
const { code, stderr } = await common.spawnPromisified(process.execPath, [
'--test',
fixtures.path('test-runner', 'concurrency', 'a.mjs'),
fixtures.path('test-runner', 'concurrency', 'b.mjs'),
]);
assert.strictEqual(stderr, '');
assert.strictEqual(code, 0);
});

Copy link
Member

Choose a reason for hiding this comment

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

also, better to test the --test-concurrency within https://github.com/nodejs/node/blob/966e3d34936b28bfb31f00a6f862226baa3bf9d0/test/parallel/test-runner-concurrency.js to avoid the two tests using this "mutex" in parallel (and it is related to this file)

Copy link
Member

Choose a reason for hiding this comment

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

I now relaized this assersion. deepStrictEqual(readdirSync(tmpdir.path), [])
anyway we can still move the test into that file but not a blocker.
also, probably skip second test if os.availableParallelism() < 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does this test if the files really run concurrently?

We know that they are not running sequentially because each file never finishes. Since we know that the tests aren't running sequentially, we just need to verify that the batch size is respected. Plus, this change is not introducing any new concurrency functionality, it's just assigning a different value using the existing functionality.

cwd: tmpdir.path,
timeout: common.platformTimeout(1000),
});

strictEqual(cp.stderr.toString(), '');
strictEqual(cp.error.code, 'ETIMEDOUT');
deepStrictEqual(readdirSync(tmpdir.path), []);
});