Skip to content

Test: fail CI when a vitest project is not run by any npm script - #343

Open
AmaadMartin wants to merge 2 commits into
fix/run-unit-integrations-vitest-projectfrom
fix/vitest-project-drift-guard
Open

Test: fail CI when a vitest project is not run by any npm script#343
AmaadMartin wants to merge 2 commits into
fix/run-unit-integrations-vitest-projectfrom
fix/vitest-project-drift-guard

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Stacked PR. Base is fix/run-unit-integrations-vitest-project (#236), not main. That PR adds --project unit:integrations to the root scripts and de-rots integrations/test/version_test.ts; this PR adds the structural guard that stops the same drift from recurring. Merge #236 first.

Problem: The root package.json test scripts select vitest projects with hand-written --project flags, and nothing keeps that hand-written list in sync with the projects declared in vitest.config.ts. When they drift, the failure is silent — the affected tests simply stop running and nobody is told.

This is not hypothetical. unit:integrations was declared in vitest.config.ts and named by no script, so integrations/test/** never ran in CI. Its only test asserted expect(version).toBe('1.3.0') while integrations/src/version.ts had been bumped to 1.4.0 by release-please. That test was red and invisible. .github/workflows/validation.yaml runs npm run test:coverage, which could not see it.

The rename half of the same drift is equally silent: renaming a project in vitest.config.ts without updating the scripts leaves a --project flag that matches nothing, and vitest is happy to run zero files.

Solution: One meta test, tests/integration/repo_config/vitest_projects_test.ts, asserting set equality between the two lists in both directions:

  • every project declared in vitest.config.ts is named by some root npm script;
  • every --project flag in a root npm script names a declared project.

Each assertion computes the offending names and compares against [], so the failure diff prints the drifted name (expected [ 'unit:integrations' ] to deeply equal []) alongside a message stating the remedy. That failure message is the point of the guard, so the two directions are deliberately not collapsed into one boolean assertion.

Design notes:

  • No new CI step and no workflow edit. The guard lives under tests/integration/**, which is the integration project — already selected by test, test:integration and test:coverage, and test:coverage is what validation.yaml runs on ubuntu-latest, windows-latest and macos-latest. A scripts/check_vitest_projects.mjs + workflow step was the larger option, not the smaller one: plain node cannot import vitest.config.ts, so it would have to regex-parse TypeScript.
  • The config is read structurally, not textually. The test imports vitest.config.ts directly, so it sees the real test.projects array rather than a regex over source. (vitest.config.ts uses __dirname, which does not exist in native ESM; vitest's module runner injects it into transformed modules. This was verified empirically before the assertions were written.)
  • Unreadable input throws, it never skips. A project entry the guard cannot read a string test.name from raises an error naming the index, because a shape it silently skipped would be exactly the drift it exists to catch. Same for a config with no test.projects and for a script that passes --project with no value.
  • No allowlist. All six projects are referenced after Fix: run the orphaned unit:integrations vitest project from the root test scripts #236 (cross-language by test:cross-language, which cross-language-integration.yml runs), so an allowlist would be dead code today and a place to hide drift tomorrow. If a future project genuinely cannot be run by a script, the fix is a one-line script.
  • Flags are parsed with node:util parseArgs, not substring matching — String.includes('unit:core') would false-positive on a hypothetical unit:core:slow. Both --project name and --project=name are read. strict: false is required because the non-test scripts carry flags this guard does not model. Note parseArgs types a valueless --project as boolean; that case is rejected explicitly rather than cast away.

Known limitations (deliberate scope boundaries, not oversights):

Collision check (before implementation): gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus a gh pr diff --name-only sweep of every open PR for vitest.config.ts / root package.json / repo_config hits. Found #236 and #245 as near-duplicate implementations of the prerequisite edits (script flags + version test) and #311/#237/#261 as adjacent. No open PR ships the guard. Since #236 is the older of the duplicate pair and is already the base of #311, this PR stacks on it rather than re-landing those edits.

Note on integrations/test/version_test.ts (in the parent PR, #236, not this diff): it rewrites an existing test rather than adding one, which the contributor guidelines normally forbid. The exception applies here — the old assertion pinned a stale '1.3.0' literal that was already wrong and could never fail because the project was never run. Bumping the literal to '1.4.0' would just re-arm the same trap at the next release, so it now reads the expected value from integrations/package.json, which release-please bumps in the same commit as src/version.ts.

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.

$ npx vitest run --project integration tests/integration/repo_config/vitest_projects_test.ts
 ✓ |integration| tests/integration/repo_config/vitest_projects_test.ts (6 tests) 10ms
 Test Files  1 passed (1) · Tests  6 passed (6)

$ npx vitest run --project unit:integrations      # the newly-wired project
 ✓ |unit:integrations| integrations/test/version_test.ts (1 test) 4ms
 Test Files  1 passed (1) · Tests  1 passed (1)

Every test was proven able to fail. Each was run against mutated code and confirmed to FAIL, so none of them is a green light with no signal:

# Mutation Result
1 Restore package.json to its state on main, where no script names unit:integrationsthe exact bug FAILS: expected [ 'unit:integrations' ] to deeply equal []
2 Rename project e2ee2e2 in vitest.config.ts Both directional assertions FAIL: expected [ 'e2e2' ] to deeply equal [] (unreferenced) and expected [ 'e2e' ] to deeply equal [] (dangling flag)
3 Drop the missing-test.projects guard FAILS: expected [Function] to throw error matching /declares no test.projects/ but got 'Cannot read properties of undefined…'
4 Accept a non-string project name instead of throwing FAILS: expected [Function] to throw an error
5 Stop reading the --project=name spelling FAILS: expected [ 'unit:core' ] to deeply equal [ 'unit:core', 'e2e' ]
6 Accept a valueless --project instead of throwing FAILS: expected [Function] to throw an error

Mutation 1 is worth calling out: removing the flag from only test:coverage does not trip the guard, because test and test:unit still name the project. The honest reproduction is main's actual state, where no script names it — that is what was run.

Coverage. This change ships no production code, and vitest.config.ts's coverage include is limited to core/src, dev/src, integrations/src, none of which are touched — so new-line coverage of shipped source is vacuous. The new code is the test, and all six of its cases are exercised and individually proven falsifiable above. Running unit:integrations can only raise the measured floor (integrations/src/version.ts becomes covered), so the existing thresholds (86/87/88/86) need no edit.

CI status: absent. This is a stacked PR whose base is fix/run-unit-integrations-vitest-project, not main; validation.yaml triggers on pull_request: branches: [main], so no test job runs for this PR. Validated locally on the exact pushed commit instead:

$ npm run build            # ok
$ npm run lint             # ok — eslint "**/*.ts", exit 0
$ npm run format:check     # ok — "All matched files use Prettier code style!"
$ bash scripts/check_license.sh
                           # ok — "All files have the correct license header."
$ npm run ts:check         # 308 errors, all pre-existing; 0 in this file
                           # (identical count with the file removed — see below)

Honest reporting on two repo-wide commands:

  • npm run ts:check is already red on main (308 errors, the subject of a separate open PR). Verified this change adds none: the count is 308 both with and without the new file, and no error line references repo_config.
  • npm run test:coverage does not pass in this sandbox, with 26 failing tests across 24 files. All are pre-existing environment failures — fixture npm install timeouts, ambient-env and shell-dependent cases, several with their own open PRs. Verified pre-existing rather than assumed: the new file passed inside that run (✓ |integration| tests/integration/repo_config/vitest_projects_test.ts), and re-running the failing integration files with the new file physically removed from the tree reproduced the failures unchanged.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

npm install && npm run build

# 1. The guard passes as the repo stands.
npx vitest run --project integration tests/integration/repo_config/vitest_projects_test.ts

# 2. Reproduce the original bug: remove every `--project unit:integrations`
#    from the root package.json scripts, then re-run. It fails, naming the
#    orphaned project and stating the remedy.

# 3. Reproduce rename drift: rename any project in vitest.config.ts
#    (e.g. `e2e` -> `e2e2`) without touching package.json, then re-run.
#    Both assertions fail — one for the unreferenced new name, one for the
#    now-dangling --project flag.

# 4. Revert both edits; the guard goes green again.

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.

Amaad Martin added 2 commits July 30, 2026 15:21
The root npm scripts pick vitest projects with hand-written --project
flags, so a project declared in vitest.config.ts runs only once some
script names it. Nothing kept the two lists in sync, and the drift was
silent: unit:integrations was declared but selected by no script, so its
tests never ran and its version assertion rotted unnoticed.

Add a meta test under the existing integration project (already selected
by test:coverage, which validation.yaml runs on all three OSes) asserting
set equality in both directions, so an orphaned project and a stale
--project flag each fail the build naming the offender.
Addresses simplicity-audit feedback on the first commit: read the root
manifest with a JSON import instead of readFileSync + JSON.parse + a type
assertion (matching integrations/test/version_test.ts), drop two
single-use constants, and replace the hand-rolled flag tokenizer with
node:util parseArgs.

parseArgs also types a valueless `--project` as boolean rather than
silently yielding undefined from an out-of-range index, so that case is
now rejected explicitly instead of leaking a non-string into the
comparison.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant