Skip to content
Open
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
29 changes: 16 additions & 13 deletions lib/internal/test_runner/reporter/rerun.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,24 @@ function reportReruns(previousRuns, globalOptions) {
}


if (type === 'test:pass') {
let identifier = getTestId(data);
if (disambiguator[identifier] !== undefined) {
identifier += `:(${disambiguator[identifier]})`;
disambiguator[identifier] += 1;
if (type === 'test:pass' || type === 'test:fail') {
const baseIdentifier = getTestId(data);
let identifier = baseIdentifier;
if (disambiguator[baseIdentifier] !== undefined) {
identifier += `:(${disambiguator[baseIdentifier]})`;
disambiguator[baseIdentifier] += 1;
} else {
disambiguator[identifier] = 1;
disambiguator[baseIdentifier] = 1;
}
if (type === 'test:pass') {
const children = ArrayPrototypeMap(currentTest.children, (child) => child.data);
obj[identifier] = {
__proto__: null,
name: data.name,
children,
passed_on_attempt: data.details.passed_on_attempt ?? data.details.attempt,
};
}
const children = ArrayPrototypeMap(currentTest.children, (child) => child.data);
obj[identifier] = {
__proto__: null,
name: data.name,
children,
passed_on_attempt: data.details.passed_on_attempt ?? data.details.attempt,
};
}
}

Expand Down
9 changes: 5 additions & 4 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -791,13 +791,14 @@ class Test extends AsyncResource {
}

if (this.loc != null && this.root.harness.previousRuns != null) {
let testIdentifier = `${relative(this.config.cwd, this.loc.file)}:${this.loc.line}:${this.loc.column}`;
const disambiguator = this.root.testDisambiguator.get(testIdentifier);
const baseIdentifier = `${relative(this.config.cwd, this.loc.file)}:${this.loc.line}:${this.loc.column}`;
let testIdentifier = baseIdentifier;
const disambiguator = this.root.testDisambiguator.get(baseIdentifier);
if (disambiguator !== undefined) {
testIdentifier += `:(${disambiguator})`;
this.root.testDisambiguator.set(testIdentifier, disambiguator + 1);
this.root.testDisambiguator.set(baseIdentifier, disambiguator + 1);
} else {
this.root.testDisambiguator.set(testIdentifier, 1);
this.root.testDisambiguator.set(baseIdentifier, 1);
}
this.attempt = this.root.harness.previousRuns.length;
const previousAttempt = this.root.harness.previousRuns[this.attempt - 1]?.[testIdentifier];
Expand Down
51 changes: 51 additions & 0 deletions test/fixtures/test-runner/rerun-shared-helper-swallows-failure.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Regression coverage for https://github.com/nodejs/node/issues/63424:
// `--test-rerun-failures` could mark a failing test as passing on retry
// without executing its body.
//
// The runtime disambiguator at lib/internal/test_runner/test.js keys tests
// by `file:line:column`. A `t.test()` registered inside a factory function
// gets the same source location regardless of which parent invoked the
// factory. Three independent bugs interacted to make the failure-on-retry
// vanish:
// 1. Runner counter was set against the suffixed key, so it never
// advanced past 1 - every 3rd+ same-loc registration collided on :(1).
// 2. Reporter had the same off-by-one against the suffixed key.
// 3. Reporter only bumped its counter on test:pass, so failing tests at
// a shared location desynchronised the writer and runner counters.
// On retry, the failing sibling could inherit a counter slot that, in
// attempt 0, belonged to a different (passing) sibling. Node matched by
// that slot, replaced `this.fn` with a synthetic noop replay, and reported
// the failure as a pass.

import { describe, it } from 'node:test';
import { strict as assert } from 'node:assert';

function makeSuite(shouldPass, label) {
return async (t) => {
await t.test('inner', async () => {
if (!shouldPass) assert.fail(`${label} should fail`);
});
};
}

// Four siblings with the failure in the middle, placed first so that the
// passing sibling at global position 2 (E) ends up recorded at base:(1).
// With the runner-side off-by-one (bug 1), the buggy counter on retry would
// alias F's id onto base:(1), match F against E's recorded "passed" entry,
// replace F's assert.fail with a synthetic noop, and swallow F.
describe('parents (middle failure)', { concurrency: false }, () => {
it('D passes', makeSuite(true, 'D'));
it('E passes', makeSuite(true, 'E'));
it('F fails', makeSuite(false, 'F')); // the only real failure in this group
it('G passes', makeSuite(true, 'G'));
});

// Three-sibling case from the issue, kept verbatim to exercise the writer
// bugs (off-by-one storing the counter against the suffixed key; reporter
// ignoring test:fail when bumping the counter). Either bug shifts C's
// recorded slot from base:(6) to a position B would inherit on retry.
describe('parents', { concurrency: false }, () => {
it('A passes', makeSuite(true, 'A'));
it('B fails', makeSuite(false, 'B')); // the only real failure in this group
it('C passes', makeSuite(true, 'C'));
});
77 changes: 77 additions & 0 deletions test/parallel/test-runner-test-rerun-failures.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const common = require('../common');
const fixtures = require('../common/fixtures');
const assert = require('node:assert');
const { rm, readFile } = require('node:fs/promises');
const { relative } = require('node:path');
const { setTimeout } = require('node:timers/promises');
const { test, beforeEach, afterEach, run } = require('node:test');

Expand Down Expand Up @@ -152,6 +153,82 @@ test('test should pass on third rerun with `--test`', async () => {
assert.deepStrictEqual(await getStateFile(), expectedStateFile);
});

test('failing test is not swallowed when siblings share its source location', async () => {
// Regression coverage for https://github.com/nodejs/node/issues/63424.
//
// The fixture runs the 4-sibling group (D, E pass; F fails; G passes)
// before the 3-sibling group from the issue (A pass, B fails, C pass) so
// that the passing sibling at global position 2 (E) ends up recorded at
// base:(1). With the runner-side off-by-one reintroduced, the buggy
// counter on retry collides every sibling after the first onto base:(1)
// and matches the failing F (and B) against E's "passed" entry, replacing
// their assert.fail with a synthetic noop and reporting exit 0.
//
// The state-file shape additionally pins down the writer-side bugs: the
// writer off-by-one or its prior pass-only counting both shift the
// recorded slots away from the expected base:(N) layout.
const fixturePath = fixtures.path('test-runner', 'rerun-shared-helper-swallows-failure.mjs');
const args = ['--test-rerun-failures', stateFile, fixturePath];

// getStateFile() normalises backslashes to '/'; match that on Windows.
const fixtureKey = relative(process.cwd(), fixturePath).replaceAll('\\', '/');
const innerLoc = `${fixtureKey}:25:13`;
const passingInner = { passed_on_attempt: 0, name: 'inner' };
const expectedInnerSlots = {
[innerLoc]: passingInner, // D
[`${innerLoc}:(1)`]: passingInner, // E - fills the slot a buggy runner aliases F/B onto
[`${innerLoc}:(3)`]: passingInner, // G - :(2) reserved for F's failure
[`${innerLoc}:(4)`]: passingInner, // A
[`${innerLoc}:(6)`]: passingInner, // C - :(5) reserved for B's failure
};
const passingParents = {
[`${fixtureKey}:37:3`]: 'D passes',
[`${fixtureKey}:38:3`]: 'E passes',
[`${fixtureKey}:40:3`]: 'G passes',
[`${fixtureKey}:48:3`]: 'A passes',
[`${fixtureKey}:50:3`]: 'C passes',
};
const failingParentSlots = [
`${fixtureKey}:39:3`, // F fails
`${fixtureKey}:49:3`, // B fails
];
const failingInnerSlots = [`${innerLoc}:(2)`, `${innerLoc}:(5)`];

function assertAttempt(state, attemptIndex) {
for (const [key, expected] of Object.entries(expectedInnerSlots)) {
assert.deepStrictEqual(state[attemptIndex][key], expected, `attempt ${attemptIndex} missing or wrong entry for ${key}`);
}
for (const [key, name] of Object.entries(passingParents)) {
assert.strictEqual(state[attemptIndex][key]?.name, name, `attempt ${attemptIndex} missing parent ${name} at ${key}`);
assert.strictEqual(state[attemptIndex][key]?.passed_on_attempt, 0);
}
for (const key of failingInnerSlots) {
assert.strictEqual(state[attemptIndex][key], undefined, `attempt ${attemptIndex} should not record failing inner at ${key}`);
}
for (const key of failingParentSlots) {
assert.strictEqual(state[attemptIndex][key], undefined, `attempt ${attemptIndex} should not record failing parent at ${key}`);
}
}

// Attempt 0: F and B fail.
let { code, signal } = await common.spawnPromisified(process.execPath, args);
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
let state = await getStateFile();
assert.strictEqual(state.length, 1);
assertAttempt(state, 0);

// Attempt 1: F and B must STILL fail. Before the fix this run reported
// exit 0 because the failing tests were matched to passing siblings'
// previous-attempt slots and replaced with synthetic noops.
({ code, signal } = await common.spawnPromisified(process.execPath, args));
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
state = await getStateFile();
assert.strictEqual(state.length, 2);
assertAttempt(state, 1);
});

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