Skip to content

Fix: stop per-file timeout arguments from lowering the integration project's budgets - #860

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/integration-hook-timeout-overrides
Open

Fix: stop per-file timeout arguments from lowering the integration project's budgets#860
AmaadMartin wants to merge 4 commits into
mainfrom
fix/integration-hook-timeout-overrides

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: A per-call Vitest timeout argument replaces the project value in both directions. It never raises it, so a file that passes 40000 runs under 40 s even though the integration project grants hookTimeout: 120000. agent_dirname_test.ts ran its npm install hook on 40 s three times over, once per describe.each fixture. The 40 s and 60 s numbers were not tied to any measurement.

Solution: I removed every timeout argument that shadows a project budget, so hooks run on hookTimeout: 120000 and bodies on testTimeout: 60000. No file under tests/integration/ passes a hook argument now. I kept the two per-test caps that a comment already justifies. No budget is reduced anywhere.

Audit of every per-call timeout argument under tests/integration/:

File Site Value Decision
agent_loader/agent_dirname_test.ts beforeAll, it, afterAll 40000 dropped
app_loader/app_loader_test.ts 2 beforeAll, 4 it, 2 afterAll 40000 dropped
skills/script_js/agent_test.ts beforeAll, it 60000 dropped
adk_web/webui_test.ts beforeAll, outer describe 20000 dropped
a2a/basic/a2a_agent_test.ts beforeAll 60000 dropped
a2a/stream/stream_test.ts beforeAll 60000 dropped
a2a/input_required/input_required_test.ts beforeAll 60000 dropped
build_setup/build_setup_test.ts beforeAll, afterAll 120000 dropped
build_setup/build_setup_test.ts 4 it 20000 kept
tools/run_skill_script_tool_test.ts 4 it 40000 kept

Why the two keeps are not misses. The build_setup bodies are a deliberate 20 s cap, and its comment tells readers not to raise it for hook flakes. The run_skill_script_tool_test bodies are capped at 40 s to stay above UnsafeLocalCodeExecutor's 30 s timeoutSeconds, so the executor's own error surfaces first. Both record their measurement, which is the standard vitest.config.ts now states.

build_setup's HOOK_TIMEOUT only restated the project value, so I deleted it. Its comment held the measurement behind the 120 s budget, so that text moves to INTEGRATION_HOOK_TIMEOUT_MS, together with the invariant the dropped hook arguments now rely on: the budget must stay above the 60 s server start watchdog in test_api_server.ts, so a server that boots but never announces itself fails with the watchdog's message.

I renamed TEST_TIMEOUT to SERVER_START_TIMEOUT in the two a2a suites. It is a rename only. The constant's one remaining reader is startFailureTimeout, so the old name described a budget it no longer sets.

vitest.config.ts changes two doc comments and no value. The old test-budget wording, "matches the largest per-file timeout in the repo", stops being true once the 60000 per-file constants go.

Prettier collapses each it(name, body, timeout) call back to it(name, body) once the third argument goes, so most of the diff is re-indentation. Read it with git diff -w: no assertion, fixture, spawn command, cleanup step or test name changed.

Collision check. I listed all 200 open PRs on the fork and matched them against every file here. Two siblings overlap. #791 drops the beforeAll arguments in the three a2a suites and in webui_test.ts, and #792 rewrites the INTEGRATION_HOOK_TIMEOUT_MS comment and deletes HOOK_TIMEOUT. I first shipped only the rows they do not cover, but a review found that subset incoherent: it left five hook arguments shadowing the project budget, one of them in a file this PR already edits. This PR now sweeps all of them, so it supersedes both siblings on those lines. Two differences from #791 are deliberate: it deletes startFailureTimeout from the a2a suites, which removes the watchdog that makes a failed start report itself, and it leaves webui_test.ts's suite-level 20000. #662 and #766 also touch two of these files, but only their afterAll bodies.

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 new test file. This change adds zero executable statements; it only removes arguments. The resolved timeout is Vitest runner state, so a test asserting "the hook budget is 120000" would assert the config file back to itself. The existing integration tests are the subject of the change and I ran them.

npx vitest run --project integration \
  tests/integration/agent_loader/agent_dirname_test.ts \
  tests/integration/app_loader/app_loader_test.ts \
  tests/integration/skills/script_js/agent_test.ts \
  tests/integration/adk_web/webui_test.ts
=> Test Files 4 passed (4) / Tests 12 passed (12)

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/build_setup/build_setup_test.ts
=> Test Files 5 passed (5) / Tests 28 passed | 4 skipped (32)

tests/integration/tools/run_skill_script_tool_test.ts keeps its budget and is not in the diff; it passes too (28 passed | 8 skipped together with build_setup).

npm run build, npm run lint and npm run format:check all pass on the pushed commit.

CI note. On the first push run-tests passed on all three runners, after the windows-latest job failed twice and passed on the third attempt. Both failures were a pre-existing flake this PR does not touch: a test with no explicit budget hits Vitest's 5000 ms default while it waits on a child process. The two attempts failed on two different tests, tests/e2e/tools/mcp/load_mcp_resource_e2e_test.ts (project e2e) and core/test/code_executors/unsafe_local_code_executor_test.ts (project unit:core). Neither project is touched here; this PR changes only the integration project. The same unsafe_local_code_executor timeout failed on the unrelated branch fix/vertex-session-test-single-genai-apiclient.

Proof the change is load-bearing. I appended await new Promise((r) => setTimeout(r, 45_000)); to the beforeAll body of agent_dirname_test.ts and ran the file twice.

With the 40000 argument restored, all three fixtures fail:

FAIL tests/integration/agent_loader/agent_dirname_test.ts > Agent with __dirname
FAIL tests/integration/agent_loader/agent_dirname_test.ts > Agent with __filename
FAIL tests/integration/agent_loader/agent_dirname_test.ts > Agent with import_meta_url
Error: Hook timed out in 40000ms.
 Test Files  1 failed (1)
      Tests  3 skipped (3)

With the argument removed, the same sleep passes on the project's 120 s budget: Test Files 1 passed (1) / Tests 3 passed (3). I reverted the sleep before committing; it is not part of the diff. The other dropped hook arguments are the same mechanism at a different number.

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

npm install
npm run build
npx vitest run --project integration tests/integration/agent_loader/agent_dirname_test.ts

To see the old failure, add const T = 40000; and pass it as the third argument of that file's beforeAll, then append a 45 s sleep to the hook body.

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 4 commits August 9, 2026 04:25
…k budget

A per-call Vitest timeout argument replaces the project value in both
directions, so these files ran their `npm install` hooks on 40s or 60s
while the `integration` project grants 120s. Nothing recorded a
measurement behind those numbers.

Prettier collapses the `it()` calls once the third argument goes; the
bodies are unchanged. Read this diff with `git diff -w`.
A `describe(name, body, timeout)` argument applies to every test in the
suite and replaces the project's 60s `testTimeout`. No measurement
justifies capping this suite lower.
"Matches the largest per-file timeout in the repo" stops being true once
the per-file 60000 constants are deleted. Record the bound the budget
actually provides instead.
No file under tests/integration passes a hook timeout argument now, so
every hook runs on the project's 120s hookTimeout. build_setup's
HOOK_TIMEOUT only duplicated that value; its measurement and the server
start watchdog invariant move to the constant they describe.

Renames TEST_TIMEOUT to SERVER_START_TIMEOUT in the two a2a suites: the
constant's only remaining reader is startFailureTimeout.
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