Fix: finish the per-file vitest timeout-budget audit under tests/integration - #554
Open
AmaadMartin wants to merge 5 commits into
Open
Fix: finish the per-file vitest timeout-budget audit under tests/integration#554AmaadMartin wants to merge 5 commits into
AmaadMartin wants to merge 5 commits into
Conversation
This was referenced Aug 4, 2026
Open
Owner
Author
|
Rebase and narrow. Four of the eight files overlap #405, and |
added 5 commits
August 6, 2026 23:36
…ook timeout Four integration test files passed a local timeout argument to their lifecycle hooks that was smaller than the `integration` project budgets in vitest.config.ts (hookTimeout 120000, testTimeout 60000). A local argument shadows the project value rather than raising it, so these hooks silently opted out of the cold-runner headroom the project config provides, and a slow `npm install` was reported as a test-logic timeout. Drop the argument from the install/teardown hooks so they inherit the project hookTimeout, and from webui_test's suite so its tests inherit the project testTimeout. The webui hook budget now also exceeds AdkTsApiServer's own 60s start watchdog, so the server's captured-stdout diagnostic surfaces instead of a bare vitest hook timeout. Every `it()` assertion budget is left exactly as it was: setup cost and assertion cost must not share one number.
The four explanatory comments repeated the rationale already recorded in vitest.config.ts. Tighten each to the non-obvious why -- that the hooks pass no timeout on purpose -- which is the part that stops the arguments being re-added. No behavioural change: 8 fewer lines, same guard.
Five hook sites still passed a local timeout that shadowed the project hookTimeout, including three that this change had just documented as wrong. The three a2a server-start hooks each construct AdkTsApiServer with startFailureTimeout: 60000 and then bound the hook at the same 60000, so vitest's hook timer races the server's own start watchdog and the bare hook timeout can win instead of the server's captured-stdout diagnostic. That is the ordering rule webui_test.ts now carries a comment about, so leaving the three in place contradicted the fix. build_setup_test.ts declared HOOK_TIMEOUT = 120000, byte-identical to INTEGRATION_HOOK_TIMEOUT_MS in vitest.config.ts, under a comment restating that constant's docstring. Inheriting keeps one source of truth; the file's separate 20s TEST_EXECUTION_TIMEOUT is untouched. Also restores the outer suite argument in webui_test.ts. A suite-level timeout is a test budget, not a hook budget, so it never shadowed the hook timeout and removing it only widened those assertions.
Remove the last per-file budgets that restate or undercut the integration project defaults: the file-wide 20s suite budget in webui_test.ts, and the per-test budgets in agent_dirname_test.ts and skills/script_js/agent_test.ts. Rename the a2a TEST_TIMEOUT constants to SERVER_START_TIMEOUT_MS. They now only feed the server's own start watchdog, so the old name described a job they no longer do. Give build_setup's surviving 20s per-test budget the stated reason it needs: the install and the build happen in the hook, not in the tests.
…budget The evidence for 120000 lived in a comment in build_setup_test.ts that this branch deletes. Fold it into the constant's own doc comment so the next person to trim the number can see what it covers.
AmaadMartin
force-pushed
the
fix/integration-hook-timeout-inheritance
branch
from
August 7, 2026 07:02
55e998c to
ce1c829
Compare
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
Link to an existing issue (if applicable):
Or, if no issue exists, describe the change:
Problem:
vitest.config.tssetshookTimeout: 120000andtestTimeout: 60000for theintegrationproject, but a per-file argument overrides that value in both directions. Several integration files predate the config and still pass their own budget, so they now silently lower the limit: annpm installhook capped at 40s, and a server-start hook capped at 20s. Those caps also sit at or below the 60s start watchdog intests/integration/test_api_server.ts, so vitest kills the hook before the server can report its captured stdout.Solution: I deleted every per-file budget under
tests/integration/that restates or undercuts the project default, sovitest.config.tsis the single source of truth. Two overrides survive because they have a stated reason: the 20s per-test budget inbuild_setup_test.ts, and the 40s budget intools/run_skill_script_tool_test.tsthat must exceedUnsafeLocalCodeExecutor's own 30s timeout. I renamed the a2aTEST_TIMEOUTconstants toSERVER_START_TIMEOUT_MS, because they now only feed the server's start watchdog. I also moved the measurement that justifies 120000 into thevitest.config.tsdoc comment, since it used to live in a comment this change deletes.Audit result:
a2a/basic/a2a_agent_test.tsbeforeAll60sa2a/input_required/input_required_test.tsbeforeAll60sa2a/stream/stream_test.tsbeforeAll60sadk_web/webui_test.tsbeforeAll20s, suite 20sagent_loader/agent_dirname_test.tsbuild_setup/build_setup_test.tsskills/script_js/agent_test.tstools/run_skill_script_tool_test.tsDuplicate check: I listed all 652 open PRs on the fork and read every plausibly adjacent diff. #478 and #405 attempt the same audit but are superseded. #405 re-lands the
vitest.config.tsvalues that #548 already merged, and #478 deletes therun_skill_script_tool_test.tsoverride this change deliberately keeps. #506 ownsapp_loader_test.tsand removes the same hook budgets there. This branch already carried those hunks, so I left them as they are.Deviation:
app_loader_test.tskeeps a 40s per-test budget under the 60s default with no concrete reason. That file belongs to #506, so editing it here would only create a conflict. Every other file intests/integration/now satisfies the rule.Note on the diff size: removing the third argument from an
it(name, fn, timeout)call lets Prettier hug the call, which re-indents the test body. That is whyskills/script_js/agent_test.tsshows more changed lines than it has changed behaviour.Testing Plan
Please describe the tests that you ran to verify your changes.
Unit Tests:
Commands run on the pushed commit:
Proof the change is load-bearing. I restored the old 40s hook budget in
agent_loader/agent_dirname_test.tsand re-ran the suite. It fails:With the budget removed, the same suite passes in 228.7s for three fixtures whose tests take 4.7-4.9s each, so the install and teardown hooks account for about 76s per fixture. That is over the old 40s cap and inside the 120s project budget.
skills/script_js/agent_test.tsmeasures the same way: 94.0s for the file against a 6.6s test, so about 87s of hook time under a cap that used to be 60s.Known pre-existing failure:
npx tsc --noEmitreports 280 errors. The identical 280 errors are present onmain, all incore/test/**, and none in a file this change touches. CI does not runts:check, so this is unrelated breakage and I did not fix it.Manual End-to-End (E2E) Tests:
npm run build.npx vitest run --project integration tests/integration/agent_loaderon a cold npm cache. The install hook now has 120s instead of 40s.adk_web/webui_test.tsnow reports the server's ownTimeout waiting for cli to start.and its captured stdout, instead of a bareHook timed out in 20000ms.Checklist