Skip to content

test_runner: fix suite rerun edge case #62860

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.

Sign up for GitHub

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

Open
MoLow wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −23
Open

test_runner: fix suite rerun edge case #62860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion lib/internal/test_runner/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,13 @@ class Test extends AsyncResource {
loc: [child.line, child.column, child.file],
}, noop);
t.endTime = t.startTime = hrtime();
t.start();
// For suites, Suite.run() starts the subtests via SafePromiseAll.
// Starting them here as well would run them twice, re-invoking the
// synthetic children-creator against a now-incremented disambiguator
// and producing spurious failures.
if (this.reportedType !== 'suite') {
t.start();
}
}
};
}
Expand Down
19 changes: 19 additions & 0 deletions test/fixtures/test-runner/rerun.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,22 @@ describe('describe rerun', { timeout: 1000, concurrency: 1000 }, () => {
});
test('a');
});


// Shared helper creates subtests at the same source location each time it's
// called, producing ambiguous test identifiers (disambiguated with ":(N)"
// suffixes in the rerun state file). Regression coverage for a bug where the
// suite's synthetic rerun fn double-started its direct children, which then
// re-invoked the synthetic descendant-creator against an already-incremented
// disambiguator map and emitted spurious failures.
function ambiguousHelper(t) {
return Promise.all([
t.test('shared sub A', () => {}),
t.test('shared sub B', () => {}),
]);
}

describe('rerun with ambiguous shared helper', { timeout: 1000, concurrency: 1000 }, () => {
test('first caller', (t) => ambiguousHelper(t));
test('second caller', (t) => ambiguousHelper(t));
});
63 changes: 41 additions & 22 deletions test/parallel/test-runner-test-rerun-failures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ const expectedStateFile = [
'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
'test/fixtures/test-runner/rerun.js:47:3': { passed_on_attempt: 0, name: 'a' },
'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
'test/fixtures/test-runner/rerun.js:59:7': { passed_on_attempt: 0, name: 'shared sub A' },
'test/fixtures/test-runner/rerun.js:60:7': { passed_on_attempt: 0, name: 'shared sub B' },
'test/fixtures/test-runner/rerun.js:65:3': { passed_on_attempt: 0, name: 'first caller' },
'test/fixtures/test-runner/rerun.js:59:7:(1)': { passed_on_attempt: 0, name: 'shared sub A' },
'test/fixtures/test-runner/rerun.js:60:7:(1)': { passed_on_attempt: 0, name: 'shared sub B' },
'test/fixtures/test-runner/rerun.js:66:3': { passed_on_attempt: 0, name: 'second caller' },
'test/fixtures/test-runner/rerun.js:64:1': { passed_on_attempt: 0, name: 'rerun with ambiguous shared helper' },
},
{
'test/fixtures/test-runner/rerun.js:9:1': { passed_on_attempt: 0, name: 'ok' },
Expand All @@ -38,11 +45,17 @@ const expectedStateFile = [
'test/fixtures/test-runner/rerun.js:39:1': { passed_on_attempt: 0, name: 'nested ambiguous (expectedAttempts=0)' },
'test/fixtures/test-runner/rerun.js:30:16:(1)': { passed_on_attempt: 0, name: '2 levels deep' },
'test/fixtures/test-runner/rerun.js:35:13:(1)': { passed_on_attempt: 0, name: 'ok' },
'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
'test/fixtures/test-runner/rerun.js:45:13': { passed_on_attempt: 0, name: 'nested' },
'test/fixtures/test-runner/rerun.js:45:13:(1)': { passed_on_attempt: 1, name: 'nested' },
'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
'test/fixtures/test-runner/rerun.js:47:3': { passed_on_attempt: 0, name: 'a' },
'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
'test/fixtures/test-runner/rerun.js:59:7': { passed_on_attempt: 0, name: 'shared sub A' },
'test/fixtures/test-runner/rerun.js:60:7': { passed_on_attempt: 0, name: 'shared sub B' },
'test/fixtures/test-runner/rerun.js:65:3': { passed_on_attempt: 0, name: 'first caller' },
'test/fixtures/test-runner/rerun.js:59:7:(1)': { passed_on_attempt: 0, name: 'shared sub A' },
'test/fixtures/test-runner/rerun.js:60:7:(1)': { passed_on_attempt: 0, name: 'shared sub B' },
'test/fixtures/test-runner/rerun.js:66:3': { passed_on_attempt: 0, name: 'second caller' },
'test/fixtures/test-runner/rerun.js:64:1': { passed_on_attempt: 0, name: 'rerun with ambiguous shared helper' },
},
{
'test/fixtures/test-runner/rerun.js:3:1': { passed_on_attempt: 2, name: 'should fail on first two attempts' },
Expand All @@ -53,15 +66,21 @@ const expectedStateFile = [
'test/fixtures/test-runner/rerun.js:29:13': { passed_on_attempt: 0, name: 'nested' },
'test/fixtures/test-runner/rerun.js:35:13': { passed_on_attempt: 0, name: 'ok' },
'test/fixtures/test-runner/rerun.js:39:1': { passed_on_attempt: 0, name: 'nested ambiguous (expectedAttempts=0)' },
'test/fixtures/test-runner/rerun.js:29:13:(1)': { passed_on_attempt: 2, name: 'nested' },
'test/fixtures/test-runner/rerun.js:30:16:(1)': { passed_on_attempt: 0, name: '2 levels deep' },
'test/fixtures/test-runner/rerun.js:29:13:(1)': { passed_on_attempt: 2, name: 'nested' },
'test/fixtures/test-runner/rerun.js:35:13:(1)': { passed_on_attempt: 0, name: 'ok' },
'test/fixtures/test-runner/rerun.js:40:1': { passed_on_attempt: 2, name: 'nested ambiguous (expectedAttempts=1)' },
'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
'test/fixtures/test-runner/rerun.js:45:13': { passed_on_attempt: 0, name: 'nested' },
'test/fixtures/test-runner/rerun.js:45:13:(1)': { passed_on_attempt: 1, name: 'nested' },
'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
'test/fixtures/test-runner/rerun.js:47:3': { passed_on_attempt: 0, name: 'a' },
'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
'test/fixtures/test-runner/rerun.js:59:7': { passed_on_attempt: 0, name: 'shared sub A' },
'test/fixtures/test-runner/rerun.js:60:7': { passed_on_attempt: 0, name: 'shared sub B' },
'test/fixtures/test-runner/rerun.js:65:3': { passed_on_attempt: 0, name: 'first caller' },
'test/fixtures/test-runner/rerun.js:59:7:(1)': { passed_on_attempt: 0, name: 'shared sub A' },
'test/fixtures/test-runner/rerun.js:60:7:(1)': { passed_on_attempt: 0, name: 'shared sub B' },
'test/fixtures/test-runner/rerun.js:66:3': { passed_on_attempt: 0, name: 'second caller' },
'test/fixtures/test-runner/rerun.js:64:1': { passed_on_attempt: 0, name: 'rerun with ambiguous shared helper' },
},
];

Expand All @@ -81,26 +100,26 @@ test('test should pass on third rerun', async () => {
let { code, stdout, signal } = await common.spawnPromisified(process.execPath, args);
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 11/);
assert.match(stdout, /pass 17/);
assert.match(stdout, /fail 4/);
assert.match(stdout, /suites 1/);
assert.match(stdout, /suites 2/);
assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 1));

