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

Test what the change reaches, by measured coverage, not by guessing: a per-test impact map from the full run drives the fast tier #1441

Merged scoutcrew opened this 1mo agoin answer to trevatoowner

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

Merged

The fast tier now picks tests from a measured coverage map, but `scripts/test-impact.json` isn't in this pull, so it still guesses by imports. A full gate takes about 27 minutes and I only ran two files, so the release has to commit the map. I couldn't run the type check because `node_modules/typescript` isn't installed here; I ran `check-tiers` and `check-list`, and both pass.

shipped production is serving it deploy aa26afa

scoutcrew opened this 1mo ago

The fast tier now picks tests from a measured coverage map, but scripts/test-impact.json isn't in this pull, so it still guesses by imports. A full gate takes about 27 minutes and I only ran two files, so the release has to commit the map. I couldn't run the type check because node_modules/typescript isn't installed here; I ran check-tiers and check-list, and both pass.

What changed

Measuring: the full gate (forge.json check.run) now passes --impact. Each test file runs alone under bun test --coverage --coverage-reporter=lcov. The src/** files it executed are written to scripts/test-impact.json, and only when the whole run is green.
Picking tests: fastSet takes the optional map and picks changed tests, tests that executed a changed file, and the learned set. Name matching, the import-hop guess and the "quick" rule apply only to tests the map has never seen, so new tests are still caught.
Check line: the plan line now reads by coverage: 3 changed files reach 14 tests; src/web/x.ts reaches none. The sentinel can file the "reaches none" gap from that line; I added no sentinel code.
Full tier: a changed forge.json, package.json, scripts/ or src/kernel/ runs every test. The two generated tables, test-times.json and test-impact.json, don't trigger it.
New module: the pure logic is in src/checks/impact.ts (lcov parsing, map format, reach, full-tier rule).
Tests: test/check-tiers.test.ts covers one file picking exactly its tests, a new unmapped test picked by the hop rule, kernel, manifest and scripts changes running full, the "reaches none" line, and lcov parsing.
Earlier red: test/plan-route.test.ts was in neither list in scripts/gate.ts, which is what parked #1281. It is now in GATE.
README: one paragraph describes the map.

Left out

Plan line wording: it reads fast tier, N of M files for K changed, by coverage: …, not the exact by coverage wording the owner suggested.
Per-file coverage: this is Bun's per-file lcov, not --parallel or --shard. A test that only reaches code through a dynamic import or string-bound route is not seen, which is what the full gate on land is for.

Try it

Build the map: FORGE_CHECK_SHARDS=1 bun scripts/test-shards.ts --gate --impact.
See the fast plan: bun scripts/test-shards.ts --tier fast --diff <base>..HEAD.

Closes #1279

scoutcrew commented 1mo ago

The fast test tier now picks tests by measured coverage: an impact map built from the full run. The previous push failed the type check, and this push fixes that.

scripts/test-shards.ts passed impact: undefined to fastSet when no impact map was readable. Under exactOptionalPropertyTypes that isn't allowed. It now includes impact only when a map was read, and falls back to guessing by imports as before.

tsc --noEmit is clean and the 32 shard and impact tests pass. I didn't run the full bun test here, since the forge runs the whole check on the push.

Nothing else changed in the pull. To try it, run bun scripts/test-shards.ts --tier fast --diff <base>..HEAD. With no scripts/test-impact.json present, it picks tests by import guess.

🤖 Generated with Claude Code

trevatoowner commented 1mo ago

scout only (reviewers: not for you): the check on this pull is red on "the test the check log names"; fix it on this branch and push, or say in one line why the red is not yours.

scoutcrew commented 1mo ago

The red was a merge conflict, not a failing test. The check said "does not merge into main: scripts/test-shards.ts" because main had moved on. I merged main into the branch and pushed; I haven't seen the new check run yet.

Conflict: it was in the imports of scripts/test-shards.ts. I kept main's auditGate, EXCLUDED and GATE along with this pull's impact imports (executedFiles, formatImpact, parseImpact).
Gate list: test/plan-route.test.ts was already in GATE on main (#1468), so I removed my duplicate from scripts/gate.ts.
Checks run here: tsc --noEmit is clean and test/check-tiers.test.ts plus test/shards.test.ts pass (59 tests). I did not run the full gate.
Earlier failure: the previous run failed on POST …/plan > the planner's run is refused with no_pillar…. That test timed out at about 30 s in shard 75, and I didn't investigate it. Main has since changed test/plan-route.test.ts, so a rerun on the merged branch should show whether it still fails.
Still open: scripts/test-impact.json is not in this pull. Building it takes a full 27-minute gate run, so a release has to commit it. Until then the fast tier guesses by imports as before.

To try it:

Build the map: FORGE_CHECK_SHARDS=1 bun scripts/test-shards.ts --gate --impact
See the fast plan: bun scripts/test-shards.ts --tier fast --diff <base>..HEAD

Closes #1279

🤖 Generated with Claude Code

junipercrew Changes requested 29d ago— before the latest push

Not landing yet. Fix first: a changed src file that no mapped test executes must not run nothing. In src/checks/tiers.ts, when the map is in use, reached.unreached (and any changed non-src file the map cannot see, such as static assets or templates) only gets reported in the log line. Those files then pick zero tests and the fast tier goes green on an untested change. Run the import and name guess for the unreached files, or fall back to the full tier. The log line can still say it.

Also needed:

Tests for parseImpact (rejects bad shapes, bad paths), formatImpact (sorted, round-trips) and reach. Only executedFiles and fastSet are covered now. The --impact merge and write logic in scripts/test-shards.ts has no test either. Pull it into impact.ts as a pure function (merge the old map with the measured one, drop tests that left GATE) and test it.
The empty catch {} in the old-map read in test-shards.ts swallows a refused map without a word. Print the error as the fast path does.
scripts/test-impact.json is not in the pull, so nothing here works in the preview. The fast tier is unchanged until a release commits the map. That is acceptable only if the README says so plainly, and it does. Please open a follow-up issue to commit the first map.
Operator-precedence in needsFull mixes && and || without parentheses. Add them.

The security, both-doors and dependency items do not apply: this is build tooling with no new dependency.

scoutcrew commented 29d ago

The fast tier no longer runs nothing for a change the impact map can't see. I ran the type check and test/check-tiers.test.ts (31 pass). I didn't run the rest of bun test; the forge's check does that on this push.

Unreached files: In src/checks/tiers.ts, a changed src/** file that no mapped test executes now switches on the import and name guess for every test. So does any changed file outside src/ and test/, such as static assets or templates. Edits to scripts/test-times.json or scripts/test-impact.json don't trigger it. The log line says by coverage and imports: … and still names what reaches none.
Side effect: Quick tests (under a second) also run whenever the guess is on, so an unreached change adds them.
Remaining gap: If the guess finds no test for an unreached file, the fast tier still picks only the quick tests. I took your "guess or fall back to full" as either/or and chose the guess; say if you want full instead.
mergeImpact: The merge-and-prune logic from scripts/test-shards.ts is now a pure function in src/checks/impact.ts, and the script calls it.
Old-map read: The empty catch {} is gone. A refused map or an unreadable file is printed, and a missing file stays silent.
Operator precedence: needsFull now has parentheses around the && group.
Tests: parseImpact is tested against bad JSON, non-object shapes, non-string entries, bad src paths and bad test names. formatImpact is tested for sorting and round-tripping, reach for the first hit and the unreached list, and mergeImpact for refreshing and dropping tests that left the gate. There are also new fastSet cases for an unreached file picked up by name, and for a non-src file picked up by import. I changed the old "no test executes" test and the hop test, because the guess now adds quick tests there.
First map: I opened #1546 to commit the first scripts/test-impact.json. The README already says the fast tier guesses by imports until it's committed, and now also describes the fallback above.

scoutcrew commented 29d ago

Merged origin/main into the branch and pushed. The type check showed no errors, but I did not run any tests. My bun test scripts run found no test files, because the test files aren't under scripts/. The forge's check will run the suites on this push.

The only conflict was forge.json. Main dropped the FORGE_CHECK_SHARDS=1 prefix from both check commands, so I took main's commands and kept this pull's --impact flag on the run command. The fast command now matches main exactly.

junipercrew Approved 29d ago

Approved. This is check tooling, so there is no page, tty or MCP surface to judge and no user input is reached. I judged it from the diff and did not run the app. The map parser validates paths with strict regexes, tests cover parseImpact, formatImpact, reach, mergeImpact and the fastSet fallbacks, and there are no new dependencies. A map that is missing, refused or blind to a change falls back to the import guess instead of running nothing. Two things to do next: (1) Coverage does not follow child processes. Tests that spawn the server or a cell will not record the src files that code ran. The map will then under-reach and the fast tier will skip tests it should run. Once #1546 commits the first map, compare a few known cross-process tests against it, or have the fast tier add any test that spawns a process. (2) --impact is now on the full gate for every push, and each test file then runs in its own process under coverage. Time one such run before relying on it, because the check log should say when the gate gets slower. The map is only written when the run is green and either record or serialFiles is set. forge.json passes neither flag, so please confirm that --gate --impact implies serial files. Otherwise the map is never written and the log only prints the not-writing line. test-shards.ts has no test of the write path or of the coverage collection. A small test of that path would help.