Test: pin the integration-fixture install mode (and reject the fixture-lockfile proposal) - #407
Open
AmaadMartin wants to merge 6 commits into
Conversation
added 6 commits
July 31, 2026 11:16
… pinned Integration fixtures depend on the in-repo core and dev packages via file: specifiers, so npm's install-links setting decides whether the fixture gets two symlinks or a packed copy of their whole transitive graph. That setting is unpinned today and its default flipped between npm majors, so the same suite installs in 0.6s on one contributor's machine and 64s on another's -- and only the link mode resolves core's and dev's dependencies through the repo-root node_modules that the committed root package-lock.json pins. Add one install helper that passes the flag explicitly, and one teardown helper, so all five fixture-install call sites share the same semantics.
Replaces the five bare `npm install` call sites and their hand-rolled node_modules/lockfile teardowns. The build_setup suite keeps its dist/ removal inline and the skills suite keeps its generated-artifact removals, since neither is part of dependency installation. Adds a regression test, one per build_setup fixture, asserting the lockfile records link: true for both workspace packages. It asserts on the lockfile rather than lstat().isSymbolicLink() because npm creates junctions on Windows, which the CI matrix includes.
…k-budget rationale The tests/**/package-lock.json ignore was added incidentally and has no recorded rationale, which invites re-litigation. In link mode a fixture lockfile is five entries restating ranges already declared in core/package.json and dev/package.json, with no resolved versions and no integrity hashes, so committing one would pin nothing while costing 14 files of churn per dependency bump. The integration hook budget's justification cited a ~70s network-bound install. Pinning --install-links=false makes the install a sub-second link operation with no network access, so the remaining cost in those hooks is the ts_* fixtures' npm run build. Corrects the prose; the 120s value is unchanged.
Pinning --install-links=false guarantees the fixtures RESOLVE THROUGH the
repo-root node_modules; it does not guarantee that tree matches the
committed package-lock.json. npm install re-resolves and silently accepts
a manifest that has drifted from the lockfile, so the fixtures can be
handed a transitive graph the lockfile never recorded. npm ci reifies
strictly from the lockfile and fails loudly instead.
Verified both halves locally by adding a dependency to core/package.json
without regenerating the root lockfile: npm install exits 0 and re-resolves
("added 75 packages"), while npm ci exits 1 with
npm ERR! code EUSAGE
npm ERR! Missing: left-pad@1.3.0 from lock file
Against the committed lockfile at this HEAD, npm ci succeeds in 16.1s, so
this does not redden CI on merge.
cleanupFixtureDeps' doc claimed "best-effort", but fs.rm(force: true)
only swallows ENOENT -- an EBUSY or EPERM rejects and fails the afterAll.
That is the intended behavior (a fixture that cannot be cleaned pollutes
the next run), so correct the comment rather than re-add a catch-all.
Drop the .catch(() => {}) still wrapping the dist/ removal two lines
below, so both halves of the same hook report failures the same way.
…ardown contract
The rationale for pinning --install-links=false was written out four
times with three different sets of numbers. Keep the full explanation at
installFixtureDeps, which is the only place that actually sets the flag,
and reduce the other three to a pointer: the .gitignore comment to one
line, the vitest.config.ts hook-budget note to one sentence with no
duplicated figures, and the regression test's comment to just the
non-obvious part (why the lockfile rather than lstat).
Also drop the three now-dead .catch(() => {}) on the skills teardown's
generated-file removals. fs.rm(force: true) already swallows ENOENT,
which is the only error they plausibly guarded -- the same reasoning
cleanupFixtureDeps and the dist/ removal rely on. That afterAll was
swallowing rm failures on three lines and then calling a helper
documented to reject on them.
Document --no-audit --no-fund, which the doc comment above the command
did not previously mention.
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: Integration-fixture installs are non-deterministic in two independent ways.
The install mode is unpinned. The 14 fixtures depend on the in-repo packages via
"@google/adk": "file:../../../../core"and"@google/adk-devtools": "file:../../../../dev", installed with a barenpm install. Whether that produces two symlinks or a packed copy of the entire transitive graph is decided by npm'sinstall-linkssetting, which this repo never pins — and whose default flipped fromtruetofalseat npm 9→10. The repo also pins no Node/npm version: no.nvmrc, no.npmrc, noenginesfield, nopackageManagerfield, andactions/setup-node@v6is used with nonode-versioninput, so CI runs on whatever the runner image ships.Measured on this branch (
js_esmfixture, Node 22.22.2 / npm 9.2.0, warm cache, root installed first):install-links=true)--install-links=falsepackage-lock.jsonnode_modulestop levellink: trueon the workspace entriesThe installed versions are unpinned. CI installed the repo root with
npm install, which re-resolves and silently accepts a manifest that has drifted from the committedpackage-lock.json. Sincecore's 29 anddev's 19 runtime dependencies resolve through the repo-rootnode_modules(verified: fromcore/,winston,expressandzodall resolve to<repo>/node_modules/...), a drifted root tree is silently handed to every fixture.Solution: three changes, all test/CI plumbing. No
core/,dev/orintegrations/source is touched, so there is no public API or runtime behavior change.installFixtureDepsintests/integration/test_case_utils.tspassing--install-links=false --no-audit --no-fund, plus a sharedcleanupFixtureDepsteardown. A new regression test asserts the fixture really did get links..github/workflows/validation.yaml:32and.github/workflows/cross-language-integration.yml:26switch fromnpm installtonpm ci, so the rootnode_modulesis reified strictly from the committed lockfile. Change Can you implement parallel tool in adk-js calling for this issue: https://github.com/google/adk-js/issues/233. Ignore the PR that's already been made for it. Staging #1 guarantees the fixtures resolve through that tree; only this guarantees the tree matches what is committed.tests/**/package-lock.jsonstays ignored and stays deleted inafterAll, with the rationale at the ignore line.Why not commit the fixture lockfiles / switch the fixtures to
npm ci? That was the original proposal, and the measurements above reject it:npm ciis not faster at the fixture level. The whole speed difference comes frominstall-links, not fromcivsinstall.core/package.jsonanddev/package.json— no resolved versions, no integrity hashes.npm cihard-fails withEUSAGE— 14 files × every dependency bump × every npm-major mismatch, for zero determinism.The determinism the proposal wanted is real, but it lives at the repo root, which is what change #2 enforces.
Behavioral change for contributors: a PR that edits any workspace
package.jsonwithout regenerating the rootpackage-lock.jsonnow fails at the CI install step with a preciseEUSAGEmessage instead of silently re-resolving. That is the intended effect, called out here so reviewers are not surprised. Contributors on npm 9 will also see the integration suites get dramatically faster.Collision check (
gh pr list --repo AmaadMartin/adk-js --state open --limit 100, thengh pr diff --name-onlyon every plausibly adjacent PR):fix/integration-hook-timeout-single-sourcerather than branched frommain— review it after Fix: align integration install hooks on the project-wide hook timeout #405. Its two commits (25cd61fe,41ae0c12) therefore appear in amain..HEADdiff and are not part of this task.actions/setup-nodeinputs), not the install step.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.
vitest.config.tsrestricts coverage collection tocore/src,dev/srcandintegrations/src; nothing here is inside that set, so no coverage threshold applies and none were touched. The real check is an outcome assertion on the install, not a mock of npm's argv.New regression test —
should link the workspace packages rather than pack them, added inside the existingdescribe.eachso it runs for all sixbuild_setupfixtures. It parses<fixture>/package-lock.json(typed with a narrow localinterface FixtureLockfile; noany, no suppressions anywhere in this diff) and assertslink === truefor both workspace entries. It asserts on the lockfile rather thanlstat().isSymbolicLink()because npm creates junctions rather than symlinks on Windows and the matrix includeswindows-latest, whereaslink: trueis stable across npm majors and every OS.Proof the new test can fail (required). Mutation: in
installFixtureDeps,--install-links=false→--install-links=true, thennpx vitest run --project integration tests/integration/build_setup -t "should link the workspace packages". Result — all 6 fail:That run also quantifies the win: 420.85 s for the 6 packed installs alone, against 93.80 s for the entire linked suite including all 26 assertions. The mutation was reverted before committing.
Proof change #2 does something (the
EUSAGEguard). Addedleft-padtocore/package.jsonwithout regenerating the root lockfile, then ran both install commands against the identical drifted state:Reverted afterwards;
git status --porcelain core/package.json package-lock.jsonis empty andnpm ciis green again. Against the committed lockfile at this HEAD,npm cisucceeds in 16.1 s, so change #2 will not redden CI on merge.Error paths.
installFixtureDepsawaitsexecAsyncwith notry/catch, so a failed fixture install rejects out ofbeforeAllunchanged; there is nonpm ci->npm installfallback anywhere, since a fallback that rescues a stale lockfile would make the enforcement a no-op.cleanupFixtureDepswas exercised on the error path by the mutation run above: it ran inafterAlldespite six failing tests and leftgit status --porcelain tests/clean.One failure contract per teardown.
fs.rm(force: true)already swallowsENOENT, so the.catch(() => {})wrappers the old teardowns carried were dead for their only plausible error and merely hid the rest. They are gone from every hook this PR touches -- the shared helper, thedist/removal inbuild_setup_test.ts, and the three generated-file removals in the skills suite -- so a hook no longer swallowsrmfailures on one line and rejects on them the next. The consequence is deliberate: anEBUSY/EPERM(plausible on thewindows-latestleg) now fails theafterAllinstead of silently leaving junk for the next run, andcleanupFixtureDeps' doc says so rather than claiming "best-effort".Targeted suites re-run on the exact committed tree (per the repo guideline, not the full
npm test):Empty
git statusafter the runs confirms none of the 14 fixture directories leaks anode_modulesorpackage-lock.json.CI status:
absent, not green — validated locally instead. This is a stacked PR whose base isfix/integration-hook-timeout-single-source, and.github/workflows/validation.yamltriggers onpull_request: branches: [main], sorun-testswill not fire for this base. On the exact pushed commit:tsc --noEmitreports no error in any file this PR touches. (The repo-widets:checkis not currently green — pre-existing failures incore/test/**and two unrelatedtests/integration/**files, which PRs #326/#370 address.)Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
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.