Skip to content

Fix: correct the integration timeout comments to the budgets in effect - #792

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/integration-timeout-comment-accuracy
Open

Fix: correct the integration timeout comments to the budgets in effect#792
AmaadMartin wants to merge 2 commits into
mainfrom
fix/integration-timeout-comment-accuracy

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 7, 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

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

Problem: Two comments under tests/integration justify their timeout constants with Vitest's stock defaults. The integration project in vitest.config.ts sets hookTimeout to 120000 and testTimeout to 60000, so those defaults never apply to these files. run_skill_script_tool_test.ts claims 40000 clears a 5000ms default, when a per-test argument replaces the project budget and so lowers it from 60000. build_setup_test.ts claims its hooks would otherwise hit a 10s default, and its HOOK_TIMEOUT = 120000 restates the project budget the runner already applies.

Solution: I corrected both comments and deleted the duplicate constant. run_skill_script_tool_test.ts keeps 40000 and all four it() overrides; only the stated reason changes. build_setup_test.ts loses HOOK_TIMEOUT and inherits the identical project budget, and the cold-install measurement moves to vitest.config.ts next to the number it explains. No timeout value in effect changes.

A complexity review found three claims in my first draft that the code did not support, and the second commit fixes them. The build_setup comment said each test spawns the sample app, but only one of the four does. The run_skill_script comment speculated about where the margin above 30s goes; it now states the fact it had omitted, that only the four it.skipIf(!IS_WINDOWS) tests pass the constant. The config comment attributed the shared budget to one suite's ~16s.

Notes for the reviewer:

  • The HOOK_TIMEOUT deletion is behaviour-neutral, and I measured it rather than assuming it. Every entry point that collects these files passes --project integration: test, test:integration, test:coverage and record:samples in package.json, and .github/workflows/validation.yaml runs npm run test:coverage. See the proof below.
  • Out of scope on purpose. The hook arguments in app_loader_test.ts, agent_dirname_test.ts and skills/script_js/agent_test.ts pass 40000/40000/60000, which really does lower the inherited 120000. Removing them would change a value in effect, so I left them alone. build_setup_test.ts is the only file whose hook override is a no-op.
  • The ~70s figure is the repo's own recorded measurement, moved from the comment being deleted. I did not re-measure it. The old comment also carried a ~16s figure for build_setup's twelve hook runs. I dropped that one: the ~70s cold install is what sizes 120000, and app_loader, agent_loader and skills/script_js also install in beforeAll, so pinning a shared budget to one suite misleads.

Overlap with the open timeout pull requests. I checked the open pull requests on this fork before starting. Five touch these lines and none lands this change:

PR What it does Why this is not a duplicate
#405 Deletes build_setup's HOOK_TIMEOUT and rewrites the INTEGRATION_HOOK_TIMEOUT_MS comment Also removes the hook arguments in app_loader, agent_dirname and script_js, which are in effect
#478 (on #405) Deletes run_skill_script_tool_test.ts's 40000 constant and all four overrides This PR takes the opposite decision: the 40000 is deliberate, so it stays
#554 A wider sweep across a2a, adk_web, agent_loader, app_loader, script_js and build_setup Changes hook budgets that are in effect
#545 Deletes HOOK_TIMEOUT inside an app_loader diagnostics change Bundled with unrelated work
#610 Scales both TEST_EXECUTION_TIMEOUT values by a platform multiplier Changes values in effect

This PR is the minimal main-based subset: it corrects the two false comments and removes the single override whose removal changes no effective value. It is deliberately narrower than those sweeps and does not stack on them.

Testing Plan

No test is added. The change adds no executable line, so there is nothing new to cover. The coverage thresholds in vitest.config.ts measure core/src, dev/src and integrations/src, none of which is touched.

The change carries exactly one risk: that deleting HOOK_TIMEOUT leaves the hooks with a different budget. I proved it does not.

