Skip to content

Fix: stop per-file vitest budgets undercutting the integration project defaults - #478

Open
AmaadMartin wants to merge 5 commits into
fix/integration-hook-timeout-single-sourcefrom
fix/integration-test-timeout-budgets
Open

Fix: stop per-file vitest budgets undercutting the integration project defaults#478
AmaadMartin wants to merge 5 commits into
fix/integration-hook-timeout-single-sourcefrom
fix/integration-test-timeout-budgets

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 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):
    N/A — no public issue tracks this flake.
  2. Or, if no issue exists, describe the change:

Problem:
tests/integration/app_loader/app_loader_test.ts > 'AgentLoader discovery and loading integration' > 'should discover apps vs agents across directories and standalone files' intermittently fails on the macos-latest and windows-latest legs of the validation workflow with Error: Test timed out in 40000ms. A plain re-run of the same commit passes, and no source file the test exercises changed — it is a budget problem, not a logic bug.

The mechanism is not "40s is too small". It is that the file declares its own budget and thereby shrinks the one the repo already agreed on:

  • vitest.config.ts sets testTimeout: 60000 and hookTimeout: 120000 for the integration project, precisely because these suites are install- and compile-bound.
  • app_loader_test.ts declared const TEST_EXECUTION_TIMEOUT = 40000 and passed it as the explicit per-call timeout argument to every it() in the file. A per-call timeout argument replaces the project value rather than raising it, so the file lowered the test budget from 60s to 40s on exactly the work the default exists to protect.
  • Inside that shrunken window the test body called loader.listApps(), which reaches AgentLoader.preloadAgents() and esbuild-bundles all four discovered entrypoints (bundle: true, packages: 'bundle', the whole @google/adk graph, per dev/src/utils/agent_loader.ts). Measured locally on an idle machine, that is 23,898 ms of the 40,000 ms budget spent before the first assertion runs.

Solution: a deletion, not a timeout bump.

  1. Delete TEST_EXECUTION_TIMEOUT and every per-test timeout argument that used it, so the tests inherit the project budget (60s tests / 120s hooks).
  2. Move the bundling cost out of the timed assertion window: call the already-public, memoized loader.preloadAgents() in the discovery beforeAll, where it is charged to the 120s hook budget. AgentFile.load() is memoized too, so the subsequent listApps() / listAgents() in the test body are in-memory lookups returning identical results.
  3. Sweep the rest of tests/integration for the same defect (table below). After this change no file under tests/integration passes a vitest timeout argument below the project floors — verified by grep, see Testing.
  4. State the override rule once, above both constants in vitest.config.ts: a per-hook or per-test argument replaces the budget rather than adding to it, so add one only to raise a specific operation, never to restate or lower one. The previous wording ("matches the largest per-file timeout in the repo") is stale once those constants are gone, and the two blocks had drifted into stating the same vitest semantics as two different rules.

No production source is touched. Every expect(...) is semantically unchanged.

File Removed Was
tests/integration/app_loader/app_loader_test.ts TEST_EXECUTION_TIMEOUT = 40000 + 4 it() args 40s test budget vs 60s default
tests/integration/agent_loader/agent_dirname_test.ts TEST_EXECUTION_TIMEOUT = 40000 + 1 it() arg 40s vs 60s
tests/integration/build_setup/build_setup_test.ts TEST_EXECUTION_TIMEOUT = 20000 + 4 it() args 20s vs 60s (the cited ts_esm Windows flake)
tests/integration/skills/script_js/agent_test.ts TEST_EXECUTION_TIMEOUT = 60000 + 1 it() arg equal to the default; removing it retires the constant
tests/integration/tools/run_skill_script_tool_test.ts TEST_EXECUTION_TIMEOUT = 40000 + 4 it() args 40s vs 60s, on tests that shell out to PowerShell/cmd
tests/integration/adk_web/webui_test.ts beforeAll(..., 20000) and a suite-level describe(..., 20000) 20s hook vs 120s, and a 6x downgrade of the whole suite's test budget — on the serveDebugUI server start this PR exists to de-flake
tests/integration/a2a/basic/a2a_agent_test.ts beforeAll(..., 60000) 60s hook vs 120s, on an A2A server boot
tests/integration/a2a/stream/stream_test.ts beforeAll(..., TEST_TIMEOUT) same; constant kept for startFailureTimeout but renamed SERVER_START_TIMEOUT_MS
tests/integration/a2a/input_required/input_required_test.ts beforeAll(..., TEST_TIMEOUT) same

The one durable claim in run_skill_script_tool_test.ts's comment — the vitest budget must outlast UnsafeLocalCodeExecutor's default 30s timeoutSeconds so the executor's own error surfaces first — is a constraint on INTEGRATION_TEST_TIMEOUT_MS, so it moved into that docblock where anyone editing 60000 will read it. Its stale opening clause ("can exceed vitest's 5000ms default") went with the constant.

