Skip to content

Fix: scrub ambient DATABASE_URL across the whole dev CLI unit suite - #348

Open
AmaadMartin wants to merge 1 commit into
fix/vitest-env-stub-hermeticityfrom
fix/hermetic-otel-and-database-url-unit-tests
Open

Fix: scrub ambient DATABASE_URL across the whole dev CLI unit suite#348
AmaadMartin wants to merge 1 commit into
fix/vitest-env-stub-hermeticityfrom
fix/hermetic-otel-and-database-url-unit-tests

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:

Stacked PR. This targets fix/vitest-env-stub-hermeticity (#281), not main. See the collision check below — #281 already lands the OTLP half of this work, and this PR is only the residual it leaves behind. Review #281 first.

Problem: dev/test/cli/cli_test.ts is not hermetic against an ambient DATABASE_URL.

Every web / api_server / run case routes through getSessionServiceFromOptions, which resolves options['session_service_uri'] || process.env.DATABASE_URL || 'memory://' (dev/src/cli/cli.ts:62). On the base branch only the five cases inside describe('session service resolution') pin DATABASE_URL; the other 21 inherit whatever the developer's shell exports. That produces two failure modes, both measured on the base commit (ae2dcbde):

Ambient value Result on base branch
DATABASE_URL='bogus://nope' (unrecognised scheme) 9 of 26 failgetSessionServiceFromUri throws Unsupported session service URI, the CLI error handler calls process.exit(1)
DATABASE_URL='postgres://u:p@localhost:5432/db' (recognised scheme) 26/26 pass, silently wrong — the CLI hands AdkApiServer a DatabaseSessionService where the tests assume an InMemorySessionService. Nothing asserts on it, so the suite stays green while exercising the wrong object.

The second is the worse of the two: it is invisible.

Solution: Stub DATABASE_URL to undefined in the suite's shared beforeEach, so every case owns the variable instead of inheriting it, and add one case that deliberately leaves it unstubbed to pin that scrub.

Two details that are load-bearing rather than incidental:

  • The ambient value is assigned at module scope, not in a hook. The base branch sets unstubEnvs: true for unit:dev, and Vitest snapshots each variable at its first vi.stubEnv call and restores to that snapshot between tests. An ambient value injected after that point is therefore erased rather than inherited, which makes the test vacuous — see mutation M2 below, which is exactly the trap this hit during development. A plain assignment made before any stub is the only way to simulate a shell-exported variable faithfully.
  • The new case asserts the resolved URI rather than the concrete class. importOriginal is now typed (importOriginal<typeof import('@google/adk')>()), which drops an as object cast and lets the mock factory wrap the real getSessionServiceFromUri in a spy. Asserting toHaveBeenCalledWith('memory://') states precisely what the CLI decided, avoids instanceof (unreliable when two copies of @google/adk share a runtime), and names the leaked value in the failure message. The five sibling cases keep their toBeInstanceOf assertions — this PR does not rewrite existing tests.

No production source file is modified. dev/src/cli/cli.ts is behaving as designed; the DATABASE_URL fallback is a documented CLI feature and is left intact.

Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 100 (100 open PRs), then gh pr diff --name-only on every plausibly adjacent one. This found #281 fix/vitest-env-stub-hermeticity, which touches both files this task originally targeted. I checked out its head and re-ran the repro rather than reading the diff:

Because the overlap is partial, this branches from fix/vitest-env-stub-hermeticity instead of main and targets it as the base. Other PRs scanned and ruled out: #302 (tests/hermetic_env_setup.ts; its scrub list is credentials-only — GOOGLE_* — and deliberately excludes OTLP and DATABASE_URL), #308 (tracing_test.ts, ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS), #259 (cli_create_test.ts).

Out of scope. dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults fails on this machine both before and after this change. It is caused by an ambient GOOGLE_CLOUD_PROJECT (dev/src/cli/cli_create.ts:104 prefers the env var over the mocked execSync lookup) and belongs to the separate credential-scrub work. Untouched 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.

dev/test/cli/cli_test.ts27/27 pass in all three environments, where the base branch passes only two of the three:

npx vitest run --project unit:dev dev/test/cli/cli_test.ts                              # 27/27
DATABASE_URL='bogus://nope' npx vitest run --project unit:dev dev/test/cli/cli_test.ts  # 27/27 (base: 9 failed)
DATABASE_URL='postgres://u:p@localhost:5432/db' \
  npx vitest run --project unit:dev dev/test/cli/cli_test.ts                            # 27/27

Whole unit suite, clean vs. fully polluted — identical, which is the acceptance criterion:

npx vitest run --project unit:core --project unit:dev
#   Test Files  1 failed | 176 passed (177)
#        Tests  1 failed | 2430 passed (2431)

OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 \
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=http://localhost:4318 \
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=http://localhost:4318 \
OTEL_EXPORTER_OTLP_LOGS_ENDPOINT=http://localhost:4318 \
DATABASE_URL='bogus://nope' \
  npx vitest run --project unit:core --project unit:dev
#   Test Files  1 failed | 176 passed (177)
#        Tests  1 failed | 2430 passed (2431)

The single failure in both runs is the pre-existing cli_create_test.ts case described above.

Proof the new test can fail. Three mutations, each applied, run, and reverted. The working tree is clean and this PR contains no src changes.

# Mutation Result
M1 Remove vi.stubEnv('DATABASE_URL', undefined) from the shared beforeEach should ignore an ambient DATABASE_URL FAILS on a clean environment — AssertionError: expected "getSessionServiceFromUri" to be called with arguments: [ 'memory://' ], received "postgresql://ambient:pass@localhost:5432/ambient". 1 failed | 26 passed.
M2 Keep the scrub removed, but move the ambient assignment from module scope into a beforeAll nested inside describe('session service resolution') — i.e. after the sibling cases have already stubbed DATABASE_URL 27/27 pass — the test becomes vacuous. The automatic unstub restores DATABASE_URL to the value recorded at the first vi.stubEnv (undefined), wiping the injection before the test body runs. This is why the assignment sits at module scope. An outer beforeAll also works, since it too runs before the first stub; only a nested one placed after the sibling stubs is silently ineffective.
M3 dev/src/cli/cli.ts:62 — delete the process.env.DATABASE_URL || term 3 of the base branch's fallback cases FAIL (web, api_server, run). The new case correctly does not fail: it asserts memory://, which is what this mutation produces. That confirms it pins the hermeticity scrub and not the fallback, so the two sets of tests are orthogonal. Source restored.

M2 is the one worth flagging to reviewers: it is a genuine trap, and the first version of this test fell into it and passed for the wrong reason.

Repo gates, run on the exact pushed commit:

npm run build         # exit 0
npm run lint          # exit 0
npm run format:check  # "All matched files use Prettier code style!"
npm run ts:check      # 308 errors before AND after -- all pre-existing repo-wide;
                      # dev/test/cli/cli_test.ts appears in none of them

CI is expected to be absent on this PR, not green. Every test workflow is gated on pull_request: branches: [main] (cross-language-integration.yml, license-check.yml), and this PR's base is fix/vitest-env-stub-hermeticity, so those jobs will not trigger. The local runs above are the validation.

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

From the repository root, after npm install && npm run build:

# 1. Confirm the defect on the base branch.
git checkout fix/vitest-env-stub-hermeticity
DATABASE_URL='bogus://nope' npx vitest run --project unit:dev dev/test/cli/cli_test.ts
#    -> Tests  9 failed | 17 passed (26)

# 2. Same command on this branch.
git checkout fix/hermetic-otel-and-database-url-unit-tests
DATABASE_URL='bogus://nope' npx vitest run --project unit:dev dev/test/cli/cli_test.ts
#    -> Tests  27 passed (27)

# 3. The silent form: a recognised scheme is green on both branches, but on the base
#    branch the 21 pre-existing cases are exercising a DatabaseSessionService.
DATABASE_URL='postgres://u:p@localhost:5432/db' \
  npx vitest run --project unit:dev dev/test/cli/cli_test.ts
#    -> Tests  27 passed (27)

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.

Every web/api_server/run case in dev/test/cli/cli_test.ts resolves a session
service through cli.ts's `options.session_service_uri || process.env.DATABASE_URL
|| 'memory://'` fallback, but only the session-service-resolution cases pinned
DATABASE_URL. On a machine exporting it the remaining cases either failed
outright (an unrecognised scheme makes getSessionServiceFromUri throw and the
CLI exit 1 - 9 of 26 tests) or silently exercised a DatabaseSessionService
instead of the InMemorySessionService they assume.

Stub DATABASE_URL to undefined in the shared beforeEach so every case owns it,
and add one case that leaves it unstubbed to pin that scrub. The ambient value
is assigned at module scope rather than in a hook: the automatic unstub between
tests restores each variable to the value it held at the first vi.stubEnv, so an
injection made after that point is erased instead of inherited.

Also type the @google/adk importOriginal call, which drops an `as object` cast
and lets the factory wrap the real getSessionServiceFromUri in a spy.
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