Fix: name the webui_test hook timeouts and give the teardown an explicit budget - #359
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: name the webui_test hook timeouts and give the teardown an explicit budget#359AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
July 30, 2026 20:34
`beforeAll` and the outer `describe` passed bare `20000` literals, and `afterAll` passed no timeout at all, so the teardown budget was whatever the runner happened to supply rather than a value this suite chose. A suite-level timeout (the third argument to `describe`) sets the default *test* timeout and never reaches hooks, which is why the untimed teardown went unnoticed. Declare `SERVER_START_TIMEOUT`, `SERVER_STOP_TIMEOUT` and `TEST_EXECUTION_TIMEOUT` at module scope and wire each to the call site it governs. `SERVER_STOP_TIMEOUT` pins the 10s the untimed hook effectively had, so a hung `http.Server.close()` can no longer inherit a project-wide `hookTimeout` sized for other suites' work. Prettier re-wraps the `describe` call because its third argument is no longer a numeric literal, so its test-call hug no longer applies; `git diff -w` is 31 lines.
Simplicity-audit follow-up. Naming the `describe` timeout was not worth its cost: Prettier only hugs a test callback when the third argument is a numeric literal, so substituting an identifier de-hugged the call and re-indented the whole 60-line suite body — 153 changed lines for a cosmetic gain on a number that already sits on the construct it governs. Restore the literal and drop the now-unused constant. Also trim the two surviving comments to the facts that are local and non-obvious, and state plainly that the `afterAll` budget equals Vitest's current default so nobody reads it as a behaviour change.
Simplicity-audit follow-up. The comment claimed the argument "changes nothing today" because it equals Vitest's default hook timeout. That is true only of this branch's base, which is behind upstream: the project-wide `hookTimeout` for the `integration` project is already merged upstream, so the clause both expires on the next sync and invites a future reader to delete the line as inert. Give the reason that does not expire instead.
This was referenced Jul 31, 2026
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
Closes: #issue_number
Related: #issue_number
Problem:
tests/integration/adk_web/webui_test.tshas two timeout defects in its server-lifecycle hooks.beforeAllends}, 20000);. The literal is 40 lines below the thing it budgets and carries no explanation of what 20 seconds pays for (booting a server — for the CLI-backed case by spawning the ADK CLI as a child process and waiting for it to listen).afterAllpasses no timeout at all, so its budget is whatever the runner supplies rather than a value this suite chose.stop()forAdkApiServerishttp.Server.close()(dev/src/server/adk_api_server.ts:966-985), whose callback fires only once every open connection has drained —close()does not force keep-alive sockets shut. A connection that never closes therefore does not fail fast; it parks the run for the full inherited budget and reports with a generic hook-timeout message.This went unnoticed because the file looks like it has a timeout policy: the outer
describepasses20000as its third argument. That argument is a suite test timeout — Vitest applies it to theit()bodies and it never reaches hooks. Measured, by setting it to1: the failure isTest timed out in 1ms, and the hooks run unaffected.The inherited teardown budget is about to get much worse. #548 ("set project-wide integration hookTimeout/testTimeout in vitest.config.ts") is merged upstream and adds
hookTimeout: 120000to theintegrationproject that owns this file. That value is sized forbeforeAllhooks that runnpm installper fixture (build_setup_test.ts:29,app_loader_test.ts:31,78,agent_dirname_test.ts:28,skills/script_js/agent_test.ts:33); it is a nonsensical ceiling for a socket close.Solution: declare the two hook budgets as named, commented module-level constants and pass both to the hooks they govern.
SERVER_START_TIMEOUTkeepsbeforeAll's existing value — this names it, it does not retune it.SERVER_STOP_TIMEOUTpins the 10s the untimed hook effectively had, so the passing path and the failure path are both unchanged on this base, while the budget becomes a property of what the teardown does instead of a project-wide knob tuned for other suites' work.Nothing else changes: no assertion, no test name, no HTTP behaviour, no dependency, no export, and
vitest.config.tsis untouched.Two deliberate deviations from the task spec, both because a spec claim did not survive verification:
afterAllcurrently inherits 120000ms. It does not — on this branch's base it inherits 10000ms. The base (main@1210acc7) is 23 commits behind upstream and predates Chore: enforce the node: protocol for Node built-in imports in src (no new deps) #548, sovitest.config.tshere sets nohookTimeoutand Vitest's 10s node default applies. Verified by injecting a never-settling await intoafterAllon the unmodified file:Hook timed out in 10000ms.SERVER_STOP_TIMEOUTis therefore 10000, not the 20000 the spec proposed — 20000 would have loosened teardown 2x against this base while the description claimed a 6x tightening. 10000 is the zero-regression value on the base and still a 12x tightening once Chore: enforce the node: protocol for Node built-in imports in src (no new deps) #548 arrives.20000ondescribeis intentionally left as a literal. The spec put naming it in scope on the grounds that it "costs one line and zero risk". It costs neither: Prettier hugs a test callback only when the third argument is a numeric literal, so substituting an identifier de-hugs the call and re-indents the entire 60-line suite body. Measured — that one substitution took the diff from 16 lines to 153 (90 insertions / 63 deletions, of whichgit diff -wshowed only 31 were semantic). A whole-file re-indent that destroysgit blameis not worth naming a number that already sits on the construct it governs, and it is a test timeout, not one of the hook timeouts this change is about.Collision check (required before implementation):
gh pr list --repo AmaadMartin/adk-js --state open --limit 100returned 100 open PRs; none names this file.gh pr diff --name-onlyon every plausibly adjacent PR (#254, #256, #257, #260, #261, #276, #299, #305, #311, #324, #343, #349 — the timeout, vitest-config andtests/integrationPRs) confirms no open PR touchestests/integration/adk_web/webui_test.ts. A PR search for "webui" and agit ls-remotescan forwebui/adk_webbranches on the fork both came back empty. No collision, nothing to stack on.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 case was added, deliberately. The change adds no
core/src,dev/srcorintegrations/srcline, so it contributes nothing to the coverage denominator (vitest.config.tscoverage.include), and a test asserting that a constant equals10000would restate the source and protect nothing. The existing suite is the test, and the verification burden is proving the constants are actually wired to the hooks — a rename that silently failed to bind would look identical in review. No existing test was modified, skipped, weakened, or deleted.Test Files 1 passed (1) | Tests 2 passed (2)— both parameterised cases (Run from ADK CLI,Using ADK API server). Suite time 2610ms against a pre-change baseline of 2615ms, confirming the explicit teardown budget did not clip a legitimately slowstop().Mutation proofs. A timeout change is invisible on the happy path, so each binding was proven on the failure path. Every mutation was reverted and
git diffre-checked clean afterwards.SERVER_STOP_TIMEOUT→1500, never-settling await appended toafterAllError: Hook timed out in 1500ms.afterAll. Before this change the identical hang reported10000msno matter what the file declared.SERVER_START_TIMEOUT→1Error: Hook timed out in 1ms.(tests skipped)beforeAlland is not merely declared.1Error: Test timed out in 1ms.(hooks unaffected)describeargument governs tests, not hooks — the asymmetry that let the untimed teardown hide.hookTimeout: 120000added to theintegrationproject, never-settling await inafterAll, with this changeError: Hook timed out in 10000ms.— wall time 29.15shookTimeout.afterAll)Error: Hook timed out in 120000ms.— wall time 248.78snpm install.Mutations 4 and 5 are the before/after pair for the state of the tree once #548 lands here: 248.78s → 29.15s, and a hook-timeout message reporting a number this file chose.
Manual End-to-End (E2E) Tests:
Full validation gate, run on the exact pushed commit, mirroring
.github/workflows/validation.yaml:npm run ts:checkreports 308 errors in 48 files both with and without this change (verified by stashing the diff and re-running). All are pre-existing onmainand none is inwebui_test.ts; this change adds zero. The full repo suite was not run — this change cannot affect any file other than the one it edits.To reproduce the teardown behaviour manually: append
await new Promise(() => {});to theafterAllbody and run the command above. It now fails in ~10s per case withHook timed out in 10000ms. Revert the injected line afterwards.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.
No suppressions of any kind were added:
git diff main -U0matches no@ts-expect-error,@ts-ignore,eslint-disable,as any,as never, or coverage-ignore pragma. Noconsole.log. One file changed; no lockfile,package.json, orCHANGELOG.mdchurn.