test_runner: fix --test-rerun-failures swallowing failures on retry by atlowChemi · Pull Request #63431 · nodejs/node · GitHub
Skip to content
Closed
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
31 changes: 17 additions & 14 deletions lib/internal/test_runner/reporter/rerun.js
9 changes: 5 additions & 4 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
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
Loading