Skip to content

Fix: require TEST_API_SERVER_PORT in the A2A multi-hop test agent - #314

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/a2a-test-agent-require-test-api-server-port
Open

Fix: require TEST_API_SERVER_PORT in the A2A multi-hop test agent#314
AmaadMartin wants to merge 3 commits into
mainfrom
fix/a2a-test-agent-require-test-api-server-port

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):

No existing issue.

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

Problem: tests/integration/a2a/input_required/test_agents/multi_hop_remote_agent.ts silently fell back to a hardcoded port:

const port = process.env.TEST_API_SERVER_PORT || '40000';

That fallback can never be correct. This agent is a self-loopback multi-hop: it is loaded inside the spawned test ADK API server and its agent card points back at that same server, whose port the harness picks at random — BaseTestServer.getRandomPort() (tests/integration/test_case_utils.ts:275) returns 40000 + Math.floor(Math.random() * 10000), so 40000 is right 1 time in 10000, by coincidence.

The consequence is a misattributed failure. AdkTsApiServer.start() (tests/integration/test_api_server.ts:39-53) is the only writer of TEST_API_SERVER_PORT; if it ever stops propagating it, the agent still constructs fine and the breakage surfaces layers away, either as a connection error against localhost:40000 or — worse — as a successful connection to an unrelated process listening there. I simulated exactly that regression on main (see the E2E section below): the suite fails with AssertionError: expected [] to include 'call-hop', which names neither the variable nor the harness. Non-numeric garbage was accepted too, since the value was only checked for truthiness.

Solution: validate TEST_API_SERVER_PORT at module evaluation time and throw a descriptive Error when it is missing, empty, or not a positive integer. Because the module is evaluated during initA2A() -> AgentLoader.preloadAgents(), the throw fails server startup, the CLI logs Error starting API server: <message>, and the harness reports CLI exited prematurely with code 1 with the message in the captured output.

Design notes:

  • Fail the whole suite, not just the multi-hop test. The throw happens at server startup, so all three tests in input_required_test.ts fail rather than one. That is intended: if the harness is not propagating the port, the harness is broken and the suite's result is meaningless.
  • Number(), not parseInt(). parseInt('41234abc', 10) returns 41234 — exactly the silent coercion this change removes. Number('41234abc') is NaN. There is a test case pinning this.
  • No upper-bound (<= 65535) check. Deliberately omitted, not an oversight: the only producer is getRandomPort() (range [40000, 49999]), and an extra branch here would be scope creep with no real-world input to exercise it.
  • Two distinct messages so a reader can tell "the harness never set it" from "the harness set it to junk". Only error.message is logged by the CLI (never the stack), so the message is self-contained. It names AdkTsApiServer as the remediation point but deliberately does not hardcode that file's path, which would rot on the next file move.
  • Scope. grep -rn TEST_API_SERVER_PORT over the repo returns exactly two hits (the writer and this reader), and grep -rniE 'process\.env\.[A-Z_]*PORT' returns only this reader plus core/src/telemetry/setup.ts, which reads OTLP endpoint URLs and is out of scope. No other test agent reads a port from the environment, so the change is one file plus one new test file. No core/src, dev/src or integrations/src file is touched; there is no public API, export, or TypeDoc impact, and no dependency change.
  • Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 300 (222 open PRs) plus a keyword search for TEST_API_SERVER_PORT / multi_hop_remote_agent / input_required. Nothing lands or overlaps this change. The closest neighbours were checked by diff: Fix: use REQUEST_CONFIRMATION_FUNCTION_CALL_NAME instead of the hardcoded wire literal in tests #269 touches the sibling test_agents/tool_confirmation.ts and input_required_test.ts (a wire-literal constant swap — different files, no conflict), Fix: stabilise install-bound and server-spawning integration suites #218 mentions TEST_API_SERVER_PORT in its body but its diff does not touch it, and Fix: publish the bound port in A2A agent card URLs #300 concerns ephemeral-port agent card URLs in dev/src/server/adk_api_server.ts, not the test agent. Branched from main; no stacking needed.

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.

New file tests/integration/a2a/input_required/multi_hop_remote_agent_test.ts (9 cases). It sits one directory above test_agents/ on purpose: input_required_test.ts:20 passes test_agents/ as the server's agentsDir and AgentLoader.preloadAgents() esbuild-bundles every file it finds there, so a test file dropped inside would be bundled with vitest into the spawned server. It is still matched by the integration project's tests/integration/**/*_test.ts glob.

Each case stubs the environment with vi.stubEnv and re-imports the module after vi.resetModules() (the port is read once at module evaluation, so without the reset every case after the first silently reuses the first evaluation):

TEST_API_SERVER_PORT Expectation
'41234' resolves; rootAgent.name === 'multi_hop'; agent card is http://localhost:41234/a2a/multi_hop/
unset, '' rejects with TEST_API_SERVER_PORT is not set.
'abc', '41234abc', '0', '-1', '1.5', ' ' rejects with TEST_API_SERVER_PORT must be a positive integer, got "<raw>".

The agent card URL is not publicly readable off a constructed RemoteA2AAgent (a2aConfig is private), so the happy-path case captures the constructor argument via a vi.mock of @google/adk that subclasses the real RemoteA2AAgent. Reaching in with rootAgent['a2aConfig'] would have been a banned private-field escape hatch. No suppressions of any kind were added — no any, @ts-expect-error, eslint-disable, or coverage pragma — and the mock factory is typed via importOriginal<typeof import('@google/adk')>().

