Skip to content

Fix: make env-var stubbing hermetic across the test suite (vitest unstubEnvs + ambient-env coverage) - #281

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/vitest-env-stub-hermeticity
Open

Fix: make env-var stubbing hermetic across the test suite (vitest unstubEnvs + ambient-env coverage)#281
AmaadMartin wants to merge 3 commits into
mainfrom
fix/vitest-env-stub-hermeticity

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:

Problem: Test results depend on which environment variables happen to be exported on the machine running them, and on which test ran before them.

  1. vi.stubEnv leaks between tests. Vitest's unstubEnvs defaults to false, so a value set with vi.stubEnv stays applied to process.env for every subsequent test until someone calls vi.unstubAllEnvs() by hand. core/test/code_executors/agent_engine_sandbox_code_executor_test.ts stubs GOOGLE_CLOUD_PROJECT='' mid-file and never unstubs. It does not fail today, but the next test added after it that expects GOOGLE_CLOUD_PROJECT to be 'test-project' will pass or fail purely based on its position in the file. process.env is process-global, so the blast radius is every file sharing the worker.
  2. core/test/utils/vertex_ai_utils_test.ts hand-rolled process.env save/restore, replacing the whole object with a shallow copy in beforeEach and restoring the reference in afterEach. Reassigning process.env swaps out Node's special env object for a plain one, and cannot interoperate with vi.unstubAllEnvs(), which writes restored values onto whatever process.env points at when it runs.
  3. Ambient-env reads with no coverage. Nothing pinned dev/src/cli/cli.ts:62's DATABASE_URL fallback or dev/src/utils/telemetry_utils.ts:129-134's four OTEL_EXPORTER_OTLP_* reads. A developer or CI runner exporting DATABASE_URL silently gets a DatabaseSessionService instead of an InMemorySessionService from adk web / api_server / run; one exporting any OTEL_EXPORTER_OTLP_* silently switches setupTelemetry() onto its env-driven branch. The suite stays green either way.
  4. Two existing tests assert negative cases without neutralising the ambient variable they depend on, so they genuinely fail on a polluted machine (reproduced below).

Solution: Make the harness guarantee env isolation instead of relying on every author remembering an afterEach. Zero production-code changes — the diff is vitest.config.ts plus files under core/test/ and dev/test/.

  • vitest.config.ts: add unstubEnvs: true and unstubGlobals: true to each of the six inline projects. They are deliberately not at the root: Vitest's Test Projects guide states "None of the configuration options are inherited from the root-level config file", and inline project entries default to extends: false, so a root placement is a silent no-op. extends: true was rejected because it would also pull the root poolOptions, globalSetup and coverage into every project. This is verified empirically by mutation M4 below.
  • core/test/utils/{env_stub_hermeticity_test.ts}, dev/test/utils/{env_stub_hermeticity_test.ts} (new): two ordered probe tests per project that pin the flags. Two near-identical files is intentional — the non-inheritance above means each project needs its own probe.
  • core/test/utils/vertex_ai_utils_test.ts: migrated off process.env mutation to vi.stubEnv, with a beforeEach that neutralises GOOGLE_GENAI_USE_VERTEXAI and GOOGLE_API_KEY. No manual vi.unstubAllEnvs() — that is now the config's job, and its absence is part of what proves the config works. All eight existing cases are unchanged in intent.
  • core/test/telemetry/setup_test.ts: replaced the now-redundant vi.unstubAllEnvs() with explicit neutralisation of all four OTEL_EXPORTER_OTLP_* variables. The old call cleared stubs; the new loop neutralises ambient values, which is the actual defect.
  • core/test/sessions/vertex_ai_session_service_test.ts: same class of defect, found by running the suite with a polluted environment (see "Manual E2E Tests"). Express mode resolves a key from the ambient env, which stops the constructor throwing, so 'throws an error if no client and no project/location provided' fails on a machine exporting GOOGLE_GENAI_USE_VERTEXAI + GOOGLE_API_KEY. Now stubs both to undefined.
  • dev/test/cli/cli_test.ts, dev/test/utils/telemetry_utils_test.ts (new): cover the two previously untested ambient reads, stubbing the variables explicitly (including to undefined for the "not set" cases).

