Repository navigation
Conversation
…urrently and tighten its assertions The first 50 cases shared the dummy registry's module-level handler, request counter and package dir, so they ran one after another. Each case now gets a context of its own (createTestContext) and runs as it.concurrent. The per-name registry builds its tarballs with Bun.Archive instead of spawning bun pm pack for every version. Installs and updates now assert the exact stderr (minus the two progress lines), the cases compare whole package.json objects and the matching bun.lock workspaces entries, and the update summaries are inline snapshots. The verdaccio half of the file is unchanged. Locally (debug build) the file goes from 42 to 45 seconds to 22.
|
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 |
|
Status: test-only change, ready for review. Measured with CI (build 101233): the file passed on all 10 lanes. In its logs the 50 converted cases take about 3s on x64-asan (about 15s for the old file at the same point of the job, build 100212), 1.5s on windows 2019 x64 (3.4s) and 2s on windows 11 aarch64 (6.8s). The file's totals in a PR build are dominated by the verdaccio start, which is slow while the job is still starting its services; the PR description has the breakdown. The two red tests in the build ( Self-review: no defects. It found that the fan-out and per-name groups duplicate coverage in the verdaccio half and in |
![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 (~50 cases rewritten to it.concurrent with per-test contexts, new expectWorkspaces/setupWorkspaces/run helpers, and 18 new inline snapshots pinning exact stderr/stdout), a human look would still be worthwhile — mainly to confirm the tighter assertions don't flake across CI platforms.
What was reviewed
- Verified the
createTestContext/setContextHandler/dummyRegistryForContextAPI indummy.registry.tsmatches the new usage, and thatlinker: "hoisted"moves cleanly from CLI flags into the context's bunfig. - Checked
Bun.Archive({...}, { compress: "gzip" }).blob()is a real API used elsewhere in the suite. - Confirmed the removed
for (const { input } of [...])loop had a single element so itstilde === falsebranches were dead, and that the #5888 case still asserts byte-identical lockfiles across two runs plus registry-request counts.
Extended reasoning...
Overview
This PR rewrites the first ~50 test cases of test/cli/install/bun-update.test.ts to run concurrently using the per-test-context API from dummy.registry.ts instead of the deprecated module-level package_dir/setHandler/requested globals. It introduces shared helpers (testContext, serve, run/runInstall/runUpdate, setupWorkspaces, expectWorkspaces, nestedBazRepo, fanOutRepo, perNameRegistry), replaces not.toContain("error:") with exact stderr matching (Saved lockfile\n or empty), converts stdout arrays to toMatchInlineSnapshot, and swaps bun pm pack subprocesses for in-process Bun.Archive tarball construction. The verdaccio second half of the file is unchanged. Net: −1179/+778 lines, one file, test-only.
Security risks
None. This touches only a test file; no auth, crypto, network, or production code paths are modified.
Level of scrutiny
Medium. It is test-only and cannot affect users, but it is a large rewrite of existing coverage with materially tighter assertions (exact stderr, whole-object toEqual on every workspace's package.json plus the mirrored bun.lock workspaces section). The repo's review rules put weight on not silently weakening or deleting existing tests; here most assertions get stronger, but a few things shrink (the #5888 loop drops from 5 to 2 iterations, the single-element for loop and its dead tilde === false branches are removed). Those are well-justified in the description, but the sheer volume of assertion changes and the move to it.concurrent (which changes scheduling and could surface platform-specific stderr ordering or progress-line variations) warrant a maintainer confirming the CI matrix is green before merge.
Other factors
- The PR description is unusually thorough — it documents every assertion change, why the loop was dead, the local before/after timings, and migration notes for open PRs that add cases here.
- I verified
createTestContext({ linker: "hoisted" })writes the linker into bunfig, so dropping--linker=hoistedfrom every command line is behavior-preserving;useTextLockfilealso carries it forward. bun-install.test.tsandbun-remove.test.tsalready use this same concurrent context pattern, so this is following an established precedent rather than inventing one.- No prior human or bot review comments to address; CodeRabbit skipped the file.
Given the scale (~1000 lines rewritten across 50 cases) and the exact-output snapshots that could behave differently on Windows/ASAN, I'm deferring rather than approving.
|
On the platform question from the review: the file passed on all 10 lanes of build 101233 (both Windows lanes, x64-asan, darwin, alpine). The snapshots contain package names and versions and the normalized header only. The two lines whose content varies ( |
|
This is Claude (an AI assistant) writing on Jarred's behalf. Included in #44968. |
Problem
test/cli/install/bun-update.test.tstakes 23s on debian 13 x64-asan, 11s on windows 11 aarch64, 6s on windows 2019 x64 and 2.2s to 2.9s elsewhere (build 101153): it starts its bun processes one after another.package_dirofdummy.registry.ts, so they run serially. The 106 verdaccio cases are already concurrent.runInstallonly checked stderr forerror:, and most cases checked one.dependencies.bazfield per file.Fix
createTestContext()and runs asit.concurrent. The hooks are gone. Locally these cases take 25.8s serially and 8.4s concurrently.perNameRegistrybuilds its tarballs withBun.Archiveinstead of 20bun pm packchildren. The issue bun update always change lockb file #5888 case runs 2 updates instead of 6.runInstallrequires stderr to be exactlySaved lockfile(empty with--frozen-lockfile),runUpdatethat line or nothing.expectWorkspaceschecks whole package.json objects and the matchingbun.lockworkspacesentries in one call. The install, update and outdated summaries are 18 inline snapshots.bun bd test test/cli/install/bun-update.test.ts, 156 pass. Whole file locally (debug build): 42.5s and 45.3s before, 21.8s to 22.5s after (4 runs). In CI (build 101233) the 50 cases take about 3s on x64-asan instead of 15s. The totals of a PR build do not show it: see the notes.Background
dummy.registry.tsroutes each test's requests by a/test-N/URL prefix, andcache = falsekeeps the install cache in the test dir. The cases share nothing.bun testruns consecutiveit.concurrentcases together (5 at a time under ASAN, 20 otherwise). A hook or a plainit()between them is a barrier.bun.lockrepeats each workspace's name, version and dependency groups underworkspaces, keyed by path (""is the root).Notes
Reading the CI numbers. The runner puts the files a PR touches first in the job, so in a PR build this file runs while the job is still starting its services (the
coordinator: ... readylines land in the middle of its output). That inflates the verdaccio start in the middle of the file from under 0.5s (build 101153, where the file ran 8th to 44th) to 4s to 11s, in this PR's build and in any other PR's build that touches the file. The dots reporter flushes about once a second with a timestamp, so the three parts can still be read off the logs. Build 100212 (#38770, the same file plus one case, also first in the job) is the comparison:At the file's normal position (build 101153: 22.7s on x64-asan, of which the first 50 cases were about 13s; 5.7s and 10.8s on the two Windows lanes, first 50 cases about 2.3s and 4.5s), the same 3x to 5x on that part gives roughly 12s, 5s and 8s. The verdaccio start and the verdaccio half are the rest and are unchanged. On a quiet machine with the release build, the whole file goes from 2.7s to 2.8s to 1.8s to 2.2s (3 runs each).
The file also passes with the release build (
USE_SYSTEM_BUN=1).bun-install.test.tsandbun-remove.test.tsalready run concurrently on the same context API.The verdaccio half of the file (from
describe("bun update <name> semantics")on) is byte for byte unchanged. It is now about 17s of the 22s locally, all of it the 3 to 4 processes each case needs (install, sometimes a second install to make the state stale, the update, and the--frozen-lockfileinstall inexpectInSync). Removing those would remove coverage, so they stay.Considered and not done: writing the starting
bun.lockby hand instead of runningbun installin the 40 workspace cases. It would save about 50 of the roughly 130 child processes left in this half, but the synthesized lockfile would replace the state a real install leaves behind (nested copies, what is installed, whether the first update re-saves), which several cases are about. It is about one second of the file in CI.Assertion changes in detail:
runremoves the two progress lines (Resolving dependencies,Resolved, downloaded and extracted [N]) from stderr and normalizes stdout, so callers assert stderr exactly and snapshot stdout.runInstallreturns the stdout; the three single-package cases snapshot it.Saved lockfilewhere the lockfile must move, and empty stderr where nothing may be written:--dry-run,--no-save, the second run of the idempotence case, a named update run from a workspace that does not own the row, and the bun update always change lockb file #5888 case (which now also compares against the lockfile the install wrote).setupWorkspaces,fanOutRepoandnestedBazRepocheck the lockfile mirrors the files right after the install, so a later "unchanged" means something.bun.lockis byte-identical. The fan-out cases check every untouched file is byte-identical and every rewritten one differs in the version only.--no-savecase used to install 0.0.5 and then check that 0.0.5 was installed. It now installs from a registry that only has 0.0.3 and bumps it, like the dist-tag cases, so node_modules really moves while package.json and bun.lock stay byte-identical.overridescases also check the member's literal is left alone.--recursive tolerates a member with no dependency groupsalso checks the root moved.bun outdated -ris an exact snapshot of the empty output instead ofnot.toContain("baz"). The UTF-8 case checks the whole row↑ baz 0.0.3 → 0.0.5after stripping the colors, and that package.json was rewritten.--filter excluding a workspacechecksbun pruneprinted nothing to stderr.for (const { input } of [...])loop had one input, sotildewas always true. The loop and its dead branches are gone, and the stdout arrays are snapshots. Those two cases lost the(~)suffix of their names. All other names are unchanged.lockedSharedResolutionsassertions, plus exact stderr and snapshots. install: keep one copy of a package when one version satisfies every range #38770 changes their behavior, so nothing else about them is pinned.Overlap found by the self-review, left for a follow-up. Two of the converted groups duplicate coverage that exists elsewhere. The fan-out group (
FAN_OUT, 7 cases) overlaps the verdacciobun update <name> -r / --filterdescribe in this file:-r/--filter *rewrites every workspace,--filterrewrites one, the two error texts,--dry-run, and the indentation case. Only-Fand the stale registry move (root's exact pin and pkg-b's^0.0.3stay while pkg-a's~moves) are not in the describe. The per-name group (7 cases) overlapsbun-update-transitive.test.ts, whoseserveRegistryis the same Bun.Archive construction asperNameRegistry, including the case where a named update run from the root leaves a member's entry alone. This PR keeps all of them: the task was to speed the file up without removing cases and without touching the verdaccio half, which the open PRs listed below add to. Consolidating the two groups (about 120 lines and about 20 child processes) and lifting the in-process registry intodummy.registry.tsare separate changes. In the same spirit,testContext()exists only becausecreateTestContext()returns a plain object; giving that object a[Symbol.dispose]indummy.registry.tswould remove this wrapper and thewithContextcopies inbun-install.test.tsandbun-install-tarball-integrity.test.ts.dummy.registry.tsis shared by about nine files, so that is also left out of this PR.perNameRegistryshould not grow.For the open PRs that add cases to this half (#35542, #34109, #29794, #38687, #38770): a case becomes
it.concurrent, starts withusing ctx = await testContext();, callsserve(ctx, versions, urls)instead ofsetHandler(dummyRegistry(urls, versions)), readsctx.package_dir,ctx.registry_urlandctx.requestedinstead of the module-levelpackage_dir,root_urlandrequested, and starts bun withrunInstall(ctx),runUpdate(ctx, [...args])orrun(ctx, [...args]).[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · 6 surviving concerns
bun outdated#34109 needs; the BUN_…update -rmoves pkg-a'sbaz: "latest"0.0.3->0.0.5 but prints no row, while--dry-run/--lockfile-onlyon the identical repo, the~0.0.3control, and the named form all print `^ baz 0.0…-r / --filterdescribe's matrix (both copies landed together in install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 five days earlier); its residual is two shapes,-Fand "declaring member whose literal cannot move under n…46 concerns were raised and did not survive verification.