Test: fail CI when a root test script targets a vitest project no workflow runs - #418
Open
AmaadMartin wants to merge 2 commits into
Open
Test: fail CI when a root test script targets a vitest project no workflow runs#418AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
July 31, 2026 15:51
…kflow runs
Nothing today proves the test scripts in the root package.json are actually
executed by a GitHub Actions workflow. Dropping a --project flag from
test:coverage, or adding a test:* script nobody wires into a workflow, leaves
CI green while those tests silently stop running. The integration project
hosts the repo's own meta-guards, so that failure mode hides itself.
Add a dependency-free guard that walks root test script -> vitest project ->
workflow step by reading package.json and .github/workflows/*.{yml,yaml}, and
fails naming both the orphaned project and the script that declares it.
The guard filtered candidate scripts with a /^test(:|$)/ name pattern, which proxies the condition it actually cares about: whether the command runs vitest. Replace it with a direct check on the command itself, applied inside the project extractor so both sides of the comparison use one rule. This also closes a false positive the name filter was silently covering: --project is not vitest-specific. `tsc --project tsconfig.json` is the same flag, and ts:check is actively moving toward a -p form, so a script picking up the long spelling would have been reported as an unwired vitest project. Behaviour on the current tree is unchanged: every root script carrying --project today runs vitest. Dropping the name filter also widens the guard to a vitest script that is not named test*, which the old pattern ignored.
This was referenced Aug 1, 2026
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
No existing issue.
Problem: Nothing today proves that the test scripts declared in the root
package.jsonare actually executed by a GitHub Actions workflow. If someone drops a--project <name>flag fromtest:coverage, or adds a newtest:*script that no workflow ever calls, CI stays green while those tests silently stop running. Because theintegrationvitest project is where the repo's own meta-guards live, that failure mode is self-concealing — the guards stop running too, so nothing is left to notice.Solution: Add one dependency-free meta test,
tests/integration/repo_config/workflow_scripts_test.ts, that walks the chain root npm test script -> vitest project -> GitHub workflow step and fails loudly when a link is missing.It reads only two things off disk (no network, no subprocess, no new dependency): the root
package.jsonscriptsmap (via a static JSON import, so the path is resolved relative to the test file and verified at compile time — neverprocess.cwd()), and the raw text of.github/workflows/*.{yml,yaml}. From those it derives:npm run <script>occurrence in the workflow files, with whole-line#comments stripped first so a commented-out step does not read as a live invocation.--projectflags in the commands of those invoked scripts./^test(:|$)/, every vitest project that script targets must be a member of Workflow-run projects.The failure output is the vitest diff of a message array against
[], so every broken link is spelled out as a full sentence naming both the orphaned project and the script that declares it.There is a second, cheaper assertion that keeps the guard from ever passing vacuously: every
npm runname found in a workflow must exist in the rootscriptsmap, and both the workflow-file count and the workflow-run project set are asserted non-empty. If the regex ever stops matching, the suite goes red instead of quietly finding nothing.Collision check (required before implementation):
gh pr list --repo AmaadMartin/adk-js --state open --limit 100returned 100 open PRs. The only adjacent one is this fork's PR #343, "Test: fail CI when a vitest project is not run by any npm script" (fix/vitest-project-drift-guard), which addstests/integration/repo_config/vitest_projects_test.ts. It asserts the other half of the chain (vitest.config.tsprojects <-> root script--projectflags) and never reads.github/workflows, so it does not land this change. I did not stack on it: the two PRs add two distinct new files and share no code, so there is no rebase conflict to avoid, and stacking would set this PR's base to a non-mainbranch, which thepull_request: branches: [main]workflow trigger would never fire for. The remaining workflow-touching PRs (#416 Node pinning, #403timeout-minutes, #393setup-gocache) edit workflow YAML but add no scripts and no vitest projects, so they cannot conflict with this guard's logic.Deliberate deviations from the approved plan (all disclosed, none reduce scope):
readRootScripts()helper doingfs.readFile+JSON.parse(...) as {scripts?: Record<string, string>}+?? {}. I used a staticimport rootPackage from '../../../package.json' with {type: 'json'}instead. It removes anany-typedJSON.parse, removes a??branch that no test could ever reach, resolves the path at compile time rather than at runtime, and matches how the neighbouring file in this directory reads the manifest.tsc --noEmitresolves the JSON import under this repo'smodule: nodenextconfig (verified — see Testing Plan).[...new Set(...)]. Each message embeds the script name, so two scripts targeting the same missing project produce two different strings and theSetcan never deduplicate anything real. Dropped as dead code.itblocks. I kept both of those verbatim in thedescribeit specified, and added a seconddescribe('workflow script parsing')with unit tests over the pure helpers. Without them,stripCommentLineswould have zero coverage of its actual purpose (no workflow file in the repo currently contains a#comment line, so it is a no-op against live data), and the two regexes would have no negative test. Each is backed by a mutation proof below.Known, deliberate limitation: the guard models CI coverage via explicit
--projectflags. A workflow-invoked script that ranvitestwith no--projectflag would in fact run every project, but this guard would still report the targeted projects as unwired. No such script exists in the repo, and modelling that case would add an unreachable branch, so it is intentionally out of scope.Deliberately not included: the companion guard for the other half of the chain (every project declared in
vitest.config.tsis named by some root script). It is tracked as a separate task and is the subject of this fork's PR #343. This PR imports nothing from it and stands alone.Review round 1 — why this is not re-anchored on
vitest.config.ts. A reviewer asked for the guard's universe of projects to come fromvitest.config.tsprojects[]rather than from the root scripts, on the grounds thatunit:integrationsis declared atvitest.config.ts:64, is named by no root script, and so never runs in CI — a real orphan this guard does not report. The orphan is real; re-anchoring is the wrong place to fix it, for three checkable reasons:unit:integrationsis "declared but targeted by no script" (link A). This guard asserts "targeted by a script but run by no workflow" (link B). No amount of tuning link B can reach a project that no script mentions.vitest.config.tsis run by a root npm script". Re-anchoring here ships a second implementation of that same assertion.main— it is stacked on PR Fix: run the orphaned unit:integrations vitest project from the root test scripts #236, "Fix: run the orphanedunit:integrationsvitest project from the root test scripts", which adds--project unit:integrationstotest,test:unitandtest:coverage. Test: fail CI when a vitest project is not run by any npm script #343 is stacked on Fix: run the orphaned unit:integrations vitest project from the root test scripts #236 precisely because a link-A guard cannot go green until the orphan is wired. Re-anchoring would inherit that redness, and the only way to clear it is Fix: run the orphaned unit:integrations vitest project from the root test scripts #236'spackage.jsonedit — which the approved plan explicitly forbids here ("Additive. Do not editpackage.json,vitest.config.ts, any workflow file").The two guards are complementary, and this one covers cases #343 cannot. Verified rather than asserted: simulating #236 as landed and then unwiring
unit:integrationsfromtest:coverageonly (leaving it intestandtest:unit, so link A stays green) makes this guard fire:The same asymmetry holds for the headline regression: dropping
--project e2efromtest:coverageleavese2estill referenced bytest:e2e, so link A stays green and only this guard catches it.Review round 1 — the
/^test(:|$)/script-name filter is gone (second commit). The reviewer was right that scoping by script name proxies the condition the code actually cares about. I did not simply delete the filter, because the stated premise for deleting it — that "presence of--projectalready establishes a vitest invocation" — is false:--projectistsc's flag too, andts:checkis actively moving onto it (PRs #326 and #414 both rewrite it astsc -p tsconfig.check.json; the long spelling is one keystroke away). Deleting the filter outright would make a future"ts:check": "tsc --project tsconfig.check.json"reporttsconfig.check.jsonas an unwired vitest project.So the name heuristic is replaced by the direct condition, pushed down into the extractor (
vitestProjects) so both sides of the comparison use one rule instead of two. Behaviour on the current tree is identical — every root script carrying--projecttoday runs vitest — and the guard is now strictly broader in the useful direction: a vitest script not namedtest*is no longer ignored. A regression test pins it, and mutatingVITEST_COMMAND_PATTERNto match everything fails it:Additive and test-only:
package.json,package-lock.json,vitest.config.ts, every.github/workflows/*file, and every existing test are untouched. The new file is picked up automatically becausevitest.config.tsgives theintegrationprojectinclude: ['tests/integration/**/*_test.ts'], andvalidation.yamlrunsnpm run test:coverage, which includes--project integration— so the guard guards itself.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.
Full gate, all green on the pushed commit:
npm run ts:checkis not green onmainatb390217eindependently of this change (pre-existing errors across ~100 files, mostly unresolved@google/adkspecifiers). I verified the narrower, honest claim instead:tsc --noEmitattributes zero errors to the new file. To confirm the file is genuinely in the program rather than silently skipped, I temporarily appendedconst _probe: number = rootPackage.scripts.test;and re-ran —tscreportedtests/integration/repo_config/workflow_scripts_test.ts:162:7 - error TS2322: Type 'string' is not assignable to type 'number', which also proves the JSON import resolves to a precisely typed value.CI on this PR. Green on the current head (
9c184ebf) — the real test jobs ran and passed on every platform, and the new file passed 7/7 on each:For the record, the first push of this branch saw
windows-latestgo red, and it is worth stating why it is not a flake in this change. It failed on two pre-existing integration tests this PR does not touch —tests/integration/app_loader/app_loader_test.tstiming out at 40 s, andtests/integration/test_case_utils.ts:341throwingCLI exited prematurely with code 1. The validation run for the unmodified base commitb390217ereproduces the identicalCLI exited prematurely with code 1at the same line and showsapp_loader_test.tsburning 72,989 ms on Windows, i.e. that test is already far over budget onmainand tips over on a slower runner. It passed on the re-run here with no change to either test. That flake is queued as separate work rather than fixed in this diff.Coverage. The change adds zero lines under
core/src,dev/srcorintegrations/src, the only paths in the vitest coverageincludelist, so it cannot move the thresholds and none were edited. Coverage of the new file itself is not measurable:@vitest/coverage-v8drops files matched by the project's testincludefrom the report, and every override I tried (--coverage.include,--coverage.exclude,--coverage.all=false,--coverage.excludeAfterRemap=false) still reportedAll files | 0 | 0 | 0 | 0with no row for the file. Verified by construction instead: the file has exactly one branch — the vitest-invocation ternary invitestProjects, whose both arms are pinned by dedicated tests — noif, no&&/||/??, no default parameters, and all five module-level functions are executed by the seven tests.Proof that each test can fail. Eight mutations, each applied, run, and reverted;
git statuswas clean afterwards.Against the guard (mutating the repo, not the test):
--project e2efromtest:coveragein the rootpackage.json:"test:foo": "vitest --project foo"to the rootpackage.json:validation.yamlstep torun: npm run test:coverageX(parser self-check):Against the parser unit tests (mutating the file under test):
stripCommentLinesmade a no-op (.filter(() => true)):PROJECT_FLAG_PATTERNnarrowed to/--project\s+([^\s"']+)/g(loses the--project=namespelling):VITEST_COMMAND_PATTERNwidened to match everything, i.e. the review-round-1 proposal of dropping the scope filter entirely:lastIndexleak on the shared/gregex (PROJECT_FLAG_PATTERN.test(command)before thematchAll):unit:integrationsfromtest:coverageonly — the link-A guard stays green, this one fires (quoted in full in the review-round-1 section above).All eight were re-run against the revised code after the round-1 change, not carried over from the first round.
One mutation I tried did not produce a failure and is reported for honesty: rewriting the project extractor as an
execloop over the shared/gregex still passes, becauseRegExp.execresetslastIndexto 0 when it returnsnull. Mutation 7 is the one that actually pins the statelessness requirement, which is why it is the one recorded above.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Expect 6 passing tests. To watch the guard fire, edit the root
package.jsonand remove--project e2efromtest:coverage, then re-run — the suite fails naminge2eand both scripts that declare it. Revert.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.