Skip to content

Fix: stabilize app_loader integration test timeouts and stop matrix fail-fast - #235

Closed
AmaadMartin wants to merge 2 commits into
mainfrom
fix/app-loader-test-timeout-and-matrix-fail-fast
Closed

Fix: stabilize app_loader integration test timeouts and stop matrix fail-fast#235
AmaadMartin wants to merge 2 commits into
mainfrom
fix/app-loader-test-timeout-and-matrix-fail-fast

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 29, 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 — no existing tracking issue.

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

Problem: Two independent defects make the validation workflow untrustworthy.

Defect 1 — unrealistic timeout budget. tests/integration/app_loader/app_loader_test.ts
used a single TEST_EXECUTION_TIMEOUT = 40000 for every hook and every test body,
but those two phases do wildly different amounts of work:

  • Each beforeAll runs a real npm install for a fixture project. The fixtures depend on
    file: links to core and dev, so npm still resolves and installs the entire
    transitive dependency graph of @google/adk and @google/adk-devtools.
  • The discovery test body calls loader.listApps(), which bundles and minifies that
    whole graph with esbuild once per discovered entrypoint (the discovery fixture has four).

Vitest runs test files in parallel, so several of these installs contend for CPU, disk and
the npm cache at once. On macos-latest this flaked as
Error: Test timed out in 40000ms at app_loader_test.ts:82 — a pure timing flake, with no
assertion failing, on a file that completed in 71043 ms on windows-latest.

Defect 2 — fail-fast destroys the signal. The run-tests matrix declared no fail-fast
key, so it inherited GitHub's default fail-fast: true and the macOS flake cancelled the
in-flight windows-latest leg
. A maintainer could not tell "macOS-only problem" from
"broken everywhere".

Solution:

Timeouts. Replace the one shared constant with two that match the two phases:
INSTALL_TIMEOUT = 180_000 for the install/cleanup hooks (removing a node_modules tree is
itself slow on Windows) and TEST_EXECUTION_TIMEOUT = 120_000 for the four it(...) bodies.
These are ~3-4x headroom over the slowest observed individual phase, so normal work fits
while a genuine hang is still caught. No assertion was changed, no test was skipped,
reordered, or gated on process.platform
— the diff is timing budgets and install flags
only.

Install flags. Both fixture installs now use a single NPM_INSTALL constant with
--prefer-offline --no-audit --no-fund. The job's own Install dependencies step runs a
repo-level npm install before the test step, and the fixtures depend only on file: links
to core/dev, so ~/.npm is already warm by the time fixture installs run;
--prefer-offline therefore resolves from that cache instead of revalidating against the
registry, and --no-audit --no-fund drop two more network round trips. A cold cache still
falls back to the network, so this stays correct — only slower.

Workflow. Add fail-fast: false so every matrix leg reports its own result, and
timeout-minutes: 60 on the job. The job previously inherited GitHub's 6 hour default,
which was only tolerable because the in-test budgets were small; now that they are larger, an
explicit cap keeps a genuine hang from occupying a runner for hours.

Deliberately not included. An earlier revision also added cache: 'npm' to the
Use Node.js step. I dropped it: the premise does not hold. The repo-level
Install dependencies step already warms ~/.npm within the same job before any fixture
install runs, so the action cache changes nothing for --prefer-offline. It would only speed
the repo-level install across runs, which is a separate concern from this flake fix and
belongs in its own PR.

The esbuild bundle/minify cost in dev/src/utils/agent_loader.ts is the real underlying
expense, but reducing it needs its own design and benchmarks; this PR does not touch
production code. Sibling integration tests (agent_dirname_test.ts,
build_setup_test.ts, skills/script_js/agent_test.ts) have the same latent
npm install-in-beforeAll shape and are intentionally left alone to keep this reviewable.

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:
[ ] I have added or updated unit tests for my change.
[ ] All unit tests pass locally.

