Repository navigation
test(install): run bun-pm-pkg tests concurrently and verify via file reads - #36344
Conversation
…reads The file spawned 131 bun subprocesses serially, taking ~35s under a debug+ASAN build locally (12s in CI). Changes: - describe.concurrent: every test owns its own tempDir, so independent spawns overlap instead of serializing. - Share one read-only tempDir across the pure `get` tests that never mutate package.json. - Verify `set`/`delete`/`fix` by reading package.json directly instead of spawning a second `bun pm pkg get` per test. This also turns string-match assertions into structural toEqual/toMatchObject checks. - Run the per-notation `get` loops via Promise.all. - Drop redundant rmSync around `using tempDir` fixtures. - Assert stdout/stderr before exitCode per the test conventions. Result locally (debug+ASAN): 34.9s -> 12.7s, 74 pass / 3 todo unchanged, 219 expect() calls (fewer because toContain chains became single toEqual calls on parsed JSON).
|
Warning Review limit reached
Next review available in: 12 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 |
|
Test-only speedup: b0bb4f5 addresses two self-review findings: restores CI status: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re: the duplicate check against #33704: that PR is a multi-file test-speedup omnibus that is currently conflicting with main (last updated July). Its change to This PR is standalone for the one file and additionally cuts the spawn count (131 → 93) by verifying |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup with no coverage loss.
What was reviewed:
- Verified
tempDirfrom harness has bothSymbol.disposeandSymbol.asyncDispose, so switchingawait using→usingis valid; the boxed-Stringcwdpath was already exercised by the pre-existingbracketDir/workspaceDircases. - Checked every test now reading
readonlyDiris a pureget/helpthat never mutatespackage.json, so sharing one fixture underdescribe.concurrentis race-free. - Confirmed the removed follow-up
getspawns are replaced by directpackage.jsonreads asserting the same (or a strictly tighter) property, and thetoContain→toEqual/toMatchObjectswaps do not weaken any assertion. 74 pass / 3 todo unchanged.
Extended reasoning...
Overview
This PR refactors a single test file, test/cli/install/bun-pm-pkg.test.ts, to cut wall-clock time from ~35s to ~13s under debug+ASAN. The changes: wrap the suite in describe.concurrent with per-test tempDir for mutating tests, share one read-only fixture across pure get tests, replace verification-via-second-spawn with direct Bun.file(...).json() reads, batch per-notation get loops with Promise.all, drop redundant rmSync/try…finally around using tempDir, reorder assertions to check output before exitCode, and tighten toContain chains into toEqual/toMatchObject on parsed JSON. No src/ code is touched.
Security risks
None. Test-only change; no runtime, network, auth, or dependency surface is affected.
Level of scrutiny
Low-to-moderate. The main risks for a concurrent-test refactor are (a) shared-state races and (b) silently weakened assertions. I checked both: every test that writes now owns its own makeTestDir() / tempDir(...), and the shared readonlyDir is only referenced by get/help tests that never write. On assertions, each removed secondary bun pm pkg get spawn is replaced by a direct read of package.json that asserts an equal-or-stronger property (e.g. toEqual on the parsed object vs. the old toContain on stdout substrings). No test is deleted or skipped; the reported 74 pass / 3 todo count is unchanged.
Other factors
I confirmed in test/harness.ts that tempDir returns a DisposableString implementing both Symbol.dispose (sync rmSync) and Symbol.asyncDispose, so the await using → using change is safe and the removed manual rmSync calls were genuinely redundant. Passing the DisposableString directly as cwd and into join(...) was already done by the existing bracketDir/workspaceDir cases, so that coercion path is proven. The readonlyDir lifetime is handled explicitly via beforeAll/afterAll (the String(...) wrap discards the disposable so the dir survives until afterAll). The change also aligns the file with repo test conventions from CLAUDE.md/REVIEW.md (concurrent subprocess suites, using tempDir, output-before-exitCode, exact assertions over toContain). PR author reports three consecutive flake-free runs.
…ive spawns Self-review surfaced two issues in the bun-pm-pkg.test.ts rewrite: - `pm pkg get` on a float value (pm_pkg_command.rs format_json, non-integer ENumber branch) lost its only coverage when the follow-up `get` spawn was replaced by a direct file read. Add testFloat: 3.14 to the shared fixture and assert its `get` stdout in the data-types batch so the path stays exercised. - Four Promise.all fan-outs ran inside a `using dir` scope with expectSuccess=true. If one child exited non-zero the helper threw, Promise.all short-circuited, and the tempDir was rmSync'd while sibling processes were still alive with it as cwd (EBUSY/SuppressedError on Windows). Pass expectSuccess=false at those sites and rely on the explicit exit-code assertions that follow, so every child is awaited before disposal.
There was a problem hiding this comment.
LGTM — test-only speedup with tightened assertions and preserved coverage.
What was reviewed:
- Concurrency safety: mutating tests each own a
using tempDir; the sharedreadonlyDiris only touched by pureget/helptests and removed inafterAll. Promise.allfan-outs inside ausingscope passexpectSuccess=falseso every child is awaited before disposal (b0bb4f5).- Float stdout coverage: restored via the
testFloatfixture field and the data-types loop; other removed follow-upgetspawns are covered by the dedicatedgetsuite. toContainchains replaced bytoEqual/toMatchObjecton parsed JSON are equal-or-stronger; 74 pass / 3 todo unchanged.
Extended reasoning...
Overview
Single-file, test-only refactor of test/cli/install/bun-pm-pkg.test.ts. Switches the outer describe to .concurrent, gives every mutating test its own using tempDir, shares one read-only fixture across pure get tests, replaces follow-up bun pm pkg get verification spawns with direct package.json reads, parallelises per-notation loops via Promise.all, drops redundant rmSync/try…finally around using fixtures, reorders assertions to check stdout/stderr before exitCode, and tightens toContain chains into toEqual/toMatchObject on parsed JSON. No src/ changes.
Security risks
None. Test-only; spawns the local bunExe() against per-test temp dirs, no network, no new inputs.
Level of scrutiny
Low-medium. The risks with a serial→concurrent conversion are shared mutable state and fixture lifetime vs. child-process lifetime. I checked both:
- Shared state: the only cross-test fixture is
readonlyDir, created inbeforeAll(wrapped inString()so it isn't auto-disposed) and removed inafterAll. Every test that references it is a read-onlyget/helpinvocation; everyset/delete/fixtest allocates its ownusing dir = makeTestDir(). - Fixture lifetime: the four
Promise.allfan-outs that run inside ausing tempDirscope passexpectSuccess=false, sorunPmPkgnever throws mid-batch and every child is awaited (viaawait using proc+proc.exited) before the scope exits and the dir is removed. The remaining fan-outs targetreadonlyDir, which outlives all tests. This was the concern addressed in b0bb4f5 and it checks out.
Other factors
- Coverage: the removed secondary
getspawns only re-read a value the test just wrote;getitself is still exercised by the dedicatedgetsuite plus the notation/compat blocks. The one case where the follow-upgetwas the only stdout check for a value shape (float3.14) was restored via thetestFloatfixture field and the data-types loop. Readingpackage.jsondirectly is an equal-or-stronger assertion on the on-disk result. - Assertion strength: replacing multi-
toContainon raw stdout withtoEqualon parsed JSON is strictly tighter (exact shape, no substring false positives). Test count unchanged at 74 pass / 3 todo. - Conventions: matches CLAUDE.md/REVIEW.md guidance —
tempDirfrom harness withusing, stdout/stderr asserted beforeexitCode,test.concurrentfor independent subprocess tests, no manual cleanup alongsideusing. - CI on 9a3387d passed this file on every lane; the one red job was an unrelated proxy stress test.
- Overlap with stale omnibus #33704 is acknowledged; this is standalone and goes further (spawn reduction, structural assertions).
What
test/cli/install/bun-pm-pkg.test.tsspawned 131bun pm pkgsubprocesses serially and verified everyset/delete/fixby spawning a follow-upget. On a debug+ASAN build this took ~35s locally (reported as 12s ondebian 13 x64-asanin CI build #84540).Changes
describe.concurrent: every mutating test owns its owntempDir, so independent subprocess spawns overlap instead of serializing.tempDiracross the puregettests that never mutatepackage.json.set/delete/fixresults by readingpackage.jsondirectly instead of spawning a secondbun pm pkg getper test (131 → 93 spawns).getloops viaPromise.allso their spawns overlap.rmSync/try…finallycleanup aroundusing tempDirfixtures.exitCodeper the test conventions.toContainchains on multi-keygetoutput withtoEqualon the parsed JSON, and replace per-field.toBeclusters withtoMatchObject.Why this is correct
Every test still spawns the
bun pm pkgsubcommand it is exercising, so CLI coverage is unchanged. The only spawns removed are the secondarygetcalls that re-read a value the test just wrote; readingpackage.jsondirectly asserts the same (or a stronger) property on the on-disk result. No test is deleted or skipped: the file still reports 74 pass / 3 todo.Verification
Local debug+ASAN (
./build/debug/bun-debug test test/cli/install/bun-pm-pkg.test.ts):The lower
expect()count is from collapsing multi-linetoContainchains into singletoEqual/toMatchObjectassertions on parsed JSON, which are strictly tighter.Ran 3 consecutive times with no flakes.
[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