test(bunx): install fixture packages from a local registry instead of real ones - #38467
test(bunx): install fixture packages from a local registry instead of real ones#38467robobun wants to merge 3 commits into
Conversation
… real ones The install-heavy cases in bunx.test.ts pulled real packages from npm and GitHub (@angular/cli@20, @babel/cli, eslint, typescript, http-server x2, cowsay x2, 25 versions of semver, and `bun init` + typescript in the Windows-only case), leaving ~35k files in TMPDIR per run. Extracting them and then deleting them (the CI runner removes TMPDIR after each file) is what made this file take 23-30s on the alpine and Windows lanes; the @angular/cli case alone was 21.6k files and 30s on windows-aarch64. Those cases assert bunx's own behavior, so they now install tiny packages served by a per-test in-process registry (and a stand-in for the GitHub tarball API via GITHUB_API_URL). While here, the assertions check the exact bin output, that the cached runs make no registry requests, that versioned requests skip a decoy bin on PATH, that --version reaches the package, and that the corrupted .bunx case falls through to the exe and reports the corruption instead of merely not panicking.
|
Warning Review limit reached
Next review available in: 16 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 (1)
Comment |
|
Status: ready for review at 007bb0b.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a test-only change that strengthens assertions and improves hermeticity, but it introduces ~130 lines of new test infrastructure (in-memory ustar builder, per-test npm/GitHub registry servers) and rewrites the semantics of ~10 test cases, so a human look at whether the helpers should live in harness.ts and whether the fixture shapes preserve every invariant would be worthwhile.
What was reviewed:
- The ustar header layout (offsets, checksum-over-spaces, magic/version) matches the spec; padding and end-of-archive blocks are correct.
- Each converted case's fixture preserves the shape the original relied on (scoped bin ≠ package name, github repo named after its bin,
typescript→tscspecial-casing), and per-testsetup()gives each concurrent case its own TMPDIR so bunx cache dirs don't collide. - Tightened assertions (exact stdout,
stderr === "", request-path counts) are strictly stronger than what they replace; the Windows.bunxcase drops thenot.toContain("panic")anti-pattern for the documented fallback messages. localRegistrypopulates its maps afterBun.servestarts but before any request can arrive — no race.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bunx.test.ts, replacing real-package installs (@angular/cli, @babel/cli, eslint, typescript, http-server, github:piuccio/cowsay, 25× semver) with tiny fixture packages served from per-test in-process Bun.serve registries. It adds ~130 lines of helpers — ustarHeader/tarball (in-memory .tgz builder), fixtureServer/localRegistry/localGithub — and rewrites ~10 concurrent test cases plus the Windows-only corrupted-.bunx case. No production code is touched.
Security risks
None. Test-only; the new servers bind port: 0 and are torn down via using/Symbol.dispose. If anything the change reduces exposure by removing dependence on the public npm registry and github.com for these cases.
Level of scrutiny
Moderate. It's test-only, so a bug here manifests as a red or (worse) vacuously-green CI lane rather than a runtime defect. But it is not mechanical: it hand-rolls a ustar writer, changes what several tests actually assert, and introduces a new local-registry pattern that other files may adopt. The PR description is unusually thorough (verified on linux debug/release and windows-aarch64 with before/after timings and file counts; each fixture's shape is justified against the bunx code path it exercises), and every converted assertion is strictly stronger than before (toBe on exact output, request-count checks proving the cache was used, positive assertions on the .bunx fallback replacing not.toContain("panic")). I checked the ustar header field offsets and checksum computation against the format and they're correct; the 512-byte body padding and 1024-byte trailer are right; Bun.gzipSync over the concatenated blocks yields a valid .tgz.
Other factors
The main reason I'm not approving outright is size and the embedded design decision: the author notes these helpers "can be lifted" for init.test.ts, which suggests a maintainer may prefer them in harness.ts or a shared test/cli/install/ helper rather than inlined here. The tightened request-path assertions (/@bunx-fixture%2fcli, exact tarball URLs) and stderr === "" on cached runs are new invariants that a maintainer should sanity-check against how bunx actually behaves across platforms — the author reports capturing them from a release build first, but a second pair of eyes on ~350 changed lines of test semantics is cheap insurance.
|
On where the helpers live: they are inlined on purpose for now. #35149 (open) is already adding GitHub tarball fixture helpers to On the fixture shapes, the PR description lists the bunx code path each one keeps exercising (scoped name with an unrelated bin, repo named after its bin, |
On Windows two concurrent installs that both need to extract the same
package into the shared cache fail roughly half the time with
"ENOENT: failed opening cache/package/version dir" (13 failing processes
in 20 runs on windows-aarch64; 0 without the shared dependency). The real
semver 7.0.0 and 7.1.0 that this case runs on Windows have no dependencies,
so the fixtures there do not either; the other platforms keep the shared
dependency that exercises the concurrent extraction.
The assertions are also collapsed into one toEqual over per-version
{stdout, stderr, exitCode} records so a failing process shows its stderr
and exit code instead of only an empty stdout.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a well-reasoned test refactor that moves the file toward the repo's own "never contact live registries" guidance and tightens several previously-vacuous assertions. A human look would still be worthwhile: it's ~350 lines introducing new in-file test infrastructure (ustar builder, per-test registry/GitHub servers), the author left an open question about whether those helpers should live in harness.ts/dummy.registry.ts instead, and CI on the latest commit (173043e, the Windows semver follow-up) hasn't reported yet.
Checked: the ustar header layout (offsets, checksum-over-spaces, 512-byte padding, two-zero-block trailer) matches the spec; localRegistry manifest/tarball path shapes match what bun install requests; each converted case's fixture keeps the shape the original real package exercised (scoped-with-unrelated-bin, typescript→tsc special-case, github repo named after its bin); using disposes each server via stop(true). Real-registry coverage remains via the untouched uglify-js/esbuild cases.
Extended reasoning...
Overview
Test-only change to test/cli/install/bunx.test.ts (~350 lines). Adds three in-file helpers — an in-memory ustar/gzip tarball() builder, localRegistry(...) (per-test Bun.serve({port:0}) npm registry), and localGithub(...) (stand-in for the GitHub tarball API via GITHUB_API_URL) — then rewires eight test cases that previously installed real packages (@angular/cli@20, @babel/cli, eslint, typescript, http-server, cowsay, 25× semver, and Windows bun init+typescript) to install tiny fixture packages instead. Assertions are tightened throughout: exact stdout lines instead of toContain/not.toBeEmpty, cached-run cases now assert stderr === "" and that the registry received no further requests, the semver case plants a decoy in PATH (which the test title claimed but never checked), and the Windows corrupted-.bunx case asserts the documented fallback (bin metadata is corrupt, exit 255) instead of not.toContain("panic").
Security risks
None. Test code only; no production paths touched. The new servers bind to port 0 and are disposed via using.
Level of scrutiny
Medium. Test-only lowers risk, and the change moves the file toward the repo's own guidance (REVIEW.md: "Never contact external network hosts or live registries"). But the diff is large, introduces ~130 lines of hand-rolled infrastructure (ustar header encoding, registry manifest shapes) that future test authors will copy from, and changes what several tests semantically verify — real-package integration becomes fixture-package behavior. The PR argues convincingly that the affected cases assert bunx's own behavior (name→bin resolution, cache reuse, arg passthrough) and that real-registry coverage remains via the untouched uglify-js and esbuild cases, but that trade-off is a maintainer call.
Other factors
- The author explicitly left an open question in a PR comment about whether the helpers should be promoted to
harness.ts/dummy.registry.ts(noting #35149 is adding similar GitHub-tarball helpers there) and offered to do it here "if a maintainer prefers" — that's a design decision waiting on maintainer input. - CI reported failures on 4d8ade9; 173043e (removing the shared
lru-cachedependency from the Windows semver fixtures, already reflected in the diff) is the follow-up, but the timeline doesn't yet show a green build on it. - The ustar builder, manifest shapes, request-path assertions (
/@scope%2fname,/repos/.../tarball/HEAD), and.bunxfallback messages were all captured from real runs per the description, and I spot-checked the ustar field offsets/checksum against the format — they look correct. No bugs found by the bug-hunting system or by me.
… live-registry cases Replaces the hand-written ustar writer with Bun.Archive (a leading directory entry is still required: bun install reads a GitHub tarball's first entry as its <owner>-<repo>-<commit> root and refuses the tarball without it), and corrects the comment on why these fixtures exist alongside dummy.registry: its per-test contexts only serve checked-in tarballs, return one version list for every name, and have no GitHub shape. The five cases that still installed uglify-js and esbuild from npm now use fixtures too, so the file no longer contacts any registry. Their assertions check what the old ones could not: that `latest` beat the older version and stdin reached the bin, that the requested tarball was the one fetched, that a relative argument resolved against the caller's directory, that --version reached the package, and that the postinstall actually ran through the bunx-named copy (the bin reports a marker the script writes), which is the path #17076 fixed.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites ~12 test cases to use synthetic fixture packages in place of real ones — a coverage trade-off the author explicitly flags as open to maintainer preference (helper placement, and whether to keep any live-registry cases) — a human look would still be worthwhile.
What was reviewed:
localRegistry/localGithub/tarballhelpers:Bun.ArchiveAPI andGITHUB_API_URLoverride both exist as described;usingdisposal viaserver.stop(true)is correct.- Fixture shapes preserve the bunx code paths named in comments (scoped bin-name mismatch,
typescript→tsc, GitHub tarball root-dir layout, trusted-dependency postinstall foresbuild). - Assertions were tightened, not loosened; the Windows
.bunxcase now asserts the documented fallback instead ofnot.toContain("panic"). - Note: the description says the
uglify-js/esbuildcases stay on the real registry, but commit 007bb0b converted those too — after this PR the file is fully local-registry.
Extended reasoning...
Overview
Test-only change to test/cli/install/bunx.test.ts (~400 lines). Adds in-file helpers (tarball, fixtureServer, localRegistry, localGithub, fixturePackage, echoBin) that build tiny npm/GitHub tarballs with Bun.Archive and serve them from a per-test Bun.serve({port: 0}). Twelve concurrent cases that previously installed real packages (@angular/cli, @babel/cli, eslint, typescript, http-server, semver ×25, cowsay, esbuild, uglify-js) now install one- or two-file fixtures shaped to exercise the same bunx resolution paths. Assertions are tightened throughout (exact stdout, request-path lists, cached-run-makes-no-requests, positive .bunx fallback assertions on Windows). Verified Bun.Archive exists in packages/bun-types/bun.d.ts and GITHUB_API_URL is read in src/install/PackageManager/runTasks.rs.
Security risks
None. Test file only; no runtime code touched. The change removes external network contact, which aligns with the repo's own test-hermeticity rule.
Level of scrutiny
Medium. It's test-only with no production impact, but it's a substantial rewrite that changes what the tests exercise. The core judgment call — that these cases test bunx's name→bin resolution / cache / arg passthrough rather than the real packages' contents — is well-argued and the fixture shapes are chosen carefully, but it's still a coverage decision a maintainer should sign off on. The author also explicitly left helper placement (inline vs. harness.ts/dummy.registry.ts) as an open question pending maintainer preference and #35149.
Other factors
- The PR description is slightly out of sync with the final state: it says the
uglify-js×3 andesbuild×2 cases remain on the real registry, but commit 007bb0b ("convert the remaining live-registry cases") converted those as well, so the file is now fully offline. Not a bug, but a maintainer reading the description would expect residual live-registry coverage that no longer exists. - The Windows semver flake in the first revision was diagnosed (concurrent shared-dependency extraction race) and fixed by making the two Windows fixtures dependency-free, matching real semver 7.0.0/7.1.0; 30/30 loop-verified.
- The tightened Windows
.bunxassertions (bin metadata is corrupt, exit 255) were captured from a real Windows run before being asserted, per the description. - No prior claude[bot] reviews on this PR; no outstanding human review comments.
|
The description has been updated for 007bb0b: no case in the file contacts a live registry any more (the earlier text about |
|
#39115 adds a |
Problem
test/cli/install/bunx.test.tsis a serial-phase file that takes 23-26s on the alpine lanes and 30s on windows 11 aarch64 (builds 95391 / 95331), against 3s on debian and ubuntu.@angular/cli@20,@babel/cli,eslint,typescript,http-servertwice,github:piuccio/cowsaytwice, 25 versions ofsemver, andbun init+typescript@5.0.0in the Windows-only case;uglify-jsx3 andesbuildx2 are small but also live) and leaves ~35k files / 400-680MB in TMPDIR per run.scripts/runner.node.mjs(spawnBun) gives each file a fresh TMPDIR andrmSyncs it afterwards, so every lane pays to extract those files and then to delete them. In the alpine logs that deletion is the 11-13s gap betweenRan 35 testsand the next file's header; the remaining time is the extraction inside the tests.should handle package that requires node 24:@angular/cli@20is ~250 packages, 21.6k files / 300MB per run. Measured alone on windows-aarch64 it takes 25.4s plus 4.9s to delete, which is the whole 30s the CI lane reports for the file. Next are@babel/cli(3.2k files),http-serverx2 (2.9k),semverx25 (2.8k),eslint(2.4k),cowsayx2 (1.9k),typescript(1k files, 62MB), and on Windowsbun init's@types/bun+typescript@5.0.0(1.3k, in a sequential test).test/cli/init/init.test.tsshows the same alpine/Windows-slow signature;bun initinstalls@types/buninto TMPDIR per test, so it is very likely the same mechanism (files written to TMPDIR, then deleted by the runner), and the helpers here can be lifted for it.Fix
tarball()(aBun.Archivegzip tarball with everything under one root directory),localRegistry(...packages)(a per-testBun.serveon port 0 serving manifests and tarballs for fixture packages, reached throughnpm_config_registry) andlocalGithub()(a stand-in for the GitHub tarball API, reached through theGITHUB_API_URLoverride thatalloc_github_urlinsrc/install/PackageManager/runTasks.rsalready honors). Both servers record the request paths they receive.dummy.registry's per-test contexts were not usable for this because they only serve the.tgzfiles checked in next to it, answer every package name with the same version list, and have no GitHub shape; the comment in the file says so.REVIEW.md); the real-registry install path itself is covered by the install suites against the local registry. The fixtures keep the shape each real package exercised: a scoped package whose bin is named like neither the scope nor the package (@angular/cli->ng,@babel/cli->babel), packages literally namedtypescript(bintsc),http-serverandeslintso the name special-casing inbunx_command.rsis still what gets exercised,uglify-jswith its bin nameduglifyjs(found by reading the installed package.json) in two versions,esbuildwith a postinstall script (it is in the default trusted dependencies, which is why the real one's script ran), a GitHub repo named after its bin likecowsay, and 25semverversions sharing one dependency so the simultaneous installs still race to extract the same package into the shared cache (what Make duplicate simultaneousbun installwork better #9738 grew this case to 25 versions for). Not on Windows: the two versions it runs there are dependency-free like the real7.0.0/7.1.0, because two concurrent installs extracting the same package fail about half the time on Windows (details below). Therequires node 24fixture applies the sameprocess.versions.nodegate Angular'sngapplies.bunx add/bunx exec, the.bunxfast-path fallback), none of which depends on what the installed package contains. The pinned real@angular/clialso broke twice when its engines range moved (Skip bunx.test.ts in CI until the Node.js version bump #32042, test: unskip bunx.test.ts, pin the Angular CLI version #33948, which took the whole file out of CI for 3.5 weeks); the fixtures cannot.toContain("Usage: ..."),not.toBeEmpty(),not.toContain(Bun.version), or, foruglify-js, output that any version produces);/@scope%2fname,github:owner/repo/#HEADbecoming/repos/owner/repo/tarball/and/tarball/HEAD, and foruglify-jsthat the bare name fetched thelatesttarball while@3.14.1fetched that one (the registry also serves a newer version, so these can fail);--no-install, and the second plain run in the@scopedandgithubcases) assertstderr === ""and that the registry received no further requests, so they fail if bunx re-installs instead of using its cache (before, a re-install passed them);default (latest) versionasserts stdin reached the bin;current working directoryasserts the bin resolved its relative argument against the caller's directory (plus the existing check that bunx wrote nothing there);requires node 24asserts the bin ran under Bun and sawprocess.versions.nodeof the Bun under test (the old version passed even if--bunwas ignored and a system node ranng);semvercase plants a decoysemverfirst in PATH, which its title claimed to test but nothing checked, and compares one{stdout, stderr, exitCode}record per version (stderr used to be ignored);--versionpassthrough asserts the package received--version; the postinstall case asserts the script actually ran (its bin reports a marker the script writes) and that the install went through thebunx-named copy, which is the fixbunxon windows with postinstall scripts #17076 path; removing the script from the fixture flips the output, so the assertion is live;--no-installandesbuild --versioncases strip PATH entries that provide the bin (as thebunx claudecase already does), so a binary installed on the machine cannot satisfy them;.bunxcase writespackage.jsoninstead of runningbun init(which installed@types/bun), checks thatbun addsucceeded, and asserts the documented fallback (bin metadata is corruptfrom the spawnedtsc.exe,"tsc.exe" exited with code, exit 255) instead ofnot.toContain("panic").setDefaultTimeoutis left alone: thesemvercase still starts 25 bunx processes that each spawn an install, which is several seconds under the ASAN build, so the 5s default would flake and per-test timeouts are discouraged intest/CLAUDE.md. Test names and order are unchanged so the open PRs that add cases to this file stay rebasable. The helpers stay in this file for now:dummy.registry.tsis being extended by test(install): serve GitHub tarball fixtures locally in bun-add.test.ts #35149 (GitHub fixtures) and bunx: resolve colliding bins from the selected package #37150/bunx: support anonymous URL packages #37283 (context tarball dirs) at the same time, and a second consumer (init.test.ts, which would import fromharness.ts) does not exist yet; moving them is mechanical when it does, or now if preferred.bun bd test test/cli/install/bunx.test.ts(debug build, linux): 68.0s before, 13.7s after the first revision, which removed the large installs; the later revisions only remove the remaining network calls. TMPDIR left behind 34,927 files before, ~420 after. (should set "npm_config_user_agent" to bunfails underbun bd testboth before and after; that is the env inheritance test: strip inherited npm_config_user_agent from bunEnv #31928 fixes in the harness, and it passes in CI.)rm -rfof TMPDIR 2.5s -> 0.1s.dummy.registrycases). 33 pass / 2 skip each time; thesemvercase passed 30/30 looped runs after the Windows change..bunxfallback messages asserted above were captured from the release bun first (the.bunxone on Windows) and then written into the assertions. A files-only GitHub tarball is refused by bun install (tarball root directory ... is not a valid folder name), which is whytarball()emits the directory entry first.Background
bunx <pkg>looks for a bin on PATH (only when no version is given), then in its cache dir$TMPDIR/bunx-<uid>-<pkg>@<version>/node_modules/.bin/, and otherwise spawnsbun add <pkg>into that dir and runs the result (src/runtime/cli/bunx_command.rs). A second run within 24h is served from that dir, which is what the "cached" halves of these cases test;--no-installskips the install step and errors if nothing is cached.package.jsonto find the real bin; forgithub:owner/repoit guesses the repo name. The fixtures keep those shapes.bun installfetchesgithub:dependencies as tarballs from$GITHUB_API_URL/repos/<owner>/<repo>/tarball/<ref>(defaulthttps://api.github.com) and reads the tarball's single root directory,<owner>-<repo>-<commit>/, as the resolved commit, so the fixture tarball uses that layout. npm tarballs use apackage/root.<bin>.exeshim plus a<bin>.bunxmetadata file.bun run <bin>parses the metadata in-process (the BunXFastPath); on corrupt metadata it is supposed to fall through to spawning the.exe, whose standalone copy of the parser reportsbin metadata is corruptand exits 255 (src/install/windows-shim/bun_shim_impl.rs). That fall-through is what the corrupted-.bunxcase now asserts.scripts/runner.node.mjs,spawnBun) pointsTMPDIR,BUN_TMPDIRandBUN_INSTALL_CACHE_DIRof each test file at a fresh directory and removes it synchronously after the file exits, so everything a file installs is written once and deleted once per lane, inside the file's measured wall time.Per-test timings on windows-aarch64 before the change
-t "requires node 24"alone: 25.4s, 21,602 files / 3,529 dirs / 300MB, 4.9s to delete.After: every converted case is 0.1-0.6s on linux; the file's concurrent batch on the Windows VM is bounded by the remaining
uglify-js/esbuilddownloads (~3s).Concurrent installs sharing one package are not race-free on Windows (why the Windows semver fixtures have no dependency)
The first revision gave the two Windows
semverfixtures the same shared dependency as the other platforms and flaked on the windows 11 aarch64 lane: one of the two processes printed nothing. Looping the case on a windows-aarch64 VM reproduced it 10/25 times. A standalone probe spawning twobun x semver@<v> --helpagainst a local registry, 20 runs each:The message is the
Step::OpeningCacheDirfailure fromsrc/install/PackageInstaller.rs; thebun addchild exits 1 and bunx propagates the exit code without printing anything itself (bunx_command.rs, after the install spawn). That is the existing "Windows does not support race-free installs" limitation this case already works around by running only two versions there, and those two real versions (7.0.0,7.1.0) have no dependencies, so the fixtures now do not either. The bun-side race is tracked separately; it is not changed by this PR.Earlier revisions of this PR
uglify-jsx3 andesbuildx2 on the live registry and built tarballs with a hand-written ustar writer; revision 3 replaced the writer withBun.Archive(the only thing the writer did thatBun.Archivedoes not do by default is emit the leading directory entry, which is now passed explicitly) and converted the five remaining cases.semverfixtures the shared dependency and flaked on the windows 11 aarch64 lane; see the Windows block above.Left as is
dummy.registry's module-level handler (--package flag, scoped-collision and alias suites) are 10-90ms each on release builds; converting them to per-test servers would touch the areas several open PRs add cases to.should handle postinstall scripts correctly with symlinked bunxstill copies the bun binary twice (the copy namedbunxis the point of that regression test, fixbunxon windows with postinstall scripts #17076); on debug builds that is two copies of a large binary, but it is one sequential file copy each, not the file-count problem above.setDefaultTimeout, see above.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.