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: add shards support #48639

Merged
merged 18 commits into from
Jul 5, 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
22 changes: 22 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1500,6 +1500,27 @@ changes:
Configures the test runner to only execute top level tests that have the `only`
option set.

### `--test-shard`
rluvaton marked this conversation as resolved.
Show resolved Hide resolved

<!-- YAML
added: REPLACEME
-->

Test suite shard to execute in a format of `<index>/<total>`, where

`index` is a positive integer, index of divided parts
`total` is a positive integer, total of divided part
This command will divide all tests files into `total` equal parts,
and will run only those that happen to be in an `index` part.

For example, to split your tests suite into three parts, use this:

```bash
node --test --test-shard=1/3
node --test --test-shard=2/3
node --test --test-shard=3/3
```

### `--throw-deprecation`

<!-- YAML
Expand Down Expand Up @@ -2177,6 +2198,7 @@ Node.js options that are allowed are:
* `--test-only`
* `--test-reporter-destination`
* `--test-reporter`
* `--test-shard`
* `--throw-deprecation`
* `--title`
* `--tls-cipher-list`
Expand Down
5 changes: 5 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,11 @@ changes:
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
* `watch` {boolean} Whether to run in watch mode or not. **Default:** `false`.
* `shard` {Object} Running tests in a specific shard. **Default:** `undefined`.
* `index` {number} is a positive integer between 1 and `<total>`
that specifies the index of the shard to run. This option is _required_.
* `total` {number} is a positive integer that specifies the total number
of shards to split the test files to. This option is _required_.
* Returns: {TestsStream}

