* chore(#4603): retire the test-full CI job Phase 2 (#4591) added test-conformance but left test-full (the pre-existing full-suite Windows/macOS replay) running unchanged, gated on the same full_matrix flag, downgraded only from a hard gate to a non-blocking ::warning:: -- framed as "a non-gating safety net for one release cycle." No phase or issue ever retired it. Result: every full_matrix=true PR ran 10 OS-specific jobs (test-full's 6 + test-conformance's 4, purely additive) instead of the original 6 -- the epic's own goal (reduce runner-minutes) was measurably regressing, not improving, for the majority of PRs. This phase was missing from the original 4-phase epic decomposition; the epic (#4589) has been amended to add it as Phase 5 (see its comment thread), and this issue was filed as the tracked sub-issue. Deletes the test-full job from .github/workflows/test.yml entirely, along with every reference to it: required-tests' needs/FULL_TEST_RESULT warning branch, ci-timeout-report.cjs's JOB_RULES entry, ci-test-job-timeout-budget.test.cjs's LANE_COSTS/staticLanes/testFullRule entries, ci-test-scope.test.cjs's test-full-specific tests (preserving three unrelated tests that were nested in the same describe block, moved under a renamed describe rather than deleted), and docs mentions. test-conformance is now the sole gating signal for real-OS coverage. Two separate defects found and fixed while auditing every test-full reference: - tests/ci-pr-mergeability.test.cjs's GATED['test.yml'] safety-critical array (jobs that must needs: the mergeability preflight) had test-full but was missing test-conformance entirely -- Phase 2 never added it. Verified the real workflow wiring was already correct (test-conformance does have needs: [changes, preflight]); this was a test-coverage gap, not a live defect. Fixed by swapping the array entry. - docs/TESTING-SUITES.md's "## CI matrix" section was substantially stale independent of this phase (predating even #2952's coverage-gate split). Rewritten against the real, current job topology, verified directly against test.yml rather than trusted from memory. An isolated code-review pass found and fixed two minor inaccuracies in the rewritten docs table (two jobs' "Gated on" column didn't match their real if: condition exactly). An isolated security-review pass found no qualifying findings -- every compute-provisioning job already carries needs: preflight directly, unaffected by this deletion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): isolate 7 more heavy test files from chunk-weight packing `next`'s own push-triggered Tests run failed: `conformance test (windows-latest, 24, shard 2/3)` chunk 3/6 was killed after 600019ms. Root cause: state.test.cjs (weight 21.35, measured) was packed alongside companions by run-tests.cjs's LPT chunk packer, the same failure mode that previously hit codex-config.test.cjs (weight 17.87) twice and got a dedicated fix (ISOLATED_HEAVY_FILES, #4497) -- but state.test.cjs was never added to that set. This is a direct, unintended consequence of epic #4589 Phase 2: the new platform-conformance-tier job packs only ~546 files per shard (vs. the ~950-file full suite the packer used to balance against), so the same absolute-weight outlier now represents a larger share of a smaller, more homogeneous pool -- the LPT packer has fewer light files to pad around it with. This was a real, foreseeable side effect of shrinking the packing pool that nobody checked for when Phase 2 shipped. A first attempt at this fix hand-picked 4 candidates by eyeballing a truncated weight list and missed 3 heavier ones -- caught by an isolated code-review pass (blocker: emitted-attribution.test.cjs at 66.2% of the Windows chunk budget, install-minimal-hooks.test.cjs at 61.1%, install.test.cjs at 47.1%, all above codex-config.test.cjs's own 44.7% -- the ratio that already proved dangerous twice). Corrected by systematically computing weight/budget for every unit-suite file and isolating everything at or above that same ratio: 7 files total, plus the pre-existing codex-config.test.cjs (8 total). Added a durable regression test (tests/run-tests-harness.test.cjs) that re-derives this exact computation from the live tests/test-timings.json on every run, so a future heavy file crossing this threshold fails the test instead of silently reintroducing this failure -- not just a one-time manual sweep. Verified end-to-end: simulated the real 3-way windows shard split of the actual conformance-tier file list with the real packing functions. Max packable-chunk weight across all 3 shards is now 27.04 / 24.10 / 23.91 (shard 2 is the exact shard that failed on next), comfortably under the 40 budget -- versus 40+ and a 600s kill before this fix. A second isolated code-review + security-review pass on the corrected diff found nothing further. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -655,7 +655,7 @@ describe('ci-pr-mergeability: CLI', () => {
|
||||
|
||||
/** workflow file -> job ids that must be gated on the preflight. */
|
||||
const GATED = Object.freeze({
|
||||
'test.yml': ['lint-tests', 'test', 'test-inert', 'test-full', 'coverage-gate', 'qa-loop-walk', 'required-tests'],
|
||||
'test.yml': ['lint-tests', 'test', 'test-inert', 'test-conformance', 'coverage-gate', 'qa-loop-walk', 'required-tests'],
|
||||
'install-smoke.yml': ['smoke', 'smoke-unpacked'],
|
||||
'mutation.yml': ['detect', 'mutation-gate'],
|
||||
'security-scan.yml': ['security'],
|
||||
|
||||
@@ -96,17 +96,6 @@ const LANE_COSTS = [
|
||||
// once one exists, the same discipline every other entry here follows.
|
||||
evidence: 'run 33278340189 — 13m48s completed; run 33285384930 — CANCELLED at ~14m51s (#4070)',
|
||||
},
|
||||
{
|
||||
job: 'test-full',
|
||||
measuredMinutes: 27,
|
||||
// Lane moved from windows-22 to windows-latest/24 and is now sharded three
|
||||
// ways. Worst observed shard is `full test (windows-latest, 24, shard
|
||||
// 3/3)`: 26m18s on run 32614439702 (shard 2/3 23m36s, shard 1/3 19m22s),
|
||||
// and 23m17s for shard 2/3 on run 32603886007. The previous 18m59s /
|
||||
// windows-22 figure recorded here predated this cost and is stale — the
|
||||
// lane is measurably slower now, not merely relabeled.
|
||||
evidence: 'run 32614439702 — 26m18s, windows-latest/24 shard 3/3',
|
||||
},
|
||||
{
|
||||
job: 'coverage-gate',
|
||||
measuredMinutes: 2,
|
||||
@@ -252,7 +241,6 @@ test('mutation.yml mutate job timeout budgets (#4036)', async (t) => {
|
||||
test('near-cap check CI_JOB_TIMEOUT_MINUTES literals match each job\'s own timeout-minutes (#4036)', async (t) => {
|
||||
const staticLanes = [
|
||||
{ workflowFile: 'test.yml', jobKey: 'test', envLiteral: '32' },
|
||||
{ workflowFile: 'test.yml', jobKey: 'test-full', envLiteral: '45' },
|
||||
{ workflowFile: 'test.yml', jobKey: 'test-conformance', envLiteral: '45' },
|
||||
{ workflowFile: 'install-smoke.yml', jobKey: 'smoke', envLiteral: '12' },
|
||||
];
|
||||
@@ -281,11 +269,6 @@ test('near-cap check CI_JOB_TIMEOUT_MINUTES literals match each job\'s own timeo
|
||||
'test.yml jobs.test.name no longer starts with "test (" — update JOB_RULES in scripts/ci-timeout-report.cjs to match');
|
||||
assert.equal(testRule.test('test (ubuntu-latest, 24, shard 1/3)'), true);
|
||||
|
||||
const testFullRule = JOB_RULES.find((r) => r.workflowFile === 'test.yml' && r.jobKey === 'test-full');
|
||||
assert.ok(testWorkflow.jobs['test-full'].name.startsWith('full test ('),
|
||||
'test.yml jobs.test-full.name no longer starts with "full test (" — update JOB_RULES to match');
|
||||
assert.equal(testFullRule.test('full test (windows-latest, 24, shard 1/3)'), true);
|
||||
|
||||
const testInertRule = JOB_RULES.find((r) => r.workflowFile === 'test.yml' && r.jobKey === 'test-inert');
|
||||
assert.equal(testWorkflow.jobs['test-inert'].name, 'test (inert CI)',
|
||||
'test.yml jobs.test-inert.name changed — update JOB_RULES to match');
|
||||
|
||||
@@ -461,80 +461,9 @@ describe('test.yml changes job contract (#837)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('test-full shard matrix parity (#1212)', () => {
|
||||
// DEFECT.GENERATIVE-FIX: the sharded windows full-test lane has TWO surfaces
|
||||
// that must agree — the `shard:` matrix array (how many parallel jobs run)
|
||||
// and the `/N` denominator in `run-tests.cjs --suite unit --shard i/N` (how
|
||||
// many slices the runner partitions the suite into). If they diverge (e.g.
|
||||
// someone grows `shard: [1,2,3,4]` but leaves `--shard ${{ matrix.shard }}/3`),
|
||||
// shards silently overlap and one shard errors out. This parity assertion
|
||||
// fails the moment the two drift.
|
||||
describe('test.yml gating internals (#2472 / #4241)', () => {
|
||||
const yaml = require('js-yaml');
|
||||
|
||||
function loadTestFull() {
|
||||
const text = fs.readFileSync(path.join(WORKFLOWS_DIR, 'test.yml'), 'utf8');
|
||||
const doc = yaml.load(text);
|
||||
return { text, job: doc.jobs['test-full'] };
|
||||
}
|
||||
|
||||
test('distinct shard values are 1..N matching the --shard /N denominator, on every leg', () => {
|
||||
const { job } = loadTestFull();
|
||||
const include = job.strategy.matrix.include;
|
||||
assert.ok(Array.isArray(include), 'test-full matrix must enumerate `include:` rows');
|
||||
assert.ok(
|
||||
include.every(r => Number.isInteger(r.shard)),
|
||||
'every include row must carry an integer `shard:` key',
|
||||
);
|
||||
|
||||
const distinctShards = [...new Set(include.map(r => r.shard))].sort((a, b) => a - b);
|
||||
const n = distinctShards.length;
|
||||
|
||||
// Distinct shard values must be exactly 1..n (1-based, contiguous) so the
|
||||
// runner's cost-balanced shard selection covers every file with no gaps/overlaps.
|
||||
assert.deepStrictEqual(
|
||||
distinctShards,
|
||||
Array.from({ length: n }, (_, i) => i + 1),
|
||||
`distinct shard values must be 1..${n} (1-based, contiguous), got ${JSON.stringify(distinctShards)}`,
|
||||
);
|
||||
|
||||
// Every OS/node leg must appear once per shard (full cross-product) — no
|
||||
// leg may silently skip a shard, which would drop a third of its coverage.
|
||||
const legs = [...new Set(include.map(r => `${r.os}|${r['node-version']}`))];
|
||||
for (const leg of legs) {
|
||||
const [os, node] = leg.split('|');
|
||||
const shardsForLeg = include
|
||||
.filter(r => r.os === os && String(r['node-version']) === node)
|
||||
.map(r => r.shard)
|
||||
.sort((a, b) => a - b);
|
||||
assert.deepStrictEqual(
|
||||
shardsForLeg,
|
||||
distinctShards,
|
||||
`leg ${leg} must run all shards ${JSON.stringify(distinctShards)}, got ${JSON.stringify(shardsForLeg)}`,
|
||||
);
|
||||
}
|
||||
// Full cross-product: every (leg, shard) pair is present exactly once, so
|
||||
// the row count equals legs × shards with no duplicate/missing combination.
|
||||
const pairKey = r => `${r.os}|${r['node-version']}|${r.shard}`;
|
||||
assert.strictEqual(new Set(include.map(pairKey)).size, legs.length * n);
|
||||
assert.strictEqual(include.length, legs.length * n);
|
||||
|
||||
// Find the `--shard ${{ matrix.shard }}/<N>` denominator in the unit step.
|
||||
const unitStep = job.steps.find(
|
||||
s => typeof s.run === 'string' && s.run.includes('run-tests.cjs') && s.run.includes('--shard'),
|
||||
);
|
||||
assert.ok(unitStep, 'test-full must have a step running run-tests.cjs --shard');
|
||||
const m = /--shard\s+\$\{\{\s*matrix\.shard\s*\}\}\/(\d+)/.exec(unitStep.run);
|
||||
assert.ok(m, `could not parse --shard i/N denominator from: ${unitStep.run}`);
|
||||
const denominator = Number(m[1]);
|
||||
|
||||
assert.strictEqual(
|
||||
denominator,
|
||||
n,
|
||||
`shard count (${n}) and --shard /N denominator (${denominator}) must match — ` +
|
||||
`update both the per-row \`shard:\` values and the \`/N\` in the run command together.`,
|
||||
);
|
||||
});
|
||||
|
||||
// #2472: every job of a run must merge ONE base commit. Each job runs the
|
||||
// rebase-check step independently, minutes apart across the matrix, so
|
||||
// merging the moving branch ref lets jobs see different trees when the base
|
||||
@@ -591,19 +520,14 @@ describe('test-full shard matrix parity (#1212)', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('required-tests fan-in still needs test-full and keeps the protected name', () => {
|
||||
test('required-tests fan-in keeps the protected name', () => {
|
||||
// Hyrum's Law: branch protection requires a status check literally named
|
||||
// "Required tests". Renaming it (or dropping test-full from its needs)
|
||||
// would silently break the gate. Pin both.
|
||||
// "Required tests". Renaming it would silently break the gate.
|
||||
const text = fs.readFileSync(path.join(WORKFLOWS_DIR, 'test.yml'), 'utf8');
|
||||
const doc = yaml.load(text);
|
||||
const fanIn = doc.jobs['required-tests'];
|
||||
assert.ok(fanIn, 'required-tests job must exist');
|
||||
assert.strictEqual(fanIn.name, 'Required tests', 'the branch-protection check name must stay "Required tests"');
|
||||
assert.ok(
|
||||
Array.isArray(fanIn.needs) && fanIn.needs.includes('test-full'),
|
||||
'required-tests must `needs: test-full` so all shard legs aggregate into the gate',
|
||||
);
|
||||
});
|
||||
|
||||
test('workflow triggers on merge_group (#4241)', () => {
|
||||
|
||||
@@ -2875,9 +2875,20 @@ describe('analyzeChunkEvents (#3889)', () => {
|
||||
// its chunk and retrigger the per-chunk timeout two prior incidents already
|
||||
// hit. These tests pin partitionIsolatedFiles directly — the pure split, not
|
||||
// the chunk-execution loop around it.
|
||||
//
|
||||
// 2026-09-10 (#4603): epic #4589 Phase 2's platform-conformance-tier job packs
|
||||
// a much smaller file pool per shard than the full suite did, which exposed
|
||||
// the SAME failure on state.test.cjs (weight 21.35, heavier than
|
||||
// codex-config.test.cjs) on `next` itself. Re-running the same weight-table
|
||||
// analysis found two more unisolated files at or above the same ~45%-of-budget
|
||||
// threshold: run-tests-harness.test.cjs (31.23) and phase.test.cjs (23.31),
|
||||
// plus config.test.cjs (19.76) just under codex-config.test.cjs's own 45% but
|
||||
// still heavier than several already-risky files. All four added to
|
||||
// ISOLATED_HEAVY_FILES; see scripts/run-tests.cjs's own comment for the full
|
||||
// weight/budget accounting.
|
||||
const { ISOLATED_HEAVY_FILES, partitionIsolatedFiles } = require('../scripts/run-tests.cjs');
|
||||
|
||||
describe('partitionIsolatedFiles (#4497 codex-config.test.cjs chunk isolation)', () => {
|
||||
describe('partitionIsolatedFiles (#4497 codex-config.test.cjs chunk isolation, extended #4603)', () => {
|
||||
test('an isolated-heavy file is split out, in its own bucket, everything else stays packable', () => {
|
||||
const files = [
|
||||
'/repo/tests/a.test.cjs',
|
||||
@@ -2889,6 +2900,30 @@ describe('partitionIsolatedFiles (#4497 codex-config.test.cjs chunk isolation)',
|
||||
assert.deepStrictEqual(packable, ['/repo/tests/a.test.cjs', '/repo/tests/b.test.cjs']);
|
||||
});
|
||||
|
||||
test('#4603: every newly-isolated heavy file is split out individually, in original order', () => {
|
||||
const files = [
|
||||
'/repo/tests/a.test.cjs',
|
||||
'/repo/tests/state.test.cjs',
|
||||
'/repo/tests/b.test.cjs',
|
||||
'/repo/tests/phase.test.cjs',
|
||||
'/repo/tests/run-tests-harness.test.cjs',
|
||||
'/repo/tests/config.test.cjs',
|
||||
'/repo/tests/c.test.cjs',
|
||||
];
|
||||
const { isolated, packable } = partitionIsolatedFiles(files);
|
||||
assert.deepStrictEqual(isolated, [
|
||||
'/repo/tests/state.test.cjs',
|
||||
'/repo/tests/phase.test.cjs',
|
||||
'/repo/tests/run-tests-harness.test.cjs',
|
||||
'/repo/tests/config.test.cjs',
|
||||
]);
|
||||
assert.deepStrictEqual(packable, [
|
||||
'/repo/tests/a.test.cjs',
|
||||
'/repo/tests/b.test.cjs',
|
||||
'/repo/tests/c.test.cjs',
|
||||
]);
|
||||
});
|
||||
|
||||
test('matches by BASENAME, so it isolates regardless of platform path separator or directory prefix', () => {
|
||||
const files = [
|
||||
'C:\\repo\\tests\\codex-config.test.cjs',
|
||||
@@ -2918,7 +2953,66 @@ describe('partitionIsolatedFiles (#4497 codex-config.test.cjs chunk isolation)',
|
||||
assert.deepStrictEqual(partitionIsolatedFiles([]), { isolated: [], packable: [] });
|
||||
});
|
||||
|
||||
test('ISOLATED_HEAVY_FILES currently names exactly codex-config.test.cjs (documents the set the fix scoped to)', () => {
|
||||
assert.deepStrictEqual([...ISOLATED_HEAVY_FILES], ['codex-config.test.cjs']);
|
||||
test('ISOLATED_HEAVY_FILES currently names exactly the eight known-heavy files (documents the set the fix scoped to)', () => {
|
||||
assert.deepStrictEqual(
|
||||
[...ISOLATED_HEAVY_FILES].sort(),
|
||||
[
|
||||
'codex-config.test.cjs',
|
||||
'config.test.cjs',
|
||||
'emitted-attribution.test.cjs',
|
||||
'install-minimal-hooks.test.cjs',
|
||||
'install.test.cjs',
|
||||
'phase.test.cjs',
|
||||
'run-tests-harness.test.cjs',
|
||||
'state.test.cjs',
|
||||
].sort(),
|
||||
);
|
||||
});
|
||||
|
||||
// #4603: a durable guard, not a one-time snapshot. A first attempt at this
|
||||
// fix hand-picked candidates by eye and missed three heavier files (caught
|
||||
// by an isolated code-review pass) — this test closes that gap by
|
||||
// RE-DERIVING the same weight/budget computation from the live timings
|
||||
// table on every run, so a future test file crossing the same threshold
|
||||
// fails this test instead of silently reintroducing the per-chunk-timeout
|
||||
// failure this whole mechanism exists to prevent.
|
||||
test('#4603: no unisolated unit-suite file exceeds ISOLATED_HEAVY_FILES\' own established threshold', () => {
|
||||
const { suiteOf } = require('../scripts/lib/suite-detection.cjs');
|
||||
const table = require('../tests/test-timings.json');
|
||||
const timings = table.timings;
|
||||
const values = Object.values(timings).filter(
|
||||
(v) => typeof v === 'number' && Number.isFinite(v) && v >= 0,
|
||||
);
|
||||
const mean = values.reduce((sum, v) => sum + v, 0) / values.length;
|
||||
const WINDOWS_BUDGET = 40;
|
||||
|
||||
// The threshold is codex-config.test.cjs's OWN ratio — the exact file two
|
||||
// prior documented incidents proved dangerous — not an arbitrarily chosen
|
||||
// round number. This makes the test self-consistent even if
|
||||
// WINDOWS_BUDGET or the timings table changes: it always asks "is this
|
||||
// file at least as dangerous as the file we already know is dangerous?"
|
||||
assert.ok(
|
||||
Object.hasOwn(timings, 'codex-config.test.cjs'),
|
||||
'codex-config.test.cjs must remain in the timings table to anchor this threshold',
|
||||
);
|
||||
const codexRatio = timings['codex-config.test.cjs'] / mean / WINDOWS_BUDGET;
|
||||
|
||||
const exceedsThreshold = [];
|
||||
for (const [file, ms] of Object.entries(timings)) {
|
||||
if (typeof ms !== 'number' || !Number.isFinite(ms) || ms < 0) continue;
|
||||
if (suiteOf(file) !== null) continue; // suite-tagged files never enter this pool
|
||||
const ratio = ms / mean / WINDOWS_BUDGET;
|
||||
if (ratio >= codexRatio && !ISOLATED_HEAVY_FILES.has(file)) {
|
||||
exceedsThreshold.push(`${file} (${(ratio * 100).toFixed(1)}% of budget)`);
|
||||
}
|
||||
}
|
||||
|
||||
assert.deepStrictEqual(
|
||||
exceedsThreshold,
|
||||
[],
|
||||
`file(s) at/above codex-config.test.cjs's own danger ratio (${(codexRatio * 100).toFixed(1)}%) ` +
|
||||
`are not in ISOLATED_HEAVY_FILES: ${exceedsThreshold.join(', ')} — add them, following ` +
|
||||
`scripts/run-tests.cjs's ISOLATED_HEAVY_FILES comment for the pattern`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user