Skip to content

Fix: select vitest unit projects by wildcard so new unit suites cannot be silently excluded - #311

Open
AmaadMartin wants to merge 1 commit into
fix/run-unit-integrations-vitest-projectfrom
fix/vitest-wildcard-unit-project-selection
Open

Fix: select vitest unit projects by wildcard so new unit suites cannot be silently excluded#311
AmaadMartin wants to merge 1 commit into
fix/run-unit-integrations-vitest-projectfrom
fix/vitest-wildcard-unit-project-selection

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.
See the collision check below.

Problem: The three root test scripts hand-enumerate every Vitest unit project:

"test:unit": "vitest --project unit:core --project unit:dev --project unit:integrations",

vitest.config.ts is the place a project is defined, and package.json is the place it is selected. Nothing keeps the two in sync, so adding a project to vitest.config.ts silently excludes it from npm test, npm run test:unit and npm run test:coverage until someone remembers to also edit package.json. Since .github/workflows/validation.yaml:41 gates every PR on npm run test:coverage, an unselected project never runs in CI at all.

That is not hypothetical — it already happened. unit:integrations was added to vitest.config.ts:51 together with the integrations workspace but never added to the script lists, so integrations/test/ never executed. It stayed unrun long enough for its only test to rot against a release-please version bump. #236 (this PR's base) repairs the orphan and de-rots the assertion; this PR removes the mechanism that produced it.

Solution: Select unit projects by wildcard instead of by name.

"test": "vitest --project \"unit:*\" --project integration --project e2e",
"test:unit": "vitest --project \"unit:*\"",
"test:coverage": "vitest run --project \"unit:*\" --project integration --project e2e --coverage",

Three lines, each shorter than before, no new file and no new abstraction.

Why this is safe and why it is scoped to unit:*:

  • The wildcard selects exactly the unit projects. vitest 3.2.6 (^3.2.4 in the manifest, 3.2.6 resolved in package-lock.json) compiles each --project filter to an anchored, case-insensitive regex, so unit:* becomes /^unit:.*$/i. It matches unit:core, unit:dev, unit:integrations and cannot match integration, e2e or cross-language. Conversely --project integration becomes /^integration$/i, so it does not additionally pull in unit:integrations; combining the two runs each suite exactly once. Both properties are measured below, not assumed.
  • Only unit:* is globbed. unit:<workspace> is this repo's naming convention for the hermetic per-workspace unit suite, and the three that exist map 1:1 onto the workspaces array. Suites of that shape are fast and dependency-free and should always gate. Anything slow, networked or credential-dependent (integration, e2e, cross-language) stays hand-enumerated, so joining the blocking CI gate still requires a conscious, reviewed edit. A broader --project "!cross-language" ("everything except") was rejected for exactly that reason: it would enrol any future project, including a slow or network-bound one.
  • test:integration, test:e2e and test:cross-language are untouched — each targets one named suite and has no drift hazard. cross-language remains reachable only through npm run test:cross-language, as before.
  • Failure is loud, never silent. If a filter matches nothing, Vitest raises No projects matched the filter "<pattern>" and exits 1 (measured below). A quoting regression on any OS therefore turns CI red; it cannot skip the unit suites and report a false green.
  • Quoting follows existing repo precedent. JSON-escaped double quotes are used, matching package.json:18-21, which already passes \"**/*.ts\" to eslint and prettier. Those scripts run via npm run lint / npm run format:check on all three legs of the validation.yaml matrix (ubuntu-latest, windows-latest, macos-latest) today, so the idiom is already CI-proven on Windows in this repo. Double quotes also stop POSIX sh from glob-expanding unit:* against the repo root. Single quotes were deliberately not used: they are not a quote character to cmd.exe.
  • vitest.config.ts is not modified. Project names, include globs, coverage.include and the coverage thresholds are all unchanged, and no threshold was raised.

Collision check (per contribution process). gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus gh pr diff --name-only on every plausibly adjacent PR found three that touch this area:

