Repository navigation
test(update --interactive): run concurrently and replace dead formatting assertions - #36348
Conversation
…ssertions
The file spawns two subprocesses (install + update) per test in 32
serial tests, ~14s wall time on debug/ASAN. Switch to describe.concurrent
(each test owns its own tempDir; the registry is read-only).
The first six tests wrote a JSON blob to bun.lockb (binary lockfile) or
no lockfile at all, so update --interactive failed at lockfile parse and
never reached the column-formatting code they were named for. The only
assertions were expect(stderr).not.toContain('panic'|'segfault'|...).
Rewrite those six to install real registry packages, reach
process_multi_select, and assert on the rendered Current/Target/Latest
table. Drop the remaining .not.toContain('panic') checks and add stdout
checks before exitCode where missing.
Extract install() / updateInteractive() helpers so an install failure
surfaces stderr instead of 'Expected 0 Received 1'.
Local (bun bd test, debug+ASAN): ~14.2s -> ~7.4s, 32/32 pass.
|
Warning Review limit reached
Next review available in: 19 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 |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only refactor that replaces provably-dead assertions with real ones and cuts wall time ~2x. The inline nit about the two dropped //todo markers is worth a follow-up but not blocking (no coverage lost vs main).
Extended reasoning...
Overview
This PR rewrites test/cli/update_interactive_formatting.test.ts (test-only, no runtime code touched):
- Wraps the suite in
describe.concurrentand extractsinstall()/updateInteractive()helpers that drain stdout/stderr/exited concurrently and surface stderr on install failure. - Rewrites the first six tests, which previously wrote a JSON blob to
bun.lockb(the binary lockfile) sobun update -ibailed at lockfile parse and never reached the column-layout code the test names describe. Their only assertions wereexpect(stderr).not.toContain("panic"), which pass on any stderr. The rewrites install real Verdaccio packages, reach the selection prompt, and assert on the rendered header/rows. - Drops the remaining
.not.toContain("panic")assertions, moves stdout assertions ahead ofexitCodeassertions, removes strayconsole.log/console.errordebug output, and de-collides twotempDirprefixes.
Security risks
None. Test file only; spawns bunExe() in isolated tempDirs against the local Verdaccio registry with cache = false. No new network endpoints, no auth/crypto paths.
Level of scrutiny
Medium — large diff (~900 lines net removed) but the bulk is mechanical helper extraction. The substantive part is the six rewritten tests, which are strictly stronger than what they replace (the old versions could not fail for any reason related to their stated purpose). describe.concurrent is safe here: tempDir uses mkdtempSync (unique per call), each test sets cache = false, and the registry is read-only after beforeAll. The helpers use await using on the subprocess and Promise.all to drain both pipes, matching the harness conventions in REVIEW.md.
Other factors
- Author ran
bun bd teston the after-state 8 times with no failures and reports 32/32 pass at ~7.4s vs ~14.2s. - One nit was filed inline: two
//todo: actually check the catalog was updatedcomments were deleted without tightening the still-vacuoustoMatch(/^~/)assertions they flagged. This loses a signpost but no coverage relative to main, so it doesn't block. - The new assertions pin specific Verdaccio fixture versions (
1.0.10,2.0.0,1.0.2); other tests in this file already do the same (toBe("2.0.0")etc.), so this is consistent.
…rtions Per review: the column-alignment assertions used \s+ which matches single-space or wrapped output. Switch to exact-space toContain strings captured from the real output so a name_padding/current_padding regression fails. The mixed-sections test now asserts all four section headers and their tagged package rows. The filter-catalog assertion now checks the exact updated value (~3.0.0) and that the filtered-out catalog entry is unchanged.
…on change
The previous commit's message described this change but the hunk was
dropped. The filter-catalog assertion is now toBe("~3.0.0") and
checks that the filtered-out catalog entry stayed "^1.0.0".
There was a problem hiding this comment.
LGTM — 27223a0 lands the missing filter-catalog hunk, so both prior nits are addressed.
What was reviewed:
- Concurrency safety: each test uses
mkdtemp-backedtempDir+cache = false, registry is read-only afterbeforeAll; the two duplicated tempDir prefixes were suffixed-2. install()/updateInteractive()helpers drain stdout/stderr/exited viaPromise.alland useawait using;String(dir)handles theDisposableStringcwd.- The six rewritten tests now reach
process_multi_selectand assert exact-spacing rows / section headers instead ofnot.toContain("panic")on a lockfile-parse error. - Remaining
catalogs.prodprefix check left as-is per the author's note that onlydevupdates in that scenario today.
Extended reasoning...
Overview
Test-only refactor of test/cli/update_interactive_formatting.test.ts: wraps the suite in describe.concurrent, extracts bunfig() / install() / updateInteractive() helpers to remove ~900 lines of duplicated spawn boilerplate, and rewrites the first six tests — which previously wrote a JSON blob to bun.lockb (binary format) and asserted only stderr.not.toContain("panic") against a lockfile-parse error — to install real Verdaccio fixtures and assert on the rendered Current/Target/Latest table. The rest of the file swaps exitCode === 0-only checks for stdout-first assertions and drops stray console.log/console.error debug prints. No source under src/ is touched.
Security risks
None. Test-only change; spawns bun install / bun update -i against a local Verdaccio registry in per-test temp dirs. No new network endpoints, credentials, or untrusted-input parsing.
Level of scrutiny
Low-to-moderate. The diff is large by line count but almost entirely mechanical dedup; the substantive new surface is ~35 lines of helper plus six rewritten test bodies. Worst case is a CI-only assertion mismatch on the exact-spacing strings, which would surface immediately and is not a product regression.
Other factors
- Concurrency: verified
tempDirusesfs.mkdtempSync(unique per call), each test setscache = false, andVerdaccioRegistryis read-only afterbeforeAll, so concurrent execution shares no mutable state. The PR also de-duplicated the two colliding tempDir prefixes even thoughmkdtempalready made them unique. - Helper correctness: both helpers use
await usingon the subprocess andPromise.all([stdout.text(), stderr.text(), exited])so pipes can't deadlock;install()throws with stderr embedded on nonzero exit (better than the old bareexpect(exitCode).toBe(0)). - Prior feedback: my two earlier nits on deleted
//todomarkers were addressed — 27223a0 tightens the filter-catalog assertion totoBe("~3.0.0")and adds a check that the filtered-out entry stayed^1.0.0. The multi-catalogprodentry stays a prefix check with a stated reason (onlydevupdates in that fixture today); the siblingdevassertion pins^2.0.0so catalog updating is still covered. - Coverage direction: strictly upward — every replaced assertion was either vacuous (
not.toContain("panic")on an unrelated error) or weaker than its replacement. No existing meaningful assertion was removed. - No CODEOWNERS entry for
test/cli/. PR description reports 8 clean local runs; CI build #85125 will confirm cross-platform.
|
The changed file ( |
…#36442) ### What does this PR do? `test/cli/update_interactive_formatting.test.ts` spent ~8s wall time on the debian 13 x64-asan lane (build #85633). #36348 already made it `describe.concurrent`; this follow-up removes the remaining redundant subprocess spawns and tightens the assertions. #### Consolidation (32 → 24 tests, each spawns `bun install` + `bun update -i`) - Three column-alignment dry-run tests collapsed into one 4-dependency fixture that exercises short/long names (5–22 chars) and short/long latest versions (`2.0.0` vs `1.0.10`) in a single table. - Two workspace/catalog render dry-run tests collapsed into one fixture with a catalog-backed dep and a direct workspace dep. - "basic interactive update with select all" removed (identical setup and strict subset of "update packages when 'a' is used"). - Two dry-run tests merged (same setup; surviving test keeps the stronger "Dry run" + byte-equal `package.json` check). - "interactive update with workspaces.catalogs structure" merged into "catalog updates in workspaces.catalogs object" (union of both tests' assertions). - "preserve version prefixes in catalogs" / "scoped packages in catalogs" merged; the surviving test uses catalog entries that are actually outdated (`@types/no-deps`, `dep-with-tags ~1.0.0`, `no-deps >=1.0.0 <1.1.0`) so each prefix assertion now observes a real rewrite. The previous entries (`@scoped/has-bin-entry ^1.0.0`, `dep-with-tags >=1.0.0`, `a-dep >=1.0.5`) all resolved to latest on install so the `toMatch(/^.../)` checks matched the unchanged input. - "mixed workspace and catalog dependencies" removed (strict subset of "interactive update with mixed dependency types": both check `workspace:*` and `catalog:` are left untouched and direct-dep prefixes survive). #### Assertion tightening - `toMatch(/^~|^\^|^>=/)` replaced with `toEqual({...})` on the full `catalog`/`dependencies` object. Several of the old regex checks were vacuous: `a-dep ^1.0.5` and `normal-dep-and-dev-dep ^1.0.0` install their latest version, are never listed as outdated, and the assertion just matched the original string. - `.not.toContain("FileNotFound")` / `"failed to resolve"` / `"Failed to update"` replaced with positive `stdout` checks (`"Installing updates..."`, aligned-row substrings). - Mixed-section and workspace/catalog render tests now assert exact aligned rows including the section-header column position instead of bare substring presence. - Complex-range test asserts the resulting catalog values instead of `toBeDefined()`. - The misnamed "pre-release versions" test renamed to "preserve version prefixes on direct dependencies" and pinned to exact resulting values. ### How did you verify your code works? `bun bd test test/cli/update_interactive_formatting.test.ts` (debug+ASAN): | | before | after | | --- | --- | --- | | tests | 32 pass | 24 pass | | wall time (median of 5/8) | ~6.14s | ~5.30s | | lines | 1198 | 973 | 8 consecutive runs after the change, 0 failures. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
What does this PR do?
test/cli/update_interactive_formatting.test.tsruns 32 tests serially, each spawningbun install+bun update -i, for ~13s wall time on the debian 13 x64-asan lane (build #84540).Speed
Wrap the describe block in
describe.concurrent. Every test already creates its owntempDirwithcache = falseand the Verdaccio registry is read-only afterbeforeAll, so there is no shared state. Extractinstall()/updateInteractive()helpers for the boilerplate and so that an install failure surfaces stderr instead ofExpected: 0, Received: 1.Local
bun bd test(debug+ASAN, 3/5 runs each):Assertions
The first six tests in this file wrote a JSON blob to
bun.lockb(which is the binary lockfile format) or no lockfile at all, sobun update --interactivefailed at lockfile parse:and never reached
process_multi_select(the column-layout code the test names refer to). The only assertions wereexpect(stderr).not.toContain("panic"|"segfault"|"overflow"|"underflow"), which pass for any stderr that happens not to contain those words.Those six are rewritten to install real registry packages, reach the selection prompt, and assert on the rendered
Current/Target/Latestheader and row content (column alignment, section headers for each dependency type, catalog/workspace entries listed). The remaining 18.not.toContain("panic")assertions elsewhere in the file are dropped, and tests that previously asserted onlyexitCode === 0now check stdout (typically"Installing updates...") first so a failure shows the actual output.Removed a stray
console.log("Checking lockfile...")and theconsole.errordebug prints in the select-all test.How did you verify your code works?
bun bd test test/cli/update_interactive_formatting.test.tsbefore and after (numbers above); after-state run 8 times with no failures.#33704 touches this file with the minimal
describe->describe.concurrentchange as part of a wider sweep; this PR subsumes that change for this file and additionally fixes the dead assertions.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.