Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces test sharding for the ChangesTest sharding for deps-installer
🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo
1. Temp storage multiplies
|
PR Summary by QodoTest: shard deps-installer integration tests WalkthroughsDescription• Split @pnpm/installing.deps-installer Jest run into 5 sequential shards to reduce OOM risk. • Run each shard in a fresh Node/Jest process while keeping the registry-mock port setup. • Teach .meta-updater to generate the new sharded test scripts in package manifests. Diagramgraph TD
A["CI job"] --> B["deps-installer .test"] --> C["5 shards (1/5..5/5)"] --> D["Jest (new process)"] --> E[("Registry mock :7769")]
F[".meta-updater"] --> G["package.json scripts"] --> B
subgraph Legend
direction LR
_job["Job/runner"] ~~~ _tool["Tool/script"] ~~~ _db[("Service/endpoint")]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Increase Node heap in CI (max-old-space-size)
2. Split suite by Jest projects/testMatch groups
3. Run fewer Jest workers / runInBand
Recommendation: The PR’s approach (sequential sharded Jest invocations) is the most robust way to avoid heap accumulation because it guarantees a fresh Node/Jest process per shard while preserving the existing registry-mock behavior. Consider heap-size tweaks only as a fallback if sharding alone is insufficient. File ChangesTests (1)
Other (1)
|
Summary
@pnpm/installing.deps-installerJest tests in five sequential shards.meta-updaterso the generated package manifest stays in syncContext
The failed Ubuntu Node 22 job in https://github.com/pnpm/pnpm/actions/runs/27464325216/job/81186212640 still OOMed in
@pnpm/installing.deps-installerafter coverage was disabled. Windows Node 22 in the same run failed with the same deps-installer heap OOM. Splitting the suite by process lifetime gives the runner heap room without changing test assertions.Verification
cargo build -p pnpr-fixtures --bin pnpr-prepare -p pnpr --bin pnprCI=true pnpm --filter @pnpm/installing.deps-installer testpnpm meta-updater --testgit diff --checkWritten by an agent (Codex, GPT-5).
Summary by CodeRabbit