Test: Resolve integration fixtures from the workspace root instead of installing node_modules per fixture - #299
Open
AmaadMartin wants to merge 2 commits into
Open
Conversation
added 2 commits
July 30, 2026 00:10
…estor getProjectNodeModulesDir only looked for a node_modules sitting next to the nearest package.json, so a project in an npm/pnpm workspace - where deps are hoisted to the workspace root - got no node_modules symlink in the esbuild output directory and every entry on the bundler's external list became unresolvable at runtime. Walk up for the nearest ancestor node_modules instead, mirroring Node's own resolution. tryToFindFileRecursively and the new tryToFindFolderRecursively now share one bounded walk that differs only in the existence predicate.
…f installing them The app_loader, agent_loader and skills/script_js fixtures each ran their own npm install, materialising the same ~600-package dependency graph into eight throwaway node_modules trees per integration run, on three CI runner OSes. Nothing under test needs that: the workspace-root install is already a precondition of the test job, and Node resolves @google/adk and @google/genai by walking up to it. The fixtures now invoke the built dev CLI directly. `npx @google/adk-devtools` resolves the *package* against the fixture's own tree and would silently fetch the published devtools from the registry once the fixture has no node_modules - a green build proving nothing. `adk` on PATH is not an option either: npm skips linking a workspace bin whose target does not exist yet, and CI installs before it builds, so node_modules/.bin/adk never exists there. Each fixture keeps its package.json: getTypeFromPackageJson walks to the nearest one, so removing it would flip the CJS fixtures to ESM via the root manifest and erase the very distinction these suites test. build_setup is deliberately untouched. Its file: installs produce real published-layout copies of core, which is what that suite exists to verify.
This was referenced Jul 30, 2026
7 tasks
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: The
app_loader,agent_loaderandskills/script_jsintegration fixtures each run their ownnpm installin abeforeAllhook. That is 8 installs of the same ~600-package dependency graph per integration run, into 8 throwawaynode_modulestrees, onubuntu-latest/windows-latest/macos-latest, on every push and PR. Nothing under test needs it:npm installat the repository root is already a hard precondition of the test job (.github/workflows/validation.yaml:31-41), and Node resolves@google/adk/@google/genaiby walking up to that root.On a 2-4 vCPU runner those 8 concurrent installs are pure contention. Locally in this sandbox they do not even complete: a single fixture install measured 65.84 s / 606 packages, so all three suites blow their
beforeAlltimeout and are red onmain.Solution: Delete the 8 installs and let the fixtures resolve from the workspace-root
node_modulesthat the root install already populates. Each fixture'sstartscript now invokes the built dev CLI directly.Why
node <path>/cli_entrypoint.jsand notadk runornpx @google/adk-devtools run(please read — this corrects a wrong assumption)The design this PR was written against assumed
npm installpopulatesnode_modules/.bin/adk(fromdev/package.json'sbin), so the fixtures could just sayadk run agent.tsand let npm's PATH walk find it. That assumption is false, and I verified it three ways before changing the approach:npm installon a fresh checkout,node_modules/.bin/adkdoes not exist. It only appears if you runnpm installagain afternpm run build.bin), run under npm 9.2.0, 10.9.9 and 11.18.0 — all three skip the workspace bin link when the bin target file does not exist at install time.npm install(line 32) →npm run build(line 38) → tests (line 41), sodev/dist/esm/cli_entrypoint.jsnever exists during the install.node_modules/.bin/adkis therefore never created in CI.Confirmed empirically in the fixture with the bin link removed:
npx @google/adk-devtools run …(the current script) is worse, not better: npm resolves a package name against the nearest package root — the fixture — so once the fixture has nonode_modulesit goes to the registry and silently tests the published devtools instead of this working tree. A green build proving nothing is worse than a red one.node ../../../../dev/dist/esm/cli_entrypoint.js run <file>has neither problem: it cannot fall back to the registry, and it fails loudly if the workspace was not built. Verified working with no fixturenode_modulesand no.bin/adk:What changed
scripts.startswitched to the built CLI; deaddevDependencies(file:../../../../dev,file:../../../../core) removed.discovery/package.jsononly loses itsdevDependencies(it has noscripts).package.jsonstays on disk.getTypeFromPackageJson(dev/src/utils/agent_loader.ts:591-614) walks to the nearest manifest, so deleting one would let the walk reach the repo root's"type": "module"and flipapp_ts/app_js/app_default/__dirname/__filename/discoveryfrom CJS to ESM — the CJS-vs-ESM split is exactly what these suites exist to test. Notests/integration/package.jsonwas added for the same reason: 9 agent files undertests/integration/have no sibling manifest and currently resolve ESM by walking to the root.node_modules/package-lock.jsonteardown goes away. No assertion changed.script_jskeeps its three generated-artifact removals;app_loaderkeepsloader.disposeAll().tests/integration/workspace_cli.ts(new, 40 lines, 1 exported function) — asserts the built CLI exists. It earns its place:getResponse(tests/integration/test_case_utils.ts:376) only subscribes tostdout, so a missing build surfaces asexpected '' to contain "I'm stubby model response!"with node'sMODULE_NOT_FOUNDswallowed. With the guard you getMissing …/dev/dist/esm/cli_entrypoint.js. Runnpm install && npm run buildat the repository root first.dev/srcdrive-by fix (required, not optional) — see below.tests/integration/build_setup/**is deliberately untouched. Itsfile:installs arenpm pack+ extract, so each fixture gets a real published-layout copy ofcorehonouring thefilesallowlist — which is the property that suite exists to prove. It keeps all 6 installs.Trade-off, stated explicitly: the 7 affected fixtures no longer demonstrate a self-contained consumer install;
@google/adknow resolves through a workspace symlink intocore/. Node still enforcescore/package.json'sexportsmap, soimport '@google/adk'behaves identically; what those fixtures no longer exercise is thefilesallowlist. That coverage lives inbuild_setup, which keeps its real installs.Required
dev/srcfix:getProjectNodeModulesDirdid not walk upadk rundefaults tocompile: true, bundle: trueand writes the bundle to a temp directory outside the repo. esbuild'sexternallist (sqlite3,onnxruntime-node,pg-native,jiti, …) is not inlined, so at runtime Node must resolve those from the temp dir — which only works becauselinkProjectNodeModulessymlinks anode_modulesinto it.getProjectNodeModulesDirfound the nearestpackage.jsonand then checked only for anode_modulessibling of that file. These fixtures keep theirpackage.jsonbut lose theirnode_modules, so it returnedundefined, no symlink was created, and any external reached at runtime would beMODULE_NOT_FOUND. That is a regression this PR would otherwise introduce, and it is the same latent bug for any real user runningadk runinside an npm/pnpm workspace with hoisted dependencies.It now walks up for the nearest ancestor
node_modules, mirroring Node's own resolution. Strictly additive: for a standalone project with its own install the nearestpackage.jsonand the nearestnode_modulesare the same directory, so the result is identical; it never removes a link that used to be created.tryToFindFileRecursivelyand the newtryToFindFolderRecursivelynow share one bounded walk that differs only in the existence predicate, rather than the second being a copy-paste of the first.tryToFindFileRecursivelykeeps its exact signature, contract and error string — it still has a live caller atdev/src/cli/deploy/deploy_utils.ts:157.Collision check against open PRs on this fork (required, recorded here)
gh pr list --repo AmaadMartin/adk-js --state open --limit 100+gh pr diff --name-onlyon every plausibly adjacent PR. Four overlap; none lands this change:fix/flaky-install-bound-integration-suitestests/integration/fixture_project.ts(installFixtureProject) and routes all four suites through it; keeps every install, adds cache flags + CI tweaks.feat/trim-integration-test-npm-installsdiscoveryinstall only (1 of 8) and deletesdiscovery/package.json, accepting the CJS→ESM flip.discoveryhunk.I branched from
mainrather than stacking, for three reasons: (a) #276 deletesdiscovery/package.json, which contradicts the CJS/ESM invariant above — stacking would inherit a design decision this change is specifically arguing against; (b) both bases are unmerged fork branches and stacking would drag #218's workflow/go_server.ts/test_server_test.tshunks into this diff; (c) the changes are semantically disjoint — this PR deletes the very installs thefixture_project.tshelper exists to wrap. If #276 lands first, thediscoveryhunk here becomes a trivial conflict to resolve in its favour or mine; there is no logical dependency in either direction.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.
All runs below use
node v22.22.2/npm 9.2.0, withgit clean -xdf tests/integrationbefore each run (a stale fixturenode_modulessilently satisfies resolution and invalidates the measurement) andnode_modules/.bin/adkdeleted, to reproduce the CI state exactly.Measured before/after — CI,
Run tests and check code coveragestepThe number this task actually owes. Baseline = the three most recent
validationruns on branches that touch none of these suites.ubuntu-latestmacos-latestwindows-latestThe step total does not improve, and I am not going to claim it does. On
ubuntuandwindowsit is flat; themacossample is higher but that runner swings 181 → 247 s across baseline runs alone, so one sample says nothing.Per test file,
ubuntu-latest(the low-noise runner, same four runs):app_loader/app_loader_test.tsagent_loader/agent_dirname_test.tsskills/script_js/agent_test.tsbuild_setup/build_setup_test.ts(unchanged control)The three changed files get 11-31% faster and the untouched control gets ~7% slower — because it now competes for a machine the deleted installs used to occupy. Read together, those two rows say the same thing: the integration step is bounded by
build_setup_test.ts's six sequential installs, which this change deliberately does not touch, so freeing 8 installs elsewhere converts to slack rather than to wall clock. The win here is 8 fewer concurrentnpm installprocesses and 8 fewernode_modulestrees written and deleted per run per OS, not a faster step.CI flakiness: 3 attempts were needed, and this change plausibly contributes (please read)
Attempt 1:
macosfailed onAgentLoader discovery … should discover apps vs agentsat 40848 ms against its 40000 ms limit. Attempt 2:windowsfailed ontests/integration/a2a/basic/a2a_agent_test.tswithlisten EACCES: permission denied ::1:49881(a random ephemeral port landing in a Windows reserved range). Attempt 3:windowsfailed onrun_skill_script_tool_test.ts > successfully executes a real PowerShell skill scriptat 5022 ms. Attempt 4: all three OSes green.None of those three is in a file this PR touches, each already has a dedicated open PR, and all four of the suites in scope here passed on all three OSes in every attempt. The discovery test in particular is pre-existingly marginal: 22.3 s and 31.3 s on baseline
macos, and 28.4 s onubuntuin the same run that timed out onmacos— statistically identical to the 25.7-28.6 s baseline, so this change did not make that test slower.But the honest read is not "unrelated": removing 8 install hooks means more test files execute concurrently, which raises contention and therefore the flake rate of tests that are already sitting near a hard timeout or racing for a port. I have deliberately not retuned
TEST_EXECUTION_TIMEOUTor split the vitest projects here — that is separate work with several PRs already open against it, and folding it in would muddy the measurement above.Local measurements (
node v22.22.2/npm 9.2.0,git clean -xdf tests/integrationbefore every run — a stale fixturenode_modulessilently satisfies resolution and invalidates the measurement — andnode_modules/.bin/adkdeleted to reproduce CI exactly). This sandbox proxies npm through a slow gateway, so a single fixture install costs 65.84 s / 606 packages and every install-bound suite times out onmain; the numbers are therefore an upper bound on the benefit, not a CI prediction.npx vitest run --project integrationmain)app_loader/app_loader_test.tsHook timed out in 40000ms, 6 skippedagent_loader/agent_dirname_test.tsskills/script_js/agent_test.tsHook timed out in 60000ms, 1 skippedbuild_setup/build_setup_test.ts(unchanged control)Hook timed out in 10000msUnit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
npx vitest run --project unit:dev dev/test/utils/file_utils_test.ts dev/test/utils/agent_loader_test.ts→ 50 passedtryToFindFolderRecursivelycases: found in the source folder; found several levels up; skips a same-named plain file and keeps walking (the branch that distinguishes it from the file variant); throws oncemaxIterationsis exhausted.AgentFile.load()cases: links an ancestornode_modulesinto the compiled output dir (asserts the realfs.readlinktarget and that the walk starts from the agent's own directory); skips the link entirely when no ancestor has one.dev/test/utils/agent_loader_test.ts's explicitvi.mockfactory was updated to exporttryToFindFolderRecursively(the suite fails to load otherwise). No test was deleted, skipped,.only'd or weakened.npx vitest run --project integration tests/integration/workspace_cli_test.ts→ 2 passedEvery new test was proven to fail against the unfixed code. Mutations run and their exact failure messages:
getProjectNodeModulesDirreverted to nearest-dir-only (no upward walk)links an ancestor node_modules into the compiled output directoryENOENT: no such file or directory, readlink '/tmp/agent-loader-output-testxFqI9l/node_modules'try/catchremoved fromgetProjectNodeModulesDirskips the node_modules link when no ancestor has oneError: No node_modules found in /nowheretryToFindFolderRecursivelygivenisFileExistsinstead ofisFolderExistspromise rejected "Error: No node_modules found in /a/b/c or…" instead of resolving…throws when … not found within maxIterationscasespromise resolved "'/a/node_modules'" instead of rejectingassertWorkspaceAdkCliAvailablemade a no-oprejects naming the missing CLI path…promise resolved "undefined" instead of rejectingDEV_CLI_PATHpointed atdist/cjsworkspace_clicasespromise rejected "Error: Missing /tmp/workspace-cli-…"/expected [Function] to throw error including 'Missing …'Coverage.
dev/src/utils/file_utils.ts: every line of the new code is covered (the only uncovered lines,36-51,58-61,96-99, are pre-existingconsole.errorpaths increateFolder/removeFolder/listFiles/saveToFile).getProjectNodeModulesDiris fully covered on both branches (try 37 hits, catch 1 hit). I deliberately did not add coverage for the pre-existinglinkProjectNodeModulesbranches at632-633(link already exists) and642-645(EEXISTtolerance), or for itswin32'junction'argument: none of them is new code, and covering the platform branch needs aprocess.platformglobal mutation that buys no signal about this change.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Step 3 output — fails in ~7 s naming the path, not on a 40 s timeout, and above all not via a silent registry download:
Postconditions verified.
git grep -n "npm install" -- tests/integrationreturns onlybuild_setup_test.ts:29plus the two documentation/message strings in the new helper.git grep -l adk-devtools -- tests/integrationreturns onlybuild_setup/**and a pre-existing in-processimport {AdkApiServer} from '@google/adk-devtools'inadk_web/webui_test.ts.git statusis clean after a full integration run, with nonode_modulesorpackage-lock.jsonleft underapp_loader/,agent_loader/orskills/.Known risk, verified rather than assumed. The 6 fixture agents that
import {createModelContent} from '@google/genai'now resolve 1.52.0 from the workspace root instead of 2.9.0 from their own tree (the lockfile hoists 1.52.0 for@google-cloud/vertexai@^1.45.0and pushes core's^2.9.0down tocore/node_modules). Confirmed by resolving from a fixture directory: it lands on the root copy. All three suites pass on it, andtscalready type-checked these files against the root 1.52.0 typings, so the type-check and the runtime now agree where they previously disagreed.core's own import still resolves tocore/node_modules/@google/genai@2.9.0. Deduping@google/genaiis separate, already-tracked work and is not absorbed here.Repo-wide gates. CI on the pushed commit
eacb087:run-tests (ubuntu-latest)✅,run-tests (macos-latest)✅,run-tests (windows-latest)✅, cross-language ✅,check-license✅. Locally on the same commit:npm run build✅,npm run lint✅,npm run format:check✅.npm run ts:checkreports 308 errors before and after this change — all pre-existing incore/test/**; this PR adds none, and none of my files appear in the output.No suppressions.
git diff main -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|as any|: any|v8 ignore|istanbul ignore'returns nothing. NoCHANGELOG.md, no version bumps, no lockfile churn, no new dependency, no CI workflow edits.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.