Visitar URL original
test_runner: fix --test-rerun-failures swallowing failures on retry · nodejs/node@d257eec · GitHub
Skip to content

Commit d257eec

Browse files
atlowChemisxa
atlowChemi
authored and
sxa
committed
test_runner: fix --test-rerun-failures swallowing failures on retry
Three independent bugs interacted to let a real failure-on-retry be reported as a pass: 1. The runner's disambiguator stored the counter against the suffixed identifier (after mutation) instead of the base key, so the counter never advanced past 1 and every 3rd+ same-loc registration collided on :(1). 2. The reporter had the same off-by-one when writing the state file. 3. The reporter only bumped its counter on `test:pass`, so any failing test at a shared source location desynchronised the writer and runner counters - on retry, the surviving failing sibling would inherit a slot that in the previous attempt 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. Track the base identifier separately in the runner, bump the counter against the base key in both the runner and the reporter, and bump the reporter's counter on `test:fail` in addition to `test:pass`. Fixes: #63424 Signed-off-by: atlowChemi <chemi@atlow.co.il> PR-URL: #63431 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
1 parent 388323c commit d257eec

4 files changed

Lines changed: 150 additions & 18 deletions

File tree

‎lib/internal/test_runner/reporter/rerun.js‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -48,22 +48,25 @@ function reportReruns(previousRuns, globalOptions) {
4848
}
4949

5050

