test(install): run bun-pack.test.ts concurrently and assert the exact pack output - #40959
Conversation
… pack output Each test builds its tree in its own tempDir instead of the shared beforeEach directory. The describe blocks are describe.concurrent and the top-level tests are test.concurrent. A local runPack() replaces the harness pack() helper. It returns the normalized stdout and stderr. The shasum, the integrity and the packed size depend on the compressor, so it masks them. Each test asserts that stderr is empty (or the exact script echo), the exact stdout, the exit code, and the full entry list of the tarball.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Updated 4:18 AM PT - Aug 30th, 2026
✅ @robobun, your commit a3ddc38f93c815ba66298c10cf8783071a4ce1a0 passed in 🧪 To try this PR locally: bunx bun-pr 40959That installs a local version of the PR into your bun-40959 --bun |
![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 rewrites ~80 tests across a ~3100-line diff, a human spot-check that no case lost coverage in the conversion would still be worthwhile.
What was reviewed:
runPackhelper — drains stdout/stderr/exited concurrently, usesawait using,bunEnv, andnormalizeBunSnapshot; masking regexes only match the intended summary lines.- Assertion direction —
toMatchObject/toContainreplaced withtoEqual/inline snapshots anderrchecked beforeexitCode; assertions are strictly tighter than before. - Concurrency safety — shared module-level
packageDirremoved in favor of per-testtempDir, sodescribe.concurrentgroups don't share state. test.eachtables (scoped names, invalid package.json,--destination,--gzip-level) — each row still exercises the same input as the oldfor-loop version, plus new negative assertions that no tarball was written on error.
Extended reasoning...
Overview
This PR is a test-only refactor of test/cli/install/bun-pack.test.ts (~1731 additions / ~1364 deletions, net ~3100 diff lines). It replaces the module-level beforeEach + tmpdirSync() shared directory and the harness pack/packExpectError helpers with per-test using dir = tempDir(...) fixtures and a local runPack helper. Tests move to test.concurrent/describe.concurrent, tarball assertions switch from toMatchObject on entry objects to toEqual on exact pathname arrays, and stdout is asserted via toMatchInlineSnapshot with shasum/integrity/packed-size masked. Error paths now assert exact stderr, exit code 1, and that no stray files were written. No production code is touched.
Security risks
None. This is a test file for bun pm pack; it spawns the debug bun binary against local tempDir fixtures, contacts no network, and adds no new dependencies or FFI. The only "sensitive" content is a path-traversal test case (../../outside-pkg), which was already present and now asserts more (exact error text plus a full directory listing showing nothing escaped).
Level of scrutiny
Medium. The change is mechanical and aligns tightly with the repo's test conventions (tempDir over tmpdirSync, normalizeBunSnapshot, stderr-before-exit-code, concurrent subprocess tests, await using proc, Promise.all on the three pipes). Assertions move in the tightening direction throughout, which is what REVIEW.md asks for. That said, 80+ test bodies were rewritten by hand — the risk isn't a bug in any one helper but a transcription slip in one row of a test.each table or a snapshot that quietly encodes weaker behavior than the old toMatchObject covered. The bug hunt ran to dry_streak without findings, but a human skim of a handful of conversions (especially the bundledDependencies, workspace: lockfile, and lifecycle-script blocks I didn't excerpt here) would give more confidence than an automated pass alone.
Other factors
No CODEOWNERS entry covers this path. There are no prior review comments or objections on the timeline. The PR description reports local verification (80→83 tests, 20.6s→6.9s under debug+ASAN, --rerun-each=3 clean) and is candid about behavior it snapshots as-is (e.g. // vs @// name handling, --dry-run size reporting). Test count increased and no test/test.each was skipped or removed based on the diff structure I read. The size alone is why I'm deferring rather than approving.
|
For the spot check of the three blocks the review names, this is the old assertion next to the new one.
Lifecycle scripts
The |
…script cases The scripts embed the path to the bun binary. On the darwin CI agent that path is long enough for package.json to pass 512 bytes, and the pack summary then prints its size as 0.58KB instead of NNNB.
2 participants
Problem
test/cli/install/bun-pack.test.tstakes 10.7s on debian 13 x64-asan in the serial phase (build 108487). Its 80 tests run one at a time, each with one to fivebun pm packspawns.pack()helper only checks that stderr lackserror:,warning:,failedandpanic:, tarballs are checked withtoMatchObject, and the--filename="out/foo.tgz"error case accepts any outcome.Fix
tempDirinstead of the sharedbeforeEachdirectory. The describes aredescribe.concurrent, the top-level teststest.concurrent.runPack()returns stdout and stderr, raw and normalized withnormalizeBunSnapshot. The normalized stdout masks the shasum, the integrity and the packed size, which depend on the compressor.erris""(or the exact$ scriptecho), the exact stdout, the exit code, and the full entry list withtoEqual. Error cases assert the exact message and that nothing was written.--rerun-each=3passes 249 of 249. CI debian 13 x64-asan: 10.7s before, 3.0s after (build 108529).Background
describe.concurrentruns a group's async tests up to--max-concurrencyat a time (20, or 5 in ASAN builds). Groups and top-leveltest.concurrenttests overlap, so a shared module-level directory is not safe.toMatchInlineSnapshotworks in concurrent tests, but one call site cannot hold different values acrosstest.eachrows. The tables compare a line array instead.Notes
--gzipis split into three rejected-level cases and one level 0 vs level 9 case, and the--filename="out/foo.tgz"error row is its own test. No test was removed or skipped.readTarballfrombun:internal-for-testingparses a tarball into its entries, shasum and integrity.expect.stringMatchinginstead of an exact value: the package.json size and the unpacked size in the tables whose rows change package.json (scoped names,workspace:specs,bundledDependenciesspelling), and in the two lifecycle tests whose scripts embedbunExe(), so the size depends on the path of the bun binary. On the darwin CI agent that path pushes package.json past 512 bytes and the size prints as0.58KB, so those two matchers accept any size format (build 108529 caught theNNNB-only version).//writes-1.1.1.tgzbut prints//-1.1.1.tgz; the name@//fails withfailed to open tarball file destination: ".../-/-1.1.1.tgz"(the old test only asserted a non-zero exit); transitive scoped bundled deps print without their scope (bundled dep3for@scoped/dep3);--dry-runprints the on-disk package.json size while a real pack prints the re-serialized size; empty files print as0KB. None of these is changed here.bun installstill runs once perworkspace:lockfile case (7 runs). They are workspace-only and contact no registry. ThebundledDependenciestests already builtnode_moduleson disk.bun pm packruns, 5 at a time.tempDirtree plusrunPack(dir).no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-pack.test.ts