test(install): cut 46 redundant spawns from bun-add-catalog.test.ts and assert whole files

[robobun]

Problem

• test/cli/install/bun-add-catalog.test.ts is the slowest install test file without a speed pass: 16 s on debian 13 x64-asan (build 100495), 1.3 s with a release build.
• The file spawns 272 bun processes for 149 tests. On a sanitized build a spawn costs about 145 ms, and about 105 ms of that is process startup (bun --version alone). The asan runner also caps describe.concurrent at 5 tests (src/options_types/context.rs:506). So the wall time of this file is about the spawn count divided by 5.
• 82 of the spawns are a bun install or bun install --frozen-lockfile run after the command under test. All 82 prove the same property: the bun.lock the command wrote is in sync with the package.json files it edited. Three more spawns are setup installs whose result no assertion used.
• Most tests asserted one field of each file (pkg1().dependencies, root().workspaces.catalog, lock.catalog) and proved the absence of a version with lockText().not.toContain("no-deps@2.0.0"). 16 tests checked only stderr not.toContain("error:") plus the exit code. Two error tests checked only that stderr contains error:.

Fix

• Keep the re-install check in 29 tests (33 runs, four of them are loop rows), one or two per bun.lock write path: a seeded entry (default catalog, named catalog, root target, --optional, a dist-tag the target already declared, a seed into the catalog that a catalog:name reference points at, an override, workspaces.catalog winning over a top-level catalog), a reused entry (a new consumer, a declared range, a tarball the target declared, catalogs.default spelled both ways, both objects present), a moved resolution (hoisted, isolated, named catalog), a replaced entry other members use, a converted direct range, nameless tarball positionals (member, root, --filter, mixed with a name, kept direct next to a different entry, local tarball), npm: aliases, --filter with mixed outcomes and with two catalogs, a plain bun add that uses the default entry, bun update --latest, bun remove. Drop the other 31 runs.
• Keep --frozen-lockfile where it is the subject (pnpm windows: bunx fails when node is not installed #8795, two tests) or the install step of the --lockfile-only tests. Drop the 12 runs that followed a re-install check or replaced one.
• Drop the setup install in three tests where nothing read its result: '*' alone edits the members..., edits only the filtered member..., and the bun pm pack test whose bun add writes the bun.lock that pack needs.
• Result: 272 spawns become 226 (144 add, 31 setup installs, 33 re-installs, 6 frozen installs, 12 update, pack, remove and install <pkg> runs). 149 tests before and after. No test was removed or merged.
• Every test that dropped a spawn, and most others, now assert more than before:
  • lockState() returns the catalog sections, every workspace row, and the resolved version of every non-workspace package from bun.lock. 92 call sites compare it with toStrictEqual. This replaces the single-field checks and the not.toContain version checks, and adds bun.lock coverage to tests that did not read it (placement, --peer, --only-missing, the override update rows, the plain-add rows).
  • root(), pkg1() and pkg2() are compared as whole objects.
  • expectOk compares the exact list of note: lines the command printed (none by default) and rejects warn:. This asserts the catalog entry X changed from A to B (also used by pkg2) note in six tests and its form without the suffix in one. This file never asserted that note before.
  • installSavesNothing also asserts the install reported (no changes). Tests with a tarball package pass relinks, because bun re-links a tarball package on every install, with or without a catalog (see details).
  • The 16 bare exit-code sites are gone. --silent tests assert stdout and stderr are empty. --dry-run and --no-save assert stdout reports installed no-deps@2.0.0. --lockfile-only asserts Saved bun.lock (3 packages) and that the frozen install reports 2 packages installed. The override update rows assert (no changes) and an unchanged root. bun remove asserts its - no-deps rows, that no-deps left the packages section, and that both catalog sections stayed.
  • The two unresolvable-package tests assert the exact error: GET <url> - 404 line and exit code 1. The pnpm windows: bunx fails when node is not installed #8795 tests assert error: lockfile had changes, but lockfile is frozen, the note that names the catalog, and that bun.lock kept the old entry.
• The name only in peerDependencies test is byte for byte unchanged, so install: move existing dependency when bun add is given an explicit group flag #36284 still applies on top of this branch (checked with git apply --check). No test was renamed or reordered.
• Verified with bun bd test test/cli/install/bun-add-catalog.test.ts, 149 pass. Timing, three interleaved rounds on one machine, before then after: 14.31 s / 13.44 s, 15.05 s / 14.08 s, 14.72 s / 13.60 s. Best of each: 14.31 s before, 13.44 s after. About 3.7 s of both numbers is fixed: the debug runner starts in 2.75 s and verdaccio in 1 s. USE_SYSTEM_BUN=1 bun test also passes, in 1.25 s.

Background

• bun add <pkg> --catalog writes the catalog entry into the root package.json and a catalog: reference into the target. After the install it derives the catalog sections of bun.lock from the final package.json files. The re-install check proves that derivation matched: a second bun install finds nothing to save.
• A re-install check costs one spawn and does not touch the registry, so it costs the same as the command under test. That is why the count of these checks, and not the work each bun add does, decides the wall time of this file.
• bun.lock keys a package by its name when it is hoisted and by <workspace>/<name> when it is nested under a workspace. lockState().resolved uses those keys, so a test with two resolutions of one name shows both.

Measurements and levers that did not pay

Spawns before (counted by wrapping Bun.spawn): 144 add, 34 setup installs, 64 re-installs, 18 --frozen-lockfile, 3 install <pkg>, 5 update, 2 pm pack, 2 remove. After: 144, 31, 33, 6, 3, 5, 2, 2.

Per spawn on the debug asan build, uncontended: bun --version 106 ms, bun add --catalog with a fresh cache 145 to 180 ms, no-op bun install 148 ms, bun add --dry-run 190 ms, --lockfile-only 183 ms. So flags that skip the install work do not save anything.

In-flight spawns during the file never exceeded 5, which is the asan default for --max-concurrency. The nested plain describes do inherit the concurrency. Completion order interleaves across describes.

A warm cache copied into every test dir (one bun install in beforeAll, cpSync per test) makes every bun add resolve without a registry request, because a cached manifest is fresh for 300 s (src/install/npm.rs:578). It still lost about 3 s here: the cpSync runs in the sanitized runner process and costs more than the downloads it saves. Not included.

With CI set, the harness runs verdaccio on the build under test. On the debug build that start takes 32 s here. The 149 tests take the same 13 to 14 s either way, so request serving is not the bottleneck on the asan lane either. That start time belongs to the harness and is out of scope for this file.

test.each tables were not introduced. They would not remove a spawn, and they rename tests, which #36284 and the slow-test tracking key on.

Observation, unrelated to catalogs: after a package was installed from a tarball url or a local tarball, every later bun install prints 1 package installed for it again (reproduced with plain bun add <url> in a project without workspaces, released build). The lockfile is not re-saved. The relinks option in installSavesNothing only skips the (no changes) check for the six tests with such a package.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)

