* fix(#2456): weight test chunks by measured cost and pack with LPT scripts/run-tests.cjs guessed each test file's cost from its filename (basename matching /^(?:install|codex-)/ scored 12, everything else 1). Measured durations show that guess is wrong in both directions: installer-migration-authoring.test.cjs scored 12 while running ~0.1s, and the two most expensive files in the suite both scored 1 — run-tests-harness.test.cjs never matched the prefix, and release-tarball-smoke.install.test.cjs was missed because the regex is anchored to the START of the basename. Chunks were therefore balanced by file COUNT, not cost. On the real shard 2/3 the two heaviest files packed into the SAME chunk, leaving the slowest chunk 2.8x the lightest and sitting near the 600s per-chunk timeout while other chunks idled. Weight each file by its measured duration from a checked-in, regenerable timings table and pack with LPT (heaviest first, into the lightest chunk). On the same shard this drops the slowest chunk from 383s to 238s and the imbalance from 2.79x to 1.00x, and separates the two heavy files. Timings are advisory, never gated: an unknown file falls back to the table's median weight, a missing or corrupt table falls back to uniform weight, and a count-based floor guarantees the packer never produces fewer chunks than plain count-based packing would. Closes #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): harden chunk packing against degenerate knobs and table keys Follow-up hardening found while reviewing the packer, fixed inline. The chunk knobs are read from the environment with Number(), so a typo (RUN_TESTS_MAX_FILES_PER_CHUNK=abc) yields NaN and an explicit 0 yields 0. Both flow into the new chunk-count arithmetic: NaN made Math.ceil return NaN, Array.from({length: NaN}) produce zero bins, and packChunks' retry loop spin forever — a hung CI job with no output. Zero made the count Infinity and threw RangeError: Invalid array length. The previous count-based packer degraded to a single chunk instead, so this was a regression introduced by the LPT rewrite. Normalize the knobs at the environment boundary (positiveNumberEnv: anything not a positive finite number falls back to the default) and guard packChunks itself, since it is exported and cannot assume its caller normalized. Non-finite weights from an arbitrary weightOf are clamped too. RUN_TESTS_CHUNK_TIMEOUT_MS gets the same treatment. Also resolve timing-table lookups with Object.hasOwn: the table is JSON-parsed, so a bare index would walk the prototype chain and return a function for a file named constructor.test.cjs or toString.test.cjs. The typeof guard already rejected that, but the lookup now resolves correctly rather than relying on the downstream check. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): correct prototype-lookup rationale and guard generator keys Two findings from independent security review, fixed inline. The makeFileWeigher comment claimed a bare table lookup "would return a FUNCTION for a file named constructor.test.cjs". That premise is false: basename('constructor.test.cjs') is 'constructor.test.cjs', which is not an Object.prototype key, and walkTestFiles only ever collects *.test.cjs. The prototype chain was never reachable from a real selection, and the existing typeof guard already rejected the function it would return, so Object.hasOwn is defense-in-depth rather than a behavior change. The comment now says that instead of asserting something untrue. The accompanying test inherited the same false premise: it fed constructor.test.cjs and asserted a median fallback that would have held with or without the guard, so it passed for a reason unrelated to what it claimed to prove. It now uses BARE keys (constructor, toString, valueOf, hasOwnProperty, __proto__) — the only inputs that actually resolve on Object.prototype — and asserts the real exported contract: any key absent from the table weighs the median, never a function. gen-test-timings.cjs built its output object by computed-key assignment from basenames taken out of a reporter stream it does not control — the js/prototype-polluting-assignment shape, and this repo has a CodeQL barrier for exactly that pattern. It was not exploitable (the value is always a rounded number, so the __proto__ setter is a silent no-op), but it silently DROPPED such an entry rather than reporting it. Validate every key against a test-basename pattern and fail loudly instead, and build the table with a null prototype. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): replace tautological chunking tests and clamp chunk count Six findings from independent correctness review, all reproduced and fixed inline. The two subprocess tests written to carry the #2088 guarantee forward were tautological: every seeded file weighed exactly 1, so both passed under the OLD prefix-heuristic packer and with the timings file deleted entirely. Neither could fail for the reason it existed. Both are rebuilt so the old algorithm produces a different packing and the assertion goes red: the spread test now uses three expensive files named so the old heuristic scored them 1 alongside three trivial `install-`-prefixed files it scored 12 — inverted from real cost, giving {2,2,1,1} under the old packer versus {2,2,2} under measured weights. The companion test covers the other direction: four trivial `install-` files the old heuristic split into four single-file chunks now stay in one. packChunks clamped the chunk count from below but not above, so a legitimate but tiny budget (RUN_TESTS_MAX_FILES_PER_CHUNK=1e-9, which positiveNumberEnv accepts) asked for 637,000,000,000 bins and threw RangeError. More chunks than files is never useful; the count now clamps at one file per chunk. The generator's basename-collision guard compared full dirnames, so two OS lanes reporting the same file under different container roots (/work/tests vs C:/work/tests) flagged every shared basename as a collision — on the script's own documented multi-lane usage. Detection is now scoped per stream, where the root is constant; a genuine same-lane collision is still caught. Also: the LPT tie-break compared raw paths, so a path separator (0x2F vs 0x5C) could order a subdir file differently per platform, contradicting the documented byte-identical guarantee — it now normalizes separators. loadTestTimings now honors schema_version instead of writing it and never reading it, falling back to uniform weight on an unknown version. A comment claiming an all-uniform suite "chunks exactly as it did before" was false and contradicted by this PR's own test: the chunk count is preserved, the composition is not. And the missing-table test created a temp dir it never cleaned up, for a path that only needed to not exist. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
201
scripts/gen-test-timings.cjs
Normal file
201
scripts/gen-test-timings.cjs
Normal file
@@ -0,0 +1,201 @@
|
||||
#!/usr/bin/env node
|
||||
// Regenerate the per-file test timing table used to weight chunk packing in
|
||||
// scripts/run-tests.cjs (issue #2456).
|
||||
//
|
||||
// The chunk packer needs to know what each test file actually COSTS. Before
|
||||
// #2456 it guessed from the filename (`^(?:install|codex-)` scored 12x,
|
||||
// everything else 1) and was wrong in both directions — installer-migration-
|
||||
// authoring.test.cjs scored 12x but runs ~0.1s, while the two heaviest files in
|
||||
// the suite (run-tests-harness.test.cjs and release-tarball-smoke.install
|
||||
// .test.cjs) both scored 1. This script replaces the guess with measurement.
|
||||
//
|
||||
// Input is one or more node:test reporter event streams as emitted by
|
||||
// `gsd-test` (`~/.local/state/gsd-test/runs/<run-id>/test-events-<os>-node<v>
|
||||
// .jsonl`). Each stream carries one `test:summary` event per test FILE, whose
|
||||
// `data.duration_ms` is that file's total wall-clock and whose `data.file` is
|
||||
// its absolute in-container path.
|
||||
//
|
||||
// Usage:
|
||||
// node scripts/gen-test-timings.cjs <events.jsonl> [<events.jsonl> ...]
|
||||
// node scripts/gen-test-timings.cjs ~/.local/state/gsd-test/runs/*/test-events-*.jsonl
|
||||
// node scripts/gen-test-timings.cjs events.jsonl --out tests/test-timings.json
|
||||
//
|
||||
// When several streams are supplied (multiple lanes, e.g. node22 + node24), a
|
||||
// file's recorded time is the MAX across them, not the mean: the packer exists
|
||||
// to keep the SLOWEST lane's slowest chunk away from the per-chunk timeout, so
|
||||
// the conservative bound is the right one to balance against.
|
||||
//
|
||||
// The table is ADVISORY and deliberately un-gated — there is no `--check` mode
|
||||
// and no CI lint that fails on staleness, because timing data legitimately
|
||||
// varies run to run. A file missing from the table falls back to the table's
|
||||
// median weight, so a stale table degrades chunk BALANCE gracefully instead of
|
||||
// failing the build. Regenerate it when the suite's cost profile has visibly
|
||||
// drifted, not on a schedule.
|
||||
'use strict';
|
||||
|
||||
const fs = require('fs');
|
||||
const { basename, dirname, join } = require('path');
|
||||
const { ExitError, runMain } = require('./lib/cli-exit.cjs');
|
||||
|
||||
const DEFAULT_OUT = join(__dirname, '..', 'tests', 'test-timings.json');
|
||||
const SCHEMA_VERSION = 1;
|
||||
|
||||
function parseArgs(argv) {
|
||||
const inputs = [];
|
||||
let out = DEFAULT_OUT;
|
||||
for (let i = 0; i < argv.length; i++) {
|
||||
const arg = argv[i];
|
||||
if (arg === '--out') {
|
||||
const value = argv[++i];
|
||||
if (!value) return { error: '--out requires a path' };
|
||||
out = value;
|
||||
} else if (arg.startsWith('--out=')) {
|
||||
const value = arg.slice('--out='.length);
|
||||
if (!value) return { error: '--out requires a path' };
|
||||
out = value;
|
||||
} else if (arg.startsWith('-')) {
|
||||
return { error: `unknown flag "${arg}"` };
|
||||
} else {
|
||||
inputs.push(arg);
|
||||
}
|
||||
}
|
||||
if (inputs.length === 0) {
|
||||
return { error: 'usage: gen-test-timings.cjs <reporter-events.jsonl> [...] [--out <path>]' };
|
||||
}
|
||||
return { inputs, out };
|
||||
}
|
||||
|
||||
// Fold one reporter event stream into `acc`, keeping the MAX duration seen for
|
||||
// each test file. Returns per-stream counters plus any basename collisions
|
||||
// found WITHIN this stream.
|
||||
//
|
||||
// Keying is by BASENAME, matching how run-tests.cjs weights a selected file:
|
||||
// the reporter reports absolute in-container paths (/work/tests/foo.test.cjs)
|
||||
// while the harness carries paths relative to its test dir, so the basename is
|
||||
// the only stable join key between the two. A basename seen in two different
|
||||
// directories would make the table ambiguous, so it must fail loudly.
|
||||
//
|
||||
// Collision detection is scoped to a SINGLE stream deliberately. Every lane
|
||||
// writes its own stream under its own container root (`/work/tests` on Linux,
|
||||
// `C:/work/tests` on Windows), so comparing directories ACROSS streams reports
|
||||
// every shared basename as a collision — which is the script's own documented
|
||||
// usage (globbing `test-events-*.jsonl` across lanes). Within one stream the
|
||||
// root is constant, so a differing directory is a real collision.
|
||||
function foldStream(text, acc) {
|
||||
const dirsByBase = new Map();
|
||||
let files = 0;
|
||||
let malformed = 0;
|
||||
for (const line of text.split('\n')) {
|
||||
if (line.trim() === '') continue;
|
||||
let event;
|
||||
try {
|
||||
event = JSON.parse(line);
|
||||
} catch {
|
||||
malformed++;
|
||||
continue;
|
||||
}
|
||||
if (!event || event.type !== 'test:summary') continue;
|
||||
const data = event.data;
|
||||
if (!data || typeof data.file !== 'string') continue;
|
||||
const ms = data.duration_ms;
|
||||
if (typeof ms !== 'number' || !Number.isFinite(ms) || ms < 0) continue;
|
||||
const path = data.file.replace(/\\/g, '/');
|
||||
const base = basename(path);
|
||||
if (!dirsByBase.has(base)) dirsByBase.set(base, new Set());
|
||||
dirsByBase.get(base).add(dirname(path));
|
||||
const prev = acc.get(base);
|
||||
if (prev === undefined || ms > prev) acc.set(base, ms);
|
||||
files++;
|
||||
}
|
||||
const collisions = [...dirsByBase.entries()]
|
||||
.filter(([, dirs]) => dirs.size > 1)
|
||||
.map(([base, dirs]) => `${base} (${[...dirs].sort().join(', ')})`);
|
||||
return { files, malformed, collisions };
|
||||
}
|
||||
|
||||
function main() {
|
||||
const parsed = parseArgs(process.argv.slice(2));
|
||||
if (parsed.error) throw new ExitError(2, `gen-test-timings: ${parsed.error}`);
|
||||
|
||||
const acc = new Map();
|
||||
const allCollisions = new Set();
|
||||
const sources = [];
|
||||
for (const input of parsed.inputs) {
|
||||
let text;
|
||||
try {
|
||||
text = fs.readFileSync(input, 'utf8');
|
||||
} catch (err) {
|
||||
throw new ExitError(2, `gen-test-timings: cannot read "${input}": ${err.message}`);
|
||||
}
|
||||
const { files, malformed, collisions } = foldStream(text, acc);
|
||||
for (const c of collisions) allCollisions.add(c);
|
||||
sources.push(basename(input));
|
||||
console.error(
|
||||
`gen-test-timings: ${basename(input)} — ${files} file summaries` +
|
||||
(malformed > 0 ? `, ${malformed} unparseable lines skipped` : ''),
|
||||
);
|
||||
}
|
||||
|
||||
if (acc.size === 0) {
|
||||
throw new ExitError(
|
||||
2,
|
||||
'gen-test-timings: no `test:summary` events with a file and duration_ms were found. ' +
|
||||
'Check that the input is a node:test reporter event stream (test-events-<os>-node<v>.jsonl).',
|
||||
);
|
||||
}
|
||||
|
||||
// A basename that resolves to two different directories within one lane makes
|
||||
// the table ambiguous: run-tests.cjs joins on basename alone, so one file's
|
||||
// measured cost would silently be applied to the other. Fail rather than emit
|
||||
// a table that lies.
|
||||
if (allCollisions.size > 0) {
|
||||
throw new ExitError(
|
||||
2,
|
||||
`gen-test-timings: basename collision — the table cannot key on basename alone:\n ${[...allCollisions].sort().join('\n ')}`,
|
||||
);
|
||||
}
|
||||
|
||||
// Every key must be a plain test-file basename. This is a data-integrity
|
||||
// check on a stream we do not control (the reporter emits whatever path the
|
||||
// runner saw), and it structurally excludes a computed key like `__proto__`
|
||||
// or `constructor` from being written into the table object below — the
|
||||
// `js/prototype-polluting-assignment` shape, even though the value here is
|
||||
// always a number and could not actually pollute.
|
||||
const SAFE_BASENAME_RE = /^[A-Za-z0-9._-]+\.test\.cjs$/;
|
||||
const rejected = [...acc.keys()].filter((base) => !SAFE_BASENAME_RE.test(base));
|
||||
if (rejected.length > 0) {
|
||||
throw new ExitError(
|
||||
2,
|
||||
`gen-test-timings: refusing to emit non-test-file keys: ${rejected.sort().join(', ')}`,
|
||||
);
|
||||
}
|
||||
|
||||
// Sorted keys keep the checked-in diff reviewable: a regeneration shows only
|
||||
// the files whose cost actually moved, not a reshuffled object.
|
||||
const timings = Object.create(null);
|
||||
for (const base of [...acc.keys()].sort()) {
|
||||
timings[base] = Math.round(acc.get(base));
|
||||
}
|
||||
|
||||
const payload = {
|
||||
schema_version: SCHEMA_VERSION,
|
||||
generated_by: 'scripts/gen-test-timings.cjs',
|
||||
unit: 'ms',
|
||||
sources: sources.sort(),
|
||||
file_count: acc.size,
|
||||
timings,
|
||||
};
|
||||
|
||||
fs.writeFileSync(parsed.out, `${JSON.stringify(payload, null, 2)}\n`, 'utf8');
|
||||
const totalMs = [...acc.values()].reduce((sum, ms) => sum + ms, 0);
|
||||
console.error(
|
||||
`gen-test-timings: wrote ${parsed.out} — ${acc.size} files, ${(totalMs / 1000).toFixed(1)}s total`,
|
||||
);
|
||||
return 0;
|
||||
}
|
||||
|
||||
if (require.main === module) {
|
||||
runMain(main);
|
||||
}
|
||||
|
||||
module.exports = { parseArgs, foldStream };
|
||||
@@ -29,7 +29,7 @@
|
||||
// See docs/TESTING-SUITES.md for full grouping policy.
|
||||
'use strict';
|
||||
|
||||
const { readdirSync } = require('fs');
|
||||
const { readdirSync, readFileSync } = require('fs');
|
||||
const { join, basename } = require('path');
|
||||
const { execFileSync } = require('child_process');
|
||||
const { ExitError, runMain } = require('./lib/cli-exit.cjs');
|
||||
@@ -220,6 +220,206 @@ function selectShard(sortedFiles, { index, total }) {
|
||||
return sortedFiles.filter((_, k) => k % total === index - 1);
|
||||
}
|
||||
|
||||
// Read an operator-supplied numeric env knob, falling back to the default for
|
||||
// anything that is not a positive finite number.
|
||||
//
|
||||
// This is a strict-input boundary (Postel's Law: a typo must fail SAFE, not
|
||||
// silently poison arithmetic downstream). `Number('abc')` is NaN and
|
||||
// `Number('')` is 0, and both are load-bearing here: a NaN chunk budget makes
|
||||
// the chunk-count computation NaN, which spins packChunks' retry loop forever
|
||||
// (a hung CI job with no output); a zero budget makes it Infinity, which throws
|
||||
// `RangeError: Invalid array length`. Neither is an acceptable response to a
|
||||
// mistyped environment variable.
|
||||
function positiveNumberEnv(raw, fallback) {
|
||||
if (raw === undefined || raw === null || String(raw).trim() === '') return fallback;
|
||||
const n = Number(raw);
|
||||
return Number.isFinite(n) && n > 0 ? n : fallback;
|
||||
}
|
||||
|
||||
// Per-file measured durations, regenerated by scripts/gen-test-timings.cjs from
|
||||
// gsd-test reporter event streams. Overridable so tests can inject a synthetic
|
||||
// table instead of depending on the real suite's cost profile.
|
||||
const DEFAULT_TIMINGS_PATH = join(__dirname, '..', 'tests', 'test-timings.json');
|
||||
// Must track SCHEMA_VERSION in scripts/gen-test-timings.cjs.
|
||||
const SUPPORTED_TIMINGS_SCHEMA = 1;
|
||||
|
||||
// Load the timing table and reduce it to what the packer needs.
|
||||
//
|
||||
// Weights are normalized by the table's MEAN duration, so an average-cost file
|
||||
// weighs exactly 1 and `MAX_FILES_PER_CHUNK` keeps its original meaning ("about
|
||||
// N average files per chunk"). When every file costs the same, total weight
|
||||
// equals file count, so the chunk COUNT matches count-based packing exactly.
|
||||
// The chunk COMPOSITION still differs — LPT balances where first-fit filled
|
||||
// greedily, so 7 uniform files at budget 3 pack {3,2,2} rather than {3,3,1}.
|
||||
//
|
||||
// `medianWeight` is the fallback for a file absent from the table (a new test,
|
||||
// or a table that has drifted). The median — not the mean — because the cost
|
||||
// distribution is heavily right-skewed (median 0.28s vs mean 4.6s across the
|
||||
// suite), so the median is the honest estimate for an unknown file.
|
||||
//
|
||||
// Returns null when the table is missing or unusable; the caller then treats
|
||||
// every file as weight 1, which reproduces the pre-#2456 count-based balance.
|
||||
function loadTestTimings(timingsPath) {
|
||||
let parsed;
|
||||
try {
|
||||
parsed = JSON.parse(readFileSync(timingsPath, 'utf8'));
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
if (!parsed || typeof parsed !== 'object') return null;
|
||||
// Refuse a table written by a future generator: a v2 schema could change the
|
||||
// unit or the key format, and consuming it under v1 semantics would silently
|
||||
// mis-weight every file. Returning null falls back to uniform weight, which
|
||||
// is the same graceful degradation as a missing table.
|
||||
if (parsed.schema_version !== undefined && parsed.schema_version !== SUPPORTED_TIMINGS_SCHEMA) {
|
||||
return null;
|
||||
}
|
||||
const timings = parsed.timings;
|
||||
if (!timings || typeof timings !== 'object') return null;
|
||||
const values = Object.values(timings).filter(
|
||||
(v) => typeof v === 'number' && Number.isFinite(v) && v >= 0,
|
||||
);
|
||||
if (values.length === 0) return null;
|
||||
const mean = values.reduce((sum, v) => sum + v, 0) / values.length;
|
||||
if (!(mean > 0)) return null;
|
||||
const sorted = [...values].sort((a, b) => a - b);
|
||||
const mid = sorted.length >> 1;
|
||||
const median = sorted.length % 2 === 1 ? sorted[mid] : (sorted[mid - 1] + sorted[mid]) / 2;
|
||||
return { timings, mean, medianWeight: median / mean };
|
||||
}
|
||||
|
||||
// Build the packer's weight function from a loaded timing table.
|
||||
//
|
||||
// A file present in the table weighs its measured duration relative to the
|
||||
// table mean. A file ABSENT from it weighs the table's median — this is the
|
||||
// "advisory, not gated" contract: a new test or a drifted table costs chunk
|
||||
// balance, never a red build. A null table (missing or unparseable file) makes
|
||||
// every file weigh 1, reproducing the pre-#2456 count-based balance exactly.
|
||||
function makeFileWeigher(timings) {
|
||||
if (!timings) return () => 1;
|
||||
return (f) => {
|
||||
const key = basename(f);
|
||||
// Own-property check before the lookup. This is defense-in-depth, NOT a
|
||||
// behavior change: the table is JSON-parsed, so a bare `timings[key]` would
|
||||
// walk the prototype chain, but the only keys that resolve there are
|
||||
// Object.prototype members (`constructor`, `toString`, …) and every real
|
||||
// selection is a `*.test.cjs` basename, which can never equal one. Even if
|
||||
// it could, the `typeof ms === 'number'` guard below already rejects the
|
||||
// function it would return. `Object.hasOwn` makes the intent explicit and
|
||||
// keeps the lookup correct for arbitrary input, since this function is
|
||||
// exported and does not control its caller's strings.
|
||||
const ms = Object.hasOwn(timings.timings, key) ? timings.timings[key] : undefined;
|
||||
return typeof ms === 'number' && Number.isFinite(ms) && ms >= 0
|
||||
? ms / timings.mean
|
||||
: timings.medianWeight;
|
||||
};
|
||||
}
|
||||
|
||||
// Pack `files` into chunks using LPT (longest-processing-time-first): sort by
|
||||
// weight descending, then place each file into the currently-LIGHTEST chunk.
|
||||
//
|
||||
// #2456: the previous packer was a sequential first-fit that appended files in
|
||||
// selection order and closed a chunk once its weight budget was hit. Because
|
||||
// sorted-adjacent files land together, the two heaviest files in a shard packed
|
||||
// into the SAME chunk, leaving the slowest chunk ~3.9x the lightest and sitting
|
||||
// near the 600s per-chunk timeout while other chunks idled. LPT is the standard
|
||||
// greedy approximation for exactly this makespan problem and balanced the same
|
||||
// real shard to ~1.0x.
|
||||
//
|
||||
// Chunk COUNT is fixed before placement so LPT has bins to balance across:
|
||||
// ceil(totalWeight / maxWeight) — the weighted budget, and
|
||||
// ceil(fileCount / maxWeight) — a floor that pins the count at what the
|
||||
// old count-based packing would produce.
|
||||
// The floor is what makes a stale or missing timings table safe: unknown files
|
||||
// fall back to a small median weight, which on its own would collapse many files
|
||||
// into few fat chunks. With the floor, a degraded table can only ever reproduce
|
||||
// today's chunking, never something coarser.
|
||||
//
|
||||
// `maxChars` still bounds each chunk's argv (Windows CreateProcess caps
|
||||
// lpCommandLine at 32,767). A chunk that cannot fit the next file is skipped for
|
||||
// that file; when no chunk has room, the chunk count grows and packing restarts.
|
||||
// A single file longer than the budget lands alone rather than looping forever.
|
||||
//
|
||||
// Ordering is fully deterministic — ties break on the separator-normalized file
|
||||
// path, and each chunk's files are emitted in their original selection order —
|
||||
// so the packing is byte-identical across Windows/macOS/Linux.
|
||||
function packChunks(files, { weightOf, maxWeight, maxChars, fixedOverhead }) {
|
||||
if (files.length === 0) return [];
|
||||
// packChunks is exported, so it cannot assume its caller normalized these.
|
||||
// A non-finite or non-positive budget makes the chunk-count arithmetic
|
||||
// non-finite, which spins the retry loop below forever or throws from
|
||||
// Array.from; a non-finite weight propagates into the same computation.
|
||||
// Degrade to a safe bound instead.
|
||||
const weightBudget = Number.isFinite(maxWeight) && maxWeight > 0 ? maxWeight : files.length;
|
||||
const charBudget = Number.isFinite(maxChars) && maxChars > 0 ? maxChars : Number.MAX_SAFE_INTEGER;
|
||||
const overhead = Number.isFinite(fixedOverhead) && fixedOverhead >= 0 ? fixedOverhead : 0;
|
||||
const safeWeight = (file) => {
|
||||
const w = weightOf(file);
|
||||
return Number.isFinite(w) && w >= 0 ? w : 0;
|
||||
};
|
||||
const entries = files.map((file, index) => ({
|
||||
file,
|
||||
index,
|
||||
weight: safeWeight(file),
|
||||
chars: file.length + 1, // +1 for the inter-arg separator
|
||||
}));
|
||||
const totalWeight = entries.reduce((sum, e) => sum + e.weight, 0);
|
||||
// Ties break on a SEPARATOR-NORMALIZED path so a subdir file orders the same
|
||||
// on Windows as on POSIX: '/' is 0x2F and '\' is 0x5C, which straddle the
|
||||
// uppercase range, so comparing raw paths can order `sub/x.test.cjs` against
|
||||
// `subZ.test.cjs` differently per platform and silently produce a different
|
||||
// (still valid, but non-reproducible) packing.
|
||||
const sortKey = (f) => f.replace(/\\/g, '/');
|
||||
const heaviestFirst = [...entries].sort((a, b) => {
|
||||
if (b.weight !== a.weight) return b.weight - a.weight;
|
||||
const ka = sortKey(a.file);
|
||||
const kb = sortKey(b.file);
|
||||
return ka < kb ? -1 : ka > kb ? 1 : 0;
|
||||
});
|
||||
|
||||
// Termination: the empty-bin rule below guarantees every file is placeable
|
||||
// once chunkCount reaches files.length, so the retry loop cannot run forever.
|
||||
// The upper clamp matters as much as the lower bound: a legitimate but tiny
|
||||
// budget (RUN_TESTS_MAX_FILES_PER_CHUNK=1e-9) would otherwise ask for
|
||||
// 637,000,000,000 bins and throw `RangeError: Invalid array length`. More
|
||||
// chunks than files is never useful — one file per chunk is the finest
|
||||
// possible packing.
|
||||
let chunkCount = Math.min(
|
||||
files.length,
|
||||
Math.max(1, Math.ceil(totalWeight / weightBudget), Math.ceil(files.length / weightBudget)),
|
||||
);
|
||||
for (;;) {
|
||||
const bins = Array.from({ length: chunkCount }, () => ({
|
||||
entries: [],
|
||||
weight: 0,
|
||||
chars: overhead,
|
||||
}));
|
||||
let overflowed = false;
|
||||
for (const entry of heaviestFirst) {
|
||||
let target = null;
|
||||
for (const bin of bins) {
|
||||
// An empty bin always accepts, so an over-long single file lands alone
|
||||
// instead of growing the chunk count forever.
|
||||
if (bin.entries.length > 0 && bin.chars + entry.chars > charBudget) continue;
|
||||
if (target === null || bin.weight < target.weight) target = bin;
|
||||
}
|
||||
if (target === null) {
|
||||
overflowed = true;
|
||||
break;
|
||||
}
|
||||
target.entries.push(entry);
|
||||
target.weight += entry.weight;
|
||||
target.chars += entry.chars;
|
||||
}
|
||||
if (!overflowed) {
|
||||
return bins
|
||||
.filter((bin) => bin.entries.length > 0)
|
||||
.map((bin) => bin.entries.sort((a, b) => a.index - b.index).map((e) => e.file));
|
||||
}
|
||||
chunkCount++;
|
||||
}
|
||||
}
|
||||
|
||||
function parseArgs(argv) {
|
||||
let suite = null;
|
||||
let seen = false;
|
||||
@@ -559,9 +759,10 @@ function main() {
|
||||
// fine there. Split into chunks sized for the tightest target so behavior
|
||||
// is identical across platforms. (#3597)
|
||||
// Operator override (also used by tests to force chunking with short paths).
|
||||
const MAX_CMDLINE_CHARS = process.env.RUN_TESTS_MAX_CMDLINE_CHARS
|
||||
? Number(process.env.RUN_TESTS_MAX_CMDLINE_CHARS)
|
||||
: 28000; // headroom below the 32,767 Windows ceiling
|
||||
const MAX_CMDLINE_CHARS = positiveNumberEnv(
|
||||
process.env.RUN_TESTS_MAX_CMDLINE_CHARS,
|
||||
28000, // headroom below the 32,767 Windows ceiling
|
||||
);
|
||||
// A full-lane shard (~171 files) fit in ONE chunk at the old cap of 180, so the
|
||||
// entire shard's wall-clock ran against a single per-chunk timeout. On the slow
|
||||
// Windows runner the install-heavy files in a shard (e.g. install-minimal-hooks
|
||||
@@ -574,26 +775,31 @@ function main() {
|
||||
// node process (also relieving per-process memory pressure from 170+ files at once).
|
||||
// Lowered from 90 to 60 after #1575 — macOS Node 22 shard 2/3 chunk 2 (~80 files
|
||||
// including state.test.cjs, perf-*, worktree-cleanup) exceeded 600s with 90.
|
||||
const MAX_FILES_PER_CHUNK = process.env.RUN_TESTS_MAX_FILES_PER_CHUNK
|
||||
? Number(process.env.RUN_TESTS_MAX_FILES_PER_CHUNK)
|
||||
: 60;
|
||||
// #2088: file COUNT is a poor proxy for a chunk's wall-clock — install-heavy
|
||||
// files (real installs; install-minimal-hooks.test.cjs alone runs ~250 cases
|
||||
// doing dozens of installs) are ~10× a unit file. When several land in the SAME
|
||||
// chunk — e.g. a PR touching the whole install surface, whose *targeted* lane is
|
||||
// unsharded (13 install-heavy files → one chunk) — that chunk blows the 600s
|
||||
// backstop while unit-only chunks finish in seconds. WEIGHT install-heavy files
|
||||
// so they fill a chunk's budget faster and therefore SPREAD across chunks
|
||||
// instead of clustering. Light files keep weight 1, so pure-unit chunking (and
|
||||
// its harness tests) is byte-for-byte unchanged. `MAX_FILES_PER_CHUNK` is now a
|
||||
// per-chunk WEIGHT budget (backwards-compatible: it equals the file count when
|
||||
// every file is light). Tune via RUN_TESTS_HEAVY_FILE_WEIGHT; classify via the
|
||||
// basename prefix (install*/installer*/codex-* are the real install-heavy suites).
|
||||
const HEAVY_TEST_RE = /^(?:install|codex-)/;
|
||||
const HEAVY_FILE_WEIGHT = process.env.RUN_TESTS_HEAVY_FILE_WEIGHT
|
||||
? Number(process.env.RUN_TESTS_HEAVY_FILE_WEIGHT)
|
||||
: 12;
|
||||
const fileWeight = (f) => (HEAVY_TEST_RE.test(basename(f)) ? HEAVY_FILE_WEIGHT : 1);
|
||||
const MAX_FILES_PER_CHUNK = positiveNumberEnv(process.env.RUN_TESTS_MAX_FILES_PER_CHUNK, 60);
|
||||
// #2088 established that file COUNT is a poor proxy for a chunk's wall-clock:
|
||||
// install-heavy files (real installs) cost ~10x a unit file, and when several
|
||||
// land in the SAME chunk it blows the 600s backstop while unit-only chunks
|
||||
// finish in seconds. #2088 approximated cost from the filename — basename
|
||||
// matching /^(?:install|codex-)/ scored 12, everything else 1.
|
||||
//
|
||||
// #2456: that approximation is miscalibrated in BOTH directions, so chunks were
|
||||
// still balanced by file count rather than by cost. Measured durations show
|
||||
// installer-migration-authoring.test.cjs scoring 12 while running ~0.1s, and the
|
||||
// two heaviest files in the whole suite scoring 1 — run-tests-harness.test.cjs
|
||||
// (never matched the prefix) and release-tarball-smoke.install.test.cjs (the
|
||||
// regex is anchored to the START of the basename, so a mid-name "install" never
|
||||
// matches). Both landed in the same chunk, leaving the slowest chunk ~3.9x the
|
||||
// lightest and sitting near the timeout.
|
||||
//
|
||||
// Weight each file by its MEASURED duration instead. `MAX_FILES_PER_CHUNK`
|
||||
// remains the per-chunk weight budget and keeps its scale — weights are
|
||||
// normalized so an average-cost file weighs 1 — so an all-uniform suite chunks
|
||||
// exactly as it did before. Timings are ADVISORY, never gated: an unknown file
|
||||
// falls back to the table's median weight and a missing table falls back to
|
||||
// uniform weight 1, so staleness degrades chunk BALANCE gracefully instead of
|
||||
// failing CI. Regenerate via `node scripts/gen-test-timings.cjs <events.jsonl>`.
|
||||
const timingsPath = process.env.RUN_TESTS_TIMINGS_FILE || DEFAULT_TIMINGS_PATH;
|
||||
const fileWeight = makeFileWeigher(loadTestTimings(timingsPath));
|
||||
|
||||
// node:test does not exit until the event loop drains. A unit test that leaks
|
||||
// an open handle (un-terminated Worker, un-killed child_process, ref'd timer)
|
||||
@@ -609,35 +815,19 @@ function main() {
|
||||
const forceExit = nodeMajor >= 22 && !process.env.RUN_TESTS_NO_FORCE_EXIT;
|
||||
|
||||
const FIXED_OVERHEAD = process.execPath.length + '--test'.length + concurrency.length + (forceExit ? '--test-force-exit'.length + 1 : 0) + 8;
|
||||
const chunks = [];
|
||||
let current = [];
|
||||
let currentLen = FIXED_OVERHEAD;
|
||||
let currentWeight = 0;
|
||||
for (const file of selected) {
|
||||
const add = file.length + 1; // +1 for the inter-arg separator
|
||||
if (
|
||||
current.length > 0 &&
|
||||
(currentLen + add > MAX_CMDLINE_CHARS || currentWeight >= MAX_FILES_PER_CHUNK)
|
||||
) {
|
||||
chunks.push(current);
|
||||
current = [];
|
||||
currentLen = FIXED_OVERHEAD;
|
||||
currentWeight = 0;
|
||||
}
|
||||
current.push(file);
|
||||
currentLen += add;
|
||||
currentWeight += fileWeight(file); // heavy install files count for more
|
||||
}
|
||||
if (current.length > 0) chunks.push(current);
|
||||
const chunks = packChunks(selected, {
|
||||
weightOf: fileWeight,
|
||||
maxWeight: MAX_FILES_PER_CHUNK,
|
||||
maxChars: MAX_CMDLINE_CHARS,
|
||||
fixedOverhead: FIXED_OVERHEAD,
|
||||
});
|
||||
|
||||
// A chunk that still hangs (a leak the backstop somehow misses, or a wedged
|
||||
// subprocess) must fail loudly rather than silently burn the job's wall-clock
|
||||
// budget until the CI runner cancels the whole job. Default 10 min per chunk:
|
||||
// well above a healthy chunk (~4-5 min on the windows lane) but below the 20m
|
||||
// job cap. Operator/test override via RUN_TESTS_CHUNK_TIMEOUT_MS.
|
||||
const chunkTimeoutMs = process.env.RUN_TESTS_CHUNK_TIMEOUT_MS
|
||||
? Number(process.env.RUN_TESTS_CHUNK_TIMEOUT_MS)
|
||||
: 600000;
|
||||
const chunkTimeoutMs = positiveNumberEnv(process.env.RUN_TESTS_CHUNK_TIMEOUT_MS, 600000);
|
||||
|
||||
let firstFailureExit = 0;
|
||||
for (let i = 0; i < chunks.length; i++) {
|
||||
@@ -685,4 +875,15 @@ if (require.main === module) {
|
||||
runMain(main);
|
||||
}
|
||||
|
||||
module.exports = { suiteOf, ensureBuiltArtifacts, ensureBuiltHooks, parseShardArg, selectShard };
|
||||
module.exports = {
|
||||
suiteOf,
|
||||
ensureBuiltArtifacts,
|
||||
ensureBuiltHooks,
|
||||
parseShardArg,
|
||||
selectShard,
|
||||
positiveNumberEnv,
|
||||
loadTestTimings,
|
||||
makeFileWeigher,
|
||||
packChunks,
|
||||
DEFAULT_TIMINGS_PATH,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user