Skip to content

test: shard deps-installer integration tests#12376

Merged
zkochan merged 1 commit into
mainfrom
fix-ci3
Jun 13, 2026
Merged

test: shard deps-installer integration tests#12376
zkochan merged 1 commit into
mainfrom
fix-ci3

Conversation

@zkochan

@zkochan zkochan commented Jun 13, 2026

Copy link
Copy Markdown
Member

Summary

  • run @pnpm/installing.deps-installer Jest tests in five sequential shards
  • keep each shard in a fresh Node/Jest process while preserving the existing registry setup
  • update .meta-updater so the generated package manifest stays in sync

Context

The failed Ubuntu Node 22 job in https://github.com/pnpm/pnpm/actions/runs/27464325216/job/81186212640 still OOMed in @pnpm/installing.deps-installer after 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 pnpr
  • CI=true pnpm --filter @pnpm/installing.deps-installer test
  • pnpm meta-updater --test
  • git diff --check
  • pre-push hook: compile, spellcheck, meta-updater check, quiet ESLint

Written by an agent (Codex, GPT-5).

Summary by CodeRabbit

  • Chores
    • Modified test script execution to run across multiple shards instead of a single invocation.

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 65209012-8d53-46c8-95b4-89ba864e93a6

📥 Commits

Reviewing files that changed from the base of the PR and between f20ad8f and 338f094.

📒 Files selected for processing (2)
  • .meta-updater/src/index.ts
  • installing/deps-installer/package.json

📝 Walkthrough

Walkthrough

This PR introduces test sharding for the @pnpm/installing.deps-installer package. A constant shard count is defined in the meta-updater, the package-specific script generation logic is updated to compose a .test script from five sequential shard runs, and a new .test:shard entry point executes Jest with the same environment setup as before.

Changes

Test sharding for deps-installer

Layer / File(s) Summary
Test sharding setup and configuration
.meta-updater/src/index.ts, installing/deps-installer/package.json
Shard count is defined as a constant, meta-updater script wiring generates a .test script that chains five pn .test:shard --shard=N/5 invocations and a new .test:shard script that runs Jest with mocked registry and Node options preserved.

🎯 2 (Simple) | ⏱️ ~10 minutes

A sharded test path unfolds,
Five Jest runners in parallel bands—
Meta-updater's logic now molds,
Deps-installer's test stand expands.
🐰✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-ci3

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@zkochan
zkochan marked this pull request as ready for review June 13, 2026 13:09
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Temp storage multiplies 🐞 Bug ☼ Reliability
Description
@pnpm/installing.deps-installer now runs Jest 5 times via shards, and each run’s with-registry
globalSetup creates a new temp registry storage directory that globalTeardown does not remove.
This increases leftover temporary storage from 1→5 directories per test invocation, which can
unnecessarily bloat disk usage on CI/local runs.
Code

installing/deps-installer/package.json[56]

+    ".test": "pn .test:shard --shard=1/5 && pn .test:shard --shard=2/5 && pn .test:shard --shard=3/5 && pn .test:shard --shard=4/5 && pn .test:shard --shard=5/5",
Evidence
The new .test script runs 5 Jest processes; the with-registry preset’s globalSetup creates a
unique temp storage dir per run and exposes it via PNPM_REGISTRY_MOCK_STORAGE, while
globalTeardown only kills the server and performs no directory cleanup, so the number of leftover
temp dirs increases with the shard count.

installing/deps-installer/package.json[49-58]
utils/jest-config/with-registry/globalSetup.js[16-29]
utils/jest-config/with-registry/globalTeardown.js[1-3]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Sharding runs Jest 5 separate times. The with-registry Jest preset creates a new temp storage directory in `globalSetup` (`mkdtempSync(...)`) but `globalTeardown` currently only kills the server and does not delete the storage directory, so sharding multiplies leftover temp directories.

## Issue Context
- The sharded `.test` script executes 5 independent Jest processes.
- Each process runs the with-registry preset’s `globalSetup`, which always creates a new temp storage directory and exports its path via `PNPM_REGISTRY_MOCK_STORAGE`.
- `globalTeardown` should remove that directory (optionally behind an env flag to keep it for debugging).

## Fix Focus Areas
- __utils__/jest-config/with-registry/globalTeardown.js[1-3]
- __utils__/jest-config/with-registry/globalSetup.js[16-29]
- installing/deps-installer/package.json[56-57]

## Suggested implementation direction
1. In `globalTeardown`, after `await global.killServer?.()`, read `process.env.PNPM_REGISTRY_MOCK_STORAGE` and recursively delete it (e.g., `fs.rmSync(storage, { recursive: true, force: true })`), guarding against missing/empty values.
2. Consider adding an opt-out env var like `PNPM_REGISTRY_MOCK_KEEP_STORAGE=1` for debugging.
3. Ensure deletion happens even if server kill fails (best-effort cleanup).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@zkochan
zkochan merged commit 03143ca into main Jun 13, 2026
12 of 15 checks passed
@zkochan
zkochan deleted the fix-ci3 branch June 13, 2026 13:09
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Test: shard deps-installer integration tests
🧪 Tests ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

Walkthroughs

Description
• 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.
Diagram
graph 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
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Increase Node heap in CI (max-old-space-size)
  • ➕ Minimal change to test scripts and developer workflows
  • ➕ May immediately unblock OOM failures if memory headroom exists
  • ➖ Pushes memory pressure to the runner; may still OOM on constrained hosts
  • ➖ Doesn't address long-lived process memory growth/leaks during large suites
2. Split suite by Jest projects/testMatch groups
  • ➕ Explicit grouping by test areas; can balance shard sizes better
  • ➕ Can run groups independently in CI matrix if desired
  • ➖ More maintenance overhead (keeping groups balanced as tests change)
  • ➖ Still requires multiple invocations to guarantee fresh processes
3. Run fewer Jest workers / runInBand
  • ➕ May reduce peak memory from worker concurrency
  • ➕ Easy to toggle in CI
  • ➖ Often increases wall-clock time significantly
  • ➖ Does not guarantee avoidance of main-process heap growth over a long run

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.

Grey Divider

File Changes

Tests (1)
package.json Shard deps-installer '.test' into 5 sequential Jest runs +2/-1

Shard deps-installer '.test' into 5 sequential Jest runs

• Updates the package’s '.test' script to run Jest as five shards sequentially and adds a '.test:shard' helper script that encapsulates the registry-mock and NODE_OPTIONS setup. Keeps 'test' as 'pn compile && pn .test' so behavior remains consistent while reducing OOM risk.

installing/deps-installer/package.json


Other (1)
index.ts Generate sharded deps-installer Jest scripts via meta-updater +3/-1

Generate sharded deps-installer Jest scripts via meta-updater

• Introduces a shard-count constant and changes @pnpm/installing.deps-installer’s generated '.test' script to run 5 sequential shard invocations. Adds a new '.test:shard' script that performs the actual Jest call with the required registry-mock port and Node options.

.meta-updater/src/index.ts


Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant