Skip to content

Chore(test): hoist the shared vitest alias map into one constant - #434

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/vitest-config-shared-alias-map
Open

Chore(test): hoist the shared vitest alias map into one constant#434
AmaadMartin wants to merge 1 commit into
mainfrom
fix/vitest-config-shared-alias-map

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):

No issue; internal maintenance refactor of vitest.config.ts.

  1. Or, if no issue exists, describe the change:

Problem: vitest.config.ts declares six test projects (unit:core, unit:dev, unit:integrations, integration, e2e, cross-language) and each one repeats a byte-identical two-entry alias object (vitest.config.ts:38-44, 52-58, 66-72, 82-88, 96-102, 110-116 before this change; grep -c "'@google/adk': path.resolve" vitest.config.ts returned 6). There is no per-project variation. Adding a fourth workspace package means editing six places, and wiring it into five of six is a silent failure: the sixth project resolves the specifier through the npm-workspace symlink in node_modules to built output instead of source, and nothing in CI notices.

Solution: Define the map once at module scope and reference it from every project with the ES shorthand alias,. The name alias (rather than SHARED_ALIAS) is what makes the shorthand possible; the two existing module-scope constants in this file are SCREAMING_SNAKE because they are bare numeric literals whose meaning is not otherwise apparent, whereas here the property name carries the meaning. The file goes from 141 to 121 lines and the resolved configuration is unchanged — proven by equivalence, see below. No type annotation or satisfies clause is added: the inferred {'@google/adk': string; '@google/adk-integrations': string} is already assignable to Vitest's AliasOptions, and tsc --noEmit verifies it.

@google/adk-devtools is deliberately not added to the map, and the constant's doc comment records why so the omission is not "fixed" later. dev/src/server/adk_api_server.ts:208 serves the web UI with express.static(path.join(__dirname, '../../browser')), a path calibrated to the built layout (dev/dist/{esm,cjs}/serverdev/dist/browser, populated by ensureBrowserAssets() in dev/build.js:90). Under a dev/src alias, vite-node supplies __dirname = <repo>/dev/src/server, ../../browser resolves to the nonexistent <repo>/dev/browser, express.static calls next(), and GET /dev-ui/ 404s. This is measured, not argued — see Mutation C below. tests/integration/adk_web/webui_test.ts is also a describe.each over two server implementations, the other of which spawns node <repo>/dev/dist/esm/cli_entrypoint.js as a child process (tests/integration/test_api_server.ts:59-62), which no vitest alias can reach. Tests that want dev sources import them relatively, as test_api_server.ts:9 and tests/integration/app_loader/app_loader_test.ts:13 do.

Scope: exactly one file, vitest.config.ts. No source, no tests, no lockfile, no CHANGELOG.

Overlap check (open PRs on this fork, run before writing any code): gh pr list --state open --limit 100 surfaced #380 (fix/vitest-alias-exact-match), which touches the same six hunks of this file — it hoists the map into an exported workspaceAliases and converts the entries from plain strings to anchored RegExp finds so deep specifiers such as @google/adk/sessions/session.js stop resolving, which also requires editing two test files and adding a third. That is a behavioural change to specifier matching; this PR is a pure refactor whose resolved config is provably identical, touching one file. The two are alternatives for the same lines, not stackable (stacking would leave this PR with an empty diff), so this one branches from main. Whichever merges first makes the other a trivial rebase-or-close; reviewers should pick one. Other vitest.config.ts PRs open at the time of writing (#362 coverage include, #349 worker log level, #377 coverage-drift, #405 hook timeouts) touch disjoint keys and only conflict textually.

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.