PR Overlap Action
#236 fix/run-unit-integrations-vitest-project Adds --project unit:integrations to the three scripts by hand and de-rots integrations/test/version_test.ts. Stacked on it. This PR is based on its branch, not main, and its two commits appear in git log main..HEAD. Reviewing #236 first.
#245 fix/run-unit-integrations-project-in-ci Byte-identical package.json change to #236 (same blob d5b10ea5), plus an extra semver-shape assertion. Not built on — it duplicates #236. Flagging it here so #236 and #245 can be de-duplicated.
#237 feat/split-slow-integration-tests Adds an integration:slow project and a CI job; touches package.json and vitest.config.ts. No conflict. integration:slow does not match /^unit:.*$/i, so this PR neither selects nor deselects it; #237 continues to control it explicitly. Textual conflict in the scripts block is possible depending on merge order and is trivial to resolve.

No competing implementation was written.

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.

Environment: vitest/3.2.6 linux-x64 node-v22.22.2, after npm install && npm run build.

CI note — this PR's checks are absent, not green. validation.yaml triggers on pull_request: branches: [main] (.github/workflows/validation.yaml:7) and this is a stacked PR whose base is fix/run-unit-integrations-vitest-project, so the run-tests matrix never fires. Everything below was therefore run locally on the exact pushed commit and is reported honestly rather than deferred to CI.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

No new automated test file is added, deliberately. The change is three npm script strings; the thing under test is which projects the CLI selects, which is a property of the invocation, not of any module. A meta-test asserting package.json against vitest.config.ts would re-introduce the very duplication this PR deletes. Verification is therefore behavioural and each command and its output is recorded below.

1. Baseline — the drift produced a real, hidden failure. On unmodified main, before this stack:

$ npx vitest run --project unit:integrations --reporter=basic
 FAIL  |unit:integrations| integrations/test/version_test.ts > version > should return the correct version
 AssertionError: expected '1.4.0' to be '1.3.0' // Object.is equality
 Test Files  1 failed (1)
 exit code 1

