Refactor: hoist repeated Vertex AI placeholders to module-level constants in core tests - #565
Open
AmaadMartin wants to merge 3 commits into
Open
Refactor: hoist repeated Vertex AI placeholders to module-level constants in core tests#565AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
August 2, 2026 22:32
core/test/sessions/vertex_ai_session_service_test.ts repeated the bare app
name '12345' on 66 lines. Replace every occurrence with one module-level
APP_NAME constant, and derive the expected resource names from it
(`reasoningEngines/${APP_NAME}`) so the appName -> reasoningEngines/<id>
transformation the service performs is visible in the test source instead of
implied by two matching magic numbers.
The inline '// Must be digits or resource name' comment moves onto the
constant, where the reason lives with the value.
No assertion changes meaning: every substituted site evaluates to a
byte-identical string. The custom-engine-id, invalid-app-name and
my-project/999 literals are deliberately left alone -- each proves a specific
override or parsing behaviour and must stay visibly distinct from APP_NAME.
core/test/code_executors/agent_engine_sandbox_code_executor_test.ts repeated an 88-character sandbox resource name 9 times and its 64-character agent engine prefix 3 times. Replace them with TEST_PROJECT, TEST_LOCATION, AGENT_ENGINE_NAME and SANDBOX_NAME at module scope. AGENT_ENGINE_NAME is composed from TEST_PROJECT/TEST_LOCATION rather than written flat because several of these tests only mean anything if the project and location stubbed into the environment agree with the ones embedded in the resource name -- the executor parses project and location out of the resource name and falls back to the env vars. Composing makes that required agreement explicit. The 'us-central1' on the 'defaults location to us-central1 if missing in env' assertion stays a literal: it pins the library's hard-coded default, which merely happens to equal the test placeholder. The custom-p/custom-l, custom-location and appName: '123' literals stay literal for the same reason -- each proves a specific override or parsing behaviour.
…ently pinned Review feedback: interpolating APP_NAME into the *expected* value of the toHaveBeenCalledWith assertions made those expectations derive from the same symbol fed in as input, so they no longer pinned the appName -> reasoningEngines/<id> mapping on their own. Mutating APP_NAME left the whole suite green, which is exactly the lost signal. Restore the hardcoded expected strings at all 10 assertion sites (byte-for-byte as before this branch, so those lines leave the diff entirely). Mutating APP_NAME now fails 9 tests instead of 0. APP_NAME still covers the 47 inputs, the 3 pass-through echo assertions and the 6 mock fixtures, where naming the placeholder is the whole point and nothing is being pinned. Its doc comment now states that split so the remaining literals read as deliberate rather than as missed occurrences. Also note at the location-default assertion why it must not reuse TEST_LOCATION: it pins the library default, which only happens to share the value, and reusing the constant would make the test vacuous.
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
Closes: #issue_number
Related: #issue_number
Problem: Two
core/testsuites repeat the same Vertex AI Agent Engineplaceholder literals dozens of times.
core/test/sessions/vertex_ai_session_service_test.tsrepeats the bare appname
'12345'on 66 lines — 47 asappName: '12345', 3 inexpectassertions, and 16 embedded in
'reasoningEngines/12345/...'expectedresource names. The relationship the service actually implements
(
appName->reasoningEngines/<appName>) is invisible in the source: itreads as two magic numbers that happen to match.
core/test/code_executors/agent_engine_sandbox_code_executor_test.tsrepeatsan 88-character sandbox resource name 9 times and its 64-character agent
engine prefix 3 times, plus
'test-project'x4 and'us-central1'x2.The same coupling is invisible here: the executor parses project and location
out of the resource name and falls back to the stubbed env vars, so those
tests only mean anything if the two agree — and nothing in the source says so.
Solution: A test-only refactor. Each file gets its own module-level
constants and every repeated literal is replaced by a reference to them. No
production file is touched, no test is added, removed, renamed, reordered or
skipped, and no assertion changes meaning — every substituted site evaluates to
a byte-identical string.
core/test/sessions/vertex_ai_session_service_test.ts:APP_NAMEcovers the 47 inputs, the 3 pass-through echo assertions(
expect(session.appName).toBe(APP_NAME)) and the 6 mock fixtures. It isdeliberately not used in the expected value of the 10
toHaveBeenCalledWithassertions: those pin the service'sappName->reasoningEngines/<id>mapping, and deriving the expectation fromthe same symbol fed in as the input would stop them pinning it independently.
Those 10 keep their hardcoded strings, unchanged from before this branch. The
constant's doc comment states that split so the remaining literals read as
deliberate rather than as missed occurrences. The inline
// Must be digits or resource namecomment moves onto the constant, where thereason lives with the value.
An earlier revision of this PR did interpolate all 16 sites; a reviewer caught
that it silently removed the regression signal, and the mutation results below
confirm it.
core/test/code_executors/agent_engine_sandbox_code_executor_test.ts:AGENT_ENGINE_NAMEis composed rather than written flat so the requiredagreement between the stubbed env vars and the resource name is explicit, and
SANDBOX_NAMEis composed from it rather than repeating the prefix.Why the constants are file-local and not a shared fixture module.
vitest.config.tsdefinesunit:core,unit:dev,unit:integrations,integration,e2eandcross-languageas separate projects, and this repohas no precedent for a test importing a fixture across that boundary. The one
shared-fixture module that does exist,
core/test/artifacts/artifact_service_test_utils.ts, is imported only by itsthree directory-local siblings. So each file declares its own constants, neither
is exported, and nothing is imported from
tests/intocore/test/. This alsomatches
adk-python, which declares these placeholders file-locally(
MOCK_APP_NAME/MOCK_USER_IDintests/unittests/memory/test_vertex_ai_memory_bank_service.py), and theexisting in-repo precedent in
core/test/runner/runner_test.ts.Literals deliberately left alone. Each of these is distinct by design —
substituting a constant into any of them would turn a real assertion into a
tautology and silently destroy the regression signal:
expect(executor['location']).toBe('us-central1')indefaults location to us-central1 if missing in envTEST_LOCATIONhere makes the test assert itself.'custom-engine-id'/'reasoningEngines/custom-engine-id'agentEngineIdoverrides theappNamepassed tocreateSession. The two values must visibly disagree.'invalid-app-name''projects/my-project/locations/us-central1/reasoningEngines/999','reasoningEngines/999''projects/custom-p/locations/custom-l/...'(x2)TEST_PROJECT/TEST_LOCATION.'custom-location'locationoption beats the env var.vi.stubEnv('GOOGLE_CLOUD_PROJECT', '')appName: '123'on theInvocationContextsession fixture (x2)agent_engine_sandbox_code_executor.tsnever readssession.appName. The resemblance is coincidental.Single-occurrence literals (
projectId: 'test-project'/location: 'us-central1'in the session service's "can initialize without passing a client explicitly"
case) are also left alone — a constant for a single use is noise.
Collision check.
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000was checked before any code was written. No open PR performs this refactor: a
gh pr diffgrep across every open PR forconst APP_NAME/const TEST_PROJECT/
const AGENT_ENGINE_NAME/const SANDBOX_NAMEreturned nothing. #439("extract shared Vertex AI placeholder constants for the tests tree") is the
closest neighbour and is disjoint — it touches
tests/integration/**only, which is a different vitest project with no importrelationship to
core/test/**, so the two can land in either order. Six PRs(#512, #503, #287, #270, #201, #331) touch
core/test/sessions/vertex_ai_session_service_test.tsincidentally by addingtests to it, but none does this refactor; stacking on six independent branches
is not possible, so this branches from
mainand will need a trivial textualrebase against whichever of them lands first.
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.
No test was added: this is a pure refactor, and adding a test to it would make
the value-preservation claim harder to verify, not easier. The existing suites
are the test.
Baseline recorded on the unmodified files, then re-run after the change —
identical, with zero failures and zero skips:
Refactor-correctness proof. A literal-to-constant refactor has one failure
mode — a missed occurrence, leaving a stale hard-coded literal that no longer
tracks the constant. Proved in both directions:
The mapping is still independently pinned (File 1). Temporarily set
const APP_NAME = '99999'(still digits, so it still satisfies theservice's app-name validation) and re-ran the suite: 9 tests failed
(
Tests 9 failed | 48 passed (57)), each reportingexpected "reasoningEngines/12345", received "reasoningEngines/99999".That is the intended signal — the hardcoded expectations catch the input
changing out from under them. In the earlier revision that interpolated the
expectations, this same mutation left the suite fully green (57/57), which
is what the reviewer flagged. Reverted.
Substitution is total (File 2). Temporarily set
const TEST_PROJECT = 'mutant-project'and re-ran the suite. It passed:Tests 26 passed (26). Reverted.The remaining fixture substitutions are value-preserving (File 1). The
6 mock-fixture sites that do use
`reasoningEngines/${APP_NAME}/...`feed values into the mocks rather than asserting on them. Mutation 1 above
covers them: with
APP_NAME = '99999'none of the 9 failures came from afixture mismatch, and with
APP_NAME = '12345'all 57 pass, so each fixturestill produces the exact string it did before.
The constants are load-bearing (File 2). Changed one expectation from
sandbox_name_language_python: SANDBOX_NAMEtosandbox_name_language_python: `${SANDBOX_NAME}x`. Exactly one testfailed:
The
+line proves the composedSANDBOX_NAMEevaluates to exactly the88-character literal it replaced. Reverted.
Quality gate:
npm run ts:checkis already red on the base commit with 281 errors across therepo. I ran it on the base and on this branch and diffed the normalized
per-file error counts: identical, 281 before and 281 after. This PR adds
zero type errors and fixes none; the four base errors whose source snippet now
reads
appName: APP_NAMEare the pre-existingListSessionsRequesterrors thatpreviously read
appName: '12345'.Post-conditions verified:
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Not applicable — there is no user-visible behaviour to exercise, and no
production code is touched. The manual verification is the diff review: read
both files end to end and confirm that (a) every literal in the
"deliberately left alone" table above is intact and still visibly different from
the new constants, and (b) no comment, test name, or mock ordering moved. To
reproduce the automated verification:
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.
CI status
All checks pass on the current head (
6612141e), including windows:run-testsrun-tests (ubuntu-latest)run-tests (macos-latest)run-tests (windows-latest)check-licenseFor the record, earlier commits on this branch saw red
windows-latestand onered
macos-latestrun. Those were pre-existing platform flakes, not thischange: the same
windows-latestjob fails on the untouched base commitb390217e(run30669370416, branchmain, ubuntu and macos green) withtests/integration/adk_web/webui_test.ts > 'Run from ADK CLI': CLI exited prematurely with code 1andTests 2679 passed | 43 skipped— zero assertionfailures, a spawned-server teardown error. Across three runs the windows job
failed three different ways (
adk_web/webui_test.ts, thenunsafe_local_code_executor_test.tstiming out in 5000ms), and the macosfailure (
tests/integration/app_loader/app_loader_test.ts, 40000ms timeout)went green on re-run — it had also cancelled windows that round, since the
matrix does not set
fail-fast: false. None involve the two files changed here,which pass on every platform (57 and 26 tests). The repo-wide total is 2722
tests on both the base commit and this branch, confirming no test was added,
removed or skipped.
Review round 1 — changes made
A complexity reviewer raised three points. Two are addressed in commit
6612141e; the third is answered below.APP_NAMEintothe expected value of the 10
toHaveBeenCalledWithassertions made themrecompute the
appName->reasoningEngines/<id>mapping from the samesymbol supplied as the input, so they stopped pinning it independently. This
was real and measurable: mutating
APP_NAMEleft the suite 57/57 green.All 10 are restored to their original hardcoded strings — byte-for-byte as
on the base commit, so those lines drop out of the diff entirely — and the
same mutation now fails 9 tests. The 6 mock-fixture sites keep the constant,
as the reviewer noted they are fixtures, not expectations.
suggested inlining
TEST_LOCATIONbecause its value collides with thelibrary default asserted as a bare literal one screen below. I kept the
constant — the executor parses project and location out of the resource name
and falls back to the env vars, so the agreement between the stubbed env and
the composed
AGENT_ENGINE_NAMEis a real coupling worth making explicit —and instead removed the ambiguity at its source with a note on the
assertion explaining why it must not reuse the constant. That guards
against a future contributor "completing" the substitution and silently
making the test vacuous, which inlining would not.
the
APP_NAMEhoist as not paying for itself, largely because thederived-expectation problem made it "a small net loss". With that fixed, the
remaining change is 47 input sites where a bare
'12345'is replaced by aname that says what it is — the value is not in saving characters but in the
fact that a reader currently has to reach one inline comment at one of 66
sites to learn the literal must be digits or a resource name. Dropping it
would also leave the stated task half-done. Flagging it here for the
reviewer rather than silently keeping it.