51-
if (type === 'test:pass') {
52-
let identifier = getTestId(data);
53-
if (disambiguator[identifier] !== undefined) {
54-
identifier += `:(${disambiguator[identifier]})`;
55-
disambiguator[identifier] += 1;
51+
if (type === 'test:pass' || type === 'test:fail') {
52+
const baseIdentifier = getTestId(data);
53+
let identifier = baseIdentifier;
54+
if (disambiguator[baseIdentifier] !== undefined) {
55+
identifier += `:(${disambiguator[baseIdentifier]})`;
56+
disambiguator[baseIdentifier] += 1;
5657
} else {
57-
disambiguator[identifier] = 1;
58+
disambiguator[baseIdentifier] = 1;
59+
}
60+
if (type === 'test:pass') {
61+
const children = ArrayPrototypeMap(currentTest.children, (child) => child.data);
62+
obj[identifier] = {
63+
__proto__: null,
64+
name: data.name,
65+
children,
66+
passed_on_attempt: data.details.passed_on_attempt ?? data.details.attempt,
67+
duration_ms: data.details.duration_ms,
68+
};
5869
}
59-
const children = ArrayPrototypeMap(currentTest.children, (child) => child.data);
60-
obj[identifier] = {
61-
__proto__: null,
62-
name: data.name,
63-
children,
64-
passed_on_attempt: data.details.passed_on_attempt ?? data.details.attempt,
65-
duration_ms: data.details.duration_ms,
66-
};
6770
}
6871
}
6972

‎lib/internal/test_runner/test.js‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -759,13 +759,14 @@ class Test extends AsyncResource {
759759
}
760760

761761
if (this.loc != null && this.root.harness.previousRuns != null) {
762-
let testIdentifier = `${relative(this.config.cwd, this.loc.file)}:${this.loc.line}:${this.loc.column}`;
763-
const disambiguator = this.root.testDisambiguator.get(testIdentifier);
762+
const baseIdentifier = `${relative(this.config.cwd, this.loc.file)}:${this.loc.line}:${this.loc.column}`;
763+
let testIdentifier = baseIdentifier;
764+
const disambiguator = this.root.testDisambiguator.get(baseIdentifier);
764765
if (disambiguator !== undefined) {
765766
testIdentifier += `:(${disambiguator})`;
766-
this.root.testDisambiguator.set(testIdentifier, disambiguator + 1);
767+
this.root.testDisambiguator.set(baseIdentifier, disambiguator + 1);
767768
} else {
768-
this.root.testDisambiguator.set(testIdentifier, 1);
769+
this.root.testDisambiguator.set(baseIdentifier, 1);
769770
}
770771
this.attempt = this.root.harness.previousRuns.length;
771772
const previousAttempt = this.root.harness.previousRuns[this.attempt - 1]?.[testIdentifier];

‎test/fixtures/test-runner/rerun-shared-helper-swallows-failure.mjs‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Regression coverage for https://github.com/nodejs/node/issues/63424:
2+
// `--test-rerun-failures` could mark a failing test as passing on retry
3+
// without executing its body.
4+
//
5+
// The runtime disambiguator at lib/internal/test_runner/test.js keys tests
6+
// by `file:line:column`. A `t.test()` registered inside a factory function
7+
// gets the same source location regardless of which parent invoked the
8+
// factory. Three independent bugs interacted to make the failure-on-retry
9+
// vanish:
10+
// 1. Runner counter was set against the suffixed key, so it never
11+
// advanced past 1 - every 3rd+ same-loc registration collided on :(1).
12+
// 2. Reporter had the same off-by-one against the suffixed key.
13+
// 3. Reporter only bumped its counter on test:pass, so failing tests at
14+
// a shared location desynchronised the writer and runner counters.
15+
// On retry, the failing sibling could inherit a counter slot that, in
16+
// attempt 0, belonged to a different (passing) sibling. Node matched by
17+
// that slot, replaced `this.fn` with a synthetic noop replay, and reported
18+
// the failure as a pass.
19+
20+
import { describe, it } from 'node:test';
21+
import { strict as assert } from 'node:assert';
22+
23+
function makeSuite(shouldPass, label) {
24+
return async (t) => {
25+
await t.test('inner', async () => {
26+
if (!shouldPass) assert.fail(`${label} should fail`);
27+
});
28+
};
29+
}
30+
31+
// Four siblings with the failure in the middle, placed first so that the
32+
// passing sibling at global position 2 (E) ends up recorded at base:(1).
33+
// With the runner-side off-by-one (bug 1), the buggy counter on retry would
34+
// alias F's id onto base:(1), match F against E's recorded "passed" entry,
35+
// replace F's assert.fail with a synthetic noop, and swallow F.
36+
describe('parents (middle failure)', { concurrency: false }, () => {
37+
it('D passes', makeSuite(true, 'D'));
38+
it('E passes', makeSuite(true, 'E'));
39+
it('F fails', makeSuite(false, 'F')); // the only real failure in this group
40+
it('G passes', makeSuite(true, 'G'));
41+
});
42+
43+
// Three-sibling case from the issue, kept verbatim to exercise the writer
44+
// bugs (off-by-one storing the counter against the suffixed key; reporter
45+
// ignoring test:fail when bumping the counter). Either bug shifts C's
46+
// recorded slot from base:(6) to a position B would inherit on retry.
47+
describe('parents', { concurrency: false }, () => {
48+
it('A passes', makeSuite(true, 'A'));
49+
it('B fails', makeSuite(false, 'B')); // the only real failure in this group
50+
it('C passes', makeSuite(true, 'C'));
51+
});

‎test/parallel/test-runner-test-rerun-failures.js‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const tmpdir = require('../common/tmpdir');
55
const fixtures = require('../common/fixtures');
66
const assert = require('node:assert');
77
const { rm, readFile } = require('node:fs/promises');
8+
const { relative } = require('node:path');
89
const { setTimeout } = require('node:timers/promises');
910
const { test, beforeEach, afterEach, run } = require('node:test');
1011

@@ -155,6 +156,82 @@ test('test should pass on third rerun with `--test`', async () => {
155156
assert.deepStrictEqual(await getStateFile(), expectedStateFile);
156157
});
157158

159+
test('failing test is not swallowed when siblings share its source location', async () => {
160+
// Regression coverage for https://github.com/nodejs/node/issues/63424.
161+
//
162+
// The fixture runs the 4-sibling group (D, E pass; F fails; G passes)
163+
// before the 3-sibling group from the issue (A pass, B fails, C pass) so
164+
// that the passing sibling at global position 2 (E) ends up recorded at
165+
// base:(1). With the runner-side off-by-one reintroduced, the buggy
166+
// counter on retry collides every sibling after the first onto base:(1)
167+
// and matches the failing F (and B) against E's "passed" entry, replacing
168+
// their assert.fail with a synthetic noop and reporting exit 0.
169+
//
170+
// The state-file shape additionally pins down the writer-side bugs: the
171+
// writer off-by-one or its prior pass-only counting both shift the
172+
// recorded slots away from the expected base:(N) layout.
173+
const fixturePath = fixtures.path('test-runner', 'rerun-shared-helper-swallows-failure.mjs');
174+
const args = ['--test-rerun-failures', stateFile, fixturePath];
175+
176+
// getStateFile() normalises backslashes to '/'; match that on Windows.
177+
const fixtureKey = relative(process.cwd(), fixturePath).replaceAll('\\', '/');
178+
const innerLoc = `${fixtureKey}:25:13`;
179+
const passingInner = { passed_on_attempt: 0, name: 'inner' };
180+
const expectedInnerSlots = {
181+
[innerLoc]: passingInner, // D
182+
[`${innerLoc}:(1)`]: passingInner, // E - fills the slot a buggy runner aliases F/B onto
183+
[`${innerLoc}:(3)`]: passingInner, // G - :(2) reserved for F's failure
184+
[`${innerLoc}:(4)`]: passingInner, // A
185+
[`${innerLoc}:(6)`]: passingInner, // C - :(5) reserved for B's failure
186+
};
187+
const passingParents = {
188+
[`${fixtureKey}:37:3`]: 'D passes',
189+
[`${fixtureKey}:38:3`]: 'E passes',
190+
[`${fixtureKey}:40:3`]: 'G passes',
191+
[`${fixtureKey}:48:3`]: 'A passes',
192+
[`${fixtureKey}:50:3`]: 'C passes',
193+
};
194+
const failingParentSlots = [
195+
`${fixtureKey}:39:3`, // F fails
196+
`${fixtureKey}:49:3`, // B fails
197+
];
198+
const failingInnerSlots = [`${innerLoc}:(2)`, `${innerLoc}:(5)`];
199+
200+
function assertAttempt(state, attemptIndex) {
201+
for (const [key, expected] of Object.entries(expectedInnerSlots)) {
202+
assert.deepStrictEqual(state[attemptIndex][key], expected, `attempt ${attemptIndex} missing or wrong entry for ${key}`);
203+
}
204+
for (const [key, name] of Object.entries(passingParents)) {
205+
assert.strictEqual(state[attemptIndex][key]?.name, name, `attempt ${attemptIndex} missing parent ${name} at ${key}`);
206+
assert.strictEqual(state[attemptIndex][key]?.passed_on_attempt, 0);
207+
}
208+
for (const key of failingInnerSlots) {
209+
assert.strictEqual(state[attemptIndex][key], undefined, `attempt ${attemptIndex} should not record failing inner at ${key}`);
210+
}
211+
for (const key of failingParentSlots) {
212+
assert.strictEqual(state[attemptIndex][key], undefined, `attempt ${attemptIndex} should not record failing parent at ${key}`);
213+
}
214+
}
215+
216+
// Attempt 0: F and B fail.
217+
let { code, signal } = await common.spawnPromisified(process.execPath, args);
218+
assert.strictEqual(code, 1);
219+
assert.strictEqual(signal, null);
220+
let state = await getStateFile();
221+
assert.strictEqual(state.length, 1);
222+
assertAttempt(state, 0);
223+
224+
// Attempt 1: F and B must STILL fail. Before the fix this run reported
225+
// exit 0 because the failing tests were matched to passing siblings'
226+
// previous-attempt slots and replaced with synthetic noops.
227+
({ code, signal } = await common.spawnPromisified(process.execPath, args));
228+
assert.strictEqual(code, 1);
229+
assert.strictEqual(signal, null);
230+
state = await getStateFile();
231+
assert.strictEqual(state.length, 2);
232+
assertAttempt(state, 1);
233+
});
234+
158235
test('rerun preserves the original duration on the replayed pass', async () => {
159236
const durationFixture = fixtures.path('test-runner', 'rerun-duration.js');
160237
const args = ['--test-rerun-failures', stateFile, durationFixture];

0 commit comments

Comments
 (0)