Repository navigation
Conversation
The three invalid-format cases used file snapshots, which bun test does not support inside concurrent groups, so they were marked test.serial. Inside describe.concurrent a serial test is a barrier: the runner drains every in-flight test before running it, so these four tests split the file into phases and the slow git clone cases in each phase could not overlap. Inline snapshots work in concurrent tests, so switch to those (same expected text) and delete the now unused snapshot file. The modified-git-resolutions case now asserts the lockfile is byte for byte the one it wrote, that the registry was never contacted, and what ended up in node_modules, instead of snapshotting the lockfile.
…atform Diagnostic only, to be removed: prints the 30 slowest withContext cases with their start offsets at the end of the file so CI logs show which cases account for the file's wall time on each lane.
…k, tighten assertions - Registry URLs table and the invalid git URL case send their request through a local proxy that records and refuses it. The two entries with a single-label host (example, xn--c1yn36f) took ~45s each on the Debian CI images and ~14s on Windows waiting on the resolver, which was most of this file's wall time there. - The committish cases (resolve a tag, reject an unknown committish, de-duplicate a tag and an abbreviated hash) use a tagged repository served over dumb HTTP instead of cloning UglifyJS; they assert the resolved sha, the .bun-tag marker and the cache layout computed from that repository. - The scoped tilde name case uses the test registry instead of registry.npmjs.org and asserts the requests it makes. - The top-level cases after the describe.concurrent group are it.concurrent: a plain top-level test runs alone after everything else. - The 20-install workspace case is ten independent cases that also assert which package.json received the added dependency. - Workspace move/rename cases assert the links and lockfile instead of only not throwing; frozen lockfile cases assert the first install's output and that the refused install left the lockfile and node_modules untouched; stray console.log calls removed.
The search tries ~9000 candidates on average and spent ~60 BigInt operations on each (re-reading both lane words from bytes, the generic mix, extracting the kill word byte by byte), which took 4-10s in a debug build of JavaScriptCore while blocking the test file's event loop (the colliding-hashes case is a concurrent test). The counter word is now updated incrementally from per-digit tables, the lane is mixed inline and the byte check works on two 32-bit halves: 0.3-0.65s in a debug build, a few ms in release. The construction is unchanged and is still verified against wyhashStd here and against Bun.hash.wyhash in the test.
The workspace move/rename cases run with the default linker, which on Windows creates relative symlinks, while toHaveWorkspaceLink2 expects the absolute junction target the hoisted linker creates there. Resolve each link with realpath and compare project-relative targets instead, which holds for every kind of link. Also silence the shell calls in the glob cases, which were echoing the install output into the test log.
The per-lane measurements it was added for are in the PR description.
…ng git URL cases The two git URL form cases clone dylan-conway/install-test2 (one commit, 136KB, already used by this file) instead of the 20MB UglifyJS repository, and pin the checked out commit through the sha bun reports rather than UglifyJS's current top-level listing. The existing lockfile case used html-minifier, which also pulled its eleven dependencies from registry.npmjs.org because the case dropped the test registry from bunfig; it now installs the local tagged fixture with a dependency on bar from the test registry, and asserts what each of the three installs requested (manifest and tarball, tarball only, nothing), the cache contents and that the lockfile bytes never change. These three cases were what the file's duration came down to on the ubuntu and alpine lanes after the earlier commits (6.5-12.5s each).
robobun
changed the title
|
Warning Review limit reached
Next review available in: 2 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 ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
![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 test refactor (~1000 lines) that replaces several tests' fixtures and reshapes what they assert, a human look at the coverage-equivalence calls would still be worthwhile.
What was reviewed:
serveTaggedGitPackage/refusingProxy/expectInstallTest2Installedhelpers and every rewritten git/proxy/frozen-lockfile/workspace case for preserved coverage — bin-linking is still exercised via the tagged fixture, anddefaultOptsalready suppliescache=false/saveTextLockfile=falsefor the existing-lockfile case.- The wyhash search optimization: the inlined
mixand incrementalcounterWordmaintain the same 64-bit value as the previousread(8, steer, 0), and the result is still self-checked againstwyhashStdbefore returning. test.serial→ concurrent conversions and the top-levelit.concurrentchanges for shared-state hazards — each uses its ownwithContext/tempDir.
Extended reasoning...
Overview
This PR is a test-only speed-up and hardening pass over test/cli/install/bun-install.test.ts plus its wyhash collision helper. It replaces five UglifyJS/html-minifier GitHub clones with a locally-served one-commit tagged git repo (serveTaggedGitPackage), routes the two remaining real-URL git cases to a tiny existing test repo, short-circuits DNS-bound failure cases through a local refusing proxy, converts four test.serial snapshot cases to concurrent inline snapshots (deleting the .snap file), splits one 20-install serial case into 10 independent concurrent cases, converts ~26 top-level it cases to it.concurrent, and rewrites the wyhash collision search to do far fewer BigInt ops per candidate. Along the way it adds real assertions to two workspace-move/rename cases that previously had none, and dedupes the three frozen-lockfile setups into shared helpers with tighter checks.
Security risks
None. No production code is touched. The new refusingProxy and serveTaggedGitPackage helpers bind to 127.0.0.1/localhost on port 0 and are disposed via using.
Level of scrutiny
Medium-high, despite being test-only. The diff is large and several rewrites change what a test observes (local fixture vs. real GitHub repo, request-count assertions vs. hardcoded UglifyJS listings, per-case fresh context vs. one reused context). Each substitution is argued in the description and I checked that the properties the original tests protected (tag→sha resolution, dedupe of committish, cache re-link on second install, bin linking, frozen-lockfile leaves lockfile bytes untouched) are still asserted — usually more precisely. But these are judgment calls about coverage equivalence that a maintainer should confirm.
Other factors
- The wyhash change looks correct: both multiplication operands stay under 2⁶⁴ so the dropped
& M128and trailing& MASKwere redundant, and the incrementalcounterWordprovably equals the little-endian read ofcsBytes[digits[*]]. It is also self-verifying (wyhashStd(seed, b1) === wyhashStd(seed, b2)) and the consuming test re-checks againstBun.hash.wyhash, so a mistake would fail loudly rather than silently weaken coverage. - I confirmed
withContext(defaultOpts, ...)already writescache=false, saveTextLockfile=false, linker=hoistedto bunfig, so removing the explicit bunfig write and--linker=hoistedflag from the existing-lockfile case is behavior-preserving. - The install-test2 repo has no
bin, so the two real-URL git cases no longer assert.binlinking; that coverage moves to the committish/dedupe cases via the tagged fixture'sbinentry. - CI numbers in the description show the file passing on all lanes at the near-final commit, and the one Windows failure that appeared mid-branch (workspace link comparison) is addressed by the realpath-based
repoLinkshelper.
Given the size and the number of coverage-equivalence decisions, deferring to a human reviewer.
|
Nothing to change from this review. The one coverage point it raises (install-test2 has no bin, so the two GitHub URL cases no longer check bin linking for a git dependency) is intentional: the tagged fixture has a bin, and the committish, de-duplicate and existing-lockfile cases check the |
… them concurrent prettier only keeps an over-long test call on one line for plain it/test, so it.concurrent with the five longest names had expanded the calls and re-indented their whole bodies (about 650 lines of whitespace churn). Hoist those names into constants so the declarations stay short and the bodies are untouched; the names themselves are unchanged.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large test rewrite (~1000 lines) that reshapes coverage for a dozen-plus cases — swapping UglifyJS/html-minifier clones for local fixtures, splitting the 20-install case into 10 independent contexts, moving bin-linking coverage from the two GitHub-URL cases to the tagged-fixture cases — a human look at the coverage trade-offs would still be worthwhile.
What was reviewed:
- Verified
defaultOpts/createTestContextalready writescache: false,saveTextLockfile: false,linker: hoisted, so the dropped bunfig write and--linker=hoistedflag in the existing-lockfile case are redundant, not lost coverage. - Traced the wyhash incremental-counter optimization:
counterWordtracksread(8, steer, 0)exactly (init/step/wrap deltas), the inlined mix matchesmix(), and the trailingwyhashStdself-check plus the test'sBun.hash.wyhashcheck guard against drift. - Checked that the split workspace-install cases assert a superset of the old assertions; the old "6 packages installed" was leakage from prior sub-iterations'
install bar, not a covered invariant. - Confirmed
dummyRegistryForContexthandles%2f-encoded scoped names, so the@~39/barassertions match its URL shape.
Extended reasoning...
Overview
This PR touches only test infrastructure: test/cli/install/bun-install.test.ts (large refactor), its snapshot file (deleted, contents moved to inline snapshots), and test/cli/install/wyhash-std-collision.ts (search-loop optimization). No production code is changed. The stated goal is to cut the file's CI wall-clock from ~66s to ~11s on the slowest lane by removing DNS stalls (via a refusing local proxy), replacing five 20MB UglifyJS clones and one html-minifier+npm install with a locally-served one-commit fixture, converting four test.serial barriers to concurrent, promoting ~26 top-level cases to it.concurrent, and speeding up the wyhash BigInt search. Along the way many previously loose assertions are tightened (workspace move/rename tests had no expect at all; frozen-lockfile setup only checked exit 0; the tilde case hit npmjs.org).
Security risks
None. Test-only change; no auth, crypto, or user-facing surface. The new refusingProxy and serveTaggedGitPackage bind to 127.0.0.1/localhost on port 0 and are disposed via [Symbol.dispose].
Level of scrutiny
Medium — test-only, so no runtime risk, but per REVIEW.md the bar for "never silently weaken an existing test" applies. I checked each rewritten group for assertion strength versus the original: the workspace move/rename, frozen-lockfile, tilde, modified-git-resolutions, and existing-lockfile cases all assert strictly more than before. The two GitHub-URL cases (install-test2) drop bin-linking assertions because that repo has no bin; the description states this and the committish/de-duplicate/existing-lockfile cases now cover .bin via the tagged fixture, which does have one. The 20-install case's old "6 packages installed" values were pollution from earlier sub-iterations' install bar writes, not an invariant. The wyhash change fixes steer[8..16] per-search instead of varying it with extra, which is fine because only lane-0's counter word needs to vary and the construction is self-verified against wyhashStd and Bun.hash.wyhash.
Other factors
The diff is large and makes several judgment calls about coverage equivalence (which real-network cases stay, which move to fixtures, where bin-linking is exercised, how the sequential 20-install case decomposes). The PR description documents each call and cites CI build numbers where 209 tests passed on every lane, but the scale of reshaped coverage is beyond what I'd shadow-approve without a human confirming the trade-offs match the team's intent — particularly the install-test2 bin-linking move and the decision to leave the bitbucket/gitlab/github: cases and the 5-minute default timeout in place pending #35149/#38183.
|
Same two points as the previous pass, both already covered in the description: where bin linking for git dependencies is checked now (Fix, git clone cases) and why the bitbucket/gitlab/ |
|
Status: ready for review. Build 96272 on the current head (ba1480f) ended as failed only because its two darwin 14 aarch64 test jobs expired in the queue three times without an agent ever starting them; the other 177 jobs passed, including darwin 14 x64, and this file passed first try on every lane that ran (10.6s asan, 3.2s debian x64, 8.1s windows x64, 16.6s windows aarch64; it was 66s / 52s / 31s / 33-36s before). The remaining annotations are the runner's flaky markers on unrelated files that passed on retry. Not retriggering while that lane has no capacity; happy to re-run it once agents are back if a green check is wanted before merging. |
|
Status: this branch no longer merges. Main added about 980 lines to On current main (1ab272b, build 108217) the file takes 79s on debian 13 x64-asan and 51-53s on the debian and windows 2019 lanes, so the per-case analysis above still applies. The cases added on main since the base are inside their own |
|
This is Claude (an AI assistant) writing on Jarred's behalf. Included in #44968. |
Problem
test/cli/install/bun-install.test.tsruns in the serial phase and was among the slowest files there: 66s on debian 13 x64-asan, 50-52s on debian 13 x64 and aarch64, 31-36s on both Windows lanes, 16-18s on ubuntu/alpine x64 (builds 95391 and 95331). The debian vs ubuntu gap on identical hardware pointed at something environmental.http://example/%?%#%andhttp://xn--c1yn36f/: 45.0-45.8s each on debian x64, aarch64 and asan, 14s (example) on both Windows lanes. Both have a single-label host name.bun installlooks a failing registry host up about a dozen times per run (DNS prefetch, the request plus 5 retries, and a secondgetaddrinfowithoutAI_ADDRCONFIGafter each miss, none of it cached), and each lookup of such a name is slow on the debian images and on Windows, presumably the resolver trying the search domains; ubuntu's systemd-resolved answers single-label names locally, which fits the gap. Every other entry of the table took well under a second everywhere.test.serialcases: three file snapshots (refused inside concurrent groups) and one more. Insidedescribe.concurrenta serial case is a barrier, so the file ran as three phases whose slow network cases could not overlap.file: deps with colliding abs-path hashes, whose BigInt search took 4.5-9.6s of synchronous work in debug/asan builds, where the file's event loop is shared by only 5 concurrent tests (--max-concurrencydefaults to 5 under ASAN, 20 otherwise).should handle @scoped name that contains tildeinstalled from registry.npmjs.org.Fix
should fail on invalid Git URL: the spawned install getshttp_proxy/https_proxypointing at a localBun.servethat records and refuses every request (refusingProxy). The join under test still happens and the request for the joined URL is still built and sent, but through a proxy no host name is ever resolved, and the install fails with the samefailed to resolve/InstallFailedthe cases assert (140-190ms per entry in a debug build). The proxy only changes where the bytes go: bun builds the manifest URL before it consults the proxy, and with a proxy set it also skips the registry DNS prefetch. The git case additionally asserts the one request git made (GET http://bun.sh/no_such_repo/info/refs?service=git-upload-pack), so it now shows the clone of the URL as written was attempted.committish in dependencies,invalid committish,de-duplicate committishandGit URL with existing lockfileuseserveTaggedGitPackage(): a one-commit repository with a bin and av1.0.0tag built with the file's existing dumb-HTTP git helpers. Same paths as before (tag to sha resolution, unknown committish error, tag and abbreviated sha deduplicated into one package, re-installs from the lockfile), asserting the resolved sha, the.bun-tagmarkers and the cache layout computed from the fixture instead of hardcoded UglifyJS values. The existing-lockfile case gives the fixture a dependency onbarfrom the test registry and asserts what each install requested (manifest and tarball; tarball only once the lockfile exists; nothing once the cache exists), which the html-minifier version could not assert at all since it was talking to npm.Git URL in dependenciesand its SCP-style variant still clone from GitHub, since they cover the real URL forms (nothing local can stand in for thegithub.com:shorthand), but clonedylan-conway/install-test2(one commit, 136KB, already used by this file) instead of UglifyJS, and pin the checked out commit through the sha bun prints (.bun-tagand the@G@<sha>cache folder must agree with it) instead of UglifyJS's current top-level listing. install-test2 has nobin, so these two cases no longer check bin linking for a git dependency; the tagged fixture has one, and the committish, de-duplicate and existing-lockfile cases check it (.binlisting and link target), as the UglifyJS versions of those cases did.it.concurrent(names unchanged), with a comment at the boundary so appended cases stay that way. The five longest names are hoisted into constants: prettier keeps an over-long test call on one line only for plainit/test, andit.concurrenton those lines would otherwise re-indent their whole bodies (an earlier push of this branch did, about 650 lines of whitespace churn). The bodies of these cases are untouched.describewith ten independent cases (2 rootworkspacesspellings x 5 directories to install from), two installs each.wyhash-std-collision.ts: the search tries ~9000 candidates on average and spent ~60 BigInt operations on each; it now advances the counter word incrementally, mixes the lane inline and checks the kill word as two 32-bit halves. Same construction, still verified againstwyhashStdin the helper and againstBun.hash.wyhashin the test; 40 collisions for varied prefix lengths were also checked againstBun.hash.wyhashdirectly. 0.3-0.65s in a debug build, a few ms in release.@~39/barfrom the test registry and asserts the manifest was requested under the scoped name (@~39%2fbar) and where the package landed.should work when moving/renaming workspace packageshad noexpectat all (they passed ifbun iexited 0, and echoed its output into the log). They now check stderr/stdout/exit of each install, the lockfile'sworkspace:entries and where everynode_modules/@repo/*link points, before and after the change. Links are compared through realpath: the default linker makes relative symlinks on Windows, whiletoHaveWorkspaceLink2expects the hoisted linker's junctions there (the one CI failure of an earlier push of this branch). The rename case usestoMatchObjectfor the links because the link for the old name is currently left behind; that is filed separately and not what the case is about.--frozen-lockfile,bun ciand bunfigfrozenLockfileshared a setup whose first install was only checked for exit 0.installBazThenBumpItasserts its output lines and registry requests;expectFrozenInstallRefusedasserts the error, noSaved lockfile, that the registry was consulted, and that the lockfile bytes and the installedbaz@0.0.3are untouched.node_moduleslisting and the installed package.json, lockfile unchanged.bar(and that the others are unchanged), the hoistednode_moduleslisting and that the first install made no registry request.console.loglines and three unquietedBun.$calls no longer write into the test log.refusingProxyis the general form of what test(install): serve URL tarball dependencies locally in bun-install.test.ts #37919 adds for the URL tarball cases (that one also proxies, but serves tarballs for registered URLs and leaves https alone). The two merge cleanly about 40 lines apart, so whichever lands second should fold its helper into the other rather than leave two proxies in the file; noted on test(install): serve URL tarball dependencies locally in bun-install.test.ts #37919 as well.github:shorthand cases, which still use the network (test(install): serve GitHub tarball fixtures locally in bun-add.test.ts #35149 is the base for making those hermetic), and the file's 5 minutesetDefaultTimeoutplus the table'sInfinity: the CI runner kills the file after 3 minutes anyway, so a lower ceiling would only add flake exposure for the cases that still reach the network.Verification
Local,
bun bd test test/cli/install/bun-install.test.ts(debug + ASAN build, so 5 tests at a time; this container blocks bitbucket/gitlab/gitpkg, so the same 14 network cases fail before and after): main 37.5s, 40.8s, 48.3s for 200 tests over three runs; this branch 22.7s for 209 tests (26-29s before the last commit). What is left is CPU bound here: about 200 installs of a debug binary, five at a time.CI, whole file, before (builds 95391/95331) vs build 96026 of this branch (209 tests, all passing on the first attempt on every lane that ran; build 96272, on the current head, repeats that on all nine Linux/Windows lanes, at 10.6s asan, 3.2s debian x64, 8.1s windows x64, 16.6s windows aarch64. Its only non-passing jobs are the two darwin 14 aarch64 test jobs, which expired in the queue three times without an agent ever starting them (darwin 14 x64 ran and passed); that is lane capacity, not a test result):
Whole file on a Windows x64 machine with the released 1.4.0 binary at the final state: 206 pass, 0 fail, 3.75s.
Each rewritten group (formerly serial cases, proxied cases, git cases, frozen-lockfile cases, workspace cases, collision case) was also run in isolation with the debug build; the frozen-lockfile cases six times to confirm the registry request pattern is stable.
Scale of the CI effect, for calibration: the file has not been failing or flaking in recent builds (the failures in test(install): serve URL tarball dependencies locally in bun-install.test.ts #37919 are the exception), so this buys time rather than reliability. The serial phase is bin-packed across 20 Linux / 8 Windows shards per lane, so ~45-50s less serial work on a lane is roughly 2-3s of wall clock per build, plus about 200 lane-seconds of agent time per build summed over the lanes above, plus the local iteration time.
test/expected-durations.jsonstill carries the old durations for this file (52.6s default, 64.2s asan, 33.3s windows); it is regenerated from CI medians by the scheduled job, so the sharding hint catches up on the next refresh.Diff shape: 662 insertions / 1130 deletions across the test file, its snapshot file and the collision helper. Apart from the helpers, the changed regions are the rewritten cases listed above; everything else in the file is a one-line change or untouched, to keep out of the way of the open PRs that add cases to this file.
Background
describe.concurrentruns the async tests inside it up to--max-concurrencyat a time (20 by default, 5 in ASAN builds). Atest.serialinside it, or a plainitat top level after it, runs alone: the runner first waits for everything in flight to finish. Concurrent tests in sibling groups or at top level (it.concurrent) all overlap with each other.toMatchSnapshothas to know which test is running to name the snapshot, which is ambiguous under concurrency, so it throws there;toMatchInlineSnapshotcarries its value at the call site and works.http_proxy/https_proxyset, bun's HTTP client (and git's libcurl) connects to the proxy instead of resolving the host: a plain-http request is sent to the proxy in absolute form (GET http://example/notapackage), an https one asCONNECT host:443.Bun.servereceives both as requests, so answering 404 refuses either; bun reports the proxy's status for the joined URL and thenfailed to resolve.git update-server-infocan be cloned over git's "dumb" HTTP protocol by any static file server, whichcreateDumbHttpGitRepo+serveDirectoryalready did for four cases in this file.bun installclones a git dependency withgit clone --bareinto the cache under a name derived from the clone task id (it always starts with 8 or 9, which is why the.cachelistings below sort it before the checkouts) and checks the resolved commit out into@G@<sha>, writing the sha to a.bun-tagfile in the checkout. Withcache = falsein bunfig, that cache, and the extracted registry packages, live innode_modules/.cache.