test(install): pin both no-deps versions instead of racing a range against a pin - #38858
test(install): pin both no-deps versions instead of racing a range against a pin#38858robobun wants to merge 2 commits into
Conversation
…k tests The three peer tests added with the bun.lock peer-binding fix built their two-version no-deps graph from normal-dep-and-dev-dep (no-deps@1.0.1) plus two-range-deps (no-deps@^1.0.0). Whether the range adds no-deps@1.1.0 or dedupes onto the 1.0.1 pin depends on which manifest the registry answers first, so "ranged peer dependency resolution is stable across installs from bun.lock" failed whenever 1.0.1 was appended first (the store entry is then keyed +f8a822eca018d0a1 instead of +7ff199101204a65d), the optional peer test sometimes ran with a single no-deps version, and the aliased peer test never had its 1.1.0 decoy at all because the plain range was redirected to the workspace's no-deps -> a-dep alias. Add the second copy as a root "nd11": "npm:no-deps@1.1.0" alias instead, the way the overridden peer test in the same group already does. Two exact pins are both appended whatever the arrival order, and the saved tree keeps the shape the tests are about: 1.0.1 at the root no-deps path, 1.1.0 under nd11, no path of its own for the peer edge. Each test now asserts that shape from bun.lock before looking at the store.
|
Warning Review limit reached
Next review available in: 1 minute 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 |
|
Updated 9:42 AM PT - Aug 15th, 2026
✅ @robobun, your commit 053811131734e8aa89acdab44ff21c2978f0992e passed in 🧪 To try this PR locally: bunx bun-pr 38858That installs a local version of the PR into your bun-38858 --bun |
|
Status: ready for review. Reproduced on main two ways: the unmodified "ranged peer dependency resolution is stable across installs from bun.lock" test fails 2 of 20 debug-build runs here with the Self-review turned up one more test built on the same pin-plus-range pair, The resolver side of this (a range settling on a sibling's pin depending on arrival order) is #38832; this PR only makes the tests independent of it. The previous build's only hard failures were two |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake that pins both no-deps versions so the graph shape no longer depends on registry response order.
What was reviewed:
- The three modified tests still assert the same peer-binding invariant (same
+7ff1.../+f8a8...store hashes, same resolved versions on fresh vs. bun.lock reinstall) — the property is preserved, not weakened. - The
nd11: npm:no-deps@1.1.0construction matches the existing fourth sibling test at line ~1357;no-deps@1.1.0is present in the Verdaccio fixtures. savedTreeCopiesOfusesBun.JSONC.parse(correct for bun.lock) and existingfile/joinimports; thestartsWith(${name}@)filter can't over-match here because the graphs contain no otherno-deps*package and thetoEqualon the full map would fail loudly if it did.- Checked that the aliased-peer test's decoy (1.1.0 satisfying
^1.0.2) is now actually present, and the optional-peer test now always has both versions — both are strengthened, not just stabilized.
Extended reasoning...
Overview
Test-only change to test/cli/install/isolated-install.test.ts fixing a flaky test (and two siblings that share the same construction). The three "... across installs from bun.lock" tests previously pulled in the second no-deps version through two-range-deps' no-deps@^1.0.0 range, which the resolver may or may not dedupe onto the exact 1.0.1 pin depending on which manifest the local registry answers first. The fix replaces that range with a direct "nd11": "npm:no-deps@1.1.0" root dependency — two exact pins that cannot dedupe onto each other regardless of arrival order — and adds a small savedTreeCopiesOf helper so each test asserts the bun.lock tree shape it depends on before checking store entries.
Security risks
None. No production code is touched; only test package.json fixtures and assertions change. The tests continue to run against the local Verdaccio registry.
Level of scrutiny
Low-to-moderate. The main REVIEW.md concern for de-flakes is "keep asserting the property the original assertion protected" — verified: the tests still check the exact same store-entry hashes (peer-deps-fixed@1.0.0+7ff199101204a65d, one-optional-peer-dep@...+f8a822...) and the same resolved no-deps versions after reinstalling from bun.lock. The change swaps how the second version enters the graph, not what is asserted about the peer edge. The new premise assertions make the tests strictly stronger.
Other factors
- The
nd11: npm:no-deps@1.1.0pattern is not novel — the fourth test in the same group ("overridden peer dependency keeps the override ...") already uses it, so this brings three siblings into line with the fourth. - The PR description includes a forced-arrival-order matrix demonstrating the old graphs are order-dependent and the new ones are not, plus 10/10 repeated debug-build passes vs. 2/20 failures on main.
- The description also notes the underlying resolver order-dependence is being fixed separately (#38832) and explains why these tests should be independent of either dedupe policy — the reasoning is sound.
- The aliased-peer test is actually fixed (its 1.1.0 decoy was never present under the old construction because
two-range-deps' plainno-depsdep was captured by the workspace'snpm:a-depalias), and the optional-peer test now always exercises the two-version case it describes. Bun.JSONC.parseand thepackages: Record<string, [resolution, ...]>bun.lock shape are both established;fileandjoinare already imported at the top of the file.
"update should update all packages in the current workspace" installs one-fixed-dep@1.0.0 (pins no-deps@1.0.0) next to two-range-deps (no-deps@^1.0.0), the same pin-plus-range pair as the isolated peer tests, and masked the resulting 19-or-20 package count with an alternation whose comment blamed optional-native. The optional-native entries are identical in every run; the extra package is no-deps@1.1.0, present only when the range was resolved before the pin was appended. Use one-fixed-dep@^2.0.0 instead: its no-deps@2.0.0 pin is outside the range's major, so every install has the same two copies of no-deps and the test can assert "20 packages installed" outright.
robobun
changed the title
There was a problem hiding this comment.
LGTM — well-researched test-only flake fix that pins both no-deps versions so the peer-binding tests no longer depend on manifest arrival order.
What was reviewed:
- Confirmed
one-fixed-dep@2.0.0(pinsno-deps@2.0.0) andno-deps@1.1.0exist in the local Verdaccio fixtures, so the new graphs resolve. savedTreeCopiesOfcorrectly parsesbun.lockviaBun.JSONC.parseand filters byname@prefix; the new premise assertions match the described tree shape.- The
nd11: npm:no-deps@1.1.0construction mirrors the existing "overridden peer" sibling test; two exact pins can't dedupe onto each other regardless of arrival order. - The
19|20 → 20tightening in the registry test: the prior comment blamedoptional-native, but platform-optional counts are deterministic per machine — the dedupe race explains the run-to-run variance, andno-deps@2.0.0sits outside^1.0.0so the range always adds its own copy.
Extended reasoning...
Overview
Test-only change across two files. In isolated-install.test.ts, three peer-resolution stability tests replace two-range-deps (which pulls no-deps@^1.0.0) with a root nd11: npm:no-deps@1.1.0 alias so both no-deps copies are exact pins, and add a savedTreeCopiesOf helper that asserts the lockfile tree shape before checking store entry hashes. In bun-install-registry.test.ts, the workspace-update test bumps one-fixed-dep from ^1.0.0 to ^2.0.0 (moving its transitive no-deps pin from 1.0.0 to 2.0.0, outside two-range-deps' ^1.0.0 range) and tightens the package count from 19|20 to 20.
Security risks
None. No runtime code is touched; only test fixtures and assertions against a local Verdaccio registry.
Level of scrutiny
Low-to-medium. This is a de-flake of existing tests with an unusually thorough root-cause writeup: the PR reproduced both outcomes deterministically by forcing manifest arrival order with a proxy, and verified the new graphs are order-independent in the same harness. The construction copies the pattern already used by the fourth sibling test ("overridden peer dependency keeps the override"). I verified the referenced fixture versions exist in test/cli/install/registry/packages/.
Other factors
- The new
savedTreeCopiesOfpremise assertions are a strict improvement per REVIEW.md ("assert that setup created the precondition") — a future resolver change that reshapes the tree now fails on the premise, not on an opaque peer-hash mismatch. - The
19|20 → 20tightening replaces a comment that attributed variance tooptional-native. Platform-gated optionals are deterministic per host, so they can't explain intermittent same-machine failures; the dedupe race does. Withone-fixed-dep@2.0.0the only same-major sibling fortwo-range-deps' range is gone, so the count is stable. - The aliased-peer test now actually contains the
no-deps@1.1.0decoy it was written to guard against — previouslytwo-range-deps' range was redirected to the workspace'snpm:a-depalias and the decoy was never present, so the test was weaker than intended. - No prior human review comments to address; the resolver-side order dependence is tracked separately in #38832 and this change is neutral to either outcome there.
…y.createTestDir createTestDir now returns the env that pins BUN_INSTALL_CACHE_DIR to the project's own .bun-cache (the hunk #38255 proposes), so the file-local wrapper goes. install() takes (env, cwd) like runBunInstall. The ranged peer case pins no-deps@1.1.0 through the nd11 alias instead of reaching it through two-range-deps' range, as #38858 does.
1 participant
Problem
test/cli/install/isolated-install.test.ts> "ranged peer dependency resolution is stable across installs from bun.lock" fails intermittently on main (2 of 20 runs of the unmodified test with a debug build here; Deterministic dependency resolution: an ordered walk over a live tree #36476 also lists it as failing on the released binary):no-depsin the graph and gets them fromnormal-dep-and-dev-dep(depends onno-deps@1.0.1) plustwo-range-deps(depends onno-deps@^1.0.0). Whether the range addsno-deps@1.1.0or dedupes onto the 1.0.1 pin depends on which of the two manifests the registry answers first:Lockfile::get_package_id(src/install/lockfile.rs:2061) lets a range settle on an already appended exact pin of the same major. When 1.0.1 is appended first there is oneno-deps, the peer binds to it, and the store entry is keyed+f8a822eca018d0a1.normal-dep-and-dev-depfirst gives{ "no-deps": "no-deps@1.0.1" }and the+f8a8...entry (the failure above);two-range-depsfirst gives an extratwo-range-deps/no-deps: no-deps@1.1.0and the+7ff1...entry (the pass).no-deps, so the case it describes (a higher version present that a version scan would pick) is not exercised. "aliased peer dependency binds to its real package" never has its 1.1.0 decoy in either order: the workspace declares"no-deps": "npm:a-dep@^1.0.2", sotwo-range-deps' plainno-deps@^1.0.0is redirected to that alias (known_npm_aliases,src/install/PackageManager/PackageManagerEnqueue.rs:677), and the only realno-depsin its graph is 1.0.1, which does not satisfy^1.0.2.test/cli/install/bun-install-registry.test.ts> "update should update all packages in the current workspace":one-fixed-dep@1.0.0(depends onno-deps@1.0.0) next totwo-range-deps, with the result masked as/(?:20|19) packages installed/under a comment blamingoptional-native. With a debug build here the count was 19 in 7 of 10 runs (bun.lockhasno-deps@1.0.0only) and 20 in 3 (no-deps@1.1.0as well); theoptional-nativeentries were identical in every run, so the alternation is this race, and the comment is wrong.Fix
"nd11": "npm:no-deps@1.1.0"dependency instead of throughtwo-range-deps. This is how the fourth test of the group ("overridden peer dependency keeps the override ...") already builds the same graph.npm:dependency is not subject to alias redirection. The tree the tests are about keeps its shape: 1.0.1 is still hoisted to the rootno-depspath, 1.1.0 sits atnd11, and the peer edge still gets no path of its own, so the fresh resolve binds it to 1.1.0 by version scan (the same+7ff199101204a65dentry as before) while re-deriving it from the saved paths would still give 1.0.1. In the aliased test the decoy a wrong-name lookup would find (no-deps@1.1.0satisfies^1.0.2) is now actually present.bun.lock(savedTreeCopiesOf) before looking at the store, so a resolution change shows up as a premise failure rather than as a different peer hash.bun.lockloader does with a given tree, so they should not encode either dedupe policy: with install: resolve a range onto an existing version only when every install has it #38832 the range would always add its own copy, with Deterministic dependency resolution: an ordered walk over a live tree #36476's ordered walk it would always dedupe. Pinning both copies keeps them correct under both.bun updatetest,one-fixed-depbecomes^2.0.0: itsno-deps@2.0.0pin is outside the range's major, so every install has the same two copies ofno-depsand the assertion is a plain20 packages installed.one-fixed-dephas no other role there (it is one of thirteen dependenciesbun updateleaves at their range), so the five copies of the list and its+ one-fixed-dep@2.0.0line move with it.bun bd test test/cli/install/isolated-install.test.ts: 66/66 pass; the five... across installs from bun.locktests pass in 10 of 10 repeated debug-build runs.+f8a822eca018d0a1listing above.bun bd test test/cli/install/bun-install-registry.test.ts: the update test passes 8 of 8 repeated debug-build runs (the unmodified one printed 19 in 7 of 10); the full file passes apart from one run of the unrelatedmanifest conditional requestsetag test, which passes alone and in its own block and which install: replace an existing manifest cache entry atomically instead of unlinking it first #38916 already references.bun.lockcopies and the same store entries in both arrival orders; the old ones do not.Background
<name>@<version>, plus+<hash>of the package's peer bindings when it has peers, so the entry name says whichno-depsa peer was bound to:+7ff199101204a65disno-deps@1.1.0,+f8a822eca018d0a1isno-deps@1.0.1.bun.lock'spackageskeys arenode_modulespaths of the hoisted tree (also with the isolated linker). install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182 fixed the loader re-deriving peer edges from those paths; a peer edge satisfied by the version at an enclosing path gets no path of its own, which is why that walk lands on the hoisted version."nd11": "npm:no-deps@1.1.0"installsno-deps@1.1.0under the namend11; in the lockfile it is ano-depspackage, so the peer's scan overno-depsversions sees it. Separately, a plain dependency whose name matches an alias declared elsewhere in the tree, and whose range accepts the alias's version, is resolved to the alias target instead; that is whattwo-range-deps' range hit in the aliased test.Forced arrival orders (release 1.4.0; proxy holds the named manifest until the other parent's
no-depsrequest was served). Columns: copies of no-deps in bun.lock, store entry of the package with the peer; the reinstall from bun.lock produced the same entry in every row.