Collision check (run before implementing, per contribution hygiene): gh pr list --state open --limit 100 plus a gh pr diff --name-only scan of all 100 open PRs. No open PR adds unstubEnvs/unstubGlobals or migrates env stubbing, so this does not duplicate live work. Files that merely overlap, none of which implements this change: vitest.config.ts in #261 (hoists the duplicated alias map), #237 (adds an integration:slow project) and #247 (adds server.deps.external); core/test/utils/vertex_ai_utils_test.ts in #227, #265 and #268 (all adding new cases for other features); dev/test/cli/cli_create_test.ts hermeticity in #259 and #203. Because three mutually-conflicting siblings touch vitest.config.ts, this branches from main rather than stacking on any one of them; the conflicts are textual and trivial in every direction. dev/test/cli/cli_create.ts and its test are deliberately not touched — #259/#203 own them.

Intentionally not done: only unit:core and unit:dev get probe files, though all six projects get the flags. The other four projects contain no vi.stubEnv/vi.stubGlobal call at all (verified by grep), so a probe there would pin nothing anyone can break. Note also that the probes rely on default sequential ordering: they would pass vacuously under -t filtering or sequence.shuffle.

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 unit:core --project unit:dev176 files / 2429 tests pass, up from 2411 on the base commit (+18 new). The single failure, dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults, is pre-existing on the base commit and reproduces identically there; it is the ambient-GOOGLE_CLOUD_PROJECT defect owned by the concurrent cli_create_test task, and this machine exports that variable.

Also clean on the exact pushed commit: npm run build, npm run lint, npm run format:check. tsc --noEmit reports 308 errors before and after, byte-identical once line numbers are normalised — all pre-existing; this branch adds none.

Coverage. dev/src/utils/telemetry_utils.ts reaches 100% branch coverage and all four functions this PR targets (otelEnvVarsEnabled, setupTelemetry, setupGcpTelemetryExperimental, setupTelemetryFromEnvExperimental, lines 128-190) are fully covered. File-level statements read 51.07% because the file also contains hrTimeToNanoseconds, ApiServerSpanExporter and InMemoryExporter (lines 26-125) — pre-existing untested code that this PR does not touch and deliberately does not pad tests for.

Proof that each new test can fail. Every mutation below was applied, run, and reverted; the working tree is clean and this PR contains no src changes.

# Mutation Result
M1 Remove unstubEnvs: true from the unit:core project core/.../env_stub_hermeticity_test.ts > does not inherit stubs from the previous test FAILS — AssertionError: expected 'stubbed' to be undefined
M2 Remove unstubEnvs: true from the unit:dev project dev/.../env_stub_hermeticity_test.ts same test FAILS — AssertionError: expected 'stubbed' to be undefined
M3 Remove unstubGlobals: true from the unit:core project same test FAILS — AssertionError: expected true to be false // Object.is equality
M4 Move both flags to the root test block instead of the projects BOTH probes FAIL — AssertionError: expected 'stubbed' to be undefined. This is the empirical proof that a root placement is a no-op
M5 cli.ts:62: drop || process.env.DATABASE_URL 3 tests FAIL — expected InMemorySessionService{…} to be an instance of DatabaseSessionService (web, api_server, run)
M6 cli.ts:62: process.env.DATABASE_URL || options[...] should prefer --session_service_uri over DATABASE_URL FAILS — expected DatabaseSessionService{…} to be an instance of InMemorySessionService
M7 telemetry_utils.ts: delete OTEL_EXPORTER_OTLP_METRICS_ENDPOINT from endpointVars should add no hooks of its own when OTEL_EXPORTER_OTLP_METRICS_ENDPOINT is set FAILS — expected "spy" to be called with arguments: [ [] ]
M8 telemetry_utils.ts: invert the otelToCloud / otelEnvVarsEnabled() precedence should prefer the GCP branch over the env branch FAILS — expected "spy" to be called with arguments: [ { enableTracing: true, …(2) } ]
M9 Revert the setup_test.ts OTEL neutralisation, with OTEL_EXPORTER_OTLP_ENDPOINT exported 4 tests FAIL — e.g. expected "setGlobalTracerProvider" to not be called at all, but actually been called 1 times
M10 Revert the vertex_ai_session_service_test.ts stubs, with GOOGLE_GENAI_USE_VERTEXAI + GOOGLE_API_KEY exported throws an error if no client and no project/location provided FAILS — expected [Function] to throw error including 'Either (Project ID and Location) or a…' but got 'Authentication is not set up. Please …'

