You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Problem: With --experimental-vm-modules --runInBand, jest creates a fresh vm.createContext() per test file. Every suite calling bootstrap() re-ran migrations.run() (glob scan + DB round-trip, ~100ms × 18 suites = 1.8s wasted). For downstream projects with 89 suites (trawl), this multiplies further.
Root cause investigated: Attempted globalThis singleton, process property singleton, and process.env flag set inside vm contexts — all cleaned up by jest's GlobalProxy.clear() on suite teardown. The only anchor that survives is process.env keys set before any vm context is created (i.e., in globalSetup), which land in jest's protectProperties() protected set.
Fix: Run migrations once in jest.globalSetup.js (before vm contexts exist) and set process.env.DEVKIT_MIGRATIONS_RAN='1'. bootstrap() checks this flag and skips migrations.run() on 2nd+ calls. Each suite still gets a fresh Express app + mongoose connection for full test isolation.
globalThis per vm context — each test file gets a fresh globalThis; property is undefined for next suite
process.__property set inside vm context — cleaned by GlobalProxy.clear() on teardown
process.env key set inside vm context — also cleaned (jest's deleteProperties on process.env)
globalSetup bootstrap + shared Express app — breaks tests that mutate config.* module-level state (e.g. config.sign.up = false in auth tests) because the route handlers use globalSetup's module instance of config, not the test vm context's instance
Test plan
All 1234 devkit tests green
jest.globalSetup.unit.tests.js updated to reflect 2-phase connect/disconnect behavior
Migrations pre-run failure is non-fatal: each suite falls back to running migrations itself
Heap and timing verified with --logHeapUsage
Summary by CodeRabbit
Tests
Optimized database setup during test execution to prevent redundant migration runs across test suites.
Improved test reliability through two-phase migration initialization.
Enhanced error handling for migration failures during test setup.
Chores
Updated test configuration and assertions to reflect improved database initialization flow.
Run migrations once in jest.globalSetup (before any vm context) and set
process.env.DEVKIT_MIGRATIONS_RAN='1'. bootstrap() checks this flag and
skips migrations.run() on 2nd+ suite bootstraps.
Why process.env set in globalSetup works:
jest --experimental-vm-modules resets any process.env key that was set
INSIDE a test-file vm context on teardown. Keys present BEFORE the first
vm.createContext() call are included in jest's protectProperties() sweep
and survive all per-suite GlobalProxy teardowns unchanged. globalSetup
runs before any vm context is created — exactly that window.
Each test suite still gets a fresh Express app and mongoose connection so
per-suite module state (e.g. config.sign.up mutations) stays isolated.
Impact:
- Migrations runs: 21 → 1 per full test run (18 skips × ~100ms ≈ 1.8s saved)
- Heap peak devkit: 1.43 GB (unchanged — jest module-registry accumulation
dominates; migrations overhead is timing, not heap)
- All 1234 devkit tests green
Fixes the bootstrap accumulation root cause for downstream projects with
larger test suites (trawl: 89 suites × full migrations scan each = 8.9s
and additional file-read heap accumulation under --experimental-vm-modules).
@PierreBrisorgueil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 31 minutes and 52 seconds before requesting another review.
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.
⌛ How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Reviewing files that changed from the base of the PR and between 28e8994 and fc217bc.
📒 Files selected for processing (4)
lib/app.js
modules/core/tests/migrations.unit.tests.js
scripts/jest.globalSetup.js
scripts/tests/jest.globalSetup.unit.tests.js
Walkthrough
The changes implement a two-phase migration strategy where Jest's global setup runs migrations once during test initialization and signals completion via an environment variable, enabling per-suite bootstrap calls to skip redundant migrations.
Changes
Cohort / File(s)
Summary
Bootstrap & Jest Migration Setup lib/app.js, scripts/jest.globalSetup.js
Bootstrap now conditionally skips migrations when DEVKIT_MIGRATIONS_RAN is set. Jest global setup performs a second connect-phase to load models and execute migrations, setting the flag to prevent re-runs during test suite execution.
Test assertions updated to reflect two-phase behavior: minimum thresholds for connect/disconnect calls (at least two of each), and verification that Phase 1 connection failures don't prevent Phase 2 migration attempts.
Sequence Diagram(s)
sequenceDiagram
participant Jest Global Setup
participant Database
participant Migrations
participant Bootstrap (per suite)
Note over Jest Global Setup,Bootstrap (per suite): Jest Initialization (Global Setup)
Jest Global Setup->>Database: Phase 1: connect
Jest Global Setup->>Database: dropDatabase
Jest Global Setup->>Database: disconnect
Jest Global Setup->>Database: Phase 2: connect
Jest Global Setup->>Migrations: Load model files
Jest Global Setup->>Migrations: migrations.run()
Jest Global Setup->>Jest Global Setup: Set DEVKIT_MIGRATIONS_RAN='1'
Jest Global Setup->>Database: disconnect
Note over Jest Global Setup,Bootstrap (per suite): Per-Suite Execution
Bootstrap (per suite)->>Bootstrap (per suite): Check DEVKIT_MIGRATIONS_RAN
alt Flag is set
Bootstrap (per suite)->>Bootstrap (per suite): Skip migrations.run()
else Flag not set
Bootstrap (per suite)->>Migrations: migrations.run()
end
The title accurately describes the main change: optimizing test suite performance by skipping redundant per-suite migrations via a globalSetup pre-run with an environment flag.
Description check
✅ Passed
The PR description is comprehensive and covers all required template sections: Summary with problem/root cause/fix, Impact metrics, reasoning for design decisions, and Test plan. All critical checklist items are addressed.
Docstring Coverage
✅ Passed
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch fix/test-bootstrap-singleton-leak
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.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR is currently not up to standards due to new security issues and missing verification for core logic changes. While the architectural move to optimize migrations in globalSetup is beneficial, the implementation in scripts/jest.globalSetup.js introduces potential path traversal vulnerabilities and contains redundant database operations. Additionally, there are significant gaps in testing; specifically, the logic in lib/app.js that skips or executes migrations based on the new environment flag is completely unverified. These security and reliability concerns should be resolved prior to approval.
About this PR
This PR is currently flagged as not up to standards. The introduction of unsafe dynamic imports and the lack of unit tests for the conditional migration logic in lib/app.js are the primary blockers.
Test suggestions
Verify that globalSetup runs migrations and sets the persistent environment flag.
Verify that globalSetup handles migration failures gracefully and still disconnects from the database.
Verify that bootstrap() skips migrations when the DEVKIT_MIGRATIONS_RAN flag is set.
Verify that bootstrap() runs migrations as a fallback when the flag is absent.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that bootstrap() skips migrations when the DEVKIT_MIGRATIONS_RAN flag is set.
2. Verify that bootstrap() runs migrations as a fallback when the flag is absent.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.87%. Comparing base (d8f412b) to head (fc217bc). ⚠️ Report is 4 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Optimizes Jest integration test runtime under --experimental-vm-modules --runInBand by pre-running DB migrations once in globalSetup and letting per-suite bootstrap() skip redundant migration runs via an env flag.
Changes:
Add a migrations pre-run phase to scripts/jest.globalSetup.js and set process.env.DEVKIT_MIGRATIONS_RAN='1' on success.
Update lib/app.jsbootstrap() to skip migrations.run() when the env flag is present.
Update unit tests to reflect the new two-phase globalSetup behavior (drop DB + migration pre-run).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
File
Description
scripts/jest.globalSetup.js
Adds a second globalSetup phase to connect/load models/run migrations once and set the env flag.
lib/app.js
Skips per-suite migrations when the env flag indicates they were already run in globalSetup.
scripts/tests/jest.globalSetup.unit.tests.js
Adjusts assertions/docs for the new two-phase globalSetup behavior.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app.js`:
- Around line 73-75: The current check skips migrations whenever
process.env.DEVKIT_MIGRATIONS_RAN === '1', which can inadvertently bypass
migrations in non-test environments; update the conditional in lib/app.js to
only short-circuit migrations when both process.env.DEVKIT_MIGRATIONS_RAN ===
'1' and process.env.NODE_ENV === 'test' so that migrations.run() is still
executed in normal starts (reference the DEVKIT_MIGRATIONS_RAN check and the
migrations.run() call to locate the change).
In `@scripts/tests/jest.globalSetup.unit.tests.js`:
- Around line 113-117: The test assertion for the second connect attempt is too
weak; update the expectation on the mocked connect to assert at least two calls
so the retry path is verified. In the test around globalSetup(), replace the
loose check using connect.mock.calls.length >= 1 with a stricter assertion
(e.g., expect(connect.mock.calls.length).toBeGreaterThanOrEqual(2) or
toHaveBeenCalledTimes(2)) to prove the Phase 2 reconnect actually ran; leave the
other assertions for dropDatabase and resolves as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1e7c6d6-4a5c-4aea-8b8f-3d103d957cd3
📥 Commits
Reviewing files that changed from the base of the PR and between d8f412b and 28e8994.
- Scope DEVKIT_MIGRATIONS_RAN skip to NODE_ENV=test in bootstrap() to
prevent accidental migration bypass in prod/dev (CodeRabbit major)
- Update JSDoc @returns to Promise<{db, app}> (Copilot)
- Add explicit Array.isArray guard for config.files.mongooseModels in
globalSetup Phase 2 instead of relying on TypeError for control flow
- Pass config.db.options to mongoose.connect() in Phase 2 for SSL/auth
parity with mongooseService.connect (Copilot)
- Use pathToFileURL for ESM dynamic imports (Codacy path-traversal fix)
- Tighten Phase 2 reconnect assertion from >=1 to >=2 + assert disconnect
called once (CodeRabbit minor + Copilot)
- Add files.mongooseModels to mocks so Phase 2 is exercised in tests
Bootstrap no longer re-runs migrations.run() per suite (DEVKIT_MIGRATIONS_RAN
short-circuits it after globalSetup), so the claim path, duplicate-key
handling, and unclaim-on-error branches lost coverage. Add unit tests that
spy on migrationRepository to exercise:
- claimMigration E11000 → returns false (concurrent runner already claimed)
- claimMigration non-duplicate error → rethrows
- runMigration: import failure → unclaim + rethrow
- runMigration: missing up() export → unclaim + rethrow
- run(): all migrations already executed branch
Restores migrations.js coverage from ~55% back to ~82%.
…gistry growth (#3550)
Adds `workerIdleMemoryLimit: '512MB'` to jest.config.js. Jest forks worker
child processes that accumulate per-suite vm-context module registry under
`--experimental-vm-modules` (~14 MB retained per loaded suite). On large
downstream test suites this leaks until the worker OOMs.
The flag tells jest to inspect each worker's RSS between suites and respawn
when it exceeds the threshold. Respawn cost (~150-300 ms) is negligible vs.
the OOM crash it avoids on long-running coverage runs.
This complements PR #3549 (migration pre-warm) which addressed migration
overhead but not the vm-context module registry growth.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
--experimental-vm-modules --runInBand, jest creates a freshvm.createContext()per test file. Every suite callingbootstrap()re-ranmigrations.run()(glob scan + DB round-trip, ~100ms × 18 suites = 1.8s wasted). For downstream projects with 89 suites (trawl), this multiplies further.globalThissingleton,processproperty singleton, andprocess.envflag set inside vm contexts — all cleaned up by jest'sGlobalProxy.clear()on suite teardown. The only anchor that survives isprocess.envkeys set before any vm context is created (i.e., inglobalSetup), which land in jest'sprotectProperties()protected set.jest.globalSetup.js(before vm contexts exist) and setprocess.env.DEVKIT_MIGRATIONS_RAN='1'.bootstrap()checks this flag and skipsmigrations.run()on 2nd+ calls. Each suite still gets a fresh Express app + mongoose connection for full test isolation.Impact
Why not a full bootstrap singleton?
Explored and rejected:
globalThisper vm context — each test file gets a freshglobalThis; property isundefinedfor next suiteprocess.__propertyset inside vm context — cleaned byGlobalProxy.clear()on teardownprocess.envkey set inside vm context — also cleaned (jest'sdeletePropertiesonprocess.env)globalSetupbootstrap + shared Express app — breaks tests that mutateconfig.*module-level state (e.g.config.sign.up = falsein auth tests) because the route handlers use globalSetup's module instance ofconfig, not the test vm context's instanceTest plan
jest.globalSetup.unit.tests.jsupdated to reflect 2-phase connect/disconnect behavior--logHeapUsageSummary by CodeRabbit
Tests
Chores