test(install): run bun-install-registry.test.ts concurrently and tighten its assertions - #38504
test(install): run bun-install-registry.test.ts concurrently and tighten its assertions#38504robobun wants to merge 3 commits into
Conversation
…ten its assertions Every test now creates its own project directory and env through setupTest() instead of sharing the ones assigned by a module-level beforeEach, which lets the describes run as describe.concurrent. Tests that use toMatchSnapshot(), register registry users, or depend on manifest arrival order (hoisting > peers) are marked serial. The 44 Windows-only bin shim tests share their read-only install and run concurrently as well. While there: - "disabled dev/peer/optional dependencies are still included in the lockfile" awaited the runBunInstall function object and asserted nothing; it now installs with all three groups omitted and checks the lockfile and the reinstall. - the "(bun.lockb) git dependencies" case wrote a bun.lock because its bunfig dropped saveTextLockfile = false; bundledDependencies installs now assert the lockfile flavor, the install output, and that the reinstall is a no-op. - "exact versions stay exact" and "update different dependency groups" become one test per case with the update output asserted. - bin types runs its six bins concurrently and asserts the add summary; outdated "dependency pattern args" asserted on a misspelled package name; several installs that only checked the exit code now check their output and the resulting node_modules; the peers loop asserts the re-resolve is a no-op. - the auto-install snapshot is inlined and the duplicated generateRegistryUser delegates to the registry helper.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⛔ Files ignored due to path filters (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Status: test-only change, ready for review (head: bb64645). Reproduced the slowness locally: CI, compared at the same shard position against build 95342 (a PR that touches this file but leaves it serial): x64-asan 55.2s -> 24.1s, windows 11 aarch64 63.3s -> 36.5s, alpine x64 27.2s -> 17.5s, windows 2019 x64 30.0s -> 23.5s, debian x64 15.8s -> 14.1s. The 4-vCPU release lanes gain less than the local runs because the single-threaded verdaccio process becomes the floor there; details in the description. Commits after the first CI run: de573e4 chains the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large refactor (~250 tests moved to concurrent execution) whose correctness depends on per-platform CI behavior, a human look — mainly confirming BuildKite is green across the Windows and ASAN lanes — would still be worthwhile.
What was reviewed:
- Checked that every test that mutates shared registry state (
generateRegistryUser, htpasswd) is kept.serial, and thatcreateTestDir()'sthis.users = {}reset can't race with them since serial tests drain the concurrent group first. - Verified
test.serial.todo/test.serial.todoIf/describe.concurrent.eachchain correctly per theTest<T>/Describe<T>types. - Confirmed
registry.generateUserin harness.ts matches the deleted local copy's contract (duplicate-name guard, same endpoint). - Spot-checked the tightened assertions (
prereleases-1typo fix, the no-opawait runBunInstall;rewrite, thesaveTextLockfile: falsefix for the bun.lockb git test) against the surrounding code.
Extended reasoning...
Overview
This PR refactors test/cli/install/bun-install-registry.test.ts (~9700 lines, ~280 tests) from serial to concurrent execution and tightens a number of weak assertions along the way. The mechanical part is: delete the module-level beforeEach/packageDir/packageJson/env, add a setupTest() helper that returns per-test copies, insert const { packageDir, packageJson, env } = await setupTest() at the top of every test, and switch describe→describe.concurrent / test→test.concurrent. The non-mechanical parts are assertion tightening (fixing a no-op test, a misspelled package name, a wrong-lockfile-flavor bunfig, several exit-code-only checks) and splitting two looped tests into test.each cases. The snapshot file drops one entry that was inlined. No production code is touched.
Security risks
None. This is a test-only change with no production code paths, no auth/crypto/permissions logic, and no new external network access (the pre-existing github.com/npmjs.org fetches are explicitly called out as unchanged).
Level of scrutiny
Medium. The change is test-only, so the blast radius is CI, not user-facing behavior. The mechanical portion (~200 identical one-line insertions plus describe.concurrent markers) is low-risk and follows the same pattern already used in bun-install-lifecycle-scripts.test.ts and bun-workspaces.test.ts. The higher-risk portion is whether concurrent execution introduces flakiness on specific CI lanes (Windows shim tests, ASAN timing, Verdaccio under 20 concurrent installs). The PR description shows local verification on linux debug+ASAN and Windows Server 2019 debug, but the real signal is BuildKite across all lanes, which was still building when this review ran.
Other factors
- The PR description is unusually thorough: it enumerates every serial holdout with a stated reason, every assertion change with what it exposed, and before/after timings on two platforms. This substantially reduces review burden.
- The concurrency-safety argument checks out: each test gets its own temp dir, its own
BUN_INSTALL_CACHE_DIR/TMPDIR, and its ownport: 0servers; the only shared resource is the read-only Verdaccio registry. Tests that write to the registry (user registration → htpasswd) are all kept.serial, andcreateTestDir()'s reset ofregistry.userson the shared instance is harmless because concurrent tests never callgenerateUser()and serial tests drain the concurrent group before running. - The assertion tightening is exactly what REVIEW.md asks for (assert exact values, assert the strongest invariant, no vacuous
not.toContainon a misspelling). These are strict improvements. - The Windows shim describe now uses
bunEnvdirectly instead of the (now-removed) mutated module-levelenv; the shared install happens once at collection time and the 44 tests only exec bins from it, so sharing that directory is safe. - Given the ~800-line diff size and the fact that concurrency flakiness typically only surfaces on specific CI configurations, deferring so a human can confirm BuildKite is green across all lanes before merging.
…their own TEMP Concurrent rm() of the registry's htpasswd file rejects with EFAULT on Windows, and concurrent --bun runs share the fake node directory in TEMP.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large refactor (~280 tests moved to concurrent execution, per-test state isolation, and a dozen or so tightened assertions), a human look would still be worthwhile — particularly to confirm the new exact-output assertions hold across all CI lanes.
What was reviewed:
setupTest()promise-chaining: reads/writespreviousSetupbefore the firstawait, so concurrent callers chain deterministically; rejection is swallowed so one failure doesn't poison the chain.- Serial holdouts (
whoami, snapshot tests,hoisting > peers, npm-alias auth test) each have a stated reason;registry.generateUsermatches the removed local copy. - Tightened assertions spot-checked (
bundledDependencieshelpers, rewritten "disabled dev/peer/optional" test,prereleases-1typo fix,--configoutput) — all look consistent with the fixture packages.
Extended reasoning...
Overview
Test-only change to test/cli/install/bun-install-registry.test.ts and its .snap file. The module-level beforeEach and shared packageDir/packageJson/env are replaced with a per-test setupTest() helper, describes become describe.concurrent, and helpers that closed over the module state now take explicit parameters. Two loop-over-cases tests are split into test.each/nested describes, one no-op test is rewritten, and roughly a dozen tests gain stronger assertions on stdout/stderr/lockfile/node_modules.
Security risks
None. No production code is touched; the only runtime effect is on the test file itself.
Level of scrutiny
Moderate-to-high. The mechanical bulk (adding await setupTest() and .concurrent) is low-risk, but the non-mechanical parts warrant a human pass: the previousSetup chaining for Windows EFAULT avoidance, the per-test TMP/TEMP for the --bun shim tests, and the tightened exact-output assertions (e.g. the new bun add summary in "bin types", the (no changes) regex in expectReinstallIsNoop, the rewritten "disabled dev/peer/optional" test). Any of these could be subtly over-tight on a lane the author didn't run.
Other factors
- Follows the same shape as
bun-install-lifecycle-scripts.test.tsandbun-workspaces.test.ts, which the description cites. - Verification is thorough (multiple Linux debug+ASAN and Windows debug runs, before/after timings, CI run in progress).
- No tests are deleted or skipped; the +8 test count is accounted for by the two split loops.
- The
.snapchange only removes the one snapshot that was inlined. installAndCaptureAuthnow callssetupTest()internally, so its describe-level tests each get their own dir — checked that nothing else in that describe reads a sharedpackageDir.
|
The Windows debug-build failure of "npm manifest cache entries are only reused for the package name they were saved for" mentioned under Verification is addressed in #38580 (test-only: the final assertion no longer requires the background manifest cache write to have landed before the install exits). The lines it changes are identical in this branch and main, so whichever of the two lands second rebases trivially. |
…it can run concurrently The request is refused before anything is sent, so registering a registry user (which kept the test serial) was not needed.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a test-only refactor that follows the same per-test setupTest() + describe.concurrent pattern already used in bun-install-lifecycle-scripts.test.ts and bun-workspaces.test.ts, with an unusually thorough writeup and 18+ green runs across Linux and Windows. Because it flips ~280 tests to concurrent and tightens a number of stdout assertions in one go, a human skim would still be worthwhile before merging.
What was reviewed:
setupTest()chaining viapreviousSetup— a rejectedcreateTestDir()doesn't poison later calls (both handlers are no-ops), andcreateTestDir()resetsthis.users, so thegenerateRegistryUserdelegation is safe.- Serial holdouts (
whoami, snapshot tests,hoisting > peers) are correctly opted out viatest.serial/describe.serial; the alias host-pinning test was made concurrent by switching to a literal token (last commit). - Tightened assertions checked for order-sensitivity —
hoisting > peersreinstall uses per-deptoContain(order-agnostic);bin typessummary follows the fixedbun addargument order. - The Windows
--bunshim tests get a per-testTMP/TEMPand share one install directory built at collection time, so concurrency there only affects bin execution, not the install.
Extended reasoning...
Overview
This PR converts test/cli/install/bun-install-registry.test.ts (~280 tests, ~9,700 lines) from a serial beforeEach-driven file to a concurrent one where each test calls await setupTest() for its own packageDir/packageJson/env. Along the way it strengthens a handful of weak or vacuous assertions (a no-op await runBunInstall;, a misspelled prerelease-1 check, a bun.lockb case that was silently writing bun.lock, several installs that only checked exit code), splits two loop-over-cases tests into test.each, inlines one snapshot, and removes the corresponding entry from the .snap file. No production code is touched.
Security risks
None. The change is confined to test infrastructure and assertions. No auth, crypto, permissions, or user-facing code paths are modified. The one auth-adjacent change (the alias host-pinning test now uses a literal token instead of registering a Verdaccio user) is safe because the test asserts the request is refused before it's sent, so token validity is irrelevant.
Level of scrutiny
Moderate. This is a test-only change, so it can't break users, but it flips a large test file to concurrent execution, which is exactly the kind of change that can introduce CI flakiness that only surfaces on specific lanes under load. The mitigations are well thought out: per-test cache/tmp directories, chained createTestDir() calls to avoid the Windows htpasswd rm race, per-test TEMP for --bun shim tests, and explicitly serial blocks for the tests that share registry state (whoami) or are order-sensitive (hoisting > peers) or use file snapshots. The 11 Linux debug+ASAN runs and 7 Windows debug runs plus green CI give reasonable confidence, but a maintainer familiar with the flakiest lanes (macOS, Windows aarch64) should confirm they're comfortable with the tightened stdout assertions.
Other factors
The pattern matches sibling files (bun-install-lifecycle-scripts.test.ts, bun-workspaces.test.ts) that already went through this conversion. The bug hunting system found nothing. registry.generateUser and test.serial/describe.serial exist in the harness/runner. The previousSetup promise chain correctly swallows rejections so one failed setup doesn't wedge every subsequent test. The tightened assertions I spot-checked are either order-agnostic (toContain per item) or follow deterministic ordering (argument order for bun add, lockfile section order for the omit test). Nothing is deleted or skipped; the +8 test count is explained by the two split loops. Given the sheer size of the diff and its potential to affect CI stability across many lanes, I'm deferring rather than approving.
1 participant
Problem
test/cli/install/bun-install-registry.test.tsis one of the slowest serial-phase files: 58-60s on windows 11 aarch64, ~53s on debian 13 x64-asan, 23s on windows 2019 x64, against 9-11s on the linux release lanes (expected-durations.json: default 10.1s, asan 47.9s, windows 22.1s).bun install/bun updatespawns run one at a time: a module-levelbeforeEachassigned a sharedpackageDir/packageJsonand mutated the sharedenvobject, so no test could overlap with another. The per-test floor is one spawn (~150-200ms on a debug build), and ~120 of the cases (semver, prereleases, yarn, peer index, ...) are exactly one spawn each.describe("windows bin linking shim should work")), each of which spawns a chain of 2-3 processes; those were serial too, which is most of the gap between the Windows and Linux lanes.bun.lockbvariant was silently testingbun.lock, onebun outdatedassertion checked a misspelled package name, and a number of installs only checked the exit code.Fix
const { packageDir, packageJson, env } = await setupTest(), which wraps the sameregistry.createTestDir()call thebeforeEachmade and returns an env whose cache/tmp dirs live inside that directory. ThebeforeEachand the module-levelpackageDir/packageJson/env/usersare gone, so tests share nothing but the (read-only) registry. This is the same shapebun-install-lifecycle-scripts.test.tsandbun-workspaces.test.tsalready use.describe.concurrentand the top-level teststest.concurrent. Describe-scoped helpers that read the module variables (runBin,checkHoistedFiles/checkUnhoistedFiles,setupWorkspace,installAndCaptureAuth) take the directory/env as parameters instead. No test is deleted, skipped, or renamed; two tests that looped over independent cases are split into one test per case under a describe with the old name (update > exact versions stay exact > a-dep|aliased,update > update different dependency groups > ...), which is where the +8 in the test count comes from.serialso the choice is visible: thewhoamitests (they register registry users, andcreateTestDir()deletes the registry's htpasswd file, so an overlapping test would log the other one out; "rejects npm aliases ..." also registered a user, but its request is refused before anything is sent, so it uses a literal token now, like the test after it, and runs concurrently); the 16 tests that usetoMatchSnapshot()(bun rejects external snapshots inside concurrent tests; theoutdatedtables contain ANSI escapes and are not worth inlining); andhoisting > peers, whose outcome depends on manifest arrival order (test(install): mark "peer *" hoisting case todoIf(isFlaky) #33972, install: resolve deferred*peers after their siblings, not on arrival order #37713), so it keeps running against an idle registry rather than one serving 20 installs. The Windows shim tests only execute bins from one install made at collection time, so they share it and run concurrently.createTestDir()deletes the registry's htpasswd file, and when a concurrent group starts every test in it callssetupTest()at once. On Windows, concurrentrm(file, { force: true })of the same file rejects withEFAULT(bun bug, reproduced standalone and reported separately; node resolves all of them), which is what failed in the first CI run on windows 11 aarch64 in the test right after thewhoamiblock.setupTest()therefore chains thecreateTestDir()calls through a promise; each one takes a few milliseconds, and the Linux timings below are unchanged.bun-workspaces.test.tsandbun-install-lifecycle-scripts.test.tshave the same latent race.bun --buncreates its fakenode.exeunder%TEMP%, and debug builds wipe and recreate that directory on every run (create_fake_temporary_node_executablein src/install/lib.rs), so concurrent--bunshim tests occasionally got a debug-onlySHARING_VIOLATIONwarning on stderr, which these tests assert is empty (about 1 run in 6 with a Windows debug build; release builds, i.e. the CI lanes, never wipe the directory). Each--bunshim test now gets its ownTMP/TEMPdirectory, so nothing is shared; 12 of 12 debug runs of the block were clean afterwards, and the fakenode.exeshows up inside the per-test directory, so the--bunpath is still exercised.install.cachein its bunfig plusBUN_INSTALL_CACHE_DIR/TMPDIRin its env), and its ownport: 0mock servers; the registry is only read. Bun's default--max-concurrency(20, or 5 in ASAN builds,src/options_types/context.rs) bounds how many are in flight, the same as in the other concurrent install files.setDefaultTimeoutis left at 5 minutes on purpose: a concurrent test's wall time now includes waiting on its 19 neighbours, so it should not be tightened in the same change.bun updateoutput (and, for the groups, the installed version).bun <bin>checks were sequential; they run underPromise.allnow (0.5s) and thebun addsummary is asserted.beforeEachitself: it also ran twoBun.$rms of paths that do not exist (the real htpasswd is removed bycreateTestDir()); dropping it took the runner's own CPU time on the debug build from 23s to 18s.await runBunInstall;(the function object) and asserted nothing. It now installs with--omit=dev --omit=peer --omit=optional, checks that nothing is installed but all three packages are inbun.lock, and that a plain install then installs all three from that lockfile without re-saving it.bundledDependencies: the 20 installs hadstdout/stderr: "ignore"and checked the exit code. They now go through two small helpers that assert stderr,Saved lockfile, that the lockfile flavor named in the test title was actually written, and that the second install reports(no changes). The flavor check exposed that "(bun.lockb) git dependencies" wrote abun.lock: its private bunfig (needed because the git packages depend onzod, which the local registry does not have) predatesbun.lockbecoming the default and lackedsaveTextLockfile = false; fixed.outdated > dependency pattern argsassertednot.toContain("prerelease-1"); the package isprereleases-1, so the assertion could never fail. Fixed, and it holds.hoisting > peers: the second and third installs assertednot.toContain("package installed"), which a multi-package reinstall never prints; they now assert the lockfile-less re-resolve reports(no changes)and that the reinstall lists every root dependency. A leftoverconsole.logof stdout is gone.semver(19 cases): also checks the installednode_modules/dep-with-tags/package.json, like the prerelease cases already did.package.json indentation(one spawn wasstdio: "inherit"),--config cli flag works, "it will skip ...directories.bindoes not exist",transitive file dependencies > from root dependencies: install output and resultingnode_modulesare asserted instead of just the exit code. Theoutdatededge cases read stdout/stderr before the exit code.auto-install's 4-line snapshot is inlined; the file-local copy ofgenerateRegistryUserdelegates toregistry.generateUser(same duplicate-name guard, reset bycreateTestDir()); therunBinhelper reports which bin failed.bun bd test test/cli/install/bun-install-registry.test.ts: before 89.5s / 89.7s / 90.2s (238 tests, 3184 expect calls); after 35.8s / 36.9s / 35.8s (246 tests, 3357 expect calls). Eight further runs of the new file (the last three with the chainedsetupTest()): 36.0-40.2s, all green (shared machine, hence the spread).peer *flake), alpine x64 27.2s -> 17.5s, windows 2019 x64 30.0s -> 23.5s, debian x64 15.8s -> 14.1s. The release lanes gain less than the local runs because they have 4 vCPUs and, with the tests themselves cheap there, the single-threaded verdaccio process serving every install is most of what is left; the asan lane also only runs 5 tests at a time. One test fails on Windows with a debug build before and after this change ("npm manifest cache entries are only reused for the package name they were saved for", from Robustness and input-handling pass across install, shell, TLS/QUIC, HTTP/3, SQL and crypto #37669; it passes on the release Windows lanes); test(install): don't require the refetched manifest to be cached before install exits #38580 fixes that test on main, and the lines it touches are identical here, so the two do not conflict.tscover the file reports the same pre-existing diagnostics as before and nothing new; prettier is clean.git dependenciescases still fetch from github.com and resolvezodfrom registry.npmjs.org, and "manifest cache will invalidate when registry changes" still uses the public registry for its second install; making those hermetic is a separate change.VerdaccioRegistry.stop()leaving the registry process behind is test(harness): make VerdaccioRegistry.stop() actually terminate the process #36352. Once install: resolve deferred*peers after their siblings, not on arrival order #37713 lands,hoisting > peerscan drop itsdescribe.serial. The natural follow-up, kept out of this PR because test: replace verdaccio and the mock npm servers with an in-process spec-compliant registry #33115 and test(harness): make VerdaccioRegistry.stop() actually terminate the process #36352 are both reshaping that class, is to havecreateTestDir()itself return the per-directory env and stop deleting htpasswd (start()/stop()already do), which would replacesetupTest()here and the identical blocks inbun-install-lifecycle-scripts,bun-workspacesandbun-install-native-binlink, remove the chaining, and make the remaining serial verdaccio files (isolated-install,bun-publish,bun-lock,catalogs,bun-lockb,public-hoist-pattern,pnpm-migration) a mechanical conversion.Background
test.concurrenttests (including across describe boundaries) as one group with up to--max-concurrency(20; 5 in ASAN builds) in flight; a serial test waits for the group to drain and runs alone. So each serial holdout costs its own duration plus one drain; the remaining serial stretches are thewhoamiblock, theoutdatedsnapshot block,hoisting > peers, and the individual snapshot tests.describe.concurrentmarks every test declared inside it (including nested describes) concurrent;test.serial/describe.serialinside it opt individual tests back out. This keeps the per-test headers unchanged for tests inside describes, which is what keeps this diff mostly to one added line per test.registry.createTestDir()(test/harness.ts) makes a fresh temp directory with abunfig.tomlpointing at the file's single verdaccio instance (started once at module load, as before) and resets the registry's htpasswd file;setupTest()is a thin wrapper that adds the per-directory env.toMatchSnapshot()stores snapshots by test name in a shared.snapfile and is rejected inside concurrent tests;toMatchInlineSnapshot()is allowed, which is why the one small snapshot was inlined and the others stayed serial.