Skip to content

Fix: make cli_create_test hermetic against ambient GOOGLE_CLOUD_* env vars (duplicate of #569) - #576

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/cli-create-test-gcloud-env-hermeticity
Open

Fix: make cli_create_test hermetic against ambient GOOGLE_CLOUD_* env vars (duplicate of #569)#576
AmaadMartin wants to merge 1 commit into
mainfrom
fix/cli-create-test-gcloud-env-hermeticity

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

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

⚠️ DUPLICATE DISCLOSURE — please read before reviewing.
This change duplicates #569 (fix/cli-create-test-stub-gcp-env), which is
open, mergeable and green, and also overlaps #259 and #203. I found all
three during the pre-implementation collision check and initially stopped
without writing code, on the grounds that a fourth implementation of the same
four-line test-only fix has negative value. That stop was not accepted by the
automated review loop, which blocked twice on the empty branch, so the work is
submitted here for a human to arbitrate.
Recommendation: close this PR in favour of #569 (or close #569 and take
this one — they are equivalent). Only one should land. #203 is CONFLICTING
and should be closed regardless. I am flagging this rather than letting a
reviewer discover the collision themselves.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

No public GitHub issue is associated with this change.

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

Problem: dev/test/cli/cli_create_test.ts is not hermetic. The test
createAgent > Interactive Mode > should handle Vertex AI selection with gcloud defaults asserts that the project prompt is seeded with the stubbed value
gcloud-project, but on any machine that exports GOOGLE_CLOUD_PROJECT it
receives that machine's real project id instead and fails:

- ObjectContaining { "initialValue": "gcloud-project" }
+ { "initialValue": "some-real-project", "message": "Enter the Google Cloud Project ID" }

The root cause is not an unstubbed gcloud shell-out — node:child_process
is already fully mocked at cli_create_test.ts:35-45, and that mock is
effective. The leak is one level earlier: getGcpProject()
(dev/src/cli/cli_create.ts:103-116) and getGcpRegion() (118-131) each
short-circuit on an environment variable before reaching execSync, and
nothing pinned those variables. Adding more execSync stubbing would have
changed nothing. This makes npm run test:unit red for environment-dependent
reasons, which trains contributors to ignore local failures.

Solution: Pin the two variables to undefined in the existing beforeEach
(file-level, so the whole file stays hermetic if another test later exercises
the Vertex path) and add vi.unstubAllEnvs() to the existing afterEach.
vi.stubEnv(name, undefined) deletes the variable for the test, which lets
if (process.env.GOOGLE_CLOUD_PROJECT) fall through to the already-mocked
execSync. The teardown mirrors the sibling CLI test at
dev/test/cli/cli_deploy_agent_engine_test.ts:241-247; vitest.config.ts does
not set unstubEnvs, so the explicit teardown is required rather than optional.

Two new tests cover the ambient-input paths that were previously untested: env
vars winning over the gcloud lookup, and the empty default when the lookup
throws.

Deliberate scope decisions:

  • No production change. dev/src/ is byte-identical to main. process.env
    already is the injection seam and vi.stubEnv is the supported way to drive
    it, so threading a readGcpDefaults callback through AgentCreationOptions or
    exporting the helpers purely for the test would add production surface with no
    production caller.
  • Existing assertions preserved. initialValue: 'gcloud-project' and the
    GOOGLE_CLOUD_PROJECT=gcloud-project .env assertion are untouched — that
    pair is the regression signal for the Vertex path. Only the fixture (the
    ambient environment) was pinned; no assertion was relaxed and no test deleted.
  • vitest.config.ts untouched. Repo-wide unstubEnvs is a separate concern
    (see Fix: scrub ADK environment variables from unit test runs #302 / Fix: make env-var stubbing hermetic across the test suite (vitest unstubEnvs + ambient-env coverage) #281) and out of scope here.

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.

Targeted run, under both ambient conditions — 11 tests before, 13 after:

GOOGLE_CLOUD_PROJECT=some-real-project GOOGLE_CLOUD_LOCATION=us-central1 \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts
#   before: Tests  1 failed | 10 passed (11)
#   after:  Tests  13 passed (13)

env -u GOOGLE_CLOUD_PROJECT -u GOOGLE_CLOUD_LOCATION \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts
#   after:  Tests  13 passed (13)

Coverage of the code under test (--coverage.include='dev/src/cli/cli_create.ts')
rises from 91.78% → 93.71% statements and 82.81% → 86.56% branches,
newly covering the GOOGLE_CLOUD_PROJECT / GOOGLE_CLOUD_LOCATION early
returns and both catch fallbacks. Functions remain 100%.

Proof each test can fail (mutation testing). Every new test was run against
unfixed code and confirmed to FAIL. Mutations were applied one at a time and the
source restored byte-identically after each:

  1. Hermeticity fix — removed the two vi.stubEnv lines, ran with the env vars
    exported → should handle Vertex AI selection with gcloud defaults fails:
    - "initialValue": "gcloud-project" / + "initialValue": "some-real-project"
    (the region prompt leaks us-central1 in the same run).
  2. Precedence test — deleted the
    if (process.env.GOOGLE_CLOUD_PROJECT) { return ... } block from
    getGcpProjectshould prefer ambient Google Cloud env vars over gcloud config fails on the load-bearing assertion:
    AssertionError: expected "spy" to not be called at all, but actually been called 1 times. Result: Tests 1 failed | 12 passed (13).
  3. Fallback test — changed getGcpProject's catch to return
    'mutant-default' instead of ''should seed empty defaults when the gcloud lookup fails fails: - "initialValue": "" /
    + "initialValue": "mutant-default". Result: Tests 1 failed | 12 passed (13).

expect(execSync).not.toHaveBeenCalled() is the load-bearing assertion: it pins
the precedence (env short-circuits before the shell-out), which is the exact
behaviour that caused this bug.

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

No E2E test is applicable — this is a unit-test hermeticity fix with no
production change and no cross-package surface. To verify manually, run the file
under all three ambient configurations and confirm 13/13 each time:

# 1. env vars exported (the original failure condition)
GOOGLE_CLOUD_PROJECT=some-real-project GOOGLE_CLOUD_LOCATION=us-central1 \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts

# 2. env vars absent
env -u GOOGLE_CLOUD_PROJECT -u GOOGLE_CLOUD_LOCATION \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts

# 3. a real gcloud default project configured, env vars absent
gcloud config set project <some-real-project>
env -u GOOGLE_CLOUD_PROJECT -u GOOGLE_CLOUD_LOCATION \
  npx vitest run --project unit:dev dev/test/cli/cli_create_test.ts

npm run build succeeds and npx eslint dev/test/cli/cli_create_test.ts is
clean. Note on npm run ts:check: it exits non-zero on this branch, but it does
so identically on unmodified main (verified by stashing this change), with
errors spread across pre-existing core/test/** files. cli_create_test.ts is
not among the files it reports, so this pre-existing breakage is neither caused
nor worsened here, and fixing it repo-wide is out of scope.

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.

… vars

getGcpProject()/getGcpRegion() return process.env.GOOGLE_CLOUD_PROJECT /
GOOGLE_CLOUD_LOCATION before they ever reach the mocked execSync, so a
developer who exports those vars saw "should handle Vertex AI selection
with gcloud defaults" fail with their real project id. The node:child_process
mock was never the gap; the unstubbed env read was.

Pin both vars to undefined in beforeEach and add vi.unstubAllEnvs() to
afterEach, mirroring dev/test/cli/cli_deploy_agent_engine_test.ts. Also
cover the two previously untested ambient-input paths: env vars winning
over the gcloud lookup, and the empty default when the lookup throws.

Test-only; dev/src is byte-identical. Coverage of cli_create.ts rises
91.78 -> 93.71 statements and 82.81 -> 86.56 branches.
AmaadMartin pushed a commit that referenced this pull request Aug 4, 2026
* fix(artifacts): isolate in-memory composite keys

* test(artifacts): cover storage key boundaries

* refactor(artifacts): encode in-memory key segments
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