Skip to content

Fix: stop Vitest SSR-transforming the compiled agent bundle in app_loader integration tests - #499

Open
AmaadMartin wants to merge 2 commits into
fix/integration-hook-timeout-single-sourcefrom
fix/app-loader-macos-test-timeout
Open

Fix: stop Vitest SSR-transforming the compiled agent bundle in app_loader integration tests#499
AmaadMartin wants to merge 2 commits into
fix/integration-hook-timeout-single-sourcefrom
fix/app-loader-macos-test-timeout

Conversation

@AmaadMartin

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

Stacked PR. Base is fix/integration-hook-timeout-single-source (#405), not main. See "Collision check" below.

Problem: tests/integration/app_loader/app_loader_test.ts intermittently fails with Test timed out in 40000ms on the run-tests (macos-latest) leg only, always on the first test of the AgentLoader discovery and loading integration block. The test is not hanging — it is doing bounded work that costs far more than it should, and all of it is billed to one assertion. Two defects compound:

  1. Vitest inlines the compiled agent bundle. AgentLoader.preloadAgents() esbuilds each of the four discovery fixtures into a self-contained multi-MB bundle under <os.tmpdir()>/adk_agent_loader/<uuid>/ and dynamically import()s it. That path is outside the project root and has no node_modules segment, so Vitest's default externalization heuristics do not match it: the module runner inlines the bundle and pushes all of it through Vite's SSR transform inside the test worker, once per fixture.
  2. The cost lands inside the first it(). preloadAgents() is lazy, and the first thing to trigger it was loader.listApps() inside the failing test. Every later test hits the cache, which is why the two tests after it report ~1 ms.

Measured locally: the first discovery test takes 19,273 ms and Vitest reports a 38.35 s transform total for a file that transforms almost no source. macOS runners are slower than this box for CPU/IO-bound work, which pushes that single test across the 40 s cap — matching the observed signature (only macOS, only this file, only this test, only intermittently).

Solution: two test-infrastructure changes; no core/src, dev/src, or integrations/src file is touched, so runtime behaviour of the shipped packages is identical.

  1. vitest.config.ts — add AGENT_LOADER_BUNDLE_PATTERN (/[\\/]adk_agent_loader[\\/]/) next to the two existing integration budget constants and wire it into the integration project only as server: {deps: {external: [...]}}. A match makes Vitest bypass Vite and hand the module to Node's native loader — which is what happens in production anyway (adk run / adk web import the bundle from a plain Node process), so the test now exercises a more faithful path. The pattern brackets the directory component with [\\/] so it holds on Windows as well as POSIX; os.tmpdir() differs per platform (/tmp, /var/folders/.../T, %LOCALAPPDATA%\Temp) but the adk_agent_loader prefix does not.
  2. app_loader_test.ts — call the already-public loader.preloadAgents() in the shared beforeAll so the one-off discovery cost is attributed to setup. (The "hooks must not pass their own timeout" invariant is documented once, on the constants in vitest.config.ts, rather than restated here; the identical hook-argument removal landed in agent_dirname_test.ts, build_setup_test.ts, and skills/script_js/agent_test.ts with no local comment, and this file should match.)

No timeout value is raised anywhere, and no assertion is weakened, deleted, or rewritten — all six existing cases keep their current semantics and their 40 s per-test budget, which after this change they clear by four orders of magnitude. Raising the timeout was considered and rejected: it would leave 19 s of setup inside an assertion and hide the transform overhead that is the actual defect.

Why the visibility rule is respected: preloadAgents() is already a public method on AgentLoader; nothing was widened and no loader['…'] access was added.

Collision check (run before implementing, gh pr list --repo AmaadMartin/adk-js --state open --limit 1000): this bug has attracted several overlapping PRs, so the scope here is deliberately narrow.

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.

No new production code is introduced and no new line is added to any coverage include glob (the generated temp bundle was never in one), so there is no new line or branch to cover and the test:coverage thresholds are unaffected. No new test case was added deliberately: the correct regression suite here is the six existing cases, and the plan explicitly warns against adding a wall-clock assertion such as expect(elapsed).toBeLessThan(...), which would reintroduce exactly the machine-speed-dependent flake this change removes.

[x] All unit tests pass locally.

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts
  -> 30 passed (30), 703ms   [baseline 676ms - unaffected; these mock esbuild.build]

npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts --reporter=verbose
  -> 6 passed (6), transform 1.15s
     discovery tests: 3ms / 1ms / 1ms

npx vitest run --project integration tests/integration/runner
  -> 1 passed (1), 7.46s      [baseline 7.0s - proves the config change is inert elsewhere]

npm run lint          -> exit 0
npm run format:check  -> exit 0  ("All matched files use Prettier code style!")
npm run build         -> succeeds

Proving the change is load-bearing (mutation testing). Each change was mutated away and re-measured, scoped to the discovery block. This shows neither half is redundant and neither alone is sufficient:

scenario first discovery test Vitest transform total
C — neither change (reproduces the bug) 19,273 ms 38.35 s
B — externalization only, no warm-up 3,234 ms 1.12 s
A — warm-up only, no externalization 4 ms 38.09 s
shipped — both 3 ms 1.15 s

Which change is load-bearing for what — stated plainly, because the two are not equal:

  • The externalization is the timeout fix. It alone takes the first discovery test from 19,273 ms to 3,234 ms (row C -> row B) and the transform total from 38.35 s to 1.12 s. At 3.2 s against a 40 s cap that is ~12x headroom on this box, and still ~4-6x after a 2-3x macOS runner penalty. The reported flake is fixed by change 1 on its own.
  • The warm-up is cost attribution, not the fix. Row A shows it does nothing for the transform cost (38.09 s). Its entire effect is moving the residual 3.2 s of cold bundling out of an assertion's budget and into the hook budget, where setup work belongs. The per-test budget exists to bound the assertion; charging shared setup to it is the precise anti-pattern that produced this bug, and without the warm-up the structure stays wrong even though the number is now green — add a fifth fixture or grow the bundle and the cost creeps back into the first it().

It is one line plus one comment line and a deliberate part of the approved plan, so I kept it — but it should be judged as structural hygiene, not as part of the timeout fix. It is genuinely cheap to drop if a reviewer disagrees: preloadAgents() is idempotent and already called internally by listApps() / listAgents() / getAppFile(), so removing it changes scheduling only, not behaviour or any assertion.

Note on tsc. Root npx tsc --noEmit -p tsconfig.json reports 280 errors, but the output is byte-identical with and without this change (verified by diffing the stripped output against a stashed tree). They are pre-existing core/dist vs core/src duplicate-declaration conflicts that appear after npm run build, in files this PR does not touch. validation.yaml has no standalone tsc step; it gates on build / test:coverage / lint / format / docs, and those pass.

CI status: absent, validated locally instead. This is a stacked PR whose base is fix/integration-hook-timeout-single-source, and validation.yaml triggers on pull_request: branches: [main], so the matrix will not run here. Every command above was run against the exact pushed commit. The macOS leg — the actual end-to-end signal — can only be observed once this lands on a main-based PR.

Manual End-to-End (E2E) Tests:
Not applicable: this is CI/test-harness only, with no user-facing surface. To reproduce the measurements above:

npm install && npm run build
npx vitest run --project integration \
  tests/integration/app_loader/app_loader_test.ts --reporter=verbose

Then delete the server: {deps: {external: [AGENT_LOADER_BUNDLE_PATTERN]}} line from vitest.config.ts and re-run: the transform total jumps back to ~38 s. Additionally remove the await loader.preloadAgents() call and the first discovery test returns to ~19 s.

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 August 1, 2026 22:30
…ation project

AgentLoader esbuilds each agent into a throwaway multi-MB bundle under
<tmpdir>/adk_agent_loader/<uuid>/ and dynamically imports it. That path sits
outside the project root and has no node_modules segment, so Vitest's default
heuristics inline it and push the whole bundle through Vite's SSR transform
inside the worker.

Measured on app_loader_test.ts, the Vitest-reported transform total for the file
drops from 38.1s to 1.1s once the bundle is handed to Node's loader instead.
…ery suite

preloadAgents() is lazy, so the one-off discovery cost (an esbuild bundle and a
dynamic import per fixture) was charged to whichever test touched the loader
first rather than to shared setup. Warming it in beforeAll puts that cost under
the hook budget, where setup work belongs.

This is cost attribution, not the timeout fix: the externalization in the parent
commit is what takes the first discovery test from 19,273ms to 3,234ms, and this
takes it from 3,234ms to 3ms. No timeout value is raised and no assertion
changes.
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