From 091d93f7ebd35ed38537775e9114bcdc50c7ed5a Mon Sep 17 00:00:00 2001 From: atlowChemi Date: Tue, 19 May 2026 14:34:34 +0300 Subject: [PATCH 1/2] 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: https://github.com/nodejs/node/issues/63424 Signed-off-by: atlowChemi --- lib/internal/test_runner/reporter/rerun.js | 29 +++---- lib/internal/test_runner/test.js | 9 ++- .../rerun-shared-helper-swallows-failure.mjs | 51 +++++++++++++ .../test-runner-test-rerun-failures.js | 76 +++++++++++++++++++ 4 files changed, 148 insertions(+), 17 deletions(-) create mode 100644 test/fixtures/test-runner/rerun-shared-helper-swallows-failure.mjs diff --git a/lib/internal/test_runner/reporter/rerun.js b/lib/internal/test_runner/reporter/rerun.js index 9658a7fb70ff0b..0192547e4e25b9 100644 --- a/lib/internal/test_runner/reporter/rerun.js +++ b/lib/internal/test_runner/reporter/rerun.js @@ -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, - }; } } diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index eb888905798bcd..135d2d7b0ced0b 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -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]; diff --git a/test/fixtures/test-runner/rerun-shared-helper-swallows-failure.mjs b/test/fixtures/test-runner/rerun-shared-helper-swallows-failure.mjs new file mode 100644 index 00000000000000..22eaaa321c01b5 --- /dev/null +++ b/test/fixtures/test-runner/rerun-shared-helper-swallows-failure.mjs @@ -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')); +}); diff --git a/test/parallel/test-runner-test-rerun-failures.js b/test/parallel/test-runner-test-rerun-failures.js index 585d5f25f04a59..41f6dd79f1dcdf 100644 --- a/test/parallel/test-runner-test-rerun-failures.js +++ b/test/parallel/test-runner-test-rerun-failures.js @@ -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'); @@ -152,6 +153,81 @@ 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]; + + const fixtureKey = relative(process.cwd(), fixturePath); + 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)); From b9746280ee165d0138cffe87cf7bd97348281fb3 Mon Sep 17 00:00:00 2001 From: atlowChemi Date: Tue, 19 May 2026 19:30:58 +0300 Subject: [PATCH 2/2] fixup! test_runner: fix --test-rerun-failures swallowing failures on retry --- test/parallel/test-runner-test-rerun-failures.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-runner-test-rerun-failures.js b/test/parallel/test-runner-test-rerun-failures.js index 41f6dd79f1dcdf..53f25c48061645 100644 --- a/test/parallel/test-runner-test-rerun-failures.js +++ b/test/parallel/test-runner-test-rerun-failures.js @@ -170,7 +170,8 @@ test('failing test is not swallowed when siblings share its source location', as const fixturePath = fixtures.path('test-runner', 'rerun-shared-helper-swallows-failure.mjs'); const args = ['--test-rerun-failures', stateFile, fixturePath]; - const fixtureKey = relative(process.cwd(), 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 = {