Conversation
The file installed express and lodash from the public npm registry, four installs per test, hidden behind a 5 minute default timeout, and only asserted exit codes and one version number. It now uses the verdaccio fixtures (no-deps, one-dep, one-range-dep, a-dep), runs the seven scenarios concurrently with a per-test install cache, and asserts the install output, the installed tree, the frozen-lockfile error text, the workspace resolution and lockfile stability on the bun.lock text. The frozen-lockfile scenario previously changed the override to bytes@1.0.1, a version that does not exist, so its non-zero exit came from a resolution failure rather than from the frozen-lockfile check. It now switches between two published versions and checks the frozen-lockfile error itself. The file no longer needs its LeakSanitizer exemption.
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: test-only change, ready for review.
|
There was a problem hiding this comment.
LGTM — test-only hermeticity refactor following the established VerdaccioRegistry pattern.
What was reviewed:
- All 7 original scenarios are preserved with strictly stronger assertions (stdout/stderr/tree snapshots vs. a single
versionfield); the frozen-lockfile test now asserts the actual frozen-lockfile error instead of a coincidental resolution failure. - Verified the referenced fixtures (
no-deps1.0.0/1.0.1/1.1.0/2.0.0,one-dep,one-range-dep,a-dep1.0.1–1.0.10) exist intest/cli/install/registry/packages/and thatcreateTestDir/nodeModulesPackagesin harness match the usage here. test.concurrentisolation looks safe: each test gets its own tempDir andBUN_INSTALL_CACHE_DIRoverride, so concurrent installs don't share a cache even when CI exports that variable.no-validate-leaksan.txtremoval is a de-suppression (strictly safer); no production code touched.
Extended reasoning...
Overview
This PR rewrites test/cli/install/overrides.test.ts to use the local VerdaccioRegistry harness instead of hitting the public npm registry, and removes the file from test/no-validate-leaksan.txt. No production code is modified. All 7 existing test scenarios are kept, converted to test.concurrent, and re-expressed against small local fixtures (no-deps, one-dep, one-range-dep, a-dep) that already exist in the registry fixture directory. Assertions are upgraded from single version field reads to full stdout/stderr inline snapshots plus nodeModulesPackages tree snapshots, and the lockfile-stability helper now captures the lockfile before the follow-up install so a rewrite would actually be caught.
Security risks
None. This is a test-infrastructure change with no runtime, auth, crypto, or network-facing production code touched. It actually removes a public-network dependency from CI.
Level of scrutiny
Low-to-medium. Test-only, follows the exact pattern used by every other file in test/cli/install/ (VerdaccioRegistry + createTestDir + normalizeBunSnapshot + nodeModulesPackages). The main risks for a change like this are (a) losing coverage, (b) introducing flakiness via test.concurrent, or (c) snapshots that certify wrong behavior. I checked each: coverage is a strict superset (the frozen-lockfile test previously passed for the wrong reason and now asserts the real error message); concurrency is isolated per-test via separate tempDirs and an explicit BUN_INSTALL_CACHE_DIR override; and the snapshot contents in the diff match what the fixtures should produce (e.g. no-deps@1.0.0 under override, no-deps@1.0.1 when the override is removed since one-dep depends on 1.0.1).
Other factors
- I verified the fixture packages and versions referenced in the PR exist on disk exactly as described.
- The
progressLineregex handles both LF and CRLF and is applied beforenormalizeBunSnapshot, so Windows output should normalize correctly. - Removing the file from
no-validate-leaksan.txtre-enables LSAN for it — a de-suppression, so if it's wrong CI will simply flag it rather than hide anything. - The 5-minute
setDefaultTimeoutremoval and dropping the--forcere-download step are both justified in the PR description and consistent with the repo's test guidelines (default timeout, no public network). - No prior human reviews or outstanding comments on the PR.
|
Re-verified on main at d6af50f (633 commits after this branch's base). The file in this PR passes unchanged with The failures on Buildkite build 98009 are in other files ( |
Problem
test/cli/install/overrides.test.tsinstalledexpress@4.18.2(68 packages) orlodashfrom the public npm registry in 6 of its 7 tests, then ran three more installs per test (install,--frozen-lockfile,--force; the last one re-fetches the tree).test/cli/installnot using the local registry. Build 97275 measured it at 26s on the Windows arm64 lane and between 0.4s and 3.3s elsewhere, the spread being network and cache warmth;setDefaultTimeout(5 minutes)at the top of the file hid that.install()helper inherited stdout/stderr, so nothing bun printed was asserted; each test checked oneversionfield.bytes@1.0.1, a version that does not exist on npm, so the non-zero exit it checked came fromerror: No version matching "1.0.1" found for specifier "bytes", not from the frozen-lockfile check (output below).ensureLockfileDoesntChangeOnBunIreadbun.lockonly after an extrabun install, so a lockfile rewritten by that install would not have been noticed.Fix
VerdaccioRegistryinbeforeAlland uses fixtures that already exist intest/cli/install/registry/packages, so no fixtures are added:no-deps(1.0.0, 1.0.1, 1.1.0, 2.0.0) playsbytes/lodash,one-dep(depends onno-deps@1.0.1) andone-range-dep(no-deps@^1.0.0) playexpress,a-depis thenpm:alias target and the package an override must leave alone.npm:specifier, changed override vs--frozen-lockfile, override removed, workspaces) and run astest.concurrent. Each test owns its directory and itsBUN_INSTALL_CACHE_DIR(CI exports that variable, so the per-test bunfigcachealone would leave concurrent tests sharing one cache).stdoutandstderrof the install under test asnormalizeBunSnapshotinline snapshots (the two progress lines are stripped; their task count is not what the file tests).nodeModulesPackages, one snapshot per state, covering the overridden package and the packages that must not move (e.g.node_modules/no-deps/a-dep@1.0.1next tonode_modules/one-dep/one-dep@1.0.0).error: lockfile had changes, but lockfile is frozenplus theoverrides in package.json changed since bun.lock was savednote), and thatbun.lockandnode_moduleswere left alone.no-depsback to1.0.1(the versionone-depasks for) and drops"overrides"frombun.lock.node_modules/pkg1is the1.1.1workspace and snapshots the wholebun.lock:pkg1@workspace:packages/pkg1, override recorded, nopkg2entry.expectLockfileStablecaptures thebun.locktext right after the install under test, then requires--frozen-lockfileto pass with empty stderr, a plainbun installto print nothing to stderr (noSaved lockfile), and the text to be unchanged (toBeon the text, so a failure prints a line diff). The--forcestep is dropped: it was the re-download, and the re-resolution it exercised is what the "set later" and "reset when removed" scenarios assert directly.setDefaultTimeoutis gone. The file is also removed fromtest/no-validate-leaksan.txt: it passes with the CI LeakSanitizer settings (BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1,test/leaksan.supp), and aLSAN_OPTIONS=verbosity=1probe confirmed LSAN runs inside the spawned installs.src/install/PackageManager/PackageManagerEnqueue.rs:709-747), so each scenario needs one package with several published versions plus a parent depending on it; the express tree only added network traffic.bun bd test test/cli/install/overrides.test.ts: 7 pass, 26 snapshots. Timings on this machine (public registry reachable through a proxy):bun bd test(debug, ASAN), cold install cachebun bd test, warm install cacheHTTP(S)_PROXYpointed at a closed port)ConnectionRefused downloading package manifest expresstest/expected-durations.json) the new file will be somewhat slower because of that fixed cost; the gain is on cold or slow-network lanes, and in not depending on the network at all.Background
VerdaccioRegistry(test/harness.ts) forks a verdaccio server on a random port that serves the packages checked in undertest/cli/install/registry/packages. Itsverdaccio.yamlhas the npm uplink proxy commented out, so a package missing from that directory is a 404, never a network request.createTestDirmakes atempDirwhosebunfig.tomlpointsinstall.registryat the server andinstall.cacheinside the directory.overrides(npm) /resolutions(yarn) in the rootpackage.jsonreplace the version that every dependency edge to a given name asks for. bun applies them while enqueueing each dependency, skipping workspace edges and explicitnpm:aliases, and writes them intobun.lockso that--frozen-lockfilecan tell when they changed.nodeModulesPackages(dir)from the harness lists everypackage.jsonfound undernode_modulesaspath/name@version, so one snapshot shows what each name resolved to and whether any nested copies were installed.Frozen-lockfile test on main: resolution error, not a frozen-lockfile error
Old file with the public registry unreachable