No unit test is added, and this is not a workaround. This change adds zero lines of
production code. vitest.config.ts restricts coverage.include to core/src/**,
dev/src/** and integrations/src/**, and no file under those roots is touched, so there is
no new production symbol to test and coverage percentages are unchanged. No coverage
threshold was lowered. The file modified here is the test; it is an integration test, so it
is verified below rather than by a new unit test.

The modified file's own results (npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts --reporter=verbose).
The file contains four it(...) call sites, one of which sits inside a
describe.each over three fixtures, so six tests execute.

Paired before/after on the same machine, back to back, with fixtures removed first:

Result
Unmodified main FAILError: Hook timed out in 40000ms at app_loader_test.ts:77, 6 tests skipped (166.58s)
This PR PASS — 6/6 (254.38s)

The local failure reproduces the CI defect with the hook losing the race rather than the
test body; both shared the same 40s constant, which is exactly why both budgets needed
raising and not just the one that happened to fire first in CI.

Per-test durations with this PR (cold fixtures), all well inside the new 120s budget:

app_ts        5014ms    app_default      5108ms    load App from directory      1ms
app_js        7984ms    discovery       18465ms    synthesize App               1ms

Across three separate runs the discovery listApps() test measured 16468ms, 18465ms and
26008ms on the same idle workstation — a 58% swing on identical work. At 26s it is already
at 65% of the old 40s budget, which is precisely why a slower, contended hosted runner
crossed it.

Install-flag A/B, discovery fixture, node_modules removed before each iteration:

Command Run 1 Run 2 Mean
npm install 85424ms 71740ms 78.6s
npm install --prefer-offline --no-audit --no-fund 58700ms 57041ms 57.9s

A 26% mean reduction, and much more consistent (1.7s spread vs 13.7s). Note a single
fixture install exceeds the old 40s hook budget outright.

Cold vs warm npm cache (whole file): 348.15s cold (cache cleared with npm cache clean --force)
vs 302.72s warm — both pass. The cold run confirms --prefer-offline still succeeds when the
cache lacks entries.

No-regression sweep (npx vitest run --project integration, whole integration project):

Test Files Tests
main 4 failed | 31 passed (35) 17 failed | 68 passed | 15 skipped (100)
This PR 3 failed | 32 passed (35) 0 failed | 68 passed | 32 skipped (100)

Strictly better: same 68 passing, one fewer failing file, and no hard test failures.
app_loader_test.ts passed all 6 tests (278588ms) even under full 35-file parallel
contention. The three still-failing files are pre-existing and out of scope, each failing
in beforeAll with the same npm install root cause and its own separate budget:
build_setup_test.ts (Hook timed out in 10000ms), agent_dirname_test.ts
(40000ms), skills/script_js/agent_test.ts (60000ms). They are listed, not "fixed",
here.

Gates: npm run build, npm run lint and npm run format:check all pass.
npm run ts:check reports 308 errors, but they are byte-identical on main and on this
branch
(verified by diffing both outputs) and none are in the touched file — pre-existing
and untouched by this diff. It is not one of the workflow's steps.

Workflow YAML validated by parsing it, since format:check covers .ts only:

node -e "const y=require('js-yaml'),f=require('fs');const d=y.load(f.readFileSync('.github/workflows/validation.yaml','utf8'));const j=d.jobs['run-tests'];const s=j.strategy;console.log(JSON.stringify({failFast:s['fail-fast'],os:s.matrix.os,timeout:j['timeout-minutes']}))"
=> {"failFast":false,"os":["ubuntu-latest","windows-latest","macos-latest"],"timeout":60}

No other step was reindented; the workflow diff is two added lines.

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

To reproduce the original failure and confirm the fix, from the repository root:

  1. npm install && npm run build
  2. Clear the fixtures so the install cost is real:
    rm -rf tests/integration/app_loader/*/node_modules tests/integration/app_loader/*/package-lock.json
  3. npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts --reporter=verbose
    — expect 6 passing tests.
  4. To see the old behaviour, temporarily set INSTALL_TIMEOUT and TEST_EXECUTION_TIMEOUT
    back to 40000, repeat step 2, and re-run: the run fails with
    timed out in 40000ms without any assertion failing.

Confirmed on this PR's own validation run — all three legs ran to completion and
reported independently:

Leg Result Job duration
run-tests (ubuntu-latest) pass 4m20s
run-tests (macos-latest) pass 7m36s
run-tests (windows-latest) pass 8m57s

The previously-flaking macos-latest leg is green, and
tests/integration/app_loader/app_loader_test.ts completed there with all 6 tests in
55425 ms. That file total on its own exceeds the old 40 s constant, which is the clearest
statement of why the budget — not the code under test — was the defect. Every leg also
finished far inside the new timeout-minutes: 60 cap, so the cap bounds a hang without
threatening a healthy run.

Because fail-fast: false only becomes observable when a leg actually fails, a green run
cannot demonstrate it directly; confirming it empirically requires deliberately breaking one
OS on a scratch branch, which is deliberately not shipped here.

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 2 commits July 29, 2026 02:49
Every hook and test body in this file shared one 40s constant, but the
work they do is nowhere near comparable. A fixture beforeAll runs a real
npm install of the whole @google/adk graph, and the discovery test body
esbuild-bundles and minifies that graph once per entrypoint. On a
contended runner the install alone exceeds 40s, so the file flaked on
macOS with no assertion ever failing.

Split the constant into a 180s hook budget (install, plus node_modules
removal, which is slow on Windows) and a 120s test-body budget, so a
genuine hang is still caught while normal work fits. Also make fixture
installs cache-friendly: the fixtures depend only on file: links to
core/dev, whose transitive deps the repo-level install has already put in
~/.npm, so --prefer-offline resolves from cache instead of revalidating
against the registry, and --no-audit --no-fund drop two more network
round trips.

Measured on one workstation, per fixture install: 78.6s mean before,
57.9s after, and far more consistent (1.7s spread vs 13.7s). No
assertion changed.
The run-tests matrix relied on GitHub's default fail-fast: true, so a
single-OS flake cancelled the in-flight legs and left maintainers unable
to tell "macOS-only problem" from "broken everywhere". Set fail-fast:
false so every leg reports its own result.

Also cap the job at 60 minutes. The job previously inherited GitHub's
6 hour default, which was only tolerable because the in-test budgets were
small; now that they are larger, an explicit cap keeps a genuine hang
from occupying a runner for hours.
This was referenced Jul 29, 2026
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Close. The 120s and 180s budgets are superseded by #506, and #652 owns the fail-fast: false line. Two ideas here are worth keeping: timeout-minutes: 60 on the job, and npm install --prefer-offline --no-audit --no-fund. Please re-file those two as one small PR.

@AmaadMartin AmaadMartin closed this Aug 6, 2026
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