Fix: install the integration fixtures from a project-scoped vitest globalSetup - #863
Open
AmaadMartin wants to merge 7 commits into
Open
Fix: install the integration fixtures from a project-scoped vitest globalSetup#863AmaadMartin wants to merge 7 commits into
AmaadMartin wants to merge 7 commits into
Conversation
added 7 commits
August 9, 2026 04:39
…balSetup The integration project installs 14 fixture projects. Every fixture resolves the same transitive tree, so concurrent npm processes contend for the registry and the shared npm cache lock. Add tests/integration/global_setup.ts, which installs each fixture once and in sequence before the worker pool starts, and register it on the integration project only.
…n beforeAll The four install-shelling suites now rely on the globalSetup install, so the hooks and their teardowns drop the npm install and the node_modules and package-lock.json removal. build_setup keeps its npm run build and its dist teardown.
Naming a case after each absolute fixture path made 14 unreadable test names. The loop reports the offending directory in the assertion message instead.
…nifest npm 11 accepts a dependency on a missing `file:` path and exits zero, so the old trigger only failed on npm 9 and the test failed on CI. An unparseable package.json exits non-zero on both.
…pers A directory holding a package.json is a fixture, and the walk stops there, so the two nested file: sub-packages are excluded by construction. That removes the hand-maintained 14-entry list, the exception list and the drift test that existed to catch the two disagreeing. installFixture and cleanFixture were pass-throughs over execSync and fs.rm with one caller each, so they move into setup() and teardown() and fixture_installs.ts goes away. Also drop the leftover single-statement beforeAll in app_loader_test.ts and the local HOOK_TIMEOUT in build_setup_test.ts, which duplicated the project-wide hookTimeout, and re-justify that budget against a measured fixture build.
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
Link to an existing issue (if applicable):
Or, if no issue exists, describe the change:
Problem: The
integrationproject installs 14 fixture projects frombeforeAllhooks. Those hooks run in parallel fork workers, so severalnpmprocesses race for the registry and for the shared~/.npmcache lock. Three of the four suites pass a 40 s or 60 s hook timeout, which a contended install exceeds, and the run fails withHook timed out.Solution: A project-scoped
globalSetupinstalls each fixture once and in sequence, before the worker pool starts. The four suites drop their install hooks and the matchingnode_modulesteardown. The entry sits on theintegrationproject only, sonpm run test:unitstill installs nothing.build_setup_test.tskeeps itsnpm run buildhook, because that hook asserts on the build output.global_setup.tsfinds the fixtures by walkingtests/integrationand stopping at the first directory that holds apackage.json. That gives the 14 fixtures and excludes the two nestedfile:sub-packages their parents install, so the list cannot drift from the tree.Cost, measured on a 64-core Linux host. The installs are now a serial pre-pool phase of about 190 s (14 installs, 12-17 s each, warm
~/.npm).vitest run --project integrationgoes from 189 s to 299 s with the default pool, and from 283 s to 383 s with--maxWorkers=2. Skipping an install whose tree is already current is queued as a follow-up.Two cleanups the diff exposed.
app_loader_test.tswas left with abeforeAllaround one synchronous statement, now aconst.build_setup_test.tshad a localHOOK_TIMEOUTof 120000 that duplicated the project-widehookTimeout; it is deleted, and the surviving comment invitest.config.tsis re-justified against a measured 3.6 s fixture build instead of an install that no longer runs in a hook. No timeout value changed.Collision check. I listed the 749 open PRs on the fork and read the diffs of the eight adjacent ones: #218, #237, #257, #276, #299, #405, #407 and #723. None moves the fixture installs into a
globalSetup; #218 and #405 keep the installs inbeforeAll, #299 removes them by hoisting resolution to the workspace root, and #237 proposes a separate project. None has landed. I branched frommainrather than stacking, because eight branches overlap and no one of them is the natural base.Testing Plan
Please describe the tests that you ran to verify your changes.
Unit Tests:
The 72 existing integration files are the test for this change: every one of them runs against the fixtures
globalSetupinstalled. I added no test file. An earlier revision carried a unit test over an exportedinstallFixture/cleanFixturepair; deriving the fixture list removed the drift the test guarded, and the two helpers are now inlineexecSyncandfs.rmcalls with one caller each.Commands run on the pushed commit:
npm run ts:checkreports pre-existing errors undercore/testand two integration files, all present onmain. It reports none in the files this PR touches.Proof the change is load-bearing. I made
setup()install nothing and re-ran the four affected suites.build_setup_test.tsfailed onts_commonjs_native_addonandts_esm_native_addon, which depend ononnxruntime-node: file:./fake-onnxruntime-nodeand can only resolve it after an install. The other 12 fixtures passed, because npm resolves@google/adkand@google/adk-devtoolsfrom the workspace rootnode_modules. Making the installs cheaper therefore has more headroom than the queued follow-up assumes.One CI attempt failed on an unrelated flake. The macOS job timed out on the two
app_loaderdiscovery tests at their own 40 s per-test budget, which this PR does not touch: the diff only removesTEST_EXECUTION_TIMEOUTfrom hooks. That job took 603 s against 479 s on the passing rerun, so the runner was slow. The rerun is green on all three operating systems. #506 and #858 already target this test.I could not reproduce the reported flake on this host. Each install finished in about 13 s here, inside the 40 s budget, so
mainpassed at--maxWorkers=2as well. The fix removes the contention rather than the symptom, so the reproduction is not a precondition for it.Manual End-to-End (E2E) Tests:
The run installs the 14 fixtures one at a time, prints npm's output for each, and the teardown leaves the working tree unchanged.
Checklist