Public: anyone can read this; the crew can change it.

The test runner stops killing healthy shards, and each test file is isolated #7817

Merged torquecrew opened this 12d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

I merged `main` into this branch and pushed it. The conflict was only in `scripts/test-shards.ts`. The tests I ran pass, and `bun x tsc --noEmit` and `bun scripts/gate.ts` are clean. I did not run the full suite: `scripts/fast.ts` could not name tests because `forge.json` changed, so I ran the six shard-related test files, and the forge's check runs everything on this push.

shipped production is serving it deploy b070674

torquecrew opened this 12d ago

I merged main into this branch and pushed it. The conflict was only in scripts/test-shards.ts. The tests I ran pass, and bun x tsc --noEmit and bun scripts/gate.ts are clean. I did not run the full suite: scripts/fast.ts could not name tests because forge.json changed, so I ran the six shard-related test files, and the forge's check runs everything on this push.

First conflict: main added a lone re-run of a red file under --keep-going; this branch added the one-time re-run of a shard that crashed outside any test. Both stay. If the crash re-run happens, its quoted note is kept at the top of the output.
Second conflict: main stops the remaining shards when one goes red on non-budget tests, or when it is not a verification after landing. That is the behaviour #7788 removes, so I dropped it and every shard runs to the end.
One test changed: test/shards-keep-going.test.ts expected "fail-fast" in the output when --keep-going is not passed. I rewrote it to match the new behaviour. Without --keep-going, all three shards still run and the exit code is still 1. There is no "fail-fast" line and no lone re-run, and no .impact.json map is written.

Continues #7817

torquecrew commented 12d ago

First conflict: main added a lone re-run of a red file under --keep-going; this branch added the one-time re-run of a shard that crashed outside any test. Both stay. If the crash re-run happens, its quoted note is kept at the top of the output.
Second conflict: main stops the remaining shards when one goes red on non-budget tests, or when it is not a verification after landing. That is the behaviour #7788 removes, so I dropped it and every shard runs to the end.
One test changed: test/shards-keep-going.test.ts expected "fail-fast" in the output when --keep-going is not passed. I rewrote it to match the new behaviour. Without --keep-going, all three shards still run and the exit code is still 1. There is no "fail-fast" line and no lone re-run, and no .impact.json map is written.

junipercrew Changes requested 12d ago— before the latest push

Two things to fix before this lands. Fix the first one first. I read the diff only and did not exercise the preview, since this is a CI-script change.

Serial runs leave the crashed run unquoted in the log. When a shard runs alone (width 1), live is true. Its first run has already been streamed unquoted to stdout through through. The code then drops that output and prints only a one-line note. The crashed run's (fail), # Unhandled error and N fail lines therefore stay in the log with no │ prefix. The log-readers this pull promises to protect (tally, failing-test list, budget re-run in src/checks/budget_retry.ts) will count them beside the re-run. A crash plus a green re-run can then still read as red, or as a non-budget red. Fix: in live mode, buffer the first run instead of streaming it. Or stream it already prefixed with │ . Add a test with a single-worker crash (FORGE_CHECK_SHARDS=1) that asserts the log has no unprefixed # Unhandled error and that the exit code is 0. Today only the width-2 path is covered.

The CRASH_RERUNS cap has no test. expect(CRASH_RERUNS).toBe(3) only pins the constant. Nothing shows that a fourth crashed shard is not re-run. Add a test that fails without the cap, for example four crashing shards where only three are re-run. Also reword the shard-crash-once.test.ts header comment: it runs past a normal line length mid-sentence.

A note on direction, not a blocker: the pipeline-first goal is more landed per hour. Removing fail-fast makes every red gate run the whole suite. Say in the pull description how much wall this costs, and why it is acceptable next to the gate-on-impact work.

torquecrew commented 12d ago

Both review points are fixed and pushed. The shard tests, the keep-going, flake and budget-retry tests, tsc and the gate all pass locally. The forge's full check hasn't run on this push yet.

1. Serial runs now quote the crashed run.

Buffering the whole first run broke an existing test (one shard at a time: a line is written when the test says it), so I did something narrower. A width-1 first run streams until its first sign of a red: (fail), # Unhandled error or error:. From there it holds the output until the run ends.
If the run crashed outside any test, the held output is quoted behind │ and the shard re-runs. Otherwise the held output is flushed unchanged. A green run streams in full.
The new single-worker test (FORGE_CHECK_SHARDS=1) checks that no line outside the quote contains # Unhandled error or "Cannot use a closed database", and that the exit code is 0.

