Test: fail CI when a vitest project is run by no root package.json script - #543
Open
AmaadMartin wants to merge 4 commits into
Open
Test: fail CI when a vitest project is run by no root package.json script#543AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 4 commits
August 2, 2026 13:55
integrations/test/version_test.ts asserted toBe('1.3.0') while
integrations/src/version.ts exports '1.5.0', so the test has been
failing-if-run since the release bumps. It escaped notice because the
unit:integrations vitest project it lives in is not run by any root
script.
An existing test is modified here rather than added alongside because
the assertion encodes wrong behaviour: the literal is stale, not merely
inconvenient. release-please bumps integrations/package.json and, via
its extra-files entry, integrations/src/version.ts, but never the test,
so any literal goes stale again at the next release and turns the
automated release PR red. Deriving the expected value from
package.json cannot drift and turns a tautology into a real check that
the two release-please targets stay in sync.
vitest.config.ts has declared a unit:integrations project since the integrations package landed, but no root script ever passed --project unit:integrations, so integrations/test/**/*_test.ts has never executed -- not locally via npm test, and not in CI, which runs npm run test:coverage. Add the flag to the three scripts that enumerate unit projects. test:integration, test:e2e and test:cross-language stay untouched; they are deliberately single-project entry points.
…ript Nothing runs a vitest project unless a root script names it with --project <name>, and that coupling is invisible: adding a project to vitest.config.ts looks like it wires tests into CI, but the tests only run if someone also edits a script. unit:integrations drifted exactly this way and went unrun for its whole life. The guard reads the project names out of vitest.config.ts and the scripts out of package.json and asserts every declared project is selected by at least one script. A separate assertion fails when the project list is empty so the check can never pass vacuously, and an entry whose name cannot be read throws with its index rather than being skipped -- a silently skipped project is the drift being guarded against. Flags are matched as whole tokens, so --project unit:core-extra does not satisfy unit:core.
Both tests hand-rolled fileURLToPath + path.resolve + readFileSync +
JSON.parse to read a package.json that vitest and tsc already load
natively. The repo already uses the import-attribute form in 23 places
(e.g. tests/integration/skills/loader/agent_test.ts), and nodenext
resolution enables it with no config change.
Drops the three node: imports from each file, the two path constants,
and the single-caller readRootScripts wrapper. It also removes the two
hand-written annotations that existed only to re-type JSON.parse's any
return: the import infers real types, which in turn makes the
scripts ?? {} fallback provably dead.
This was referenced Aug 3, 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
Link to an existing issue (if applicable):
No existing issue is linked — the defect was found by inspection of the repo's own test wiring.
Or, if no issue exists, describe the change:
Problem:
vitest.config.tsdeclares a list of test projects, but nothing runs a project unless a rootpackage.jsonscript names it with--project <name>. That coupling is invisible: adding a project tovitest.config.tslooks like it wires tests into CI, but the tests only run if someone also edits a script.The repo already contains exactly that defect.
vitest.config.tsdeclares six projects;unit:integrationsis referenced by no root script:unit:coretest,test:unit,test:coverage)unit:devtest,test:unit,test:coverage)unit:integrationsintegrationtest,test:integration,test:coverage)e2etest,test:e2e,test:coverage)cross-languagetest:cross-language)unit:integrationsarrived with theintegrationspackage and was never added to a script, sointegrations/test/**/*_test.tshas never executed — not locally vianpm test, and not in CI (.github/workflows/validation.yamlrunsnpm run test:coverage). The consequence was already live:integrations/test/version_test.tsassertedexpect(version).toBe('1.3.0')whileintegrations/src/version.tsexports'1.5.0'. That test has been failing-if-run since the version bumps and nobody noticed, which is precisely the failure mode a guard should prevent. Running it confirms this rather than assuming it:Solution: three commits, one per concern, plus a fourth that simplifies how the two new tests read
package.json(see Review revisions at the end).integrations/test/version_test.ts— de-stale the assertion (41533b6d). This modifies an existing test rather than adding one beside it, because the existing assertion encodes wrong behaviour: the literal is stale, not merely inconvenient. It is done in its own commit, as the repo guideline on editing existing tests requires. The literal is replaced with the invariant the test is really protecting — thatintegrations/src/version.tsandintegrations/package.jsonstay in sync. Rationale for deriving instead of bumping'1.3.0'to'1.5.0':release-pleasebumpsintegrations/package.jsonand (via itsextra-filesentry)integrations/src/version.ts, but never the test, so any literal goes stale again at the next release and turns the automated release PR red. The derived form cannot drift, and it upgrades a tautology into a real check that the two release-please targets stay in sync. The assertion that still pins the old behaviour is the same one,expect(version).toBe(...)— only the expected value's source changed, from a hardcoded literal to the package manifest.package.json— wireunit:integrationsin (261c18b8). Added to the three scripts that enumerate unit projects (test,test:unit,test:coverage).test:integration,test:e2eandtest:cross-languageare left untouched; they are deliberately single-project entry points.tests/integration/repo_config/vitest_projects_test.ts— the guard (85b3528f). It reads the project names out ofvitest.config.tsand the scripts out of the rootpackage.json, and asserts every declared project is selected by at least one script. Notable design points:--project unit:core-extracannot "satisfy"unit:core. Both--project <name>and--project=<name>spellings are accepted, because vitest accepts both.{label, color}name form is told to extend the guard instead of quietly losing coverage of it.expect(unreferenced).toEqual([])) rather than aborting on the first.integrationproject already includestests/integration/**/*_test.ts, and CI runs it viatest:coverage --project integration.Scope notes:
.github/workflows/**is touched here.vitest.config.tsis not modified: the guard reads it, it does not reshape it. Coverage thresholds are untouched —integrations/src/**was already incoverage.includeand effectively uncovered, so runningunit:integrationscan only raise the measured numbers against thresholds that are minima.package-lock.jsonis untouched.node:fs,node:path,node:urlandvitestonly.any, no@ts-expect-error/@ts-ignore, noeslint-disable, no coverage suppressions anywhere in the diff.Collision check (performed before implementation).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000was swept for adjacent work. This change overlaps two open, unmerged PRs on the same fork: #343 (fix/vitest-project-drift-guard) proposes the same guard file, and #236 (fix/run-unit-integrations-vitest-project, which #343 is stacked on) proposes the samepackage.jsonwiring and the sameversion_test.tsfix. Both are still open and unmerged, so the defect is live onmain. Two further PRs are adjacent and should be reconciled by whoever lands this: #311 proposes an alternative fix for the same root cause (wildcard project selection instead of enumerated--projectflags), which would make part of the script wiring here moot; #479 proposes a different assertion for the same version test. #418 implements the script → workflow half and is complementary, not conflicting. This PR is branched frommainand is self-contained, so it lands regardless of which of the overlapping PRs is chosen; if #236 or #343 lands first, the corresponding commit here becomes a no-op and should be dropped rather than merged twice.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.
The deliverable is itself test code, so the verification below is what proves it works.
coverage.includecovers onlycore/src,dev/srcandintegrations/src, so neither touched test file is instrumented; the guard's own error paths are therefore covered by direct tests of its helpers rather than by a coverage number.Targeted runs (all green, on the exact pushed commit):
Quality gates:
Disclosure on
ts:check: this clone reports 281error TSlines, all in pre-existingcore/test/**andtests/integration/**files ([BASE_AGENT_SIGNATURE_SYMBOL]mismatches from@google/adkresolving to builtdist/types). Stashing this branch's changes and re-runningtsc --noEmityields the same 281 — this change adds none, andgrepfor the touched paths in the output returns nothing.Proof each test can fail. Every new assertion was run against mutated code and observed to fail:
--project unit:integrationsfrom all three root scripts (i.e. reverted commit 2)vitest projects > are each run by at least one root package.json scriptFAILED:expected [ 'unit:integrations' ] to deeply equal []bogustovitest.config.tsreferenced by no scriptexpected [ 'bogus' ] to deeply equal []vitest projects > are declared in the root configFAILED:expected [] to not have a length of +0integrations/src/version.tsto export'1.4.0', drifting it frompackage.jsonversion > should match the version declared in package.jsonFAILED:expected '1.4.0' to be '1.5.0'runsProjectuse substring matching instead of whole-token matchingrunsProject > does not match a name that merely extends the requested oneFAILED:expected true to be falsedeclaredProjectNamesinvent a placeholder name instead of throwingdeclaredProjectNames > rejects …tests FAILED:expected [Function] to throw an errorEvery file was restored after each probe and
git statuswas confirmed clean apart from the three intended files.One honest caveat on mutation 3. Setting
projects: []directly invitest.config.tsdoes not reach the guard at all: vitest itself refuses to start withError: No projects were found … The projects definition: []. That is still a hard, non-silent CI failure, but it means the empty-list assertion is proven by mutating its input (row 3 above) plus the directdeclaredProjectNames([])anddeclaredProjectNames(undefined)tests, not by emptying the real config.Manual End-to-End (E2E) Tests:
To see the guard do its job, from a clean checkout of this branch:
npm install && npm run buildvitest.config.tsand add a project the scripts do not name, e.g. a seventh entry{test: {name: 'bogus', environment: 'node', include: ['tests/bogus/**/*_test.ts']}}.npx vitest run --project integration tests/integration/repo_config/vitest_projects_test.ts— it fails and printsbogusin the diff, with the message pointing at the fix.--project bogusto any root script and re-run — it passes.To confirm the previously-orphaned project now runs through the normal entry point:
npm run test:unitincludes|unit:integrations|in its output, which it did not before this PR.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.
Review revisions
Round 2 (complexity): both new tests hand-rolled
fileURLToPath+path.resolve+readFileSync+JSON.parseto load apackage.jsonthat vitest andtscalready load natively. Replaced with a JSON import attribute, which is the established idiom here — 23 existing call sites, e.g.tests/integration/skills/loader/agent_test.ts:14— and needs no config change undernodenextresolution (4f4194fb, −26 lines):That removed three
node:imports from each file, both path constants, and the single-callerreadRootScriptswrapper. It also deleted the two hand-written annotations that existed only to re-typeJSON.parse'sanyreturn ({version: string}and{scripts?: Record<string, string>}) — the import infers real types instead, which in turn made thescripts ?? {}fallback provably dead.Because the read mechanism changed, every probe in the table above was re-run against the revised code and still fails as recorded. Probe 4 was strengthened while re-running it: rather than restoring the old
'1.3.0'literal (an assertion that no longer exists), it now mutatesintegrations/src/version.tsto export'1.4.0', which directly exercises the invariant the rewritten test protects — thatversion.tsandpackage.jsonstay in sync. All 13 + 1 tests pass on the revised commit, andnpm run ts:checkstill adds zero errors (281 pre-existing, none in the touched files).CI note on the revision commit.
run-tests (windows-latest)failed once on4f4194fbwithcore/test/code_executors/unsafe_local_code_executor_test.ts > UnsafeLocalCodeExecutor > should execute shell code and return stdout — Test timed out in 5000ms. That test spawns a real shell (echo "Hello, Shell!") against vitest's default 5s budget and is inunit:core, which this PR does not touch; the same test passed on windows-latest on the previous commit of this branch, whose only delta is the JSON import in two test files. Re-running the job passed in 9m14s. All four test jobs (run-tests, ubuntu-latest, macos-latest, windows-latest) pluscheck-licenseare green on4f4194fb.