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>

Author: atlowChemi

lib/internal/test_runner/reporter/rerun.js

@@ -48,22 +48,25 @@ 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,
+            duration_ms: data.details.duration_ms,
+          };
         }
-        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,
-          duration_ms: data.details.duration_ms,
-        };
       }
     }
 

lib/internal/test_runner/test.js

@@ -793,13 +793,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];

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'));
+});

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

@@ -5,6 +5,7 @@ const tmpdir = require('../common/tmpdir');
 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');
 
@@ -155,6 +156,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('rerun preserves the original duration on the replayed pass', async () => {
   const durationFixture = fixtures.path('test-runner', 'rerun-duration.js');
   const args = ['--test-rerun-failures', stateFile, durationFixture];