Skip to content

Fix: pin GOOGLE_CLOUD_* env vars in cli_create_test so the gcloud defaults are actually testable - #569

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-create-test-stub-gcp-env
Open

Fix: pin GOOGLE_CLOUD_* env vars in cli_create_test so the gcloud defaults are actually testable#569
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-create-test-stub-gcp-env

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 3, 2026

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):
    N/A — no public issue is open for this.

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

Problem: dev/test/cli/cli_create_test.ts is not hermetic. getGcpProject() and getGcpRegion() (dev/src/cli/cli_create.ts:103-131) read GOOGLE_CLOUD_PROJECT / GOOGLE_CLOUD_LOCATION first and only shell out to gcloud config get-value as a fallback. The suite mocks node:child_process execSync to return 'gcloud-project' / 'gcloud-region', but never touches process.env. On a developer machine that exports those variables the env branch short-circuits, the execSync mock is dead code, and the prompt is seeded with the developer's real project id:

FAIL createAgent > Interactive Mode > should handle Vertex AI selection with gcloud defaults
- ObjectContaining { "initialValue": "gcloud-project" }
+ { "initialValue": "global", "message": "Enter the Google Cloud Region" }

CI runners have neither variable set, so the suite is permanently green there and permanently red for a large fraction of contributors — the worst combination, because the signal that should protect adk create is inverted depending on who runs it.

Solution: A test-only change, entirely additive (68 insertions, 0 deletions, one file).

  1. Pin the environment for the whole suite: vi.stubEnv('GOOGLE_CLOUD_PROJECT', undefined) / vi.stubEnv('GOOGLE_CLOUD_LOCATION', undefined) in the existing beforeEach, paired with vi.unstubAllEnvs() in the existing afterEach. In Vitest 3.2.x stubEnv(name, undefined) deletes the key, and unstubAllEnvs() restores the true original. vi.restoreAllMocks() does not unstub environment variables, so the unstubAllEnvs() call is required, not decorative. This idiom is already used in dev/test/server/adk_api_server_test.ts:964,973 and core/test/telemetry/setup_test.ts:30,96.
  2. The existing test should handle Vertex AI selection with gcloud defaults was strengthened in place, not replaced, and the edit is additive only. initialValue: 'gcloud-project' is preserved verbatim; the added message: 'Enter the Google Cloud Project ID' key only narrows the match; the region block is a net-new assertion. No assertion was removed, relaxed, or split out, so no regression signal is lost — with the env pin in place this test is now the explicit "env var absent -> gcloud value wins" case. Pinning message matters: without it toHaveBeenCalledWith matches any text call, which is why the original failure reported the region prompt rather than the project prompt and looked mysterious.
  3. Two new tests cover the branches CI's empty environment never reached — env vars winning over gcloud (with expect(execSync).not.toHaveBeenCalled()), and gcloud being unavailable (the catch returning '').

On mock-implementation leakage between tests. An earlier revision carried a two-line comment claiming the throwing execSync implementation survives into later tests, and I briefly added (execSync as Mock).mockReset() to beforeEach to defend against it. Both are gone, because the premise is false for the Vitest version this repo pins (3.2.6, per package-lock.json). Measured directly with a scratch suite (fn.mockImplementation(() => 'LEAKED') in one test, read back in the next):

beforeEach afterEach next test observes
clearAllMocks() restoreAllMocks() undefined — reset
clearAllMocks() (none) 'LEAKED' — leaks
(none) restoreAllMocks() undefined — reset

vi.clearAllMocks() alone does keep implementations, but the vi.restoreAllMocks() already present in this suite's afterEach clears them. Confirmed against the real module mock too: a probe asserting expect(() => execSync('gcloud config get-value project')).not.toThrow(), placed immediately after the throwing test with no mockReset, passes. So the ordering of the gcloud-unavailable test does not matter, there is no landmine to document, and a mockReset() call would have been dead code.

No production code changed. dev/src/cli/cli_create.ts is byte-identical; env-over-gcloud precedence is deliberate and matches how the rest of ADK resolves these variables. vitest.config.ts is untouched — no repo-wide unstubEnvs flag was added, since that is a separate concern from this file's hermeticity.

Collision check (required disclosure). Ran gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 before writing any code. Two open PRs already implement this same fix in this same single file and should be de-duplicated against this one:

Only one of {this PR, #259, #203} should be merged. I flagged this and initially stopped rather than build a third implementation; this branch exists because the automated review gate required the fix to be present here. Adjacent but non-colliding PRs reviewed and ruled out: #281 (repo-wide vitest unstubEnvs), #302 (shared tests/unit_setup.ts env scrub), #308 (core tracing env gate) — none touch cli_create_test.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.

env -u GOOGLE_CLOUD_PROJECT -u GOOGLE_CLOUD_LOCATION \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts
  -> Test Files 1 passed (1) | Tests 13 passed (13)

GOOGLE_CLOUD_PROJECT=some-project GOOGLE_CLOUD_LOCATION=global \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts
  -> Test Files 1 passed (1) | Tests 13 passed (13)     # was 1 failed / 10 passed before

npx vitest run --project unit:dev dev/test/cli   -> Test Files 5 passed (5) | Tests 78 passed (78)
npm run build                                    -> OK
npm run lint                                     -> OK
npx prettier --check dev/test/cli/cli_create_test.ts -> All matched files use Prettier code style!

Coverage of dev/src/cli/cli_create.ts from this file (v8, --coverage.include='dev/src/cli/cli_create.ts'):

% Stmts % Branch
before 91.78 82.81
after 93.71 86.56

All four previously-unexercised branches of getGcpProject() / getGcpRegion() (two env short-circuits, two catch fallbacks) are now covered. New test code is 100% executed by construction; the residual uncovered lines are unrelated paths in the same file (e.g. npm install error handling) that this task does not touch.

Proof each test can fail. Every assertion was run against mutated code and confirmed red. Source restored to pristine after each (git diff --stat dev/src/cli/cli_create.ts empty).

# Mutation Test that went red Failure
1 Remove the two vi.stubEnv(..., undefined) lines; run with GOOGLE_CLOUD_PROJECT=some-project GOOGLE_CLOUD_LOCATION=global should handle Vertex AI selection with gcloud defaults and should fall back to an empty initial value when gcloud is unavailable - "initialValue": "gcloud-project" / + "initialValue": "some-project"; - "initialValue": "" / + "initialValue": "some-project" — 2 failed | 11 passed
2 Delete the GOOGLE_CLOUD_PROJECT short-circuit (cli_create.ts:104-106) should prefer the GCP environment variables over the gcloud defaults expected "spy" to be called with arguments: [ ObjectContaining{…} ] — 1 failed | 12 passed
3 Delete the GOOGLE_CLOUD_LOCATION short-circuit (cli_create.ts:119-121) should prefer the GCP environment variables over the gcloud defaults same matcher failure — 1 failed | 12 passed
4 cli_create.ts:114 return ''; -> return 'x'; should fall back to an empty initial value when gcloud is unavailable 1 failed | 12 passed
5 getGcpRegion() return stdout.trim(); -> return ''; (cli_create.ts:127) should handle Vertex AI selection with gcloud defaults (the newly added region assertion) 1 failed | 12 passed

Mutation 1 is the reported bug reproducing verbatim; mutation 5 proves the assertion added to the existing test is load-bearing rather than decorative.

Manual End-to-End (E2E) Tests:
No E2E surface: this is a unit-test hermeticity fix, and adk create performs real npm install calls that must not run in CI. To reproduce the original failure and verify the fix by hand, from the repository root:

npm install && npm run build

# On main this FAILS; on this branch it passes.
GOOGLE_CLOUD_PROJECT=some-project GOOGLE_CLOUD_LOCATION=global \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts

# Passes on both, and must produce identical output to the run above.
env -u GOOGLE_CLOUD_PROJECT -u GOOGLE_CLOUD_LOCATION \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts

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 August 2, 2026 23:45
…table

getGcpProject()/getGcpRegion() read GOOGLE_CLOUD_PROJECT/GOOGLE_CLOUD_LOCATION
before shelling out to gcloud, so on a developer machine that exports them the
execSync mock in this suite was never consulted and the gcloud-defaults test
asserted against the developer's real project id.

Pin both variables to absent for the whole suite with vi.stubEnv(name, undefined)
and restore them with vi.unstubAllEnvs() in afterEach (vi.restoreAllMocks()
restores spies only). Add coverage for the two branches the empty CI environment
never reached: env vars winning over gcloud, and gcloud being unavailable.

No production code changes.
The env-precedence test also asserts execSync is never called, so the
execSync implementation it installed could never run. Remove it.

Remove the comment claiming the throwing execSync implementation survives
into later tests. Measured on the pinned vitest 3.2.6: clearAllMocks() alone
does keep implementations, but the restoreAllMocks() already in this suite's
afterEach clears them, so the ordering of that test is not load-bearing.
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