No test is added, deliberately. This change adds zero lines of product code, and vitest.config.ts is not in the coverage denominator (coverage.include is core/src/**, dev/src/**, integrations/src/**), so the existing thresholds are untouched. A test asserting that a build-config file contains a particular constant would test the diff, not the behaviour. The substitute is an equivalence proof plus mutation testing, below.

[x] All unit tests pass locally.

1. Discovery equivalence — all six projects, before vs. after.

npx vitest list --project unit:core --project unit:dev --project unit:integrations \
                --project integration --project e2e --project cross-language

Run on the change, then again with the change stashed. Both emit 2707 tests. Raw output order is not stable, so the sorted output is the correct comparison:

sort before.txt | md5sum -> 6123c0371ede6892108dc91e6f1c44f3
sort after.txt  | md5sum -> 6123c0371ede6892108dc91e6f1c44f3   (diff: 0 lines)

Control for that ordering claim: a second vitest list on the identical post-change tree differs from the first by 734 raw lines while hashing to the same 6123c037…. So the raw-order churn is vitest's parallel collection order, not this change. Collection loads every matched test file, so an identical set across all six projects proves each project still exists, still has its include glob, and still resolves every import in every collected file.

2. Targeted runs.

Command Result
npx vitest run --project unit:dev (exercises @google/adk) 222 passed, 1 failed — pre-existing, see below
npx vitest run --project unit:integrations (the only consumer of @google/adk-integrations) 1 failed — pre-existing, see below
npx vitest run --project integration tests/integration/adk_web/webui_test.ts 2/2 passed
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts 6/6 passed (43s)

Both failures reproduce identically on the unmodified baseline (verified by git stash, re-running, git stash pop), so neither is caused by this change:

  • dev/test/cli/cli_create_test.ts > … > should handle Vertex AI selection with gcloud defaultsAssertionError: expected "spy" to be called with arguments, environment-dependent (reads local gcloud defaults).
  • integrations/test/version_test.ts > version > should return the correct versionAssertionError: expected '1.5.0' to be '1.3.0', a stale hard-coded version pin against integrations/package.json.

app_loader_test.ts failed on its first invocation with all 6 tests skipped: its beforeAll runs npm install in each fixture under a 40s per-hook budget (app_loader_test.ts:31) and the cold install exceeded it. The warm rerun passed 6/6. Unrelated to this change (no alias is involved in a hook that shells out).

3. Static gates.

  • npx prettier vitest.config.ts --check → clean. Hoisting removed four levels of indentation, so the @google/adk-integrations entry now fits on one line and prettier collapses the previously-wrapped path.resolve(...); that collapse is expected, not an accidental edit. quoteProps: 'preserve' keeps the quotes on the keys.
  • npx eslint vitest.config.ts → clean.
  • npm run ts:check → the repo-wide run is red on main today (281 pre-existing errors in test files). The error list is byte-identical before and after this change (diff of the two sorted error sets is empty) and none of the 281 is in vitest.config.ts, so the new constant type-checks.
  • No suppressions added: git diff main -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|as any|as never|: any' → no hits. No any, no casts, no console.log.

4. Mutation testing — proof the verification above can actually fail.

Since there is no new test, the thing that must be shown to have signal is the equivalence check and the claim that all six projects consume the one constant.

  • Mutation A — delete the '@google/adk-integrations' entry from the constant. unit:integrations still passed (its lone test then resolves through the workspace symlink to integrations/dist, which exists after npm run build). This mutation is reported because it did NOT produce a failure: it is exactly the silent-fallback failure mode this refactor exists to prevent, and it shows why a pass/fail suite alone is not sufficient evidence here.

  • Mutation B (the load-bearing one) — repoint the single constant at nonexistent roots (./core/src_MUTANT, ./integrations/src_MUTANT) and run vitest list per project. All six projects failed collection, which is the property the refactor must preserve (the pre-change hazard is a package wired into five of six):

    unit:core / unit:dev / integration / e2e / cross-language:
      Error: Cannot find module '@google/adk' imported from '…/core/test/a2a/a2a_agent_test.ts'.
      Serialized Error: { code: 'ERR_MODULE_NOT_FOUND' }
    unit:integrations:
      Error: Cannot find module '@google/adk-integrations' imported from '…/integrations/test/version_test.ts'.
    
  • Mutation C — add the '@google/adk-devtools': path.resolve(__dirname, './dev/src') entry that this change deliberately omits, and run the web UI test:

    × WebUI Integration Test > 'Using ADK API server' > should load the WebUI correctly …
      → AssertionError: expected 404 to be 200 // Object.is equality
    Test Files  1 failed (1) | Tests  1 failed | 1 passed (2)
    

    Exactly the predicted 404. The CLI-spawning case still passes because it never touches the alias. This is why the doc comment on the constant exists.

    vitest.config.ts was restored from a byte-checked backup after each mutation (md5sum verified) and the gates re-run before committing.

5. Fixture suite (tests/integration/build_setup/build_setup_test.ts). Vitest aliases cannot affect it by construction: every fixture is reached through a child process (execAsync('npm install'), npm run build, spawn('npm', ['run','start']), npx @google/adk-devtools --version), and module resolution does not cross a process boundary. The fixtures' own devtools_check.ts / verify_devtools.ts do not match **/*_test.ts, so vitest never loads them; each fixture resolves "@google/adk-devtools": "file:../../../../dev" against its own manifest and consumes the built package, which is the point of those fixtures. Its measured result in this environment is recorded in the final comment on this PR.

Manual End-to-End (E2E) Tests:

npm install && npm run build     # the integration project needs dev/dist

# Resolved config is unchanged (sorted, because collection order is nondeterministic):
P="--project unit:core --project unit:dev --project unit:integrations --project integration --project e2e --project cross-language"
npx vitest list $P | sort | md5sum
git stash && npx vitest list $P | sort | md5sum && git stash pop   # same hash

# Both aliases still resolve to source, and the web UI test still passes:
npx vitest run --project unit:dev
npx vitest run --project unit:integrations
npx vitest run --project integration tests/integration/adk_web/webui_test.ts

npm run ts:check && npx eslint vitest.config.ts && npx prettier vitest.config.ts --check

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 six test projects in vitest.config.ts each declared a byte-identical
two-entry alias object. Adding a workspace package meant editing six places,
and wiring it into five of six fails silently: the sixth project resolves the
specifier through node_modules to built output instead of source.

Define the map once at module scope and reference it with the ES shorthand
`alias,` from every project. The resolved configuration is unchanged.

`@google/adk-devtools` stays unaliased, and the constant's doc comment says
why: AdkApiServer serves the web UI from a path relative to dist/{esm,cjs}/
server, so resolving it from dev/src makes GET /dev-ui/ return 404.
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Measured result for the fixture suite referenced in the testing plan (item 5), run locally on the pushed commit:

npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts

 ✓ |integration| tests/integration/build_setup/build_setup_test.ts (24 tests | 4 skipped) 537369ms
   ✓ Build setup > js_commonjs   > build and run / dynamic imports / import devtools / devtools CLI
   ✓ Build setup > js_esm        > build and run / dynamic imports / import devtools / devtools CLI
   ✓ Build setup > ts_commonjs   > build and run / dynamic imports / import devtools / devtools CLI
   ✓ Build setup > ts_esm        > build and run / dynamic imports / import devtools / devtools CLI
   ✓ Build setup > ts_commonjs_native_addon / ts_esm_native_addon > build and run / devtools CLI
 Test Files  1 passed (1)

Every fixture still resolves "@google/adk-devtools": "file:../../../../dev" against its own manifest and consumes the built package, unaffected by this change — as expected, since each fixture is reached through a child process that vitest's module resolution cannot cross. The 4 skips are the suite's own conditional cases, not skips introduced here.

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