The suite was not merely unrun — it was broken, and no script selected it. (Repaired by base #236.)

2. Mutation proof — this change is not vacuous. The behaviour this PR adds is "a newly defined unit:* project is picked up with no package.json edit". Mutation: add a throwaway seventh project unit:scratch to a local, uncommitted vitest.config.ts, then run the script both ways.

With the wildcard (this PR):

$ CI=1 npm run test:unit
 ✓ |unit:scratch| integrations/test/version_test.ts (1 test) 6ms
 Test Files  1 failed | 175 passed (176)

Reverting only the package.json hunk back to the base's enumeration (the mutation):

$ npx vitest list --project unit:core --project unit:dev --project unit:integrations --filesOnly
 160 [unit:core]
  14 [unit:dev]
   1 [unit:integrations]

unit:scratch is silently absent — 175 files instead of 176, no error, exit 0. That silent omission is exactly the bug. vitest.config.ts was then restored; git diff --quiet vitest.config.ts confirms it is unmodified and it is not part of this diff.

3. The wildcard selects exactly the right set.

$ npx vitest list --project "unit:*" --filesOnly
 160 [unit:core]
  14 [unit:dev]
   1 [unit:integrations]

No [integration], [e2e] or [cross-language] entries.

4. No double-execution.

$ npx vitest list --project "unit:*" --project integration --project e2e --filesOnly
  25 [e2e]
  35 [integration]
 160 [unit:core]
  14 [unit:dev]
   1 [unit:integrations]

integrations/test/version_test.ts appears exactly once, labelled [unit:integrations]; --project integration did not additionally pull it in. Zero cross-language files.

5. Behaviour is byte-identical to the base today. The decisive check that this PR cannot change any test outcome — the wildcard and the base's enumeration select the same files:

$ diff <(vitest list --project "unit:*" --project integration --project e2e --filesOnly | sort) \
       <(vitest list --project unit:core --project unit:dev --project unit:integrations \
                     --project integration --project e2e --filesOnly | sort)
(no output — 235 files, IDENTICAL SELECTION)

6. Scripts exercised through npm, not npx, so the string passes through the shell exactly as CI invokes it (CI=1 because test/test:unit are watch-mode by design):

$ CI=1 npm run test:unit
 Test Files  1 failed | 174 passed (175)
      Tests  1 failed | 2412 passed (2413)

175 files = 160 + 14 + 1, confirming the wildcard survives npm's shell. The one failure is dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults, which reads the machine's ambient gcloud config (expected 'gcloud-project', received 'cloud-ai-agentic-coding'). It is pre-existing and unrelated — reproduced on pristine main at 1210acc7 with this PR's changes absent, and already targeted by #203 / #259.

$ CI=1 npm run test:coverage
 Test Files  21 failed | 210 passed | 4 skipped (235)
      Tests  23 failed | 2482 passed | 50 skipped (2555)

All 20 failing files are environmental in this sandbox and none are attributable to this diff — guaranteed by step 5, since the selected file set is identical to the base's: 16 e2e files fail with API key must be provided via constructor or GOOGLE_GENAI_API_KEY or GEMINI_API_KEY; 3 integration files (agent_loader/agent_dirname_test.ts, build_setup/build_setup_test.ts, skills/script_js/agent_test.ts) time out on their fixture npm install with no network; plus the ambient-gcloud cli_create_test.ts above.

7. Coverage. Not re-measured as a before/after pair, and deliberately so: step 5 proves the coverage run executes a byte-identical 235-file set before and after this diff, so the numbers cannot move. Coverage thresholds in vitest.config.ts:117-122 (statements 86 / branches 87 / functions 88 / lines 86) are not touched. The genuine coverage increase from integrations/src/** finally executing belongs to base #236, not here.

8. Fail-loud property.

$ npx vitest run --project "zzz:*"
 Error: No projects matched the filter "zzz:*".
 exit code 1

A quoting or shell-expansion regression on any OS turns CI red rather than silently skipping the unit suites.

9. Static checks, on the exact pushed commit:

$ npm run lint            # eslint "**/*.ts"       -> clean, no output
$ npm run format:check    # prettier "**/*.ts"     -> All matched files use Prettier code style!
$ npx prettier --check package.json                -> All matched files use Prettier code style!
$ node -e "require('./package.json')"              -> valid JSON, scripts parse as intended

npm run ts:check has pre-existing errors on main (see #207), unchanged here: this diff contains no .ts file. The diff introduces no any, no @ts-expect-error, no @ts-ignore, no eslint-disable and no coverage-tool suppression — verified by grepping the diff.

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

To reproduce the self-maintenance property (step 2 above) yourself:

  1. npm install && npm run build.
  2. Append a throwaway project to vitest.config.ts, copying the alias block from a sibling project (without the alias, @google/adk-integrations will not resolve and the project errors instead of passing):
    {
      test: {
        name: 'unit:scratch',
        environment: 'node',
        alias: {
          '@google/adk': path.resolve(__dirname, './core/src'),
          '@google/adk-integrations': path.resolve(__dirname, './integrations/src'),
        },
        include: ['integrations/test/**/*_test.ts'],
      },
    },
  3. CI=1 npm run test:unit|unit:scratch| is picked up with no package.json edit.
  4. git stash the package.json hunk to restore the enumerated form and re-run: unit:scratch silently disappears.
  5. git checkout vitest.config.ts to discard the scratch project.

Cross-OS matrix. windows-latest and macos-latest cannot be exercised locally, and validation.yaml does not run for a stacked base (see the CI note above). Once base #236 merges and this PR is retargeted to main, the matrix will run; check the Windows leg's "Run tests and check code coverage" step for |unit:integrations| and for the absence of No projects matched the filter. The residual risk is low and bounded: the identical double-quoted-glob idiom is already green on that leg via npm run lint, and any quoting failure is loud (step 8), not silent.

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.

The three root test scripts hand-enumerated every vitest unit project, so a
project added to vitest.config.ts was silently excluded until someone
remembered to update package.json. That drift is what orphaned
unit:integrations for a whole release cycle.

Replace the enumeration with --project "unit:*" in test, test:unit and
test:coverage. Vitest anchors each filter as /^<pattern>$/i, so the wildcard
selects exactly unit:core, unit:dev and unit:integrations and does not pull in
integration, e2e or cross-language. Non-unit suites stay enumerated by hand so
nothing slow or credential-dependent can join the CI gate by accident.
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