Skip to content

Fix: set project-wide integration hookTimeout/testTimeout in vitest.config.ts - #548

Open
AmaadMartin wants to merge 1 commit into
google:mainfrom
AmaadMartin:fix/integration-project-hook-timeout
Open

Fix: set project-wide integration hookTimeout/testTimeout in vitest.config.ts#548
AmaadMartin wants to merge 1 commit into
google:mainfrom
AmaadMartin:fix/integration-project-hook-timeout

Conversation

@AmaadMartin

Copy link
Copy Markdown
Collaborator

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 existing issue; the change is described below.

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

Problem:

The integration project block in vitest.config.ts set no testTimeout or
hookTimeout, so every hook and test in that project inherited Vitest's defaults
(hookTimeout: 10000 ms, testTimeout: 5000 ms). Many integration tests are
install-heavy — their beforeAll runs npm install (and sometimes
npm run build) inside a fixture project, which routinely exceeds 10s on a
cold/loaded machine and flakes with Error: Hook timed out in 10000ms.
Individual files worked around this by inventing per-file timeout constants with
inconsistent values (20000 / 40000 / 60000), and the most exposed hook —
the beforeAll in tests/integration/build_setup/build_setup_test.ts, which
does both npm install and npm run build — passed no explicit timeout at all
and relied on the flaky 10s default.

Solution:

Add a named constant INTEGRATION_TIMEOUT_MS = 120000 and apply it as both
hookTimeout and testTimeout on the integration Vitest project block only.

  • hookTimeout fixes the concrete npm install / npm run build hook flake.
  • testTimeout gives a matching safe default to the test bodies. Explicit
    per-it() / per-hook timeouts still override it, so nothing regresses.
  • 120000 sits comfortably above every existing per-file value
    (20000 / 40000 / 60000) and above the a2a server timeouts (60000).

The change is deliberately scoped to the integration project rather than the
top-level test block, so the fast unit:* projects keep quick failure
detection. It touches no test files. Per the repo style guideline ("Use named
constants for fixed values instead of hardcoding numbers"), the value is a
documented named constant rather than an inline literal.

Notes for reviewers:

  • Coverage is N/A: this is a test-runner config-only change. Nothing under
    the coverage include globs (core/src, dev/src, integrations/src) is
    touched, so there are no new instrumented production lines to cover.
  • Intentionally decoupled: follow-up cleanup of the now-redundant per-file
    TEST_EXECUTION_TIMEOUT constants is queued separately and must land after
    this change; the targeted build_setup_test.ts flake fix ships separately.
    This PR edits only vitest.config.ts to avoid coupling a shared-config change
    with those and to avoid merge conflicts.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

No unit test is added: the change is a static config literal in
vitest.config.ts, which is not part of the coverage-instrumented source set. A
bespoke test asserting on the raw config object would test Vitest's own config
plumbing, not ADK behavior, and is out of scope. As a regression guard, a
targeted unit run
(vitest run --project unit:core core/test/skills/skill_test.ts) passed with
unchanged fast-timeout behavior, confirming the unit:* projects are
unaffected.

Manual End-to-End (E2E) Tests:

Before/after proof that the new hookTimeout is actually in effect, run with no
mocks:

  1. Create a scratch test under tests/integration/ whose beforeAll sleeps 11s
    (longer than the old 10s default) with no explicit per-hook timeout, then
    a trivial it(...).
  2. npx vitest run --project integration <scratch_test>passes at ~11s
    with this change; fails with Error: Hook timed out in 10000ms. when the
    change is reverted. (Verified both directions locally; the scratch test was
    not committed.)
  3. Real install-heavy suite:
    npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
    — passes (20 passed, 4 skipped). This exercises the exact bare beforeAll
    (npm install plus npm run build per fixture) that was most exposed to the
    10s flake.

Tooling gates on the changed file: prettier --check vitest.config.ts (clean)
and eslint vitest.config.ts (clean) both pass; tsc --noEmit reports zero
errors on vitest.config.ts (the change adds no new type errors).

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.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

This PR is config-only: the entire diff is 11 added lines in vitest.config.ts
(a documented INTEGRATION_TIMEOUT_MS constant plus the two fields on the
integration project block).

Install-heavy `beforeAll` hooks in the `integration` Vitest project run
`npm install` (and sometimes `npm run build`) per fixture, which exceeds
Vitest's 10s default hookTimeout on a slow or loaded machine and flakes with
`Hook timed out in 10000ms`. Only 6 of the 35 integration test files declare
their own timeout constant, so the rest had no protection.

Give the `integration` project block two named budgets:

  INTEGRATION_HOOK_TIMEOUT_MS = 120000
  INTEGRATION_TEST_TIMEOUT_MS = 60000

The values differ because the evidence does. The hook budget covers
`npm install` + `npm run build` across six fixtures; the test budget matches
the largest per-file timeout in the repo, so a genuinely hung test is not
given more rope than anything has ever needed -- which matters because
validation.yaml sets no timeout-minutes and run-tests is a 3-way OS matrix.

Scoped to the `integration` project so unit/e2e/cross-language keep Vitest's
fast defaults. Per-file `it()`/hook timeouts still override both.
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