Repository navigation
test: hold each root script to a coverage floor - #841
Conversation
The root node:test suite ran with no coverage at all, so nothing noticed when a script's tests stopped reaching part of it. Every jest package has a threshold; these files had none. `pnpm test:scripts` now runs the suite with Node's coverage on, limited to scripts/, and prints the table through the spec reporter on every run. A second reporter, scripts/coverage-floors.mjs, compares each file against its row in scripts/coverage-floors.json and fails the run when a file drops below its floor, when a loaded file has no row, or when a row names a file no test loaded. `pnpm test` and `pnpm all` both call it, so CI's existing Test step gates it with no workflow change. Per file rather than Node's global --test-coverage-* flags because the total is dominated by the largest files. check-conformance.mjs alone is a quarter of the measured lines, so a small script can lose half its coverage and move the total by about a point. Floors are today's numbers rounded down to a whole percent, measured on Node 22, 24 and 25. They are a record, not a target: lowering one in the same change is the documented fix when the gap is a branch only a hollow test could reach. Floors are judged only on a green run. Without bulma-ui/dist the suites fail exactly as before, with their own "build first" messages, plus one line saying the floors were not checked. Closes #724
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://484672c0.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 1 blocking · 5 advisory
| N | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟠 Major | Robustness | Branch floor of 89 is cleared by one branch on a figure measured to swing by two (539↔541 of 605 across 7 runs); 538/605 = 88.93% reds the Test step on every open PR |
scripts/coverage-floors.json:33 |
| 2 | 🔵 Advisory | Coverage | A script no test loads is outside the gate entirely — gen-component-catalog.mjs and exports-resolve-fuzz.mjs today, and any new script tomorrow. The “loaded file with no row” rule structurally cannot see them. Disclosed in the PR body, but not where a contributor reads the gate |
scripts/coverage-floors.mjs:13 |
| 3 | 🔵 Advisory | Correctness | PR body: “counts one branch more or less from run to run, which stays inside its floor” — the measured swing is two branches, and the low state clears the floor by one. Maintainer-facing; the body is not editable from a review | PR body |
| 4 | 🔵 Advisory | Robustness | A floor of 0 for functions is an inert row: file[field] < 0 is never true, so that metric is switched off for this file permanently rather than recording a low bar |
scripts/coverage-floors.json:9 |
| 5 | 🔵 Advisory | Coverage | The default fail runs only in the unmeasured nested process, so the one line that actually reds CI is the one line this file’s own row records as uncovered. Documented and accepted, but the safety net for it is the e2e pair, not the floor |
scripts/coverage-floors.mjs:127 |
| 6 | 🔵 Advisory | Robustness | .github/CLAUDE.md:339 still describes root pnpm test as running node --test "scripts/*.test.mjs" — still true, now by way of test:scripts. .github/** is human-authored, so this is not the fix agent’s to take |
.github/CLAUDE.md:339 |
Overall: The design is right, and the reasoning in the module comment is unusually good — per-file rather than a total is the correct read of #724, “judge only a green run” is the correct read of the no-build case, and the e2e pair that drives a real node --test in both directions is the correct answer to a reporter that can only fail a run by side effect. The riskiest part is not the code but the data file: the floors were measured on macOS and rounded down to a whole percent, which hands each row somewhere between 0.00 and 1.00pp of slack depending on where its number happened to land, and seven runs here show one metric moving further than its slack allows. Look at finding 1 first — it is a one-character edit, and the cost of skipping it is a red Test step on PRs that never touched scripts/.
Residual risk:
- macOS floors vs. the first Linux measurement — refuted for 35 of 36 rows.
pnpm test:scriptsis green here on Linux / Node 24.21.0 afterpnpm run build; all 36 rows report at or above floor, 1037 tests pass. Repeated 7 times, and onlyrender-triage-comment.mjsmoved (finding 1). - Checkout without
bulma-ui/dist— refuted. Run before building, the suites failed on their own pre-existingbulma-ui/dist is absent/eslint-plugin/dist is absentassertions, the reporter printed exactly[coverage-floors] Not checked: tests failed, …and piled on nothing, and the run still exited 1. - A conditionally-skipped test making a live row read as “no test loaded” — refuted. No
skipoption,.skip, or platform gate exists in anyscripts/*.test.mjs; the onlyprocess.envuse is a set/restore insiderender-triage-comment.test.mjs. --test-coverage-excludereplacing Node’s default node_modules exclusion — refuted. The include glob is anchored atscripts/**/*.mjsand there is noscripts/node_modules, so nothing leaks into the table.- Prettier expanding the compact JSON rows and breaking the paste-in message — refuted.
format:checkglobs only ts/tsx/js/jsx/mjs/md/mdx;.jsonis not in it.pnpm format:checkandeslinton the three new files are both clean. (A bareprettier --checkon the JSON does want to expand it — a trap for anyone who later widens that glob, not a failure today.) pnpm run check:conformancefails here only onversion-regression, from this sandbox’s shallow clone — unrelated to the diff.fragile-proseandturbo-tasks, the two rules the new prose and the newallsegment could have tripped, both pass.- Open: the failing-floor path is verified only through the fixture. The PR’s e2e case proves the exit code with a real
node --test, but with one reporter; the shipped command runs two on the same destination. I could not re-drive that combination in this sandbox. Low risk — the reporter yields only after draining the stream, and its success line does land last in the real run — but it is unchecked. - Open: a test added under
scripts/lib/never runs. The run glob isscripts/*.test.mjswhile the new coverage exclude is the broaderscripts/**/*.test.mjs, so such a file would be silently un-run and silently un-measured. Pre-existing, but the asymmetry between the two globs now invites it. - Open: the include glob is
*.mjsonly. Ascripts/foo.jsor.tssits outside the gate with no complaint. None exists today.
🏄 Solid set, dude — this one actually reads the wave instead of guessing at it: per-file floors, a red run left un-judged, and a real
node --testkeeping the exit code honest. Only thing is that one floor parked a single branch above the tide line, and I watched the tide move twice that far. Drop it a point and paddle out. 🤙
Review of #841 found render-triage-comment.mjs's branch floor of 89 cleared by less than one branch. The noise behind that is V8 counting the never-run `?? 'unknown'` fallback in its execution-file error path in some runs and not in others, so the file measures 246 of 275 or 246 of 276. Rounding down to a whole percent leaves less than one unit of margin on any count under a hundred, so most rows had the same problem. A row is now the percentage the file would measure with two fewer covered lines, branches or functions than its worst run, rounded down. That survives two units of noise either way, which the test sibling checks over every count up to 600 rather than on examples. A metric at 100% keeps a floor of 100: it has no never-run code for the noise to come from, and a new function nothing calls is what the gate is for. The row a failure message offers for a new file uses the same rule. The header also says what the gate cannot see (a script no test loads at all) and that a row of 0 is no bar yet rather than a bar.
|
deep-review: fresh 73c543b replaces the hand-set floors with a rule and regenerates the table. Review the gate as it stands now. |
Preview DeploymentPreview URL: https://b8dc88f2.bestax.pages.dev |
The two-unit allowance went on all three metrics, but the only noise ever measured is V8 counting a never-run branch in some runs and not others. Lines and functions do not move between runs, and on small files the allowance cost the gate its point: fetch-retry.mjs functions dropped to a floor of 50, so four new functions nothing calls would pass, and gen-skills-rosters.mjs functions reached 0, which can never fail. The allowance is now per metric, two on branches and none on lines or functions, which go back to their rounded-down measured value. Branch rows keep their numbers. A covered metric also no longer gets a floor of 0 from the allowance. It gets 1, the smallest bar there is, which fails only if the tests stop reaching that metric, and only when two more never-run branches still clear it. A row of 0 now means the file measured 0. The row a failure message offers for a new file follows the same rule, and the property case sweeps branches only.
Preview DeploymentPreview URL: https://252668b0.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 2 blocking · 5 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Robustness | The floor side is guarded against a missing number, the measured side is not: a renamed coveredBranchPercent makes every metric pass while the reporter prints success |
scripts/coverage-floors.mjs:159 |
| 2 | 🟡 Minor | Robustness | Nothing reads the gate back — dropping --test-reporter=./scripts/coverage-floors.mjs removes it silently, while dropping --experimental-test-coverage is caught |
package.json:10 |
| 3 | 🔵 Advisory | Robustness | The noise mechanism the header and the PR body describe is not the one this platform shows: on Linux / Node 24.21.0 the drift is ±2 covered branches (541↔539 of 605), not ±1 on the total, so "The branch noise moves a count by one; two leaves a unit to spare beyond it" understates it by the whole allowance. Deleting that clause leaves the rest true — and the rule it justifies is the correctly shaped one for covered-side drift, which is the lucky part. | scripts/coverage-floors.mjs:90 |
| 4 | 🔵 Advisory | Coverage | A uniform two-covered-unit allowance erases the branch bar where the denominator is tiny: gen-api-docs.mjs 33.33 → floor 1, gen-skills-rosters.mjs 60.00 → floor 20, so losing 2 of 5 covered branches passes. Same shape as the functions case already fixed, now confined to two rows. |
scripts/coverage-floors.json:8 |
| 5 | 🔵 Advisory | Robustness | Every lines and functions floor is exactly Math.floor of a Node 24 run, so those metrics carry zero margin, and several sit under one unit: check-conformance lines 70.11 vs 70 is about 3 lines, auto-close-duplicates 50.19 vs 50 and consumer-sbom-meta 98.16 vs 98 are both under one line. Deliberate, but it makes pnpm test version-sensitive for a contributor on Node 22 (what .nvmrc pins), which I could not test — only 24 is installed here. |
scripts/coverage-floors.json:1 |
| 6 | 🔵 Advisory | Coverage | gen-api-docs.mjs functions is 0 of 12, an inert row by construction, and its lines floor is 17 — the header is honest about this, but it means the largest uncovered script is gated on one metric of three. |
scripts/coverage-floors.json:9 |
| 7 | 🔵 Advisory | Correctness | docs/docs/guides/getting-started/contributing.md:64 already tells readers what pnpm test covers and was not updated alongside CONTRIBUTING.md; nothing syncs the two. |
docs/docs/guides/getting-started/contributing.md:64 |
Overall: The gate is sound and the regeneration is faithful — I re-derived all 36 rows from four live pnpm test:scripts runs and every one is exactly what the rule produces (lines and functions are Math.floor of the measured percentage, branches are floor(100 * (covered - 2) / total) wherever the counts are recoverable), so this is a rule applied mechanically rather than a hand edit. The earlier blocking row is genuinely fixed and with room: the drift is real and bistable (render-triage-comment.mjs branch alternates 89.42 and 89.09 and nothing else moves), and floor 88 fails only below 532 of 605 — seven covered branches below the worse state. The riskiest part is not the numbers but that nothing outside this module asserts the module is still wired in, so read finding 2 first and the asymmetric guard in finding 1 second. I also settled the one thread the previous pass left open (the per-metric allowance): verified against the current code and resolved.
Residual risk:
- Drift larger than the allowance — refuted for the platform CI runs on. Four runs, bistable 89.42 / 89.09 with every other row byte-identical, and floor 88 tolerates seven lost covered branches from the worse state rather than the two the allowance names.
- Rows at 100 that are fed by spawned children — open.
handoff-message.mjsreaches 100 on all three metrics partly throughspawnSync(process.execPath, [SCRIPT, ...])children, whose coverage merges only because they inheritNODE_V8_COVERAGE;sanitize-repro-draft.mjsandparse-scan-verdict.mjshave the same shape. A metric at 100 keeps a floor of 100 by design, so one missed merge reds every PR with a message telling the reader to cover lines their tests already cover. Stable across four runs and loud rather than silent, so not a hole — but it is also the likeliest source of the covered-side drift in finding 3, and unlike a V8 enumeration quirk its magnitude is not pinned by anything measured here. - A script no test loads stays outside the gate — confirmed and documented.
gen-component-catalog.mjsandexports-resolve-fuzz.mjsappear in neither the coverage table nor any test import (the single mention is a comment inpublishable-manifests.test.mjs);gen:catalog:checkstill covers the first, nothing covers the second. - A red run is not judged — by design, so a change that both breaks a test and drops coverage is measured only once the test is fixed. The end-to-end case pins the behaviour, and the exit code is 1 either way.
- A lost test sibling taking a script out of the gate — refuted. The row-with-no-loader rule fails loudly, is covered by a unit case, and the real run shows 36 measured files against 36 rows.
- Floors never ratchet — stated and intended, so a file whose coverage climbs and then falls back is only held to where it stood when its row was written. Nothing in CI notices a row that has drifted far below its file.
🏄 Fourth set on this wave and it lines up clean, dude — the rule does the work and the table is just the rule wearing a jacket. I watched that gnarly render-triage branch bob up and down four times and it never dipped under the line. Only thing left is that nobody is watching the leash tying the gate to
pnpm test, so tie that off and paddle out.
…ired in The floor side was guarded against a missing number and the measured side was not. A field Node renamed in its coverage summary compares false against every floor, so every file would pass and the reporter would print success. Every figure the reporter reads, percentages and counts alike, must now be a real number or that file fails by name. Checking the counts too stops a new file being offered a made-up row of zeros. Nothing read the wiring back either. Dropping the reporter flag from test:scripts left the suite green with no gate. The test sibling now reads the root package.json and fails if test:scripts stops turning coverage on or loading the reporter, or if `test` or `all` stops running it. The header no longer claims the branch noise moves a count by one. It moves in both how many branches V8 counts and how many it counts as covered, and the allowance covers both.
|
On the advisories:
|
Preview DeploymentPreview URL: https://eceac500.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 2 resolved · 0 open
| # | Severity | Area | Finding | Location | Status |
|---|---|---|---|---|---|
| 1 | 🟡 Minor | Robustness | The floor side was guarded against a missing number; the measured side was not, so a renamed Node summary field would have read as a pass on every file at once | scripts/coverage-floors.mjs:178 |
✅ Verified fixed in bcb1c81 |
| 2 | 🟡 Minor | Robustness | Nothing read the wiring back: dropping the reporter flag from test:scripts left the suite green with no gate |
package.json:10 |
✅ Verified fixed in bcb1c81 |
What I checked
1 — the measured-side guard. checkFloors now screens every figure it reads before any comparison: the flatMap over METRICS yields all nine summary fields (three percentages plus six counts), and !Number.isFinite catches both a renamed field (undefined) and a NaN, so the file is reported by name and the loop continues rather than reaching file[field] < floor[name] — which was false on either operand. Widening it to the counts is load-bearing rather than incidental: the missing-row branch calls pct(file[field]) and measuredRow reads the counts, so a renamed count would otherwise have offered a new file a paste-in row of zeros. The guard sits ahead of the floors[key] lookup while measured.add(key) stays behind it, so an unusable file yields one problem instead of also tripping the orphan-row rule. The end-to-end "floors that hold" case does pin this against Node real coverage event as the author states: it asserts an exit status of 0, which a rename would break via fail().
2 — the wiring assertion. The new sibling case reads the root manifest and splits test:scripts with the repo own sh tokenizer (scripts/lib/shell-words.mjs), so the quoted globs do not confuse the word split. The coverage assertion is a word-level includes rather than a substring; the reporter assertion collects both the --test-reporter=X and two-word --test-reporter X spellings and compares resolve(REPO, path) against the absolute module path, so a respelled path still matches while a dropped flag leaves only spec and fails with the loaded reporters named.
Empirically: full pnpm build then pnpm test:scripts on this checkout (Linux, Node 24.21.0) — 1043 pass, 0 fail, all three new cases green, and the reporter prints "Every measured file (36) is at or above its floor", confirming the widened nine-field guard does not false-positive on real Node output.
Overall: This pass settled the two threads the earlier review left open and reviewed no commits — it is not a fresh review of bcb1c81 or c969f5a, and it raises no new findings. Both fixes are real, and each lands slightly wider than what was asked for in ways that hold up under reading and under the suite. The riskiest part of the change remains the floor table itself rather than this round edits, and it is green end to end on a fully built checkout.
Residual risk:
- Renamed summary fields — refuted for the silent-pass shape: all nine fields are screened, and the end-to-end case reads Node real event, so a rename fails the run rather than quietly passing it.
- The gate being unwired — the reporter-flag half, the edit most likely to happen, fails loudly. The
test/allhalf is self-referential (a change droppingtest:scriptsfrom both entry points also stops the case running); recorded here rather than reopened, since it is exactly the assertion the original finding asked for. - Include and exclude globs unpinned — refuted: a widened include surfaces as files with no row, a narrowed one as rows no test loaded, and Node excludes test files from coverage by default.
🏄 Two clean cutbacks on the last two open threads — the author paddled back out and patched both wider than the wave asked for, the suite came in 1043-for-1043, and the gate is holding its line. Nothing left in the water on this one.
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
# Conflicts: # package.json
Preview DeploymentPreview URL: https://71136025.bestax.pages.dev |
|
🎉 This PR is included in version 5.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Closes #724.
The root
node --testsuite ran with no coverage, so nothing noticed when a script's tests stopped reaching part of it. This adds the report and a gate, in the shape the numbers asked for.pnpm test:scriptsruns the suite with Node's coverage on, limited toscripts/**(test files excluded), and prints the coverage table on every run. A second reporter,scripts/coverage-floors.mjs, compares each file with its row inscripts/coverage-floors.jsonand fails the run when:pnpm testandpnpm allboth call it, so CI's existing Test step gates it with no workflow change.Why per file. The issue suspected one threshold was the wrong shape, and the numbers agree. Across
scripts/the total sits around 85% lines, 91% branches and 87% functions, but files range from 17% lines (gen-api-docs.mjs, whosemain()rewrites the docs) to 100% (the parsers).check-conformance.mjsalone is a quarter of the measured lines, so a small file losing half its coverage barely moves the total. I checked the #716 case directly: one uncovered function added toscripts/lib/shell-words.mjstakes that file to 75% functions and moves the total from 87.28% to 87.14%. The per-file gate fails on it, and no total that passes today would. Node's--test-coverage-*flags only judge the total, so the per-file check is a small reporter, with no new dependency.Where the floors sit. Lines and functions sit at the file's worst run, rounded down to a whole percent. Branches get a small allowance: two fewer covered branches than the worst run, over the largest total seen. That's because the only noise measured, across runs on Node 22, 24 and 25 and CI's Linux run, is V8 sometimes counting a never-run fallback in
render-triage-comment.mjsas a branch. Lines and functions never moved, and an allowance there would cost a third of a six-function file. Metrics at 100% keep a floor of 100, since a fully covered metric has no never-run code to be noisy about, and lowering them would let new uncalled functions through, the #716 case. A metric that covers anything never gets a floor of 0: where the branch allowance would round to 0, the floor is 1, a bar that fails only when the tests stop reaching that metric at all. Nothing is excluded, and the thin files keep honest low floors. Floors don't ratchet: coverage going up needs nothing, and when the gap is a branch only a hollow test could reach, lowering the row in the same PR with the reason is the documented fix. CI's Linux coverage table matched the macOS one to the hundredth.Without a build. Floors are judged only on a green run. Without
bulma-ui/distthe suites fail exactly as before, with their own "build first" messages, and the reporter adds one line saying it didn't check.How the gate fails the run. Node documents no way for a reporter to fail a run, so the reporter sets
process.exitCode. The test sibling runs a realnode --testagainst it in both directions, so a Node upgrade that stops honouring that turns the suite red.Known limit. Node only reports files a test loads.
gen-component-catalog.mjs(checked bygen:catalog:check) andexports-resolve-fuzz.mjs(a manual fuzz tool) are loaded by no test, so they aren't in the table, and a new script with no test at all won't be either.It adds about eight seconds to the scripts suite locally.
pnpm allpasses locally.