Repository navigation
test(update -i): consolidate duplicate scenarios and pin exact values - #36442
Jarred-Sumner merged 2 commits into
Conversation
update_interactive_formatting.test.ts spent ~8s on the debian x64-asan lane (build #85633). #36348 already made it describe.concurrent; this follow-up removes the remaining redundant subprocess spawns and tightens assertions. Consolidation (32 -> 24 tests, each test spawns install + update -i): - three column-alignment dry-run tests collapsed into one 4-dep fixture that exercises short/long names and short/long versions in one 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' dropped (identical to 'update packages when a is used') - 'interactive update dry run mode' / 'handle dry-run mode' merged - 'interactive update with workspaces.catalogs structure' merged into 'catalog updates in workspaces.catalogs object' (union of assertions) - 'preserve version prefixes in catalogs' / 'scoped packages in catalogs' merged; the surviving test uses entries that are actually outdated so the prefix assertion is not vacuous - 'mixed workspace and catalog dependencies' dropped (strict subset of 'interactive update with mixed dependency types') Assertion tightening: - toMatch(/^~/) etc. replaced with toEqual on the full catalog/deps object; several of the old regex checks matched the unchanged input because the range already resolved to latest (e.g. a-dep ^1.0.5 installs 1.0.10 and is never listed as outdated) - .not.toContain('FileNotFound') / 'failed to resolve' replaced with positive stdout checks - mixed-section and workspace render tests now assert exact aligned rows instead of substring presence - complex-range test asserts the resulting catalog values instead of toBeDefined() - misnamed 'pre-release versions' test renamed to what it tests (direct-dep prefix preservation) and pinned to exact values Local bun bd test (debug+ASAN, median of 5/8 runs): before: 32 tests, ~6.14s after: 24 tests, ~5.30s
|
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only consolidation that strengthens assertions rather than weakening them.
What was reviewed
- Each removed test traced to a surviving test that covers a strict superset (e.g. "basic select all" ⊂ "update packages when 'a' is used"; both dry-run tests → one with byte-equal
package.jsoncheck). - Regex/
.toBeDefined()→.toEqual({...})conversions checked against the Verdaccio fixture versions (a-deplatest = 1.0.10,@types/no-depshas 2.0.0) — the pinned values and "already-latest so unchanged" comments match the registry. .not.toContain("FileNotFound")removals are paired with a positive"Installing updates..."+exitCode === 0check, so failure detection isn't lost.
Extended reasoning...
Overview
Single-file test refactor of test/cli/update_interactive_formatting.test.ts: 32 → 24 tests, ~225 lines removed. No production code touched. Two categories of change: (1) merging redundant subprocess-spawning scenarios into one fixture that exercises the union of their inputs, and (2) replacing weak assertions (toMatch(/^\^/), .toBeDefined(), .not.toContain("error")) with exact toEqual/toBe on the full resulting object.
Security risks
None. Test-only; the subprocess invocations, registry fixtures, and env handling are unchanged from before.
Level of scrutiny
Medium — REVIEW.md is explicit that deleting or weakening tests needs justification, so I checked each removal against its stated superset. All eight consolidations in the PR description hold up: the surviving tests assert everything the deleted ones did, and in several cases the deleted assertions were vacuous (e.g. a-dep ^1.0.5 already resolves to the fixture's latest 1.0.10, so toMatch(/^\^/) was matching the untouched input string). The new @types/no-deps reference exists in the Verdaccio fixtures with a 2.0.0 to update to.
Other factors
The direction of every assertion change is toward stricter — exact aligned-row substrings instead of bare name presence, full-object toEqual instead of per-key regex — which is what the repo's test guidance asks for. The one place a pinned value documents "not updated" behavior (prod: { "no-deps": "~1.0.0" } in the multi-catalog test) is a faithful pin of current output, not a new claim; if that behavior ever changes the exact assertion will surface it where the old /^~/ regex would not. Author reports 8/8 green runs on debug+ASAN.
The 'multiple catalog definitions with same package' fixture references no-deps via both catalog:dev and catalog:prod. group_catalog_dependencies keys its map by package name only, so the two references collapse to one selectable row and only dev's catalog entry is rewritten. The previous commit pinned prod to the unchanged '~1.0.0'; relax that to stringMatching(/^~[12]\.0\.0$/) with a comment so a fix to the grouping doesn't read this assertion as a spec.
There was a problem hiding this comment.
LGTM — test-only consolidation that strictly strengthens assertions.
What was reviewed
- Verified every pinned latest version against the Verdaccio fixture dist-tags (
a-dep1.0.10,no-deps2.0.0,dep-with-tags3.0.0,normal-dep-and-dev-dep1.0.2,@types/no-deps2.0.0). - Traced each of the 8 removed tests to a surviving test that covers the same or a strict superset of assertions; no unique coverage lost.
- Confirmed the removed
.not.toContain("FileNotFound")-style checks are replaced by positiveInstalling updates.../ exact-row checks plusexitCode === 0, per repo test guidance. - The one intentionally-loose assertion (
prodcatalog/^~[12]\.0\.0$/) is commented as not hard-pinning a known dedup limitation.
Extended reasoning...
Overview
Single-file test refactor of test/cli/update_interactive_formatting.test.ts: 32 → 24 tests, ~225 lines removed. Eight tests are deleted (each a strict subset of a surviving test), three column-alignment tests are collapsed into one, and ~20 regex/.not.toContain/toBeDefined assertions are replaced with exact-value toBe/toEqual on the resulting package.json objects and exact-spacing toContain on rendered rows. No production code is touched.
Security risks
None. Test-only change against a local Verdaccio registry; no network, no auth, no runtime code.
Level of scrutiny
Moderate — the risk with test consolidation is silently dropping coverage or pinning wrong values. I checked both:
- Pinned values: cross-referenced every hard-coded latest version against
test/cli/install/registry/packages/*/package.jsondist-tags. All match. The "already resolves to latest, so not listed as outdated" comments (a-dep ^1.0.5→ 1.0.10,normal-dep-and-dev-dep ^1.0.0→ 1.0.2,a-dep ~1.0.5→ 1.0.10) are correct given those fixtures, which validates the PR's claim that the old regex assertions on those entries were vacuous. - Removed tests: each deletion in the description maps to a surviving test with equal-or-stronger assertions — the two dry-run tests collapse to the byte-equal
package.jsoncheck; "basic select all" is byte-identical to "update packages when 'a' is used"; "mixed workspace and catalog dependencies" is covered by "interactive update with mixed dependency types" (which now assertsworkspace:*andcatalog:are untouched viatoEqualon the full deps object); theworkspaces.catalogsduplicate is folded into the surviving test which now asserts the fullcatalogsobject including the untouchedframeworkscatalog.
Other factors
- REVIEW.md explicitly discourages the removed patterns (
.not.toContain("error string"), regex that matches unchanged input,toBeDefined()), so this is a net quality improvement, not a weakening. - The exact-spacing row assertions are intentionally brittle-by-design (commented as such) to catch
name_padding/current_paddingregressions; stdout is piped (not a tty) so width is deterministic across platforms. - The
catalogs.prodassertion deliberately accepts both~1.0.0and~2.0.0with a comment naminggroup_catalog_dependencies's name-keyed dedup — avoids certifying a known limitation as correct while still failing if the prefix is lost. - Author reports 8 consecutive debug+ASAN passes; no CODEOWNERS for
test/cli/.
What does this PR do?
test/cli/update_interactive_formatting.test.tsspent ~8s wall time on the debian 13 x64-asan lane (build #85633). #36348 already made itdescribe.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)2.0.0vs1.0.10) in a single table.package.jsoncheck).@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 thetoMatch(/^.../)checks matched the unchanged input.workspace:*andcatalog:are left untouched and direct-dep prefixes survive).Assertion tightening
toMatch(/^~|^\^|^>=/)replaced withtoEqual({...})on the fullcatalog/dependenciesobject. Several of the old regex checks were vacuous:a-dep ^1.0.5andnormal-dep-and-dev-dep ^1.0.0install 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 positivestdoutchecks ("Installing updates...", aligned-row substrings).toBeDefined().How did you verify your code works?
bun bd test test/cli/update_interactive_formatting.test.ts(debug+ASAN):8 consecutive runs after the change, 0 failures.
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.