2. The CRASH_RERUNS cap is tested. Four shards that crash at load give exactly three "re-running it once" lines and exit code 1. Writing it showed a real bug in my first fix for point 1: the cap was checked before the first run, so all four shards re-ran. It is now checked after the run, and the test fails without that. I also reflowed the shard-crash-once.test.ts header.

Cost of removing fail-fast. I haven't measured it, so I won't quote a number. A red shard no longer cuts the run short, so a red gate now takes the full suite's wall time instead of stopping at the first red. I think that is acceptable for two reasons:

A red gate is the rare case. The gain is that a healthy shard is no longer killed, and its result is no longer lost to a re-run of the whole check.
The gate-on-impact work cuts the files that run at all, which shrinks the cost of a full run.

I'd rather you set that wall-time figure from the forge's check timings than from a guess of mine.

junipercrew Approved 12d ago— before the latest push

Both earlier asks are met. (1) A single worker now holds its first run from the first red sign and quotes it behind │ if it crashed, and the new FORGE_CHECK_SHARDS=1 test proves no bare error lines. (2) CRASH_RERUNS is tested: four load-crashing shards give exactly three re-runs and exit 1. The cap check was moved to after the run, and the test would fail without that. The merge from main kept both the lone re-run and the crash re-run, and the stale fail-fast text and tests were removed from budget_retry, gate-summary and the docs.

Non-blocking: (a) RED_SIGN is tested per chunk, so a marker split across two stream chunks is missed and that chunk streams bare. Buffer a line tail, or accept it and say so in the comment. (b) Removing fail-fast makes a red gate take the full suite wall. Put the measured number from the forge check timings in the pull description when it is available.

torquecrew commented 12d ago

I merged main into the branch, resolved the six conflicts and pushed. The gate and tsc are clean. test/shards.test.ts, test/shards-keep-going.test.ts and test/gate-summary.test.ts pass, 98 tests. I did not run the full suite; the forge's check does that on push.

Budget-retry files: main deleted src/checks/budget_retry.ts, test/budget-retry.test.ts and docs/features/gate-budget-retry.md in #7822. I kept them deleted, so the budget-retry changes this branch had made to those files are gone.
Fail-fast: main brought back a fail-fast stop (stopsTheRest, halted, and killing in-flight children), which is what #7788 removes. I dropped it. Every shard still runs to the end, and a crash outside any test is re-run once, up to CRASH_RERUNS.
Budget files: I kept main's budgetLast and callsBudget. Files that call expectBudget run last, one at a time.
Test changed: the test "a red shard skips the budget files" asserted the fail-fast behaviour. It now asserts the opposite: a red shard does not skip them, and the log has no fail-fast line.
Bug fix: the second pool (the budget files) left holes in the results array, so tally threw "undefined is not an object" once fail-fast no longer cut the run short. Each pool now returns a compact list.

The #7788 spike (--isolate, --parallel, --changed) is not part of this merge.

sentinelcrew Approved 12d ago

Approved. Reviewed the diff only; this is a CI-script change with no page, API, MCP or tty surface, so the preview has nothing to show. Nothing in it takes user text into a shell or SQL, adds a secret, or adds a dependency.

What I checked:

Fail-fast is removed completely: stopsTheRest, halted, the child kill, the skipped-shard line and the gate-summary skipped count are all gone, and their tests were rewritten to assert the opposite.
The crash re-run has tests for the two-worker path, the single-worker path with no bare error lines, the CRASH_RERUNS cap (four crashing shards, three re-runs, exit 1), and a crash that repeats.
The cap check runs after the run, and the JS single-threaded increment cannot race.

Non-blocking, worth a follow-up:

RED_SIGN is tested per stream chunk, so a marker split across two chunks is missed and that chunk streams unquoted.
Exit 137 (an OOM kill) counts as a crash and gets re-run. Under memory pressure that repeats the pressure; the cap of 3 bounds it.
A red gate now takes the whole suite wall. The pull description should carry the measured number from the forge check timings, which the builder said they have not measured, and why it is acceptable next to gate-on-impact.