Skip to content

Commit f5fd326

Browse files
committed
fix(scripts): fold test:repo legs into measure-test-shard-timings samples
samplesFromSummary() kept only task.task === 'test', so the six packages split into `test` + `test:repo` by #16466 had their repo-scan seconds silently dropped -- under-weighing spec by roughly an eighth on the next refresh, in the direction --check-drift cannot see since it reads samples through the same function. Fold both legs per package by summing their execution windows; either leg being a cache HIT or a failure skips the whole package, same as today's single-leg rule. Self-test gains a two-task-summary battery (sum, either-leg- cached, either-leg-failed, and an un-split control proving the fold does not disturb ordinary packages), and its floor is raised 50 -> 56 accordingly. Fixes #16550 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
1 parent ba9f029 commit f5fd326

1 file changed

Lines changed: 132 additions & 12 deletions

File tree

scripts/measure-test-shard-timings.mjs

Lines changed: 132 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,17 @@ export function sliceOfCliArguments(args) {
140140
return null;
141141
}
142142

143-
// Pull every genuinely-executed `test` task out of one parsed run summary.
143+
// The task names whose windows this file counts as a package's test cost.
144+
// #16550: since #16466, six packages (core, objectql, rest, runtime, spec,
145+
// types) split their suite into `test` and `test:repo` (the repo-scanning
146+
// tests, hashed on the wide inputs) -- TWO task records per package in the
147+
// same run summary, both genuinely part of the suite's cost. `ci.yml` runs
148+
// them together (`turbo run test test:repo`), so a package that has a
149+
// `test:repo` task always has it in the SAME summary as its `test` task.
150+
const SAMPLED_TASKS = ['test', 'test:repo'];
151+
152+
// Pull every genuinely-executed `test` (and, for split packages, `test:repo`)
153+
// task out of one parsed run summary, folded per package into ONE window.
144154
//
145155
// The shape assertions are loud for the same reason the partitioner's are:
146156
// `--summarize` is stable but not contractual, and the failure this script can
@@ -152,6 +162,16 @@ export function sliceOfCliArguments(args) {
152162
// not need it (a shard runs exactly one slice, so its prediction follows from
153163
// the slice map, not from the summary), while buildDataset below cannot record
154164
// a correct weight without it.
165+
//
166+
// #16550: a package's `test` and `test:repo` legs are folded by SUMMING their
167+
// execution windows -- the whole-package cost is both halves together, not
168+
// either alone. The rejection rule composes across the fold rather than being
169+
// re-derived per leg: EVERY leg present for a package must be a cache MISS
170+
// with exit 0, or the WHOLE package is skipped -- one cached or failed leg is
171+
// exactly as disqualifying as a cached or failed `test` used to be on its own.
172+
// Recording one leg's seconds alone (because the other was cached or failed)
173+
// would write a partial suite's cost as the package's whole cost, a reading
174+
// worse than today's undercount by #16466's own defect this card fixes.
155175
export function samplesFromSummary(parsed, label) {
156176
const tasks = parsed?.tasks;
157177
if (!Array.isArray(tasks)) {
@@ -160,29 +180,63 @@ export function samplesFromSummary(parsed, label) {
160180
'is this a run summary at all?'
161181
);
162182
}
163-
const samples = new Map();
164-
const skippedCached = [];
165-
const slices = new Map();
183+
// One entry per package, holding whichever of its sampled tasks this
184+
// summary carries (almost always just `test`; `test` + `test:repo` for a
185+
// split package). Each leg is recorded as EITHER a seconds+cliArguments
186+
// reading, OR a `cached`/`failed` flag -- never both -- so the fold below
187+
// can tell "this leg disqualifies the package" from "this leg is a real
188+
// measurement" without re-reading the raw task.
189+
const legsByPackage = new Map();
166190
for (const task of tasks) {
167-
if (task?.task !== 'test') continue;
191+
const taskName = task?.task;
192+
if (!SAMPLED_TASKS.includes(taskName)) continue;
168193
const name = task.package;
169194
if (typeof name !== 'string' || name.length === 0) {
170-
throw new Error(`${label}: a test task carries no package name: ${JSON.stringify(task.taskId)}`);
195+
throw new Error(`${label}: a ${taskName} task carries no package name: ${JSON.stringify(task.taskId)}`);
171196
}
197+
if (!legsByPackage.has(name)) legsByPackage.set(name, new Map());
198+
const legs = legsByPackage.get(name);
172199
if (task?.cache?.status !== 'MISS') {
173-
skippedCached.push(name);
200+
legs.set(taskName, { cached: true });
174201
continue;
175202
}
176203
const { startTime, endTime, exitCode } = task.execution ?? {};
177204
if (typeof startTime !== 'number' || typeof endTime !== 'number') {
178-
throw new Error(`${label}: ${name}#test has no execution window to measure`);
205+
throw new Error(`${label}: ${name}#${taskName} has no execution window to measure`);
179206
}
180207
// A failed suite stops early, so its duration is not this package's cost.
181-
if (exitCode !== 0) continue;
208+
if (exitCode !== 0) {
209+
legs.set(taskName, { failed: true });
210+
continue;
211+
}
182212
const seconds = (endTime - startTime) / 1000;
183-
if (!(seconds >= 0)) throw new Error(`${label}: ${name}#test measured ${seconds}s`);
213+
if (!(seconds >= 0)) throw new Error(`${label}: ${name}#${taskName} measured ${seconds}s`);
214+
legs.set(taskName, { seconds, cliArguments: task.cliArguments });
215+
}
216+
217+
const samples = new Map();
218+
const skippedCached = [];
219+
const slices = new Map();
220+
for (const [name, legs] of legsByPackage) {
221+
const readings = [...legs.values()];
222+
// Cache wins over failure when both are present: either alone already
223+
// disqualifies the whole package, and `skippedCached` is what the merge
224+
// rule in buildDataset() reads as the witness for carrying a prior weight
225+
// forward -- a package skipped here for ANY reason including a failed
226+
// sibling leg still needs that witness if one of its legs was a HIT.
227+
if (readings.some((leg) => leg.cached)) {
228+
skippedCached.push(name);
229+
continue;
230+
}
231+
if (readings.some((leg) => leg.failed)) continue;
232+
let seconds = 0;
233+
let cliArguments;
234+
for (const leg of readings) {
235+
seconds += leg.seconds;
236+
cliArguments ??= leg.cliArguments;
237+
}
184238
samples.set(name, seconds);
185-
const slice = sliceOfCliArguments(task.cliArguments);
239+
const slice = sliceOfCliArguments(cliArguments);
186240
if (slice) slices.set(name, slice);
187241
}
188242
return { samples, skippedCached, slices };
@@ -425,7 +479,7 @@ export function buildDataset({ perSummary, fileCounts, provenance, carryFrom = n
425479
// must not red. A battery BELOW its floor means cases stopped running; the
426480
// remedy is to find what stopped registering, never to lower the number.
427481
const SELF_TEST_BATTERIES = Object.freeze({
428-
'measure-test-shard-timings self-test': 50,
482+
'measure-test-shard-timings self-test': 56,
429483
});
430484

431485
// DELETING an entry silences that battery's floor exactly as effectively as
@@ -474,6 +528,13 @@ function selfTest() {
474528
cache: { status },
475529
execution: { startTime: start, endTime: end, exitCode },
476530
});
531+
const testRepoTask = (pkg, start, end, status = 'MISS', exitCode = 0) => ({
532+
taskId: `${pkg}#test:repo`,
533+
task: 'test:repo',
534+
package: pkg,
535+
cache: { status },
536+
execution: { startTime: start, endTime: end, exitCode },
537+
});
477538

478539
check(() => {
479540
if (median([3]) !== 3) throw new Error('median: single value');
@@ -525,6 +586,65 @@ function selfTest() {
525586
if (failed.samples.has('a')) throw new Error('exit: a failed suite was recorded as a duration');
526587
});
527588

589+
// #16550: a two-task summary -- a split package's `test` and `test:repo`
590+
// legs are SUMMED into one whole-package reading, not either leg alone. The
591+
// un-split control (`ctl`, a plain `test`-only package in the SAME summary)
592+
// rides alongside it and must read exactly as it always has -- the
593+
// discriminating half of this case, since a probe that reads the same
594+
// before and after the fold would not catch a fold that leaked onto
595+
// packages it was never meant to touch.
596+
const twoTask = samplesFromSummary(
597+
summary([testTask('spec', 0, 400_000), testRepoTask('spec', 0, 53_900), testTask('ctl', 0, 12_000)]),
598+
'f'
599+
);
600+
check(() => {
601+
if (twoTask.samples.get('spec') !== 453.9) {
602+
throw new Error(`test:repo fold: expected the sum 453.9, got ${twoTask.samples.get('spec')}`);
603+
}
604+
});
605+
check(() => {
606+
if (twoTask.samples.get('ctl') !== 12) {
607+
throw new Error(`test:repo fold: the un-split control was disturbed (got ${twoTask.samples.get('ctl')})`);
608+
}
609+
});
610+
611+
// Either leg cached skips the WHOLE package -- recording the other leg's
612+
// seconds alone would be a reading worse than the pre-#16550 undercount.
613+
const repoCached = samplesFromSummary(
614+
summary([testTask('spec', 0, 400_000), testRepoTask('spec', 0, 53_900, 'HIT')]),
615+
'f'
616+
);
617+
check(() => {
618+
if (repoCached.samples.has('spec')) {
619+
throw new Error(`test:repo fold: a cached test:repo leg did not skip the whole package (got ${repoCached.samples.get('spec')})`);
620+
}
621+
});
622+
check(() => {
623+
if (!repoCached.skippedCached.includes('spec')) {
624+
throw new Error('test:repo fold: a package with a cached test:repo leg was not reported as skipped');
625+
}
626+
});
627+
const testCached = samplesFromSummary(
628+
summary([testTask('spec', 0, 400_000, 'HIT'), testRepoTask('spec', 0, 53_900)]),
629+
'f'
630+
);
631+
check(() => {
632+
if (testCached.samples.has('spec')) {
633+
throw new Error(`test:repo fold: a cached test leg did not skip the whole package (got ${testCached.samples.get('spec')})`);
634+
}
635+
});
636+
637+
// Either leg failed skips the WHOLE package, same as a lone `test` failure.
638+
const repoFailed = samplesFromSummary(
639+
summary([testTask('spec', 0, 400_000), testRepoTask('spec', 0, 53_900, 'MISS', 1)]),
640+
'f'
641+
);
642+
check(() => {
643+
if (repoFailed.samples.has('spec')) {
644+
throw new Error('test:repo fold: a failed test:repo leg did not skip the whole package');
645+
}
646+
});
647+
528648
const threw = (fn) => {
529649
try {
530650
fn();

0 commit comments

Comments
 (0)