Commands run (targeted only):

npx vitest run --project integration tests/integration/a2a/input_required/multi_hop_remote_agent_test.ts
  -> Test Files 1 passed (1) | Tests 9 passed (9)

npx vitest run --project integration tests/integration/a2a/input_required/input_required_test.ts
  -> Test Files 1 passed (1) | Tests 3 passed (3)   [pre-existing suite, unchanged]

npm run build            -> OK
npx eslint <both files>  -> clean
npx prettier --check <both files> -> "All matched files use Prettier code style!"
npx tsc --noEmit         -> zero errors attributable to either file

Note on npx tsc --noEmit: the repo has ~350 pre-existing type errors in core/test/** and elsewhere on main (the subject of other open PRs). Neither of my two files appears in that output.

Proving the tests can fail (mutation runs). Coverage is not proof, and vitest.config.ts:109-113 restricts instrumentation to core/src/dev/src/integrations/src, so files under tests/ produce no coverage delta at all. Both mutations were run explicitly:

Mutation 1 — restore the original const port = process.env.TEST_API_SERVER_PORT || '40000';. All 8 negative cases FAIL; the happy path still passes, correctly, since it never reaches the fallback:

FAIL  multi_hop_remote_agent > throws a "not set" error when the port is undefined
FAIL  multi_hop_remote_agent > throws a "not set" error when the port is ""
FAIL  multi_hop_remote_agent > throws a "positive integer" error when the port is "abc"
FAIL  multi_hop_remote_agent > throws a "positive integer" error when the port is "41234abc"
FAIL  multi_hop_remote_agent > throws a "positive integer" error when the port is "0"
FAIL  multi_hop_remote_agent > throws a "positive integer" error when the port is "-1"
FAIL  multi_hop_remote_agent > throws a "positive integer" error when the port is "1.5"
FAIL  multi_hop_remote_agent > throws a "positive integer" error when the port is "   "
AssertionError: promise resolved "{ …(1), …(1) }" instead of rejecting
Tests  8 failed | 1 passed (9)

Mutation 2 — keep the validation but hardcode the URL (http://localhost:40000/a2a/multi_hop/), which pins the remaining case, i.e. that the validated port is actually the one used:

FAIL  multi_hop_remote_agent > points the agent card at the port the harness supplied
AssertionError: expected 'http://localhost:40000/a2a/multi_hop/' to be 'http://localhost:41234/a2a/multi_hop/'

Both mutations were reverted and the suites re-run green.

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

This reproduces the harness regression the change exists to catch. It runs a real spawned ADK API server — no mocks.

  1. npm install && npm run build (the harness spawns dev/dist/esm/cli_entrypoint.js).
  2. In tests/integration/test_api_server.ts:45, delete the TEST_API_SERVER_PORT: this.port.toString(), line so the child is spawned with plain process.env.
  3. Run npx vitest run --project integration tests/integration/a2a/input_required/input_required_test.ts.
  4. Restore line 45 and re-run to confirm the suite is green again.

Before this change (step 3 against main) — two tests pass and the multi-hop one fails with an assertion that names neither the variable nor the harness:

✓ A2A: RemoteAgent InputRequired > Long-running tool 102ms
✓ A2A: RemoteAgent InputRequired > Tool confirmation 21ms
× A2A: RemoteAgent InputRequired > Remote Agent -> Remote Agent -> ADK Agent 35ms
AssertionError: expected [] to include 'call-hop'
Tests  1 failed | 2 passed (3)

After this change (step 3 on this branch) — startup fails immediately and the output says exactly what is wrong and who is responsible:

CLI exited with code 1
CLI Captured stdout before premature exit:
ERROR: [ADK API Server] Error during AdkApiServer startup: Error: TEST_API_SERVER_PORT is not set. This agent points back at the test ADK API server it runs inside, so it has no default port to fall back to; AdkTsApiServer must propagate TEST_API_SERVER_PORT.
[ADK CLI] Error starting API server: TEST_API_SERVER_PORT is not set. This agent points back at the test ADK API server it runs inside, so it has no default port to fall back to; AdkTsApiServer must propagate TEST_API_SERVER_PORT.

FAIL  tests/integration/a2a/input_required/input_required_test.ts > A2A: RemoteAgent InputRequired
Error: CLI exited prematurely with code 1
Tests  3 skipped (3)

Step 4 (harness restored) returns the suite to Tests 3 passed (3).

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 30, 2026 08:07
The multi_hop_remote_agent test agent is loaded inside the spawned test
ADK API server and points back at that same server, so the port it needs
is the random one the harness picked. The '|| 40000' fallback could
therefore never be correct: it made a dropped TEST_API_SERVER_PORT
surface much later as an opaque connection or agent-card failure that
never named the variable.

Validate the variable at module evaluation time and throw a
self-contained error naming both the variable and AdkTsApiServer, the
harness component responsible for propagating it. Parse with Number()
rather than parseInt() so '41234abc' is rejected instead of silently
coerced to 41234.
…gent

Re-evaluate the agent module per case with vi.resetModules() and a
stubbed environment, asserting the message for the missing and the
malformed branches and pinning the agent card URL to the supplied port.
Drop the hardcoded harness file path from the runtime message (it rots on
the next file move; AdkTsApiServer is greppable) and stop restating the
doc comment in the error string.
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