Why the preloadAgents() hoist stays even though the budget already rose. Deleting TEST_EXECUTION_TIMEOUT raises this test 40s → 60s on its own, so the hoist is a second, independent change and it is fair to ask whether it earns its four lines. It does: the bundling measures 23,898 ms on an idle local machine, and the runners where this flakes are slow enough that the suite's vitest collect phase alone was measured in the hundreds of seconds. A 2.5x slowdown puts bundling alone past 60s. Bundling is fixture setup, so it belongs in the hook that has the 120s budget, not in a test body whose every assertion is a toHaveLength on an in-memory array. The corollary is that the 60s test budget is not sized for esbuild bundling, so that clause was removed from the INTEGRATION_TEST_TIMEOUT_MS description rather than left to contradict this hunk.

Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 (378 open PRs) surfaced several adjacent ones:

Audited and deliberately left unchanged:

  • tests/integration/test_api_server.ts (DEFAULT_TIMEOUT = 60000): a server-readiness poll budget, not a vitest budget.
  • startFailureTimeout: 60000 in the three a2a suites: also not a vitest budget — it is how long the server helper waits before throwing its own error. Left at 60000 so it still fires inside the 120s hook budget, which is the point of removing the hook argument.
  • dev/test/utils/agent_loader_test.ts and tests/cross_language/**: real instances of the same pattern, but in the unit:dev and cross-language projects, which have no project-level budgets at all. Fixing those means choosing new numbers for two other projects; out of scope here.

Out of scope (intentional): removing the per-fixture npm install in favour of the workspace-root node_modules. That is the cleaner long-term fix and is already in flight (#276, #299); duplicating it here would guarantee a conflict.

Accepted trade-offs:

  1. A genuinely hung test in the touched files now takes 60s to surface instead of 20s or 40s (and a hung hook 120s). This is the same trade-off already accepted for the hook budget.
  2. With the warm-up in place, a hard (non-AgentFileLoadingError) throw during discovery now reports as a beforeAll failure rather than a test failure. Discovery results are still asserted in the test body, so a wrong-set-of-apps regression still fails the test — proven by mutation 3 below.

Formatting note: removing the third argument makes each it(...) call fit Prettier's canonical single-line-callback form, so the diff re-indents the test bodies. git diff -w reduces the whole change to 42 insertions / 93 deletions across 10 files. The only assertion line whose text changes is one expect(response.toString()).toContain('Devtools verification successful') that Prettier un-wrapped because it now fits in 80 columns; the call and its argument are identical.

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.

This change adds zero lines of production code, so there is no new code to cover and no unit test to add. Coverage cannot move: git diff --name-only against the stacked base lists only files under tests/integration/ plus the two docblocks in vitest.config.ts; no */src/** line and no unit test is added or removed, and the coverage.thresholds block in vitest.config.ts (statements: 86, branches: 87, functions: 88, lines: 86) is byte-identical. The burden of proof is therefore on showing that the budget actually changed and the cost actually moved.

Unit Tests:
[x] I have added or updated unit tests for my change. — N/A by construction: no production code is added. Explained above rather than silently omitted.
[x] All unit tests pass locally. — no unit test file is in the diff.

Commands run locally on the pushed commit:

npm install && npm run build
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts
npx vitest run --project integration tests/integration/agent_loader/agent_dirname_test.ts \
  tests/integration/skills/script_js/agent_test.ts \
  tests/integration/tools/run_skill_script_tool_test.ts
npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
npx vitest run --project integration tests/integration/a2a/basic/a2a_agent_test.ts \
  tests/integration/a2a/stream/stream_test.ts \
  tests/integration/a2a/input_required/input_required_test.ts \
  tests/integration/adk_web/webui_test.ts \
  tests/integration/tools/run_skill_script_tool_test.ts
npm run lint
npm run format:check
npm run ts:check

Results:

  • app_loader_test.ts: 6 passed (322.06s wall, dominated by the per-fixture npm install in the hooks).
  • agent_dirname_test.ts + skills/script_js/agent_test.ts + tools/run_skill_script_tool_test.ts: 12 passed, 4 skipped (253.24s). The 4 skips are the it.skipIf(!IS_WINDOWS) PowerShell/cmd cases, skipped on Linux exactly as before.
  • build_setup_test.ts: 20 passed, 4 skipped (535.11s). Slowest test 5,808 ms (ts_esm > should build and run agent successfully) — comfortably inside the old 20s budget on an idle machine, which is why this one only flakes on a loaded Windows runner.
  • the three a2a suites + adk_web/webui_test.ts + tools/run_skill_script_tool_test.ts: 16 passed, 4 skipped (14.01s).
  • Sweep verified by grep: no }, <number>) / }, <CONST>) timeout argument remains anywhere under tests/integration (startFailureTimeout: and toHaveLength(n) excluded, neither being a vitest budget).
  • npm run lint: clean. npm run format:check: "All matched files use Prettier code style!".
  • npm run ts:check: 280 pre-existing errors, all in files this PR does not touch (core/test/**, two unrelated tests/integration/** files). Identical count before and after the change (git stash A/B), so this PR neither adds nor fixes one. None of the ten touched files appears in the error list.

Mutation 1 — prove the budget moved. Temporarily set INTEGRATION_TEST_TIMEOUT_MS = 1 in vitest.config.ts and run the discovery test:

  • Before this change (stacked base, file still declares TEST_EXECUTION_TIMEOUT = 40000): the test passes in 20,109 ms. A 1 ms project budget has no effect whatsoever — the per-file constant wins outright.
  • After this change: AssertionErrorError: Test timed out in 1ms.

That is the proof the tests now inherit the project budget instead of overriding it. Reverted afterwards.

Mutation 2 — prove the cost moved. Remove await loader.preloadAgents(); from the beforeAll and compare vitest's reported duration for 'should discover apps vs agents across directories and standalone files':

  • Without the warm-up: 23,898 ms (four esbuild bundles inside the test body).
  • With the warm-up: 9 ms.

On an idle local machine the test body was already consuming ~60% of the old 40s budget; that 23.9s delta is the flake margin this change buys. Reverted afterwards.

Mutation 3 — prove the assertions still bite. With the warm-up in place, temporarily rename tests/integration/app_loader/discovery/standalone_app.ts to standalone_app.txt and re-run. The test fails on the assertion, not on a timeout:

AssertionError: expected [ 'service_alpha' ] to have a length of 2 but got 1
 ❯ ... > should discover apps vs agents across directories and standalone files 10ms

So the warm-up did not turn the test into a no-op — a genuinely wrong discovery result still fails the test body. Reverted afterwards.

Manual End-to-End (E2E) Tests:

npm install && npm run build
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts

Expect 6 passing tests and a reported duration for the discovery test in the low milliseconds (the bundling now happens in beforeAll). To see the old behaviour, delete the await loader.preloadAgents(); line and re-run — the same test's duration jumps to ~24s.

CI status: absent, validated locally instead. .github/workflows/validation.yaml is gated on pull_request: branches: [main], and this PR is stacked on fix/integration-hook-timeout-single-source, so the run-tests job (matrix ubuntu-latest, windows-latest, macos-latest) does not trigger. The commands and results above were run locally on the exact pushed commit. The workflow will run once #405 merges and this PR retargets main.

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 5 commits August 1, 2026 15:30
…budget

The file declared its own TEST_EXECUTION_TIMEOUT = 40000 and passed it to
every it(), which replaces the 60s 'integration' project budget rather than
raising it. The discovery test then paid four esbuild bundles inside that
shrunken window, which is what timed out on loaded macOS/Windows runners.

Drop the constant so the tests inherit the project budget, and warm the
loader in beforeAll so the bundling is charged to the 120s hook budget
instead. Assertions are unchanged.
…egration

Each of these files passed a flat constant below the 60s 'integration'
project testTimeout to install-, compile- or subprocess-bound work:
agent_dirname 40s, build_setup 20s, script_js 60s (a restatement) and
run_skill_script 40s. A per-test timeout argument replaces the project
budget rather than raising it, so these were silent downgrades.

The run_skill_script note about needing to outlast UnsafeLocalCodeExecutor's
30s default is kept and re-anchored to the project budget, which satisfies
it.
…the budget

The previous wording ('matches the largest per-file timeout in the repo')
is stale now that those per-file constants are gone, and it did not say
that a per-test argument replaces this value rather than adding to it.
Five sites still passed a timeout argument below the integration project
floors: webui_test's beforeAll (20s) and its suite-level describe (20s),
and the server-start beforeAll in the three a2a suites (60s vs the 120s
hook default). All boot an HTTP/A2A server, which is exactly the work the
120s hook budget exists for.

The two a2a TEST_TIMEOUT constants stay because startFailureTimeout still
reads them, but are renamed SERVER_START_TIMEOUT_MS so the name no longer
implies a vitest budget.
…dgets

The rule was stated twice in different terms - the hook block said a hook
must not pass its own argument, the test block said a per-test argument may
raise. Same vitest semantics, so state it once above both constants.

Fold in the one durable claim from the comment that outlived
run_skill_script_tool_test's constant: the test budget must stay above
UnsafeLocalCodeExecutor's default 30s timeoutSeconds so the executor's own
error surfaces first. It constrains INTEGRATION_TEST_TIMEOUT_MS, so it
belongs where someone editing 60000 will read it.

Also drop 'esbuild-bundling' from the test-budget description: bundling is
hoisted into the discovery beforeAll and is covered by the hook budget, so
claiming the 60s test budget is sized for it was inaccurate.
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