```mjs
Expand Down
3 changes: 3 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,9 @@ The destination for the corresponding test reporter.
Configures the test runner to only execute top level tests that have the `only`
option set.
.
.It Fl -test-shard
Test suite shard to execute in a format of <index>/<total>.
.
.It Fl -throw-deprecation
Throw errors for deprecations.
.
Expand Down
37 changes: 36 additions & 1 deletion lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ const { isUsingInspector } = require('internal/util/inspector');
const { run } = require('internal/test_runner/runner');
const { setupTestReporters } = require('internal/test_runner/utils');
const { exitCodes: { kGenericUserError } } = internalBinding('errors');
const {
codes: {
ERR_INVALID_ARG_VALUE,
},
} = require('internal/errors');
const {
NumberParseInt,
RegExpPrototypeExec,
StringPrototypeSplit,
} = primordials;

prepareMainThreadExecution(false);
markBootstrapComplete();
Expand All @@ -22,7 +32,32 @@ if (isUsingInspector()) {
inspectPort = process.debugPort;
}

run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters })
let shard;
const shardOption = getOptionValue('--test-shard');
if (shardOption) {
if (!RegExpPrototypeExec(/^\d+\/\d+$/, shardOption)) {
process.exitCode = kGenericUserError;

throw new ERR_INVALID_ARG_VALUE(
'--test-shard',
shardOption,
'must be in the form of <index>/<total>',
);
}

const { 0: indexStr, 1: totalStr } = StringPrototypeSplit(shardOption, '/');

const index = NumberParseInt(indexStr, 10);
const total = NumberParseInt(totalStr, 10);
Comment on lines +50 to +51
Copy link
Member

Choose a reason for hiding this comment

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

fwiw if it's NumberParseInt, the , 10 isn't required - only on the global one.

Copy link
Member Author

Choose a reason for hiding this comment

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

on the global one it's not required as far as I know

Copy link
Member

Choose a reason for hiding this comment

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

It's required in the conceptual sense, because parseInt will try to guess the radix if you omit it.


shard = {
__proto__: null,
index,
rluvaton marked this conversation as resolved.
Show resolved Hide resolved
total,
};
}

run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters, shard })
.once('test:fail', () => {
process.exitCode = kGenericUserError;
});
34 changes: 31 additions & 3 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,18 @@ const console = require('internal/console/global');
const {
codes: {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_TEST_FAILURE,
ERR_OUT_OF_RANGE,
},
} = require('internal/errors');
const { validateArray, validateBoolean, validateFunction } = require('internal/validators');
const {
validateArray,
validateBoolean,
validateFunction,
validateObject,
validateInteger,
} = require('internal/validators');
const { getInspectPort, isUsingInspector, isInspectorMessage } = require('internal/util/inspector');
const { isRegExp } = require('internal/util/types');
const { kEmptyObject } = require('internal/util');
Expand Down Expand Up @@ -416,7 +424,7 @@ function run(options) {
if (options === null || typeof options !== 'object') {
options = kEmptyObject;
}
let { testNamePatterns } = options;
let { testNamePatterns, shard } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup } = options;

if (files != null) {
Expand All @@ -425,6 +433,22 @@ function run(options) {
if (watch != null) {
validateBoolean(watch, 'options.watch');
}
if (shard != null) {
validateObject(shard, 'options.shard');
// Avoid re-evaluating the shard object in case it's a getter
shard = { __proto__: null, index: shard.index, total: shard.total };

validateInteger(shard.total, 'options.shard.total', 1);
validateInteger(shard.index, 'options.shard.index');

if (shard.index <= 0 || shard.total < shard.index) {
throw new ERR_OUT_OF_RANGE('options.shard.index', `>= 1 && <= ${shard.total} ("options.shard.total")`, shard.index);
}
Comment on lines +444 to +446
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed based on my previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

see that comment answer 😄


if (watch) {
Copy link
Member

Choose a reason for hiding this comment

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

q: why?

Copy link
Member Author

Choose a reason for hiding this comment

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

why would you use sharding if you need to watch all the files? watch is for active development

Copy link
Member

Choose a reason for hiding this comment

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

You had a bug in a certain shard in CI and you want to iteratively fix it? Is there a reason not to allow it?

Copy link
Member Author

Choose a reason for hiding this comment

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

if you add a file it will change the sharding, we can add it I just think it won't be really useful

throw new ERR_INVALID_ARG_VALUE('options.shard', watch, 'shards not supported with watch mode');
}
}
if (setup != null) {
validateFunction(setup, 'options.setup');
}
Expand All @@ -446,7 +470,11 @@ function run(options) {
}

const root = createTestTree({ concurrency, timeout, signal });
const testFiles = files ?? createTestFileList();
let testFiles = files ?? createTestFileList();

if (shard) {
testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1);
}

let postRun = () => root.postRun();
let filesWatcher;
Expand Down
4 changes: 4 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"run tests with 'only' option set",
&EnvironmentOptions::test_only,
kAllowedInEnvvar);
AddOption("--test-shard",
"run test at specific shard",
&EnvironmentOptions::test_shard,
kAllowedInEnvvar);
AddOption("--test-udp-no-try-send", "", // For testing only.
&EnvironmentOptions::test_udp_no_try_send);
AddOption("--throw-deprecation",
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class EnvironmentOptions : public Options {
std::vector<std::string> test_reporter_destination;
bool test_only = false;
bool test_udp_no_try_send = false;
std::string test_shard;
bool throw_deprecation = false;
bool trace_atomics_wait = false;
bool trace_deprecation = false;
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/a.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('a.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/b.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('b.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/c.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('c.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/d.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('d.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/e.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('e.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/f.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('f.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/g.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('g.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/h.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('h.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/i.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('i.cjs this should pass');
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/shards/j.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('j.cjs this should pass');
128 changes: 128 additions & 0 deletions test/parallel/test-runner-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,131 @@ const testFixtures = fixtures.path('test-runner');
const stdout = child.stdout.toString();
assert.match(stdout, /ok 1 - this should pass/);
}

{
// --test-shard option validation
const args = ['--test', '--test-shard=1', join(testFixtures, 'index.js')];
const child = spawnSync(process.execPath, args, { cwd: testFixtures });

assert.strictEqual(child.status, 1);
assert.strictEqual(child.signal, null);
assert.match(child.stderr.toString(), /The argument '--test-shard' must be in the form of <index>\/<total>\. Received '1'/);
const stdout = child.stdout.toString();
assert.strictEqual(stdout, '');
}

{
// --test-shard option validation
const args = ['--test', '--test-shard=1/2/3', join(testFixtures, 'index.js')];
const child = spawnSync(process.execPath, args, { cwd: testFixtures });

assert.strictEqual(child.status, 1);
assert.strictEqual(child.signal, null);
assert.match(child.stderr.toString(), /The argument '--test-shard' must be in the form of <index>\/<total>\. Received '1\/2\/3'/);
const stdout = child.stdout.toString();
assert.strictEqual(stdout, '');
}

{
// --test-shard option validation
const args = ['--test', '--test-shard=0/3', join(testFixtures, 'index.js')];
const child = spawnSync(process.execPath, args, { cwd: testFixtures });

assert.strictEqual(child.status, 1);
assert.strictEqual(child.signal, null);
assert.match(child.stderr.toString(), /The value of "options\.shard\.index" is out of range\. It must be >= 1 && <= 3 \("options\.shard\.total"\)\. Received 0/);
const stdout = child.stdout.toString();
assert.strictEqual(stdout, '');
}

{
// --test-shard option validation
const args = ['--test', '--test-shard=0xf/20abcd', join(testFixtures, 'index.js')];
const child = spawnSync(process.execPath, args, { cwd: testFixtures });

assert.strictEqual(child.status, 1);
assert.strictEqual(child.signal, null);
assert.match(child.stderr.toString(), /The argument '--test-shard' must be in the form of <index>\/<total>\. Received '0xf\/20abcd'/);
const stdout = child.stdout.toString();
assert.strictEqual(stdout, '');
}

{
// --test-shard option validation
const args = ['--test', '--test-shard=hello', join(testFixtures, 'index.js')];
const child = spawnSync(process.execPath, args, { cwd: testFixtures });

assert.strictEqual(child.status, 1);
assert.strictEqual(child.signal, null);
assert.match(child.stderr.toString(), /The argument '--test-shard' must be in the form of <index>\/<total>\. Received 'hello'/);
const stdout = child.stdout.toString();
assert.strictEqual(stdout, '');
}

{
// --test-shard option, first shard
const args = [
'--test',
'--test-shard=1/2',
join(testFixtures, 'shards/*.cjs'),
];
const child = spawnSync(process.execPath, args);

assert.strictEqual(child.status, 0);
assert.strictEqual(child.signal, null);
assert.strictEqual(child.stderr.toString(), '');
const stdout = child.stdout.toString();
assert.match(stdout, /# Subtest: a\.cjs this should pass/);
assert.match(stdout, /ok 1 - a\.cjs this should pass/);

assert.match(stdout, /# Subtest: c\.cjs this should pass/);
assert.match(stdout, /ok 2 - c\.cjs this should pass/);

assert.match(stdout, /# Subtest: e\.cjs this should pass/);
assert.match(stdout, /ok 3 - e\.cjs this should pass/);

assert.match(stdout, /# Subtest: g\.cjs this should pass/);
assert.match(stdout, /ok 4 - g\.cjs this should pass/);

assert.match(stdout, /# Subtest: i\.cjs this should pass/);
assert.match(stdout, /ok 5 - i\.cjs this should pass/);

assert.match(stdout, /# tests 5/);
assert.match(stdout, /# pass 5/);
assert.match(stdout, /# fail 0/);
assert.match(stdout, /# skipped 0/);
}

{
// --test-shard option, last shard
const args = [
'--test',
'--test-shard=2/2',
join(testFixtures, 'shards/*.cjs'),
];
const child = spawnSync(process.execPath, args);

assert.strictEqual(child.status, 0);
assert.strictEqual(child.signal, null);
assert.strictEqual(child.stderr.toString(), '');
const stdout = child.stdout.toString();
assert.match(stdout, /# Subtest: b\.cjs this should pass/);
assert.match(stdout, /ok 1 - b\.cjs this should pass/);

assert.match(stdout, /# Subtest: d\.cjs this should pass/);
assert.match(stdout, /ok 2 - d\.cjs this should pass/);

assert.match(stdout, /# Subtest: f\.cjs this should pass/);
assert.match(stdout, /ok 3 - f\.cjs this should pass/);

assert.match(stdout, /# Subtest: h\.cjs this should pass/);
assert.match(stdout, /ok 4 - h\.cjs this should pass/);

assert.match(stdout, /# Subtest: j\.cjs this should pass/);
assert.match(stdout, /ok 5 - j\.cjs this should pass/);

assert.match(stdout, /# tests 5/);
assert.match(stdout, /# pass 5/);
assert.match(stdout, /# fail 0/);
assert.match(stdout, /# skipped 0/);
}
Loading