• test/cli/install/bun-add-catalog.test.ts

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a1540b19-1d82-45e4-a765-129543f0ee71

📥 Commits

Reviewing files that changed from the base of the PR and between 4c68990 and 46fbe1b.

📒 Files selected for processing (1)

• test/cli/install/bun-add-catalog.test.ts

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: closed. The CI numbers did not support the speed claim (16.45 s in this PR's build #100573 against 15.68 s in baseline build #100495 on the asan lane, and the file's shard is not the one that sets the lane's wall time). See the closing comment below for the details and for the small assertion-only change that could be opened on request.

Verification that was done: bun bd test test/cli/install/bun-add-catalog.test.ts and USE_SYSTEM_BUN=1 bun test on the file, 149 pass each. Spawns 272 before, 226 after, counted with Bun.spawn wrapped.

[claude]

I reviewed this PR and didn't find any bugs. Because it removes 46 verification spawns (re-install / --frozen-lockfile checks) and replaces them with the new lockState() shape assertion across ~90 tests, a human look at the coverage tradeoff would still be worthwhile — specifically whether the retained 33 re-install checks cover every distinct bun.lock write path the dropped ones used to guard.

Checked: expectOk dropping the panic: check matches CLAUDE.md guidance; the new strict note: assertion is a superset of the old stderr.not.toContain("note:") sites; lockState() correctly filters @workspace: rows and conditionally spreads catalog/catalogs; the three removed setup installs each have their result covered by a later step in the same test.

Extended reasoning...

Overview

This PR refactors test/cli/install/bun-add-catalog.test.ts (test-only, ~1000 lines of diff, 149 tests before and after). It cuts 46 redundant subprocess spawns (272 → 226) by dropping re-install and --frozen-lockfile verification runs from tests where the same property is now asserted via a new lockState() helper that compares the full catalog/catalogs/workspaces/resolved shape of bun.lock with toStrictEqual. It also strengthens most remaining assertions: whole-object root()/pkg1()/pkg2() comparisons, strict note: line matching in expectOk, exact error messages in the two failure tests, and (no changes) in installSavesNothing.

Security risks

None. This is a test file for bun add --catalog; no production code, auth, or crypto is touched.

Level of scrutiny

Medium-high. Per REVIEW.md, "Never silently weaken, skip, or delete an existing test or safety net. Every deletion needs a stated reason or replacement." The PR does both — the description enumerates exactly which 29 tests keep re-install checks (one or two per bun.lock write path) and why the other 31 are redundant, and every dropped check is replaced by a stricter lockState() assertion. However, the re-install check and the lockState() check prove slightly different properties: re-install proves bun itself computes the same lockfile from the edited package.json files (self-consistency), while lockState() proves the lockfile matches a hand-authored expected shape. The PR's argument that 33 retained re-install checks suffice to cover every distinct write path is well-reasoned but is a coverage judgment a maintainer should confirm.

Other factors

• The bug hunting system found nothing. I verified the new helpers: lockState() correctly uses row[0].includes("@workspace:") to filter workspace rows and conditionally spreads catalog/catalogs so toStrictEqual still catches unexpected sections; expectOk's new note: filter is order-sensitive and defaults to [], which is strictly stronger than the old scattered not.toContain("note:") checks; dropping the panic: check aligns with the repo rule that such checks never fail in CI.
• The three removed setup installs are each justified in the description (e.g., the second bun pm pack test now relies on the preceding bun add to write bun.lock, which is the documented behavior).
• The diff is large and touches nearly every test in the file. No test names or ordering changed (verified against the description's #36284 compatibility note), so slow-test tracking and the pending PR that edits name only in peerDependencies are unaffected.
• Both bun bd test and USE_SYSTEM_BUN=1 bun test pass per the author's verification, so the strengthened assertions match current behavior.

[robobun]

The review above asks whether the kept re-install checks cover the write paths of the dropped ones. Here is the mapping, dropped check first, kept test after the arrow. The count in parentheses is the number of runs when a site is a loop body. The 31 runs add up.

• "--catalog=" and "--catalog= " (2) -> default catalog from a workspace member. Same seed into the default catalog, only the flag spelling differs.
• a bare name reuses an existing catalog entry (1) -> the test itself compares bun.lock byte for byte with the file bun install wrote before the add. Reuse is also kept in an existing entry wins over the range the target declares directly.
• a direct exact pin in devDependencies is cataloged verbatim (1) -> a tarball url the target declares is cataloged verbatim (a declared literal cataloged as written) and --optional (a group other than dependencies).
• target already on a named catalog reference rows (3) -> an explicit version inside the named entry's range moves the resolution within that catalog, in the same block, writes the same catalogs.testing section. The replaced row is also covered by explicit version replaces an entry other members reference.
• an explicit version is written into that catalog (1) -> the two seeds that catalog and keeps the reference rows in the same block write the same catalogs.legacy section.
• name@tarball-url is written verbatim, dist-tags become a range (1) -> a tarball url the target declares (tarball entry), a dist-tag already in package.json (dist-tag seed), mixed with a named package (two kinds in one command).
• the entry is inserted in alphabetical order, into a named catalog, the suggested name@url spelling (3) -> the a tarball url is cataloged under the resolved name rows and mixed with a named package keep the tarball entry path. named catalog with other entries and other named catalogs present keeps the named section. The order of keys only exists in package.json, and each test still asserts it.
• an identical entry is reused (1) -> a different existing entry keeps the package direct keeps the other branch of the same nameless-positional settlement, and reuse is kept as above.
• Nine --filter tests (9) -> members declaring different ranges (one member switched, one kept, two resolutions in one filtered command) and each member keeps its own catalog (two catalog sections in one filtered command). --filter puts every selected member on the entry is also kept. The dropped tests differ in which members switch, which is decided in package.json, and each of them now asserts every workspace row of bun.lock.
• --catalog=default with an explicit range replaces the singular catalog (1) -> the --catalog=default reuses the entry spelled catalog row keeps the alias, and the replace is kept as above.
• Eight plain bun add tests (8) -> bare name from a member keeps the path that writes the reference without the flag. run from the workspace root keeps the root row, the --catalog reuses the entry spelled catalogs row keeps the catalogs.default section, and --filter puts every selected member on the entry keeps the filtered rows.

The 12 dropped --frozen-lockfile runs: 8 followed a kept re-install in the same test, 1 was in an identical entry is reused (covered above), and 3 stood alone. Those 3 are covered by both objects present: the one defining the name is reused (both sections), the catalogs.default reuse row, and an override decides what --catalog records (an override plus an add). --frozen-lockfile after an add is still exercised as the subject of the two pnpm #8795 tests and as the install step of the two --lockfile-only tests.

[robobun]

Closing this after a self-review of the premise.

• The speed claim does not hold up in CI. In this PR's build (#100573) the file took 16.45 s on debian 13 x64-asan. In the baseline build (#100495) it took 15.68 s. The 0.9 s measured locally is inside the noise of that lane.
• In build #100495 the shard that runs this file finished about 110 s before the slowest shard of the lane. A faster version of this file does not change the lane's wall time.
• bun-audit.test.ts: install each project once and run the report cases concurrently #39034 made the same kind of change to bun-audit.test.ts with a larger, CI-measured gain (23.2 s to 18.0 s on the same lane). It was closed as not moving the numbers enough. This PR gains less than that.

Without the speed gain, what is left is a rewrite of the assertions in a file that other open PRs add tests to (#36284 for one). The coverage that is new here is small: the exact note: lines, including the catalog entry X changed from A to B note that no test asserted before, and a few bun.lock shape checks. That is a change of about 100 added lines, and it does not need this diff. I can open it separately if a maintainer wants it.

The branch stays available for reference.