Conversation
The 57 arborist snapshot cases run one after another, two bun children each, and made migrate.test.ts the longest file of the serial phase. They move to npm-arborist.test.ts with their snapshot file renamed byte for byte, and the helpers both files need move to migration-harness.ts. No test changes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical test-only split with no behavior change.
What was reviewed:
- Verified the extracted helpers in
migration-harness.tsand the movedtest.eachbody innpm-arborist.test.tsare byte-identical to what was removed frommigrate.test.ts;import.meta.dirresolves to the same directory soARBORISTis unchanged. - Confirmed the snapshot file is a 100% rename and the describe hierarchy (
package-lock.json migration fixes > arborist fixtures) is preserved, so all 57 snapshot keys still match. - Checked
test/parallel-allowlist.json:cli/install/migrationis allowlisted and the new file is not inexcludeFiles, so it lands in the parallel batch as described.
Extended reasoning...
Overview
This PR splits the 57 sequential arborist-fixture snapshot cases out of test/cli/install/migration/migrate.test.ts (a ~1800-line file) into a new standalone npm-arborist.test.ts, so the CI runner can schedule it in the parallel batch while the remainder of migrate.test.ts stays in excludeFiles. Shared helpers (fixture, run, readLock, writeExtra, OFFLINE_REGISTRY, ARBORIST, arboristFixtures, FixtureDir) are lifted verbatim into migration-harness.ts and re-imported by both test files. The snapshot file is a pure git rename (100% similarity). No production/runtime code is touched.
Security risks
None. This is test-infrastructure reorganization only. The offline-registry pattern (http://localhost:1/) and per-project BUN_INSTALL_CACHE_DIR are carried over unchanged, so hermeticity is preserved. No new network access, no credential handling, no changes to src/.
Level of scrutiny
Low. The change is mechanical: every removed line reappears identically in one of the two new files, the describe path is preserved so snapshot keys resolve without regeneration, and import.meta.dir points to the same directory in the new harness file as it did in the original. The remaining imports in migrate.test.ts (bunEnv, bunExe, pack, tempDir, tmpdirSync) are still used by the ~1700 lines of tests that stay behind. I confirmed against test/parallel-allowlist.json that the new file will be picked up by the parallel batch as the PR claims.
Other factors
The 5-minute setDefaultTimeout and sequential test.each are pre-existing behavior being moved, not introduced here (the in-code comment already explains snapshot matchers don't work in concurrent groups). REVIEW.md's guidance against creating new test files doesn't apply — this is an intentional split for CI parallelism with a stated runtime justification, and the PR description documents local verification (57 pass / 57 snapshots, none written) plus merge-tree conflict analysis against open PRs. No outstanding reviewer objections in the timeline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe pull request extracts npm-arborist fixture and command utilities into a shared migration harness, adds a dedicated fixture migration snapshot suite, and removes the duplicated infrastructure from ChangesMigration test coverage
Merge Risk: ⚪ Minimal · up to This change reorganizes existing migration tests and shared helpers without changing test behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, fix, verification steps, test results, and relevant background. It does not use the exact template headings, but it provides the required information in equivalent sections. Comment |
This branch has not been deployed
1 participant
Problem
test/cli/install/migration/migrate.test.tstook 15.6s on debian 13 x64-asan and 8.1s on windows 11 aarch64 (build 108487), in the serial phase. Its 57 arborist fixture cases run one after another, twobunchildren each, because snapshot matchers are unsupported in concurrent tests. They are about two thirds of the file's time.Fix
test/cli/install/migration/npm-arborist.test.ts. The describe path stayspackage-lock.json migration fixes > arborist fixtures, so__snapshots__/migrate.test.ts.snapis renamed byte for byte tonpm-arborist.test.ts.snap.fixture,run,readLock,writeExtra,OFFLINE_REGISTRY,ARBORIST) move totest/cli/install/migration/migration-harness.ts.migrate.test.tsimports them. No test body changes.excludeFiles, so the runner puts it in the parallel batch once it is on main. It is hermetic: offline registry, own install cache per project.bun bd test test/cli/install/migration/npm-arborist.test.ts(57 pass, 57 snapshots, none written) andbun bd test test/cli/install/migration/migrate.test.ts(69 pass, 1 todo). Local debug ASAN:migrate.test.ts29.1s before, 9.1s after.npm-arborist.test.ts20.8s.Background
bun pm migrateconverts apackage-lock.jsonintobun.lock. The arborist fixtures are lockfiles from npm's test suite. Each case migrates one and round-trips it throughbun install --frozen-lockfile --lockfile-only, and the report is snapshotted.scripts/runner.node.mjsruns the files oftest/parallel-allowlist.jsonin one parallel batch after the serial phase. A file in a listed directory qualifies unless it is inexcludeFiles, which test: pull five batch-flaky files out of the parallel batch #40419 did formigrate.test.tsafter batch-only failures.Notes
Follow-ups. #40956 rewrites the top-level cases of
migrate.test.tson top of this change: they are the cases that reach registry.npmjs.org and GitHub, and the likely reason for the batch-only failures. Once that lands, a maintainer can takecli/install/migration/migrate.test.tsout oftest/parallel-denylist.txtandexcludeFiles.Open PRs on this file.
git merge-treeagainst this branch: #38698, #38850, #38804, #38990 and #29509 merge clean. #38861 adds a describe right before the arborist block, so it conflicts on the block's removal; the resolution is to keep its describe.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/migration/migrate.test.ts