Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
test_runner: replace shards option with shard and --test-shard
  • Loading branch information
rluvaton committed Jul 3, 2023
commit 602f76703def473b53be478e10f520493825a672
8 changes: 4 additions & 4 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1500,7 +1500,7 @@ changes:
Configures the test runner to only execute top level tests that have the `only`
option set.

### `--shards`
### `--test-shard`

<!-- YAML
added: REPLACEME
Expand All @@ -1516,9 +1516,9 @@ 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 --shards=1/3
node --test --shards=2/3
node --test --shards=3/3
node --test --test-shard=1/3
node --test --test-shard=2/3
node --test --test-shard=3/3
```

### `--throw-deprecation`
Expand Down
2 changes: 1 addition & 1 deletion doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ changes:
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
* `watch` {boolean} Whether to run in watch mode or not. **Default:** `false`.
* `shards` {Object} Running tests in a specific shard. **Default:** `undefined`.
* `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
Expand Down
24 changes: 12 additions & 12 deletions lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,22 @@ if (isUsingInspector()) {
inspectPort = process.debugPort;
}

let shards;
const shardsOption = getOptionValue('--shards');
if (shardsOption) {
const shardsParts = shardsOption.split('/');
let shard;
const shardOption = getOptionValue('--test-shard');
if (shardOption) {
const shardParts = shardOption.split('/');

if (shardsParts.length !== 2) {
if (shardParts.length !== 2) {
process.exitCode = kGenericUserError;

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

const { 0: indexStr, 1: totalStr } = shardsParts;
const { 0: indexStr, 1: totalStr } = shardParts;

const index = NumberParseInt(indexStr);
const total = NumberParseInt(totalStr);
Expand All @@ -52,19 +52,19 @@ if (shardsOption) {
process.exitCode = kGenericUserError;

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

shards = {
shard = {
index,
total,
};
}

run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters, shards })
run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters, shard })
.once('test:fail', () => {
process.exitCode = kGenericUserError;
});
20 changes: 10 additions & 10 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -425,25 +425,25 @@ function run(options) {
options = kEmptyObject;
}
let { testNamePatterns } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup, shards } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup, shard } = options;

if (files != null) {
validateArray(files, 'options.files');
}
if (watch != null) {
validateBoolean(watch, 'options.watch');
}
if (shards != null) {
validateObject(shards, 'options.shards');
validateNumber(shards.total, 'options.shards.total', 1);
validateNumber(shards.index, 'options.shards.index');
if (shard != null) {
validateObject(shard, 'options.shard');
validateNumber(shard.total, 'options.shard.total', 1);
validateNumber(shard.index, 'options.shard.index');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Not used the max here as the error message is not really understandable because you don't understand what's the reason for the upper limit


if (shards.index <= 0 || shards.total < shards.index) {
throw new ERR_OUT_OF_RANGE('options.shards.index', `>= 1 && <= ${shards.total} ("options.shards.total")`, shards.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);
}

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.shards', watch, 'shards not supported with watch mode');
throw new ERR_INVALID_ARG_VALUE('options.shard', watch, 'shards not supported with watch mode');
}
}
if (setup != null) {
Expand All @@ -469,8 +469,8 @@ function run(options) {
const root = createTestTree({ concurrency, timeout, signal });
let testFiles = files ?? createTestFileList();

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

let postRun = () => root.postRun();
Expand Down
6 changes: 3 additions & 3 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -596,9 +596,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"run tests with 'only' option set",
&EnvironmentOptions::test_only,
kAllowedInEnvvar);
AddOption("--shards",
"run test specific shard",
&EnvironmentOptions::test_shards,
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);
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +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_shards;
std::string test_shard;
bool throw_deprecation = false;
bool trace_atomics_wait = false;
bool trace_deprecation = false;
Expand Down
26 changes: 13 additions & 13 deletions test/parallel/test-runner-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,46 +199,46 @@ const testFixtures = fixtures.path('test-runner');
}

{
// --shards option validation
const args = ['--test', '--shards=1', join(testFixtures, 'index.js')];
// --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 '--shards' must be in the form of <index>\/<total>\. Received '1'/);
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, '');
}

{
// --shards option validation
const args = ['--test', '--shards=1/2/3', join(testFixtures, 'index.js')];
// --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 '--shards' must be in the form of <index>\/<total>\. Received '1\/2\/3'/);
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, '');
}

{
// --shards option validation
const args = ['--test', '--shards=hello', join(testFixtures, 'index.js')];
// --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 '--shards' must be in the form of <index>\/<total>\. Received 'hello'/);
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, '');
}

