Chore: derive the vitest coverage include list from the npm workspaces - #362
Open
AmaadMartin wants to merge 1 commit into
Open
Chore: derive the vitest coverage include list from the npm workspaces#362AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
…kspaces The coverage denominator was a hand-maintained glob per workspace that duplicated the root package.json `workspaces` array with nothing keeping the two in sync. A workspace missing from that list drops out of both the numerator and the denominator, so the reported percentage stays healthy and the CI coverage gate stays green while an entire package goes unmeasured. Derive the include list from `workspaces` instead, and add a tripwire test that pins the vitest config's resolved `coverage.include` to that derivation so re-hardcoding the list fails CI.
This was referenced Jul 31, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: #issue_number
Related: #issue_number
Problem:
vitest.config.tshardcoded the coverage denominator as one glob per workspace:That list duplicates the root
package.jsonworkspacesarray with nothing keeping the two in sync. If a workspace is added and this list is not extended by hand, the workspace's sources are simply absent from the denominator.The failure is silent by construction.
coverage.thresholdsare percentages of whatever is in the denominator, so dropping a workspace removes it from the numerator and the denominator together: the reported percentage stays healthy,npm run test:coveragein.github/workflows/validation.yamlstays green, and nothing in the output hints that a whole package is unmeasured. (Contrast the opposite mistake — sweeping extra files in — which fails loudly by pushing the percentage below the threshold.)I reproduced this end to end. With a synthetic 4th workspace (
plugins/, containing one uncalled function) declared inpackage.json, comparing the key set ofcoverage/coverage-final.json:plugins/src/thing.tsmeasured?workspaces(after)This is not hypothetical drift. Commit
a65d05ff("feat(integrations): create new top-level integrations package", #449) added theintegrationsworkspace and did two of the three required updates — it extendedcoverage.includeand added theunit:integrationsvitest project — but never added--project unit:integrationsto thetest*scripts. That script omission is a separate concern and deliberately not touched here; it is cited only as evidence that hand-maintained parallel lists in this file do drift.Solution: derive the include list from the
workspacesdeclaration, so the denominator is a function of the declared workspace set and there is no second list to remember:Plus one tripwire test that pins the config's resolved
coverage.includeto that derivation, so re-hardcoding the list fails CI.Design notes:
coverage.excludewas added. In vitest, settingcoverage.excludereplaces the default exclude array rather than merging with it, so adding one entry would silently re-enablenode_modules,dist, dot-directories,*.d.tsand config files as coverage candidates. The derived include is already exact.package.jsonwere unreadable or malformed this throws at config load and vitest refuses to start, which is the correct loud behaviour; a fallback would reintroduce exactly the silent hardcoded list this change removes.import path from 'path'was left alone rather than churned tonode:path; only the new import uses thenode:prefix.coverage.include, not against an exported copy of the derivation. ExportingcoverageIncludeand asserting on that would let someone re-hardcodeinclude:while leaving the export in place and still pass; reading the real config value closes that hole and keeps the config's only export the default one.tests/integration/repo_config/, matching the existing convention — every test undertests/integration/lives in a topic subdirectory, and there are no top-level*_test.tsfiles there.Collision check (required before starting): I ran
gh pr list --state open --limit 100and inspected every plausibly adjacent PR. No open PR lands this change. Three touch the same file or area and are semantically disjoint:thresholdsblock, immediately below theincludeI replace. Different key, different concern — that PR re-measures the numerator, this one leaves the denominator identical. I did not stack on it, because doing so would bundle a threshold raise this change explicitly must not make; expect a small textual conflict at merge, resolvable by taking both hunks.projectsarray only — no overlap with thecoverageblock.package.jsonscripts only. It is the sibling that fixes the missing--project unit:integrations; it moves the numerator up and these two can land in either order.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Added
tests/integration/repo_config/coverage_config_test.ts, a tripwire asserting the config's resolvedcoverage.includeequals the derivation fromworkspaces:The new config lines carry no coverage obligation — vitest's default
**/{...,vitest,...}.config.*exclude keepsvitest.config.tsout of the denominator — so the test exists for regression value, not for a percentage.Proof the test can fail. Two mutations, both run against the test:
Mutation A — narrow the derivation to
workspaces.slice(0, 2).map(...):Mutation B — restore the hardcoded literal and declare a 4th workspace, i.e. reproduce the exact drift this change prevents:
Manual End-to-End (E2E) Tests:
The core verification is that the denominator is unchanged for the repository as it stands. Reproduce with:
npm install && npm run build CI=true npm run test:coverage -- --coverage.reportOnFailure1. Deterministic denominator check. Compare the sorted key set of
coverage/coverage-final.json(the exact file set forming the denominator) between the hardcoded and derived configs, over a single cheap coverage run:rm -rf coverage npx vitest run --project integration coverage_config_test --coverage --coverage.reporter=json node -e "console.log(Object.keys(require('./coverage/coverage-final.json')).length)"Result: 221 files both ways,
diffof the sorted key sets is empty — identical file sets. (221 = 191core/src+ 27dev/src+ 3integrations/src.)2. Full per-file coverage table.
npm run test:coveragebefore and after, on the same machine and environment:main(before)The per-file row list is byte-identical before and after — no row appears or disappears, which is the denominator claim. All four values clear 86 / 87 / 88 / 86 with ~2-4 points of headroom on every run, which is why the thresholds are left untouched.
The small percentage wobble on 10 rows is numerator flake, not a denominator change, and is demonstrated as such: running the coverage twice on this same commit produces the same wobble on the same rows, and the second run reproduces the
mainbaseline exactly (90.1 / 88.95 / 90.88 / 90.1). The moving rows —dev/src/utils/agent_loader.ts,dev/src/utils/file_utils.ts,core/src/tools/base_tool.ts— are precisely those exercised by the integration suites whosebeforeAllrunsnpm installin a fixture (agent_dirname_test,app_loader_test,build_setup_test,skills/script_js). Those suites hit hook timeouts in my sandbox, identically before and after; they are a pre-existing environment limitation, not a regression from this change.2b. CI coverage gate, measured on the real matrix.
npm run test:coverageran green on all three CI operating systems against this branch, i.e. the derived denominator, with the thresholds untouched:All filesThe tightest margin is branches on macOS at 88.76 against a threshold of 87 — the same leg that is tightest on
maintoday. The new tripwire test also passed on all three (2-6ms). This is the threshold re-justification: the values are still correct, so they are left alone.3. Bug reproduction / fix confirmation. With a synthetic
pluginsworkspace declared inpackage.json: the hardcoded config yields 221 files withplugins/src/thing.tsabsent and no warning; the derived config yields 222 files withplugins/src/thing.tspresent at 0%, so the omission becomes loud. The synthetic workspace was removed afterwards and is not part of this diff.4. Full gate sweep, on the exact commit pushed:
(
tsc --noEmittype-checks the whole tree including tests and does not pass onmaintoday; I verified the count is unchanged at 308 by running it on a stashed tree, and that no error referencesvitest.config.tsor the new test.)No suppressions were added:
git diff main -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|v8 ignore|as any|as never'returns nothing.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.