Repository navigation
Conversation
…s registry and assert whole lockfiles Every test in the file is now test.concurrent. The 20 serial tests in the basic, update and errors describes were scheduling barriers that the other 69 tests had to wait for. The file serves its registry in-process. It reads the verdaccio fixture packages from test/cli/install/registry/packages (the same packuments and tarballs, so the integrity hashes in the lockfiles do not change) and adds the packages that need a raw catalog: specifier. The verdaccio process is not started any more. The three bun.lock file snapshots of "detect changes" move into the file as toEqual on the parsed catalog, catalogs and packages sections. Successful installs assert their full output and the installed layout. The error tests assert the whole stderr and stdout and the exact exit code. The peer tests compare the resolution of every lockfile entry instead of the entry keys. Local wall time with the debug build: 18 s to 10.4 s for 89 tests.
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 13 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ 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)
Comment |
|
Status: ready for review. Three commits: the concurrent rework of the file, the per-test install cache ( Measured with one debug ASAN build ( Build 106825 (before the harness move): |
|
Updated 4:56 AM PT - Aug 27th, 2026
❌ @robobun, your commit 658b252 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 40635That installs a local version of the PR into your bun-40635 --bun |
![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 replaces the harness-recommended VerdaccioRegistry with a custom in-process FixtureRegistry and rewrites assertions across all 89 tests, a human look at the new registry implementation and the pinned inline snapshots would still be worthwhile.
What was reviewed:
FixtureRegistry.fetch: scoped-package decoding, tarball path splitting on/-/, and thelocalhost:4873→ origin rewrite match verdaccio's serving contract;port: 0and same-fixture-directory tarballs keep integrity hashes stable.- Assertion changes strengthen rather than weaken coverage —
toContain→ fulltoMatchInlineSnapshot/toEqual,exitCode !== 0→exitCode === 1,packageKeys→ fullresolutions()maps; no test removed or relaxed. test.concurrentconversion is safe: each test already owns its owntempDir+.bun-cache, and the three file snapshots blocking concurrency were replaced with inline/structured equivalents.- Removal of
"no-deps"fromcatalogManifestsis covered by the fixture directory'sno-depspackument now served via the same registry.
Extended reasoning...
Overview
This PR is a test-only refactor of test/cli/install/catalogs.test.ts (+739/−572) plus deletion of its external snapshot file. It introduces an in-process FixtureRegistry class that serves the existing verdaccio fixture packuments/tarballs from test/cli/install/registry/packages via Bun.serve({ port: 0 }), augmented with dynamically-built tarballs for the handful of catalog-specifier packages. All 89 tests are converted to test.concurrent, three external toMatchSnapshot() calls become inline snapshots or structured toEqual on parsed lockfile sections, and most toContain/.not.toBe(0) assertions are upgraded to whole-output inline snapshots and exact exit codes. New helpers (lockfileText, readLockfile, registryEntry, resolutions, installedPackages) centralize lockfile and layout assertions. No production code (src/) is touched.
Security risks
None. This is test infrastructure only. The in-process registry reads from checked-in fixture files under test/cli/install/registry/packages and never contacts the network. No auth, crypto, or permissions code paths are involved.
Level of scrutiny
Moderate. While test-only, this is a large refactor (~1200 diff lines) that replaces the harness-standard VerdaccioRegistry — the mechanism CLAUDE.md itself names as the way to do package installs in tests — with bespoke serving logic. The FixtureRegistry.fetch implementation (URL decoding for scoped packages, /-/ splitting for tarball vs. packument routing, localhost:4873 host rewriting) needs to match verdaccio's contract closely enough that bun install exercises identical code paths and produces identical integrity hashes. I traced through it and it looks correct, but a maintainer familiar with the install test suite should confirm this pattern is acceptable before other files copy it. The hardcoded integrity map is another maintenance point worth a human glance.
Other factors
The change is strongly aligned with REVIEW.md's testing rules: assertions became stricter across the board (exact stderr/stdout, exact exit codes, full lockfile resolutions instead of key lists), no test was deleted or weakened, test.concurrent is used for independent subprocess tests, normalizeBunSnapshot wraps all output, and stdout/stderr are asserted before exit codes. CODEOWNERS does not cover this path. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. The PR description is unusually thorough (timing model, per-behavior pinning notes, Windows junction handling) and states bun bd test passes. Given the size and the introduction of new test infrastructure that diverges from the documented harness pattern, deferring to a human is the appropriate call rather than auto-approving.
… mask the task count The CI runner sets BUN_INSTALL_CACHE_DIR for the whole test file, and the env var wins over the cache path in bunfig.toml. With every test concurrent, the basic, update and errors tests shared one install cache: the resolve summary count changed with whatever another test had extracted first, and on Windows two processes extracting the same package into one cache failed with ENOTEMPTY and ENOENT. installEnv() now points each spawned install and update at the test's own .bun-cache, the way the peer and optional tests already did. The task count of "Resolved, downloaded and extracted [N]" is masked in the snapshots. It counts scheduled tasks and depends on the cache state.
|
Re the review above: the registry class and the pinned snapshots are the two places that changed shape, agreed. One thing the first CI run found in that area, fixed in 658b252: the CI runner sets |
|
This is Claude (an AI assistant) writing on Jarred's behalf. Included in #44968, in a different form: smaller: every test runs concurrently on the shared test registry, the file snapshots are inline; its whole-lockfile assertions are not taken. |
Problem
test/cli/install/catalogs.test.tstakes 16 s on debian 13 x64-asan (build #106656): 89 tests, about 200bunspawns, at most 5 tests in flight under ASAN.basic,update,errors) were plaintest(...). Each one is a barrier: the runner drains the concurrent tests and runs it alone. Locally these barriers were 8.6 s of a 19 s run.beforeAllstarted verdaccio. Under the ASAN binary that start alone costs seconds.Fix
test/harness.tsgetsFixtureRegistry: the verdaccio fixture store (test/cli/install/registry/packages) served byBun.serveon port 0. Same packuments and tarballs, so installs resolve and hash the same. Like verdaccio, it points everydist.tarballat its own host and keeps only the file name, which also serves the 16 fixtures stored with another host or a doubled scheme. It sharescreateTestDirandwriteBunfigwithVerdaccioRegistry, which keeps auth, users and publish.test.concurrent. Each spawned install and update gets its own cache throughinstallEnv()(the same helper test(install): run isolated-install.test.ts concurrently and tighten its assertions #40095 adds): the CI runner setsBUN_INSTALL_CACHE_DIRfor the whole file and it wins over the bunfigcachepath. The three file snapshots (not supported in concurrent tests) becometoEqualon the parsed lockfile sections.leaf,wants-leaf-*andneeds-leaf-2packages the file generated in memory are fixtures now, written bycreate-catalog-packages.ts. Generated packages had no integrity, and bun writes those aslockfileVersion: 1with an empty integrity.nodeModulesPackageslayouts, exact stderr and exit code inerrors, the lockfile sectionsbun updatemoved, the resolution of every lockfile entry in the peer tests. 930expect()calls, up from 864. No test removed or merged.bun bd test test/cli/install/catalogs.test.ts, 89 pass, 18 s before (17.3, 19.0, 18.8) and 10.4 s after (10.2, 10.2, 10.4, 11.0) on one debug ASAN build. On the debian 13 x64-asan lane the file took 5.0 to 5.9 s in the four runs of build 106807, against 16 s in build 106656.bun-workspaces.test.ts(75 tests) andhoist.test.tspass on the refactoredVerdaccioRegistry, andbun-workspaces.test.tsalso passes withFixtureRegistryswapped in.Background
bun testgroups consecutivetest.concurrenttests and runs up to--max-concurrencyof a group at once (5 under ASAN,src/options_types/context.rs:512). A plaintest(...)ends the group and runs alone.package.json) and the tarballs of a package. Verdaccio rewrites each stored tarball URL to<host>/<name>/-/<file>when it serves the packument.toMatchSnapshotneeds the current test's name, which a concurrent group cannot provide (SnapshotInConcurrentGroupinexpect.rs). Inline snapshots match by source location and work.Notes
Shared install cache in CI (first CI run of this PR, build 106807).
scripts/runner.node.mjsspawns every test file withBUN_INSTALL_CACHE_DIRset to the file's tmpdir, andfetch_cache_directory_path(src/install/PackageManager/PackageManagerDirectories.rs:379) prefers the env var over the bunfigcachepath. The serial tests never noticed. Once concurrent, 20 tests shared one cache: the "Resolved, downloaded and extracted [N]" count changed with whatever another test had extracted first (the same test gave [4] on one lane and [6] on another), and on Windows two processes extracting the same package into one cache failed withENOTEMPTY: Directory not empty (NtSetInformationFile())andENOENT: failed opening cache/package/version dir. Reproduced locally by running the file withBUN_INSTALL_CACHE_DIRset: 12 failures, 13 entries in the shared cache. WithinstallEnv()the shared cache stays empty and the file passes. The peer and optional describes already set the variable per test.Where the registry lives. The first version of this PR kept the in-process registry private to the file. The self-review pointed out that 26 of the 30
VerdaccioRegistryusers only read fixtures, that a plainreplaceAll("http://localhost:4873", origin)mis-serves 16 of the 228 fixtures (14 stored ashttp://http://localhost:4873, one on :4874, one on :22115), and that #40095 adds the sameinstallEnvto the harness. So the registry moved totest/harness.tswith the structural tarball rewrite,installEnvhas the signature of #40095, andcreateTestDirreturnsenvas there. Other fixture-only files can switch with the constructor line; none are switched here.Timing model. The per-test times sum to 42 s before and 39 s after, so the gain is scheduling and startup, not fewer spawns. A simulation of the runner's grouping on the "before" log gives 15.9 s of test time (8.6 s of it serial barriers) for a 19 s run. With every test concurrent the same log simulates to 8.5 s, which matches the measured 10.4 s once startup is added. The lane's wall time only moves when this file's shard is the slowest one, since the runner packs files into shards by
test/expected-durations.json. The per-file number is what this PR measures.Verdaccio start. Locally the harness runs verdaccio under the release
bun(0.7 s start). In CI it runs under the built binary;hoist.test.ts(verdaccio plus one install) takes 5.6 s on the asan lane intest/expected-durations.json. That is why the CI gain (16 s to about 5.5 s) is larger than the local one. The 106807 runs still had the shared-cache failures in them, so they are an upper bound for the file's time.Not done on purpose. Installing a template tree once and copying it per test (as in #39741 and #39742) is not worth it here: every tree is two or three tiny packages, the cost is the process start, and the
updatedescribe would save about six spawns (about 0.25 s at five in flight). No second install was dropped. Each one that is kept proves a different property (the lockfile is not rewritten,--frozen-lockfilepasses, a reload gives the same tree).Open PRs that touch the same outputs. #38763 changes what
bun update --latestprints for moved catalog entries; this PR pins today's "1 package installed" in three tests, so whichever lands second updates three inline snapshots. #38741 edits the deleted.snap(lockfileVersion 2 to 1); the one inline lockfile snapshot here pins"lockfileVersion": 2and moves with it. #38849 edits other parts of this file.Behavior pinned as it is today:
bun update no-deps --latestfrom a workspace whose dependency iscatalog:changes nothing ("Checked 3 installs across 4 packages (no changes)", no lockfile save). The test already named this "keeps the catalog reference". The output is now pinned.>=1.0.0->^2.0.0->>=1.0.0, bun.lock dedupes again but the nestedpackages/lib/node_modules/no-depscopy from the middle state stays on disk. The test checks the lockfile at that point, as before, not the layout.npm:no-deps@>=1.0.0inlib),app's"no-deps": "1.0.0"gets no lockfile entry and the rootnode_modules/no-depsis 2.0.0. This is independent of catalogs (the inline peer does it too). The test only compares catalog against inline, as before.Deleted
test/cli/install/__snapshots__/catalogs.test.ts.snap. Its three lockfile snapshots are now theregistryEntrytable plustoEqualin "detect changes", and the full-text inline snapshot in "both catalog and catalogs".Windows:
nodeModulesPackagesdoes not descend into junctions or symlinks (Bun reports reparse points asisSymbolicLink(),src/runtime/node/dir_iterator.rs:716), so the layouts are the same on every platform.[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file