One negative result, reported rather than hidden: reverting the vertex_ai_utils_test.ts migration under the same polluted environment does not fail — the old file happened to be ambient-safe because each case deleted the variable it read. That change is therefore a migration off the process.env-reassignment anti-pattern (which cannot interoperate with the now-global unstub), not a defect fix, and its proof is only that all eight cases still pass. It is not claimed as a bug fix.

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

The point of the change is that a polluted environment no longer changes the result. From the repo root:

# 1. baseline
npx vitest run --project unit:core --project unit:dev

# 2. same command with a polluted environment
DATABASE_URL=postgresql://u:p@localhost:5432/db \
OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 \
GOOGLE_GENAI_USE_VERTEXAI=true \
GOOGLE_API_KEY=ambient-key \
  npx vitest run --project unit:core --project unit:dev

Both report Test Files 1 failed | 176 passed (177) / Tests 1 failed | 2429 passed (2430) — identical, with the same single pre-existing cli_create_test.ts failure described above.

On the base commit, run 2 fails two extra suites that run 1 passes (core/test/telemetry/setup_test.ts, 4 cases, and core/test/sessions/vertex_ai_session_service_test.ts, 1 case) — that divergence is the defect this PR removes.

--project integration --project e2e was also exercised, but it is not a usable signal on a machine without live credentials: three consecutive runs of the same tree produced 22, 24 and 38 failures with 50, 46 and 24 skips, i.e. it is nondeterministic here. What can be stated deterministically is that no file under tests/ or integrations/test/ calls vi.stubEnv or vi.stubGlobal at all (grep-verified), so vi.unstubAllEnvs()/vi.unstubAllGlobals() operate on an empty stub map there and are provable no-ops. The process.env reads in those trees are top-level describe.skipIf/it.skipIf guards evaluated at collection time, before any unstub runs.

Flipping the flags on cannot break an existing stub: the automatic unstub runs in onBeforeTryTask, which @vitest/runner invokes before a suite's beforeEach hooks, and all eleven files calling vi.stubEnv/vi.stubGlobal install their stubs in beforeEach (or in the test body) — none in beforeAll. That ordering is also why core/test/code_executors/agent_engine_sandbox_code_executor_test.ts keeps working with no change.

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 3 commits July 29, 2026 19:43
vi.stubEnv leaks into every later test unless unstubEnvs is enabled, so a
stub set by one test silently changes the environment the next one reads.
process.env is process-global, so the blast radius is every test file
sharing the worker, not just the file that stubbed.

Vitest projects inherit nothing from the root-level config, so both flags
have to live in each project's own test block. The two
env_stub_hermeticity_test.ts probes pin that placement: they are
deliberately order-dependent and fail if the flags are set at the root or
dropped from a project.
vertex_ai_utils_test replaced process.env wholesale with a shallow copy,
which swaps out Node's env object and cannot interoperate with vitest's
automatic unstub. It now uses vi.stubEnv and neutralises the two
variables it reads.

setup_test and vertex_ai_session_service_test assert negative cases while
leaving the ambient OTEL endpoint and express-mode variables in place, so
they fail on a machine that exports them; both now stub those to
undefined.
Nothing pinned dev/src/cli/cli.ts's DATABASE_URL fallback or
dev/src/utils/telemetry_utils.ts's four OTEL_EXPORTER_OTLP_* reads, so a
developer or CI runner exporting either silently got a different session
service or a different telemetry branch and the suite stayed green.

Both suites stub the variables they read, including to undefined for the
not-set cases, so they produce the same result on a clean machine and a
polluted one.
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