({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 13/);
assert.match(stdout, /pass 18/);
assert.match(stdout, /fail 3/);
assert.match(stdout, /suites 1/);
assert.match(stdout, /suites 2/);
assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 2));


({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 18/);
assert.match(stdout, /pass 21/);
assert.match(stdout, /fail 0/);
assert.match(stdout, /suites 1/);
assert.match(stdout, /suites 2/);
assert.deepStrictEqual(await getStateFile(), expectedStateFile);
});

Expand All @@ -110,32 +129,32 @@ test('test should pass on third rerun with `--test`', async () => {
let { code, stdout, signal } = await common.spawnPromisified(process.execPath, args);
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 11/);
assert.match(stdout, /pass 17/);
assert.match(stdout, /fail 4/);
assert.match(stdout, /suites 1/);
assert.match(stdout, /suites 2/);
assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 1));

({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 13/);
assert.match(stdout, /pass 18/);
assert.match(stdout, /fail 3/);
assert.match(stdout, /suites 1/);
assert.match(stdout, /suites 2/);
assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 2));


({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 18/);
assert.match(stdout, /pass 21/);
assert.match(stdout, /fail 0/);
assert.match(stdout, /suites 1/);
assert.match(stdout, /suites 2/);
assert.deepStrictEqual(await getStateFile(), expectedStateFile);
});

test('using `run` api', async () => {
let stream = run({ files: [fixture], rerunFailuresFilePath: stateFile });
stream.on('test:pass', common.mustCall(12));
stream.on('test:pass', common.mustCall(19));
stream.on('test:fail', common.mustCall(4));

// eslint-disable-next-line no-unused-vars
Expand All @@ -145,7 +164,7 @@ test('using `run` api', async () => {


stream = run({ files: [fixture], rerunFailuresFilePath: stateFile });
stream.on('test:pass', common.mustCall(14));
stream.on('test:pass', common.mustCall(20));
stream.on('test:fail', common.mustCall(3));

// eslint-disable-next-line no-unused-vars
Expand All @@ -155,7 +174,7 @@ test('using `run` api', async () => {


stream = run({ files: [fixture], rerunFailuresFilePath: stateFile });
stream.on('test:pass', common.mustCall(19));
stream.on('test:pass', common.mustCall(23));
stream.on('test:fail', common.mustNotCall());

// eslint-disable-next-line no-unused-vars
Expand Down
Loading