Skip to content

Commit a1f0af0

Browse files
committed
fix(observability-map): make the corpus harness cover what it claims
CodeRabbit round on #4455, four findings in the mutation corpus. The baseline scan ran in the describe callback body, which Vitest executes during collection, where the suite's timeout option does not apply and a throw has no test name to attach to. It now runs in beforeAll with its own timeout. Collection of the enabled file drops from 9.7s to 3.7s, and the full corpus passes in 282s with no --testTimeout flag. readTree filtered with an inline copy of the scanner's file predicate, so the corpus could materialize files scanDirectory never reads and still count them towards the anti-vacuity thresholds. It uses the exported isScannableFile now. No behaviour change today: the two predicates were identical. The additive-coverage assertion reads no route tree and cost nothing, so gating it behind OBS_MAP_MUTATION_CORPUS only hid a stale list from the run people actually do. Moved next to the registry assertion. merge-comma-expressions is labelled preserving but would have merged a directive prologue into 'use client', foo(), which is no longer a directive. No route has that shape today; the guard and its test are there so the label stays true.
1 parent 5158846 commit a1f0af0

3 files changed

Lines changed: 44 additions & 13 deletions

File tree

internal-packages/observability-map/src/mutationCorpus.test.ts

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { existsSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs";
22
import { tmpdir } from "node:os";
33
import { dirname, join, resolve } from "node:path";
4-
import { scanDirectory } from "./scan.js";
4+
import { isScannableFile, scanDirectory } from "./scan.js";
55
import { buildReport } from "./score.js";
66
import { ADDITIVE_IDS, MUTATIONS, type Mutation } from "./mutations.js";
77
import { CHECKS } from "./checks/index.js";
@@ -72,7 +72,9 @@ function readTree(dir: string): SourceFile[] {
7272
}
7373
continue;
7474
}
75-
if (!entry.isFile() || !/\.tsx?$/.test(entry.name) || entry.name.endsWith(".d.ts")) continue;
75+
// `scanDirectory`'s own predicate rather than a copy of it. A copy that drifts lets a mutation
76+
// report files and sites the scanner never read, which is what the thresholds below are for.
77+
if (!entry.isFile() || !isScannableFile(entry.name)) continue;
7678
take(join(dir, entry.name), entry.name);
7779
}
7880
return files;
@@ -218,6 +220,30 @@ describe("the corpus keeps up with the check registry", () => {
218220
);
219221
expect(missing).toEqual([]);
220222
});
223+
224+
// Ungated for the same reason as the sweep assertion above: it reads no route tree and costs
225+
// nothing, so gating it would only hide a stale list from the run people actually do.
226+
it("covers the additive direction, not only the subtractive one", () => {
227+
// Every corpus entry once removed or restructured real signal, and none added fake signal. The
228+
// two largest holes ever found here lived in that blind spot, so the class is asserted rather
229+
// than left to whoever edits the list next.
230+
const ids = new Set(MUTATIONS.map((m) => m.id));
231+
expect(ADDITIVE_IDS.filter((id) => !ids.has(id))).toEqual([]);
232+
expect(ADDITIVE_IDS.length).toBeGreaterThanOrEqual(8);
233+
});
234+
});
235+
236+
/**
237+
* Ungated, because a `preserving` entry that changes behaviour is a false negative in the property
238+
* the whole file exists to argue, and the shape is cheaper to state on a two-line fixture than to
239+
* wait for a route to grow it.
240+
*/
241+
describe("a preserving mutation preserves what the route does", () => {
242+
it("leaves a directive prologue alone when merging comma expressions", () => {
243+
const merge = MUTATIONS.find((m) => m.id === "merge-comma-expressions")!;
244+
const result = merge.apply("api.v1.a.tsx", '"use client";\nfoo();\nbar();\n');
245+
expect(result?.source ?? "").not.toContain('"use client",');
246+
});
221247
});
222248

223249
const describeCorpus = ENABLED && existsSync(ROUTES) ? describe : describe.skip;
@@ -231,17 +257,17 @@ const describeCorpus = ENABLED && existsSync(ROUTES) ? describe : describe.skip;
231257
const ENTRY_TIMEOUT_MS = 120_000;
232258

233259
describeCorpus("mutation corpus over the real route tree", { timeout: ENTRY_TIMEOUT_MS }, () => {
234-
const files = ENABLED && existsSync(ROUTES) ? readTree(ROUTES) : [];
235-
const baseline = ENABLED && existsSync(ROUTES) ? measure(files) : null;
260+
let files: SourceFile[] = [];
261+
let baseline: Measurement | null = null;
236262

237-
it("covers the additive direction, not only the subtractive one", () => {
238-
// Every corpus entry once removed or restructured real signal, and none added fake signal. The
239-
// two largest holes ever found here lived in that blind spot, so the class is asserted rather
240-
// than left to whoever edits the list next.
241-
const ids = new Set(MUTATIONS.map((m) => m.id));
242-
expect(ADDITIVE_IDS.filter((id) => !ids.has(id))).toEqual([]);
243-
expect(ADDITIVE_IDS.length).toBeGreaterThanOrEqual(8);
244-
});
263+
// In a hook rather than the suite body, which Vitest runs during collection where no test timeout
264+
// applies and a throw has no test name to attach to. The baseline is the single most expensive
265+
// step in the file, so it is the one that must be inside something that can be timed out and
266+
// reported. `beforeAll` takes its own timeout.
267+
beforeAll(() => {
268+
files = readTree(ROUTES);
269+
baseline = measure(files);
270+
}, ENTRY_TIMEOUT_MS);
245271

246272
it("has a baseline worth mutating", () => {
247273
expect(baseline).not.toBeNull();

internal-packages/observability-map/src/mutations.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -821,6 +821,9 @@ export const MUTATIONS: Mutation[] = [
821821
const previous = statements[i - 1]!;
822822
const current = statements[i]!;
823823
if (!ts.isExpressionStatement(previous) || !ts.isExpressionStatement(current)) continue;
824+
// A directive prologue is an ExpressionStatement, and `"use client", foo();` is no longer
825+
// a directive. That is a behaviour change, which this entry claims not to make.
826+
if (ts.isStringLiteral(previous.expression)) continue;
824827
if (source[previous.end - 1] !== ";") continue;
825828
edits.push({ start: previous.end - 1, end: current.getStart(), text: ", " });
826829
}

internal-packages/observability-map/src/scan.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1358,7 +1358,9 @@ export function scanFile(fileName: string, source: string): EntryPoint | null {
13581358

13591359
const SOURCE_FILE = /\.tsx?$/;
13601360

1361-
function isScannableFile(fileName: string): boolean {
1361+
/** Exported so `mutationCorpus.test.ts` materializes exactly the files `scanDirectory` reads. Its
1362+
* anti-vacuity thresholds count files and sites the scanner never saw if the two predicates drift. */
1363+
export function isScannableFile(fileName: string): boolean {
13621364
return SOURCE_FILE.test(fileName) && !fileName.endsWith(".d.ts");
13631365
}
13641366

0 commit comments

Comments
 (0)