{
// --shards option, first shard
// --test-shard option, first shard
const args = [
'--test',
'--shards=1/2',
'--test-shard=1/2',
join(testFixtures, 'shards/*.cjs'),
];
const child = spawnSync(process.execPath, args);
Expand Down Expand Up @@ -269,10 +269,10 @@ const testFixtures = fixtures.path('test-runner');
}

{
// --shards option, last shard
// --test-shard option, last shard
const args = [
'--test',
'--shards=2/2',
'--test-shard=2/2',
join(testFixtures, 'shards/*.cjs'),
];
const child = spawnSync(process.execPath, args);
Expand Down
42 changes: 21 additions & 21 deletions test/parallel/test-runner-run.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -159,91 +159,91 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
].map((file) => join(shardsTestsFixtures, file));

describe('validation', () => {
it('should require shards.total when having shards option', () => {
assert.throws(() => run({ files: shardsTestsFiles, shards: {} }), {
it('should require shard.total when having shard option', () => {
assert.throws(() => run({ files: shardsTestsFiles, shard: {} }), {
name: 'TypeError',
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "options.shards.total" property must be of type number. Received undefined'
message: 'The "options.shard.total" property must be of type number. Received undefined'
});
});

it('should require shards.index when having shards option', () => {
it('should require shard.index when having shards option', () => {
assert.throws(() => run({
files: shardsTestsFiles,
shards: {
shard: {
total: 5
}
}), {
name: 'TypeError',
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "options.shards.index" property must be of type number. Received undefined'
message: 'The "options.shard.index" property must be of type number. Received undefined'
});
});

it('should require shards.total to be greater than 0 when having shards option', () => {
it('should require shard.total to be greater than 0 when having shard option', () => {
assert.throws(() => run({
files: shardsTestsFiles,
shards: {
shard: {
total: 0,
index: 1
}
}), {
name: 'RangeError',
code: 'ERR_OUT_OF_RANGE',
message: 'The value of "options.shards.total" is out of range. It must be >= 1. Received 0'
message: 'The value of "options.shard.total" is out of range. It must be >= 1. Received 0'
});
});

it('should require shards.index to be greater than 0 when having shards option', () => {
it('should require shard.index to be greater than 0 when having shard option', () => {
assert.throws(() => run({
files: shardsTestsFiles,
shards: {
shard: {
total: 6,
index: 0
}
}), {
name: 'RangeError',
code: 'ERR_OUT_OF_RANGE',
// eslint-disable-next-line max-len
message: 'The value of "options.shards.index" is out of range. It must be >= 1 && <= 6 ("options.shards.total"). Received 0'
message: 'The value of "options.shard.index" is out of range. It must be >= 1 && <= 6 ("options.shard.total"). Received 0'
});
});

it('should require shards.index to not be greater than the shards total when having shards option', () => {
it('should require shard.index to not be greater than the shards total when having shard option', () => {
assert.throws(() => run({
files: shardsTestsFiles,
shards: {
shard: {
total: 6,
index: 7
}
}), {
name: 'RangeError',
code: 'ERR_OUT_OF_RANGE',
// eslint-disable-next-line max-len
message: 'The value of "options.shards.index" is out of range. It must be >= 1 && <= 6 ("options.shards.total"). Received 7'
message: 'The value of "options.shard.index" is out of range. It must be >= 1 && <= 6 ("options.shard.total"). Received 7'
});
});

it('should require watch mode to be disabled when having shards option', () => {
it('should require watch mode to be disabled when having shard option', () => {
assert.throws(() => run({
files: shardsTestsFiles,
watch: true,
shards: {
shard: {
total: 6,
index: 1
}
}), {
name: 'TypeError',
code: 'ERR_INVALID_ARG_VALUE',
message: 'The property \'options.shards\' shards not supported with watch mode. Received true'
message: 'The property \'options.shard\' shards not supported with watch mode. Received true'
});
});
});

it('should run only the tests files matching the shard index', async () => {
const stream = run({
files: shardsTestsFiles,
shards: {
shard: {
total: 5,
index: 1
}
Expand Down Expand Up @@ -271,7 +271,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
for (let i = 1; i <= shards; i++) {
const stream = run({
files: shardsTestsFiles,
shards: {
shard: {
total: shards,
index: i
}
Expand Down Expand Up @@ -299,7 +299,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
for (let i = 1; i <= shards; i++) {
const stream = run({
files: shardsTestsFiles,
shards: {
shard: {
total: shards,
index: i
}
Expand Down