Proof that the hooks inherit 120000. I wrote a throwaway file at tests/integration/build_setup/inherited_budget_test.ts whose beforeAll sleeps 11000ms with no local timeout argument, then deleted it. It is not in this diff.

$ npx vitest run --project integration tests/integration/build_setup/inherited_budget_test.ts
 ✓ |integration| tests/integration/build_setup/inherited_budget_test.ts (1 test) 11007ms
 Test Files  1 passed (1)

An 11s hook passes only because the project hookTimeout of 120000 applies.

Proof that the check can fail. I mutated the exact line it pins, hookTimeout: INTEGRATION_HOOK_TIMEOUT_MS to hookTimeout: 10000, and re-ran the same file:

Error: Hook timed out in 10000ms.
 Test Files  1 failed (1)

I then restored the line.

Unit Tests:

  • I have added or updated unit tests for my change. (Not applicable: no executable line changes. See the two proofs above.)
  • All unit tests pass locally.

Manual End-to-End (E2E) Tests:

I ran the touched suite in full, including six real npm install runs, and the suite whose comment changed:

$ npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
 Test Files  1 passed (1)
      Tests  20 passed | 4 skipped (24)
   Duration  538.06s

No "Hook timed out" failure.

$ npx vitest run --project integration tests/integration/tools/run_skill_script_tool_test.ts
 Test Files  1 passed (1)
      Tests  8 passed | 4 skipped (12)

The four skipped cases are the skipIf(!IS_WINDOWS) ones that carry the 40000 budget. They cannot run on a Linux host.

CI covers what my host cannot. The windows-latest leg ran both suites and both passed:

✓ integration tests/integration/tools/run_skill_script_tool_test.ts (12 tests | 2 skipped) 3550ms
✓ integration tests/integration/build_setup/build_setup_test.ts (24 tests | 4 skipped) 160858ms

The four Windows-gated tests on the 40000 budget ran there, and build_setup completed its twelve hooks on the inherited budget with no "Hook timed out". ubuntu-latest and macos-latest also pass.

The first windows-latest attempt failed on two suites this PR does not touch: core/test/code_executors/unsafe_local_code_executor_test.ts (Test timed out in 5000ms, the unit:core project) and tests/integration/adk_web/webui_test.ts (CLI exited prematurely with code 1). Both are known Windows flakes on this fork: run 31206516855 hit the identical executor failure on an unrelated integrations/build.js branch, and main itself fails validation the same way (run 31162222533). Open PRs #498 and #620 target the executor flake. A re-run of the same commit is green.

Repo checks on the pushed commit:

To repeat the behaviour-neutrality check yourself:

  1. Add a file under tests/integration/ whose beforeAll sleeps 11000ms and passes no timeout argument.
  2. Run npx vitest run --project integration <that file>. It passes.
  3. Set the project hookTimeout to 10000 and re-run. It fails with Hook timed out in 10000ms.

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 2 commits August 7, 2026 15:01
…n effect

Two comments under tests/integration justified their constants with Vitest's
stock defaults. The `integration` project sets hookTimeout to 120000 and
testTimeout to 60000, so those defaults never apply to these files.

run_skill_script_tool_test.ts keeps its 40000 per-test budget; the comment now
names the 60000 it replaces and the 30s executor timeout it must clear.

build_setup_test.ts's HOOK_TIMEOUT was byte-identical to the project hook
budget, so passing it selected the value the runner already applies. Deleting
it removes one number kept in two places. Its measurement moves to
vitest.config.ts, where the budget it explains lives.
Three claims in the new comments were not supported by the code.

build_setup_test.ts said each test spawns the sample app, but only one of the
four does; the others run test:db, test:devtools and the devtools CLI.

run_skill_script_tool_test.ts speculated about where the margin above 30s
goes. That sentence is replaced by the fact it omitted: only the four
it.skipIf(!IS_WINDOWS) tests pass the constant.

vitest.config.ts attributed the shared hook budget to build_setup's ~16s. That
figure justifies nothing, since the ~70s cold install sets the budget, and
four other suites also install in beforeAll.
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