Skip to content

Fix: bound and await the Go module bootstrap in the cross-language test helpers - #724

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/cross-language-go-mod-tidy-bounded-async
Open

Fix: bound and await the Go module bootstrap in the cross-language test helpers#724
AmaadMartin wants to merge 3 commits into
mainfrom
fix/cross-language-go-mod-tidy-bounded-async

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 2026

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

None.

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

Problem: AdkGoServer.start() and GoAgent.run() bootstrap their Go modules with a synchronous execSync('go mod tidy') that has no timeout and swallows every failure into a console.warn. go.sum is gitignored, so this branch runs on every fresh clone. A developer without the Go toolchain therefore waits for a Vitest timeout that names neither Go nor the real cause. AdkGoServer.DEFAULT_TIMEOUT was also 30000 against the Node API server's 60000, although go run . compiles the module before it boots.

Solution: A new tests/cross_language/go_modules.ts exports ensureGoModules(moduleDir). It skips the tidy when go.sum exists, awaits execFile under an explicit 45000ms budget, and rejects with the command, the module directory, the concrete cause and the manual remediation. Both helpers now call it. AdkGoServer.DEFAULT_TIMEOUT becomes 60000 to match AdkTsApiServer.

Notes on choices:

  • The helper lives at the cross_language root because Go module bootstrap has nothing to do with A2A, and the two callers live in different a2a/ subtrees.
  • execFile, not exec: no shell is involved, and a missing binary arrives as code === 'ENOENT'.
  • 45000 stays below the 60000 per-hook and per-test budget of both suites, so the helper's own message wins over a generic Vitest timeout.
  • The budget is the module constant, not a parameter or a constructor option: no caller would set it, and an unread knob is dead config.
  • No any, no cast, no suppression: the catch narrows unknown through an isExecFileFailure guard and rethrows with {cause: error}.

Collision check (open PRs on this fork, --limit 1000): no open PR adds this helper or edits the go mod tidy block. #629 owns ts_a2a_go_test.ts, go_a2a_ts_test.ts and vitest.config.ts; #673 owns tests/integration/test_api_server.ts; #685 owns go_backend/go.mod; #648, #427, #393 and #306 own the cross-language workflow. This PR touches none of them. #218, #545 and #546 do edit go_server.ts, but only its successLogMessage line, which this diff leaves alone.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

tests/cross_language/go_modules_test.ts covers the eight paths of the new module with a mocked node:child_process and a real mkdtemp directory, so it needs neither the Go toolchain nor the network.

npx vitest run --project cross-language tests/cross_language/go_modules_test.ts
  Tests  8 passed (8)

Coverage of tests/cross_language/go_modules.ts is 100% of statements, branches, functions and lines (v8):

npx vitest run --project cross-language tests/cross_language/go_modules_test.ts \
  --coverage --coverage.include='tests/cross_language/go_modules.ts' \
  --coverage.exclude='**/node_modules/**' --coverage.reporter=text

 go_modules.ts |     100 |      100 |     100 |     100 |

Proof that the tests can fail. I mutated the source four times and re-ran the suite.

  1. Replaced the throw with the old console.warn plus return. 6 of 8 failed:
× rejects by name when the go executable is missing
  → promise resolved "undefined" instead of rejecting
× keeps the original failure as the error cause
  → expected a rejection, got undefined
Tests  6 failed | 2 passed (8)
  1. Deleted the existsSync early return. 1 of 8 failed:
× does not tidy when go.sum already exists
  → expected "spy" to not be called at all, but actually been called 1 times
Tests  1 failed | 7 passed (8)
  1. Replaced the forwarded timeout with an unrelated constant. 1 of 8 failed:
× tidies the module directory when go.sum is absent
  → expected "spy" to be called with arguments: [ 'go', [ 'mod', 'tidy' ], …(2) ]
Tests  1 failed | 7 passed (8)
  1. Reported a budget of 30000 in the timeout message. 1 of 8 failed:
× names the budget when the tidy times out
  → expected [Function] to throw error including 'it exceeded its 45000ms budget'
Tests  1 failed | 7 passed (8)

Manual End-to-End (E2E) Tests:

Run npm install && npm run build first. Then delete both go.sum files and run each suite with a PATH that has no go on it.

Before this change, go_ts timed out twice at 60000ms and named nothing:

Error: Test timed out in 60000ms.
 ❯ tests/cross_language/a2a/go_ts/go_a2a_ts_test.ts:33:5
Tests  2 failed (2)
Duration  130.29s

After:

Error: go mod tidy failed in <repo>/tests/cross_language/a2a/go_ts/go_client: the
'go' executable was not found on PATH. Install the Go toolchain
(https://go.dev/dl/) or run 'go mod tidy' in that directory manually before
running the cross-language tests.
 ❯ ensureGoModules tests/cross_language/go_modules.ts:77:11
Caused by: Error: spawn go ENOENT
Duration  10.43s

The ts_go suite reported Failed to start go server: spawn go ENOENT before this change, not a hook timeout: its readiness watcher catches the spawn error of the go run . that follows the swallowed tidy. That message still named neither go mod tidy, nor the module directory, nor a remedy. It now reports the same ensureGoModules message, in 13ms of test time.

I also drove the timeout branch against a real go mod tidy (no mock), with the budget temporarily lowered to 5ms: it rejects with it exceeded its 5ms budget.

Cold positive path, with go back on PATH and both go.sum files deleted:

npx vitest run --project cross-language
 ✓ tests/cross_language/go_modules_test.ts (8 tests)
 ✓ tests/cross_language/a2a/ts_go/ts_a2a_go_test.ts (2 tests)
 ✓ tests/cross_language/a2a/go_ts/go_a2a_ts_test.ts (2 tests)
 Test Files  3 passed (3)
      Tests  12 passed (12)

Both go.sum files were rebuilt and both go.mod files stayed byte-identical.

Other gates on the pushed commit: npm run lint, npm run format:check and bash scripts/check_license.sh all pass. npm run ts:check reports the same 2845 lines of pre-existing errors before and after this branch, byte for byte; they come from the built core/dist/types and CI does not run it.

CI on this branch is green on all four run-tests jobs. The windows-latest job first failed on two pre-existing flakes that this diff cannot reach, core/test/code_executors/unsafe_local_code_executor_test.ts and tests/integration/a2a/stream/stream_test.ts; it passed on re-run.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 6, 2026 07:10
…e suites

The two Go test helpers bootstrap their modules with a synchronous, unbounded
execSync that swallows every failure. Add ensureGoModules, which awaits the
tidy under an explicit budget and rejects with the command, the module
directory, the concrete cause and the manual remediation.
AdkGoServer and GoAgent now call ensureGoModules, so a missing Go toolchain
fails in milliseconds with a Go-specific message instead of a hook or test
timeout. Raise AdkGoServer's default readiness budget to 60000 to match the
Node API server: `go run .` compiles the module before it boots, so the tighter
default was backwards.
…mments

No caller sets the timeout, so ensureGoModules now reads GO_MOD_TIDY_TIMEOUT_MS
directly. Inline the single-use ExecFileFailure alias into its guard, and cut
the DEFAULT_TIMEOUT and budget rationales to the reason each number holds.
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