Fix: scrub ADK environment variables from unit test runs - #302
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: scrub ADK environment variables from unit test runs#302AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
This was referenced Jul 30, 2026
Fix: gitignore the .env that adk create writes so scaffolded agents cannot commit their API key
#364
Open
Open
Closed
added 2 commits
August 2, 2026 21:08
The three unit projects run in a process that inherits the developer's shell, and a lot of ADK production code reads configuration straight from process.env. A contributor with GOOGLE_CLOUD_PROJECT, DATABASE_URL or an OpenTelemetry endpoint exported runs a different suite from the one CI runs, because Actions runners export none of these: OTEL_EXPORTER_OTLP_ENDPOINT=http://127.0.0.1:4318 \ npx vitest run --project unit:core core/test/telemetry/setup_test.ts # 4 failed / 2 passed -- maybeSetOtelProviders() installs providers # from the ambient endpoint DATABASE_URL=bogus://nope \ npx vitest run --project unit:dev dev/test/cli/cli_test.ts # 9 failed / 12 passed -- "Unsupported session service URI" Wire tests/unit_setup.ts into the three unit:* projects. It deletes the 13 variables that core/src, dev/src or integrations/src read directly, plus the ADK_ prefix family, then writes them back in an afterAll. The prefix rule cannot be a literal list: feature_registry.ts composes ADK_ENABLE_${featureName} from an enum that grows. The scrub runs at module scope, not in a beforeEach. It has to land before the test file's module body, and a top-level beforeEach would run after apigee_llm_test.ts's beforeAll and delete the three variables that suite sets for itself. GOOGLE_APPLICATION_CREDENTIALS is deliberately absent: no ADK source reads it, and deleting it would not make auth hermetic anyway, since Application Default Credentials also resolve via the gcloud well-known file and the metadata server. integration, e2e and cross-language keep the ambient environment, which is how a developer supplies real credentials to them.
One test file per unit project, because setupFiles is configured per Vitest project and each project's wiring needs its own proof. The duplication is deliberate; a shared helper would obscure what is being proven. The core file adds the two cases that pin the design rather than the config: that the scrub does not empty the environment, and that a value the test file sets for itself survives it. The second fails if anyone converts the module-scope scrub into a beforeEach.
AmaadMartin
force-pushed
the
fix/hermetic-unit-test-env
branch
from
August 3, 2026 04:13
f0feb16 to
6c432e1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A — no existing issue.
Or, if no issue exists, describe the change:
Problem: a unit test must produce the same result on a contributor's laptop and in CI, and today it does not. The three unit projects run in a process that inherits the developer's shell, and a lot of ADK production code reads configuration straight from
process.env. A contributor who hasGOOGLE_CLOUD_PROJECT,DATABASE_URL,GEMINI_API_KEYor an OpenTelemetry endpoint exported — the normal state for anyone who also uses ADK — is running a different suite from the one CI runs. GitHub Actions runners export none of these, which is exactly why this class of bug is invisible in CI.Two reproductions on
main, both green in CI:Solution: a test-only backstop.
tests/unit_setup.tsis wired assetupFilesinto the threeunit:*projects; it deletes the ADK-relevant variables from the worker's environment before the test file is imported, and writes them back in anafterAll. The policy is a unit test that needs a value sets it explicitly.Two exported constants, because one of the two rules cannot be a literal list:
SCRUBBED_ENV_VARS— the 13 names thatcore/src,dev/srcorintegrations/srcread directly.SCRUBBED_ENV_PREFIXES = ['ADK_']—core/src/features/feature_registry.tscomposesADK_ENABLE_${featureName}/ADK_DISABLE_${featureName}from an enum that grows, so the ADK family cannot be enumerated. Every ADK-owned variable isADK_-prefixed, so the prefix is the accurate invariant. No speculative extra prefixes (GOOGLE_,OTEL_) were added.Design points worth calling out:
beforeEach. This is load-bearing. The scrub has to land before the test file's module body runs, because several test files captureprocess.envat module ordescribescope and restore it later. More concretely, a top-levelbeforeEachwould run aftercore/test/models/apigee_llm_test.ts'sbeforeAll, which setsAPIGEE_PROXY_URL,GOOGLE_CLOUD_PROJECTandGOOGLE_CLOUD_LOCATIONand relies on them — it would delete the values that suite needs. Mutation 2 below demonstrates exactly that breakage, so the design is pinned by a test rather than by a comment.setupFiles, notglobalSetup.globalSetupruns in the Vitest main process and cannot touch a worker'sprocess.env; it is also run-wide, so scrubbing there would hite2e, where ambient credentials are how a developer supplies real ones.tests/global_setup.tsis untouched.unit:*blocks are wired.integration,e2eandcross-languageare unchanged and keep the ambient environment.grep -n setupFiles vitest.config.tsshows exactly three occurrences ofUNIT_SETUP_FILE, all insideunit:*blocks.delete, notvi.stubEnv. A stub is undone by any test callingvi.unstubAllEnvs()(or by theunstubEnvs: trueoption under review in Fix: make env-var stubbing hermetic across the test suite (vitest unstubEnvs + ambient-env coverage) #281), which would resurrect the ambient value. A delete is not.GOOGLE_APPLICATION_CREDENTIALSis deliberately not scrubbed. An earlier revision of this branch included it. It fails the invariant that every listed name has a reader —grep -r GOOGLE_APPLICATION_CREDENTIALS core/src dev/src integrations/srcreturns nothing — and deleting it would not make auth hermetic anyway, since Application Default Credentials also resolve via the gcloud well-known file and the metadata server. A unit test that reaches real auth is broken for a different reason.Known remaining gap (not fixable here, stated rather than hidden):
dev/test/cli/cli_test.tsimportsdev/src/cli/cli.ts, whose module scope callsdotenv.config({quiet: true}). That import happens after setup files have run, so a root.envwould repopulate scrubbed variables for that one file — and because dotenv does not override already-set variables, the scrub makes a root.envmore likely to apply there than it is today. There is no.envin the repo (it is gitignored) and no other unit test imports a module that callsdotenv.config(). A separate in-flight change removes that call; this one cannot.Collision check.
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000(a--limit 100scan silently truncates against the ~380 open PRs on this fork), thengh pr diff --name-onlyon every plausibly adjacent PR:unstubEnvs/unstubGlobalsper project, plus per-testvi.stubEnvfixes) — overlaps in intent and touches the same project blocks, but is a different mechanism: it fixes stub lifecycle, not the ambient baseline. Neither lands the other's change, and they compose:vi.unstubAllEnvs()restores a variable to its value at stub time, which after scrubbing is "absent".DATABASE_URLinsidedev/test/cli/cli_test.ts) — same class of bug, one variable, one file. This PR is the repo-wide backstop underneath it; the two compose and neither is redundant.vi.stubEnvincli_create_test.ts) and Fix: make the adk create gcloud-defaults unit test hermetic #203 (injectable gcloud defaults) — both fix reproduction 3 at the individual-test level. Deliberately does not touch either file.vitest.config.ts) — textual neighbours only../tests/setup_log_level.tsto the same threesetupFilesarrays; Fix: pin the ADK test log level inside the vitest workers #557 is the same change re-based onmain. Neither collides — they build on this. Fix: pin the test log level inside the vitest workers, not in globalSetup #349 needs a one-line update for the rename toUNIT_SETUP_FILE, which belongs on that branch rather than here.I branched from
mainrather than stacking: the overlapping PRs have no single correct base, and a stacked base would suppress CI (the workflow triggers onpull_request: branches: [main]).Rebased onto current
main. The previous head of this branch conflicted withmaininvitest.config.ts(mainaddedINTEGRATION_HOOK_TIMEOUT_MS/INTEGRATION_TEST_TIMEOUT_MSin the same place this change addsUNIT_SETUP_FILE), and GitHub could not build a merge ref, so no CI ran at all on that commit. Rebasing resolves it by keeping both constant blocks; everything reported below was re-measured on the rebased tree.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.
Three near-identical test files —
core/test/unit_setup_test.ts,dev/test/unit_setup_test.ts,integrations/test/unit_setup_test.ts. The duplication is intentional:setupFilesis configured per Vitest project, so each project needs its own test to prove its own wiring. They are deliberately not factored into a shared helper; the indirection would obscure what is being proven. Thecoreone adds two cases beyond the wiring assertions: that the scrub does not empty the environment, and that a value the test file sets itself survives — the assertion that pins the module-scope design.Proof each test can fail. Four mutations, each run against the real suite:
setupFilesfrom theunit:coreblock, dirty shellcore/test/unit_setup_test.tsstill passes 4/4 — see the honest note below.core/test/telemetry/setup_test.ts, which does not import the setup module, fails 4 / 2beforeEacha value set by the test file › survives the scrubFAILS (expected undefined to be 'explicit-project') andApigeeLlm LLMRegistry integration › ApigeeLlm is registered by defaultFAILS (Proxy URL must be provided via the constructor or APIGEE_PROXY_URL environment variable.) — exactly the breakage the module-scope design exists to avoidSCRUBBED_ENV_PREFIXES = []tracing_test.tsunderADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=falseFAILS 3 / 3DATABASE_URLandOTEL_EXPORTER_OTLP_ENDPOINTfrom the listHonest note on mutation 1. The design expected the
unit_setup_test.tsassertions to fail when the wiring is removed. They do not, and the reason is structural: those tests importtests/unit_setup.jsto get the constants, and that import evaluates the module, which performs the scrub. So the file scrubs its own environment even withsetupFilesdeleted (setup 0msin the run output confirms the setup file did not run). The wiring is therefore genuinely pinned by mutation 1's second half, plus 3 and 4 — all of which use test files that do not import the setup module — while the threeunit_setup_test.tsfiles pin the scrub policy. Flagging this rather than reporting a mutation result I did not observe.Coverage.
coverage.includeiscore/src/**,dev/src/**,integrations/src/**; everything added here lives undertests/and*/test/, so it contributes no lines to the report and cannot move the thresholds (unchanged at statements 86 / branches 87 / functions 88 / lines 86). Every branch of the scrub policy is asserted behaviourally instead: the list rule, the prefix rule, the narrowness of the scrub, and "explicit set wins".The
afterAllrestore is verified by probe, not by assertion. Its only observable moment is after the last hook of a test file has run, and hooks are'stack'-ordered, so a test-fileafterAllalways runs before it — no in-suite assertion can see it. Instead I temporarily registered a probe hook ahead of the restore (so reverse ordering put it after) that recorded the environment it saw, and randev/test/unit_setup_test.tswithGOOGLE_CLOUD_PROJECT=ambient-proj ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=falseexported. It recordedGOOGLE_CLOUD_PROJECT=ambient-proj ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=false, confirming both a listed variable and a prefix-matched one are written back. The probe was removed and is not part of this PR.Manual End-to-End (E2E) Tests:
The deliverable is suite-wide hermeticity, so the verification is the whole unit suite under both environments. After
npm install:Both report
Test Files 2 failed | 184 passed (186)andTests 4 failed | 2579 passed (2583)— identical counts and an identical set of failing tests, which is the postcondition this change exists to establish.Those 4 failures are pre-existing and unrelated: a stale expected version string in
integrations/test/version_test.ts, and threeunsafe_local_code_executor_test.tsoutput-file cases. Verified by checking outmainwith this change absent and reproducing them (Tests 4 failed | 15 passed (19)). They are the subject of #479 and #355 / #516.Both original reproductions now pass:
Other gates, run on the pushed commit:
bash scripts/check_license.sh—All files have the correct license header.npm run lint— clean.npm run format:check—All matched files use Prettier code style!npx tsc --noEmit—Found 301 errors in 49 files, identical tomain, all pre-existingcore/dist/typesvscore/srcidentity mismatches from a stale build; grepping the output forunit_setuporvitest.configreturns nothing, so this diff adds none.No
any, no@ts-expect-error, noeslint-disableand no coverage-ignore pragma appears anywhere in this diff.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.