Skip to content

Commit 4ea20b0

Browse files
committed
test(observability-map): size the real-tree timeouts for shard contention
The 30s and 60s per-test timeouts on the two real-tree tests were chosen on an idle machine, and the suite also runs inside unit-tests-internal.yml, which executes turbo run test --filter "@internal/*" as twelve concurrent shard processes on one runner. The 30s one does flake under that. Measured on an 8-core box. This file alone at load average 0.9: 6.3-6.4s for the scan, 10.8-11.2s for the sweep, both well above the 1.6-2.6s the old comment claimed. Two batches of twelve concurrent copies on those same 8 cores: 24.2-34.0s for the scan and 27.6-39.7s for the sweep, with one of the first twelve dying on "Test timed out in 30000ms". Twelve processes over 8 cores is 1.5 per core where the 32-vCPU runner is 0.375, so the reproduction is harsher than CI, which is why it is the thing to size against. Both now use one 120s constant, which is 3x the worst contended run measured. 60s was the other candidate and is not enough: the sweep already reached 39.7s. Neither test asserts anything about elapsed time, so the number is a hang detector rather than a performance budget, and the docstring says so. Reported by Devin on #4455.
1 parent 26c4059 commit 4ea20b0

1 file changed

Lines changed: 60 additions & 31 deletions

File tree

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

Lines changed: 60 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -326,41 +326,70 @@ describe("counting candidates independently of the scanner", () => {
326326
});
327327
});
328328

329+
/**
330+
* Timeout for the two real-tree tests, which do not fit the suite's 10s default: the first runs a
331+
* ts.Program per route file for the parse diagnostics and walks the tree a second time to count
332+
* candidates, the second scans the tree twice and re-scans every source with the suppression
333+
* directive prepended.
334+
*
335+
* It is a hang detector and nothing else. Neither test asserts anything about how long the scan
336+
* takes, so a number tight enough to be a performance budget would only be a way to fail on a busy
337+
* runner, and a performance budget that flakes gets the whole suite marked unreliable.
338+
*
339+
* The old 30s and 60s were chosen on an idle machine and the 30s one does flake. Measured on an
340+
* 8-core box, this file alone at load average 0.9: 6.3-6.4s for the scan, 10.8-11.2s for the sweep.
341+
* Twenty-four runs of it as two batches of twelve concurrent copies on those same 8 cores:
342+
* 24.2-34.0s for the scan and 27.6-39.7s for the sweep, with one of the first twelve dying on
343+
* "Test timed out in 30000ms". That contention is not hypothetical:
344+
* `.github/workflows/unit-tests-internal.yml` runs `turbo run test --filter "@internal/*"` as
345+
* twelve concurrent shard processes on one runner, and this file executes inside one of them.
346+
*
347+
* The local reproduction is deliberately harsher than CI, which is why it is the thing to size
348+
* against: twelve processes over 8 cores is 1.5 per core where the 32-vCPU runner is 0.375. 120s is
349+
* 3x the worst contended run measured and about 11x the idle sweep. 60s was the other candidate and
350+
* is not enough on those numbers: the sweep already reached 39.7s, which is 1.5x, and a margin that
351+
* thin on a machine nobody controls is how the 30s got here.
352+
*/
353+
const TREE_SCAN_TIMEOUT = 120_000;
354+
329355
describe("scanning the real webapp routes", () => {
330-
it("parses every route file and produces a report inside a wide band", () => {
331-
const { entryPoints, parseFailures } = scanDirectory(ROUTES);
332-
333-
expect(parseFailures).toEqual([]);
334-
expect(entryPoints.length).toBeGreaterThan(100);
335-
expect(entryPoints.length).toBeLessThan(countRouteModuleFiles(ROUTES));
336-
337-
const report = buildReport(entryPoints, parseFailures);
338-
expect(report.global).toBeGreaterThanOrEqual(0);
339-
expect(report.global).toBeLessThanOrEqual(100);
340-
expect(Object.keys(report.byFamily).length).toBeGreaterThan(1);
341-
// A full scan of the real tree runs a `ts.Program` per file for the parse diagnostics, and
342-
// `countRouteModuleFiles` walks the tree a second time. That is 1.6 to 2.6 seconds on an idle
343-
// machine and it flaked past the suite's 10s default under parallel load. Budgeted rather than
344-
// left marginal, the same way the exhaustive sweep below is.
345-
}, 30_000);
356+
it(
357+
"parses every route file and produces a report inside a wide band",
358+
() => {
359+
const { entryPoints, parseFailures } = scanDirectory(ROUTES);
360+
361+
expect(parseFailures).toEqual([]);
362+
expect(entryPoints.length).toBeGreaterThan(100);
363+
expect(entryPoints.length).toBeLessThan(countRouteModuleFiles(ROUTES));
364+
365+
const report = buildReport(entryPoints, parseFailures);
366+
expect(report.global).toBeGreaterThanOrEqual(0);
367+
expect(report.global).toBeLessThanOrEqual(100);
368+
expect(Object.keys(report.byFamily).length).toBeGreaterThan(1);
369+
},
370+
TREE_SCAN_TIMEOUT
371+
);
346372

347373
// A1, exhaustive: every scored check suppressed on every real route, zero behavioural change.
348374
// The old measured-from-visible logic took this global from 17 to 33 and measured from 412 to
349375
// 176, because every entry whose only applicable checks were suppressed dropped out of the
350376
// mean. Measured must not move: every entry point that had something applicable still does.
351-
it("suppressing every scored check on every real route does not raise the global", () => {
352-
const { entryPoints, parseFailures } = scanDirectory(ROUTES);
353-
const before = buildReport(entryPoints, parseFailures);
354-
355-
const directive = SCORED_CHECK_IDS.map(
356-
(id) => `// obs-map-disable ${id} -- exhaustive sweep\n`
357-
).join("");
358-
const suppressed = entryPoints.map((ep) => scanFile(ep.fileName, directive + ep.source)!);
359-
const after = buildReport(suppressed, parseFailures);
360-
361-
expect(after.measured).toBe(before.measured);
362-
expect(after.unmeasured).toBe(before.unmeasured);
363-
expect(after.global).not.toBeGreaterThan(before.global!);
364-
// Two full tree scans plus a re-scan of every source, which does not fit the suite default.
365-
}, 60_000);
377+
it(
378+
"suppressing every scored check on every real route does not raise the global",
379+
() => {
380+
const { entryPoints, parseFailures } = scanDirectory(ROUTES);
381+
const before = buildReport(entryPoints, parseFailures);
382+
383+
const directive = SCORED_CHECK_IDS.map(
384+
(id) => `// obs-map-disable ${id} -- exhaustive sweep\n`
385+
).join("");
386+
const suppressed = entryPoints.map((ep) => scanFile(ep.fileName, directive + ep.source)!);
387+
const after = buildReport(suppressed, parseFailures);
388+
389+
expect(after.measured).toBe(before.measured);
390+
expect(after.unmeasured).toBe(before.unmeasured);
391+
expect(after.global).not.toBeGreaterThan(before.global!);
392+
},
393+
TREE_SCAN_TIMEOUT
394+
);
366395
});

0 commit comments

Comments
 (0)