Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe pull request expands ChangesTransitive update test coverage
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review at bcf2e17 (the diff is 33aea72 plus an empty CI rerun commit). Nothing is left to do on this side. CI: the file itself and the three other callers of the changed harness code passed on every lane in builds 102381 and 102393. Both builds are red only because of How the slowness was measured: |
![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. It's a sizable test-infrastructure refactor (a new copyOf/copyTree memoization layer with hand-written cross-platform symlink handling, plus ~30 tightened stdout assertions), so a human look would still be worthwhile.
What was reviewed:
copyOfmemoization: get/set ontemplateshas no await between them, so concurrenttest.concurrentcallers can't race; recipes build on a copy fromsetup(), so shared templates are never written after their promise resolves.copyTree:.bun-cacheis skipped andrun()already pointsBUN_INSTALL_CACHE_DIRat the copy; absolute symlinks are re-targeted into the copy viarelative(template, target), relative ones kept verbatim.- Recipes that read post-build state (
staleMemberEdgestexts/lockBefore,stalePatchedpatched-file handle) do so on the per-test copy, not the shared template. resolvedEdgesslice arithmetic andexpectRowSetAndcopy-before-sort look correct.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bun-update-transitive.test.ts, with two goals: (1) memoize the ~55 distinct starting trees so that 174 concurrent test cases share installs instead of each spawning their own bun install subprocesses (594 → 410 processes), and (2) tighten stdout assertions on ~30 bun update runs that previously only checked stderr or a subset of the output. The mechanism is a new copyOf(key, layout, build) helper backed by a Map<string, Promise<string>> and a hand-written copyTree that recursively copies a built tree, skipping .bun-cache and re-pointing absolute symlinks (Windows workspace/store links) at the copy. Nine recipe helpers (stale, staleSiblings, stalePatched, staleMemberTransitive, staleMemberEdges, staleShared, staleAutoInstalledPeer, staleScoped, staleAlias, stalePeerEntry, staleMemberGroups) are wrapped in copyOf, and setup() itself is memoized. Several assertion helpers are added or generalized (expectRowSetAnd, expectCountLine, expectPicked, expectKeptPatched, resolvedEdges), expectHeaderOnly is moved up and applied to twelve error paths, and two identical patched-test bodies are folded into one helper.
Security risks
None. This is test-only code that spawns bun against a local Verdaccio registry in temp directories. No production code, no network egress, no auth/crypto surface.
Level of scrutiny
Medium-high for a test-only change. The diff is ~400 lines and introduces a new sharing pattern that every subsequent case in this file depends on. The correctness of copyTree (particularly the Windows absolute-symlink retargeting via join(copy, relative(template, target)) and the .bun-cache skip) and of the memoization keying (JSON-stringified [recipeName, ...params, bunfigOpts]) determine whether copies are truly isolated from their templates. I traced each wrapped recipe to confirm its build() closure operates on a fresh copy (via the inner setup() call, which itself returns a copy) and that nothing writes to the memoized directory after the promise resolves. I also confirmed run() already sets BUN_INSTALL_CACHE_DIR to join(dir, ".bun-cache") per copy, so skipping the template's cache is safe. The get-then-set on the templates map is synchronous, so test.concurrent callers can't double-build.
Other factors
The PR description is unusually thorough — it documents timing methodology on both Linux ASAN and Windows, explains why fs.cpSync was rejected (4× slower under debug, and its default symlink resolution would leave a Windows copy pointing into the shared tree), and enumerates every assertion change. The author reports 174/174 passing across 7 Linux and 3 Windows runs. That said, the strengthened assertions encode exact count-line semantics (installed(N), noChanges(installs, packages)) derived from install_with_manager.rs internals, and the new copyTree is bespoke filesystem code with platform branches — both are the kind of thing a maintainer familiar with the install output format and Windows CI behavior should sign off on rather than an automated approval.
|
The two areas the review points a human at are documented in the notes of the description:
|
e0e676b to
4835ed0
Compare
|
Updated 12:03 AM PT - Aug 21st, 2026
❌ @robobun, your commit bcf2e17 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39741That installs a local version of the PR into your bun-39741 --bun |
|
Pushed a reworked version (4835ed0), after a self-review of the first one. What changed:
Re-verified: 174 pass on Linux (interleaved with main: 27.3s, 35.4s, 28.7s against 24.4s, 29.8s, 25.1s on a loaded host) and on Windows x64 (14.7s against 18.0s), also with |
…h fs.cpSync
makeTreeSync, and so tempDir and createTestDir, copied a folder with
fs.cpSync. That keeps an absolute link pointing into the folder it was
copied from, and resolves a relative one against it, so a copy of an
installed project was not independent of the original. copyTreeSync
recreates each link instead: a relative target is kept, and an absolute
target inside the source is pointed at the copy. On Windows a directory
link with an absolute target is recreated as a junction, which needs no
privilege, like the fallback bun install itself uses.
The three existing callers copy fixture folders without links and are
unaffected. This lets a test install a tree once and give every case a
copy of it through createTestDir({ files: tree }).
Most cases of the file start from one of a few dozen trees, and each case installed its own: 303 of the 594 bun processes the file spawned were those installs. setup() and the recipes several cases start from (stale, staleSiblings, staleMemberEdges, staleShared, the patched trees and the rest) now build their tree once, in the first case that asks for it, and give every case a copy of it through createTestDir. The tree is never written to after it is built. Helper signatures and return shapes do not change. The file now spawns 410 processes. With a debug ASAN build, which runs 5 cases at a time, it takes about 24s instead of 27s to 28s.
…te-transitive About 30 cases did not read the stdout of the bun update they ran. Each run now asserts the exact summary where one row moves, the row set plus the count line where two packages new to bun.lock move (a real run prints those in package id order, so the order is not pinned), the header alone on every error path, and the exact "Checked N installs across M packages" line on every no-op. Several cases also pin which version each dependent's edge resolves to and what is installed. The two cases that #38770 rewrites are left as they are, so that PR and the other open PRs on this file still merge on top of this one.
4835ed0 to
33aea72
Compare
|
Rebased onto main (33aea72, no changes to the diff). The only non-flaky failure in build 102365 was the binary size check: it reported every target 2 MB over the canary, because the canary already has #39770 (the 2 MB trim) and this branch was based on a main from before it. This PR touches |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a large test refactor (~600 lines) that also changes shared harness behavior — makeTreeSync now uses a hand-written copyTreeSync with symlink/junction relocation instead of fs.cpSync, and the test file adopts a memoized-template-tree pattern under test.concurrent. Given the scope and the shared-infrastructure change, a human look would still be worthwhile.
What was reviewed:
copyTreeSyncsymlink handling: relative links kept, absolute links insidesourceretargeted viarelative(realpathSync.native(source), target); Windows absolute dir links become junctions. The three existing string-path callers ofmakeTreeSync(the migration tests) copy link-free fixtures, so no behavior change there;patch.test.tsonly passes object trees.copyOfmemoization undertest.concurrent: the promise is stored in the map before any await, so concurrent callers share one build; each test then gets an independentcreateTestDircopy, andwriteBunfigoverwrites the copiedbunfig.tomlso the copy's cache path points at itself. Recipes that callsetup()inside their owncopyOfwrite to a copy of the setup template, not the template itself.- Assertion changes strengthen coverage (exact summary/count lines,
expectHeaderOnlyon error paths) and no test is removed, skipped, or has an assertion weakened.
Extended reasoning...
Overview
Two files: test/harness.ts gains copyTreeSync (replaces fs.cpSync inside makeTreeSync's string-path branch) and test/cli/install/bun-update-transitive.test.ts is refactored so each of its ~dozen recipe helpers builds its starting tree once and hands every test case an independent copy. About 30 cases that previously ignored bun update stdout now assert it exactly.
Security risks
None. Test-only; no production code, no network, no auth/crypto surface.
Level of scrutiny
Medium. It's test infrastructure, so a mistake surfaces as a failing or (worse) vacuous test rather than a runtime bug. But two aspects deserve human eyes: (1) copyTreeSync is now the copier for every tempDir(prefix, path) call repo-wide and its Windows junction/symlink relocation logic is platform-sensitive, and (2) the memoized-template pattern is a new idiom that concurrent tests share — the author's argument that templates are never written after their promise resolves checks out, but it's the kind of invariant a maintainer should sign off on before it spreads to other test files.
Other factors
- The diff is ~380 insertions / ~220 deletions across 174 test cases; not mechanical enough to auto-approve.
- Dozens of new exact count-line assertions (
noChanges(2, 3),noChanges(3, 5),installed(1)) each encode a specific reading ofinstall_with_manager.rsoutput semantics. The PR notes explain the non-obvious ones (bundled deps, workspace links,--filter), and all 174 pass on Linux and Windows per the author, but a maintainer confirming those readings once is cheap insurance. - No prior human review on the thread; the earlier self-review already reshaped the PR (moved the copier into the harness, dropped changes that would conflict with open PRs).
|
On the two points the review leaves for a human: the folder form of |
|
Both automated reviews of the rebased version came back without findings, and no review thread is open. The remaining item is build 102393, which reruns CI after build 102381 failed only on files this PR does not touch (bun-server.test.ts on Windows 2019, which also fails on main and is reported, plus three retry-passing flakes). |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a ~600-line refactor that changes shared harness behavior (makeTreeSync now uses a hand-rolled copyTreeSync with symlink relocation and Windows junction handling) and hard-codes many exact count-line values across 174 concurrent tests, a human look would still be worthwhile.
What was reviewed:
- Template sharing via
copyOf: no await betweentemplates.get/set, build resolves before any copy starts, every caller writes only to its owncreateTestDircopy (including nestedsetup()inside recipe builds). copyTreeSync: absolute-link relocation usesrealpathSync.native(source)as the base, relative links kept as-is, dangling links fall through to"file";createTestDiroverwrites the copiedbunfig.tomlso the copy's cache path points at itself.- No assertions weakened — removed
.not.toContainchecks are subsumed by exactexpectSummary;expectRejected's dropped return value has no consumers; the removedplannedflag on the auto-installed-peer case is a strengthening.
Extended reasoning...
Overview
Test-only refactor of test/cli/install/bun-update-transitive.test.ts (174 cases) plus a harness change in test/harness.ts. Three separable pieces: (1) copyTreeSync replaces fs.cpSync inside makeTreeSync, recreating symlinks so an installed tree can be copied independently of its original; (2) a module-level templates map memoizes each starting tree so it's installed once and copied per test via createTestDir; (3) ~30 cases gain exact stdout assertions (expectSummary, expectRowsAnd, expectRowSetAnd, expectHeaderOnly, expectPicked) and several helpers are extracted or generalized (expectKeptPatched, resolvedEdges, stalePatched).
Security risks
None. Test-only; no production code, auth, or network surface touched.
Level of scrutiny
Medium-high for a test change. The harness edit reaches every caller of tempDir/tempDirWithFiles/createTestDir that passes a string path (three existing callers, all verified per the notes), and the symlink-relocation logic has platform-specific branches (junction vs. "dir"/"file" symlinks on Windows). The template-sharing model changes the isolation contract of 174 concurrent tests — correctness depends on every recipe writing only to the copy it receives, which I traced through setup, stale, staleSiblings, stalePatched, staleMemberTransitive, staleMemberEdges, staleShared, staleAutoInstalledPeer, staleScoped, staleAlias, stalePeerEntry, and staleMemberGroups. The many new hard-coded count lines (noChanges(2, 3), noChanges(3, 4), noChanges(2, 4), etc.) are each traceable to install_with_manager.rs:1170 semantics as the description documents, but a maintainer should confirm those match intent.
Other factors
The PR is unusually well-documented (every count line, every assertion change, Windows probe results, timing methodology) and reports 174/174 passing on Linux debug/ASAN and Windows x64 in both symlink modes. No prior human or claude[bot] review on the thread. The bug-hunting pass found nothing. I checked that expectRejected's dropped return is unused, that [...rows].sort() in expectRowSetAnd doesn't mutate the caller's array, and that templates.get/set in copyOf has no await between them so concurrent test.concurrent callers can't double-build. The change is sound as far as I can trace, but its size and the harness-wide reach put it outside the "simple/mechanical" bar for auto-approval.
|
One point on the many exact count lines, since each review pass mentions them: every case builds its line through |
|
Final state from this side: build 102393 (the rerun) is red for the same reasons as 102381, that is |
…like copyTreeSync shared() takes the bunfig options of its tree and writes the copy's bunfig through VerdaccioRegistry.writeBunfig instead of patching the copied one. packageTree and workspaceTree build the trees the way setup, setupWithLinker and setupWorkspaces do, from one helper each. copyTree recreates a relative link with its own kind and only an absolute directory link as a junction, the same as the harness's copyTreeSync in #39741, so the two cannot drift apart.
This branch has not been deployed
1 participant
Problem
bun-update-transitive.test.tstakes 19s on the x64 ASAN lane (build 101560). Under ASANbun testruns 5 cases at a time (context.rs:506), so the file takes its total per-case work divided by 5.bunprocesses, 303 of them installs of a starting tree, and most cases start from the same few dozen trees. The harness can not share one:makeTreeSyncusesfs.cpSync, which leaves the linksbun installwrites pointing into the original.bun updateunder test.Fix
test/harness.ts:copyTreeSyncreplacesfs.cpSync. A relative link is kept, an absolute link into the source is pointed at the copy. SocreateTestDir({ files: tree })is an independent copy of an installed tree, cache included.setup()and the recipes build each tree once and give every case such a copy. Helper signatures do not change. A case writes only to its copy. A tree is never written to after its promise resolves.bun updaterun asserts its stdout (list in the notes).Background
stale()installs a tree, then re-installs it with a widened package.json, which leaves a transitive row forbun updateto move.bun installwrites absolute links for workspace members and store entries on Windows, and for the cache's index everywhere. A Windows directory link is recreated as a junction, which needs no privilege, as bun's own fallback does.print_transitive_updates).--dry-runsorts. SoexpectRowSetAndcompares two such rows as a set.Notes
Self-review. A review of the first version of this PR (a copier local to this file, which also left the cache behind) made two points that this version takes up. The copier is a harness matter:
tempDir(prefix, folder)andcreateTestDir({ files: folder })already exist for copying a folder, theirfs.cpSyncis what made them unfit for an installed tree, and #39742 was about to add a second, different copier to bun-dedupe.test.ts for the same reason. So commit 1 fixes the harness and commit 2 is its first consumer. And the assertion commit conflicted with #38770 in the two cases that PR rewrites ("a sibling whose range rejects the picked version" and "several names in one command"), so those two are left as they are on main. Afterwardsgit merge-treemerges #38770, #38901, #38919, #39531 and #39742 cleanly onto this branch, both onto commit 2 and onto the whole PR. #39165 conflicts with main itself inmordant-baseline.tomlandpack_command.rs, not with this branch. The review also suggested removingexpectMoved,expectNoMovesandexpectNoChangesLineand givingexpectNoopa required count line. Not done: #38770 and #38901 add cases that call them with the current signatures, so that would break those PRs for no gain in the cases this PR touches (expectNoopreturns its stdout instead, and the callers here pin the line).copyTreeSync. The three existing callers of the folder form (
migration/migrate.test.ts:728,migration/pnpm-migration.test.ts:185,migration/pnpm-lock-v9.test.ts:35) copynpm-arboristandpnpmfixture folders that hold regular files and directories only (checked withfind), and all three pass with the new copier (125 pass and 1 todo, 3 pass and 11 todo as on main, 81 pass;pnpm-lock-v9also on Windows).copyFileSynckeeps the mode, ascpSyncdid.relative()is taken againstrealpathSync.native(source)because bun writes resolved targets. A link is recreated with the kind of its target (statSyncon the source link),"file"if it dangles. On Windows a probe showed the workspace link of a hoisted tree is absolute and the store links of an isolated tree are relative on a runner with the symlink privilege, and withBUN_FEATURE_FLAG_FORCE_WINDOWS_JUNCTIONS=1every link is an absolute junction thatreaddirSyncreports as a link. In both modes the copy's links point into the copy andbun install --frozen-lockfilein the copy reports no changes for both linkers, and the whole file passes in both modes (174 pass, twice in normal mode and once with junctions forced, on this version).Cost of a copy. A
stale()tree is 23 entries, 12 of them cache. With the debug runner a copy takes 8.8ms (copyTreeSync) or 12.6ms (createTestDir), with a release runner 0.8ms or 1.6ms, so the 158 copies of a run cost about 0.25s with a release runner and about 1.5s more than the first version of this PR (which skipped the cache: 22.5s to 22.8s against 26.6s to 28.5s on a quieter host) with the debug one. The cache is copied anyway because that is what each case had before: its own installs had just warmed it. The index entries of the cache are absolute links into the tree's own cache and are relocated like the rest.fs.promises.cpandfs.cpSyncwere measured too:cpSyncwalks in JS on Linux and took 27ms per 20-entry tree with the debug runner against 7ms for the hand written walk, and the async form was slower end to end.Timing method. Same machine, same debug ASAN build (
bun bd,isASANEnabled()is true), 12 CPUs of quota, reading theRan 174 testsline. The numbers in the description come fromgit checkout main -- test/harness.ts test/cli/install/bun-update-transitive.test.ts, a run,git checkout HEAD -- ..., a run, three times, with the host load average between 31 and 43, which is why the pairs differ so much from each other. The fixed cost of the file (verdaccio start plus runner start) is about 3.9s locally and is untouched. Windows: main 17.98s and 17.97s, this version 14.71s and 14.85s (not ASAN, so 20 cases at a time).CI. Build 101601 (the first version) reported the file at 20.98s on the x64 ASAN lane against 19.20s for main in build 101560. The runner executes changed files first, so the file ran third in its job, during the job's service warm-up:
coordinator: redis_unified readylands in the middle of its output and its first cases were reported done after 8.7s, against 3.4s in build 101560 where it ran eighth. From there on main completed its cases in about 15s (11.5 per second) and the branch in about 12.5s (about 13.5 per second). The same applies to every build of this PR: in builds 101601, 102381 and 102393 the file ran third in its job, its first cases were reported done after 8.7s, 14.6s and 13.7s, and it finished 12.3s, 8.9s and 10.7s after that (20.98s, 23.50s and 24.41s in total, 174 pass each time), against main's 3.4s and then 15.8s in build 101560.Count line semantics (
install_with_manager.rs:1170): installs is the number of node_modules entries found in place, packages is the number of bun.lock rows. So a bundled dependency is a package but not an install (noChanges(3, 5),noChanges(1, 3)), a workspace link is an install (noChanges(3, 4)), and--filter pkg2checks pkg2's installs only (noChanges(2, 4), the same linebun install --filter pkg2prints on that tree).Assertion changes (commit 3).
expectSummary) added to: a direct dependency's range left alone,bun up,-L(also pins the 1.0.1 start and the 2.0.0 end in bun.lock), the threestaleMemberTransitivecases, the twodisjointEdgesmove cases, from one member re-points a sibling, from a member lets a sibling follow, both runs of "from the root does not re-resolve a member's own entry" (plus where each no-deps copy is installed), the named and bare runs of "leaves the named package's own dependencies", abovelatest, the three prerelease cases, both alias cases (the row is printed under the alias, and the installed alias is checked), both peerDependencies cases,@types/*with--filter,--filter <other>(and that it saves the lockfile), the row index shift case (also pins+ a-dep@1.0.1 (v1.0.10 available)), without a lockfile (+ no-deps@1.1.0 (v2.0.0 available)), and the auto-installed peer cases. The named peer case prints the same row as the bare one, so itsplannedflag is gone.--latesthold-back cases, the bundled edge cases, and, throughexpectNoopreturning stdout, the peer edge, peer-only, dependency cycle and twoABOVE_LATESTcases.expectRowsAnd) replaces the row-only or contains checks in the two override cases, the moved dist-tag cases and the accepted scanner case.expectRowSetAndin the shared package cases (plusresolvedEdgespinning which dependent got 1.0.2 and which 2.0.2) and the--minimum-release-agecase.expectHeaderOnly(moved up from the outdated section) on the twelve update error paths,expectRejectedincluded.expectPicked, whichexpectInteractiveDryRunnow uses too.expectKeptPatched(commit 2) folds the two identical patched bodies and adds that the lockfile is not reported as saved and that the patched 1.0.0 is still the one installed.noDepsEdgesbecameresolvedEdges(dir, name, ...)so that the shared package cases can use it.expect()calls went from 2601 to 2197: the checks inside a recipe run once per shared tree instead of once per case, andinstall()'s two checks run 184 fewer times.Not changed. The 35 cases that serve a registry from memory bake their server's port into bun.lock, so their trees are not shared.
frozen()still runs after every update. No case was removed, skipped or reordered. The verdaccio start under the ASAN binary in CI is a fixed cost of every registry test file and is outside this PR.[stamp-90s] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file