fix(tests): guard jest globalSetup against non-test NODE_ENV (#3476)#3488
fix(tests): guard jest globalSetup against non-test NODE_ENV (#3476)#3488PierreBrisorgueil merged 2 commits intomasterfrom
Conversation
Downstream projects (ism_node, trawl_node, comes_node, montaine_node) export `NODE_ENV=<project>` in their shell to pick up their config. Running jest directly (`npx jest`, IDE runner) bypasses the `cross-env NODE_ENV=test` wrapper used by the npm `test*` scripts, and `scripts/jest.globalSetup.js` unconditionally called `dropDatabase()` against `config.db.uri` resolved from that env. Real incident (2026-04-20, ism_node): `NODE_ENV=ism npx jest ...` wiped 16 collections in 22ms with no confirmation. Guards added in globalSetup (defense-in-depth): 1. Refuse to drop when `NODE_ENV !== 'test'`, log clear refusal. 2. Refuse to drop when the resolved DB name does not match `/test/i`, catching cases where `config.db.uri` is overridden to a non-test DB. Both checks exit without connecting; tests run against existing state. Tests: 5 new unit tests cover project-env leak, undefined NODE_ENV, non-test DB name, happy path, and Mongo-unreachable fallback. Added `scripts/tests/**` to `testMatch` so this path is picked up. Fixes #3476
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughJest configuration updated to discover tests in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3488 +/- ##
=======================================
Coverage 85.85% 85.85%
=======================================
Files 115 115
Lines 2919 2919
Branches 809 809
=======================================
Hits 2506 2506
Misses 327 327
Partials 86 86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds defense-in-depth to Jest’s globalSetup to prevent accidental database drops when Jest is run outside of the intended test environment, and adds unit coverage for the new safety checks.
Changes:
- Add hard guards in
scripts/jest.globalSetup.jsto refusedropDatabase()unlessNODE_ENV === 'test'and the resolved DB name matches/test/i. - Add unit tests covering guard behavior and the happy path for
jest.globalSetup. - Extend
jest.config.jstestMatchto includescripts/tests/**/*.js.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/jest.globalSetup.js | Adds environment + DB-name guards before connecting/dropping the database. |
| scripts/tests/jest.globalSetup.unit.tests.js | Adds unit tests for the new globalSetup safety behavior. |
| jest.config.js | Includes scripts/tests/** in Jest test discovery so the new tests run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/jest.globalSetup.js (1)
35-40:⚠️ Potential issue | 🟠 MajorDisconnect even when
dropDatabase()fails.If
mongoose.connect()succeeds butdropDatabase()rejects, the catch swallows the error and skipsmongoose.disconnect(), which can leave an open Mongo handle during test startup.Proposed cleanup fix
- await mongoose.connect(config.db.uri); - await mongoose.connection.dropDatabase(); - await mongoose.disconnect(); + await mongoose.connect(config.db.uri); + try { + await mongoose.connection.dropDatabase(); + } finally { + await mongoose.disconnect(); + } } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/jest.globalSetup.js` around lines 35 - 40, The current try/catch swallows errors from mongoose.connection.dropDatabase() and skips mongoose.disconnect(), leaving an open connection; modify the block so that mongoose.disconnect() is always called (use a finally or separate try/catch) after mongoose.connect() and dropDatabase(), and log or rethrow the dropDatabase error as appropriate; reference mongoose.connect, mongoose.connection.dropDatabase, and mongoose.disconnect so the disconnect call is executed in all code paths even when dropDatabase() rejects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/jest.globalSetup.js`:
- Line 19: Add a JSDoc header immediately above the exported async function
signature "export default async () => {" that provides a one-line description
and an `@returns` tag; since this is an async setup function with no params,
include a line like "@returns {Promise<void>} Description of what the setup
returns" (or similar) to satisfy the rule that async functions always document
`@returns` and ensure no `@param` is required.
In `@scripts/tests/jest.globalSetup.unit.tests.js`:
- Around line 34-38: The afterEach teardown currently restores NODE_ENV by
assigning process.env.NODE_ENV = originalNodeEnv which will set the string
"undefined" if originalNodeEnv was absent; change the logic in the afterEach
block (where warnSpy.mockRestore(), process.env.NODE_ENV = originalNodeEnv,
jest.resetModules() live) to conditionally delete process.env.NODE_ENV when
originalNodeEnv is undefined and only assign when it has a real value so the
environment is correctly restored.
---
Outside diff comments:
In `@scripts/jest.globalSetup.js`:
- Around line 35-40: The current try/catch swallows errors from
mongoose.connection.dropDatabase() and skips mongoose.disconnect(), leaving an
open connection; modify the block so that mongoose.disconnect() is always called
(use a finally or separate try/catch) after mongoose.connect() and
dropDatabase(), and log or rethrow the dropDatabase error as appropriate;
reference mongoose.connect, mongoose.connection.dropDatabase, and
mongoose.disconnect so the disconnect call is executed in all code paths even
when dropDatabase() rejects.
🪄 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: 3b6e58f0-6d64-49f9-8fd6-f055aea04b68
📒 Files selected for processing (3)
jest.config.jsscripts/jest.globalSetup.jsscripts/tests/jest.globalSetup.unit.tests.js
- scripts/jest.globalSetup.js: add JSDoc @returns on the default export; wrap dropDatabase in try/finally so mongoose.disconnect() runs even when the drop rejects (no dangling connection). - scripts/tests/jest.globalSetup.unit.tests.js: afterEach now deletes NODE_ENV when the original was undefined (process.env coerces to the literal string "undefined" otherwise); add a 6th test covering the disconnect-on-drop-failure path.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR currently contains no code changes or new tests, which directly contradicts the stated intent of implementing database guards and adding at least five unit tests. This is a critical blocker as the core safety logic for scripts/jest.globalSetup.js and the verification tests are missing from the submission. While Codacy indicates the PR is up to standards, this is likely an artifact of an empty diff rather than a successful implementation.
About this PR
- The PR body describes several critical security guards and unit tests, but the submitted diff is empty. The implementation of environment guards (
NODE_ENV === 'test'), database name pattern matching, and the required unit tests inscripts/tests/must be included for review.
Test suggestions
- Successful database drop when NODE_ENV is 'test' and DB name contains 'test'
- Refusal to drop when NODE_ENV is 'test' but DB name is a production-like name
- Refusal to drop when NODE_ENV is set to a project-specific value (e.g., 'ism')
- Refusal to drop when NODE_ENV is undefined
- Graceful handling of unreachable MongoDB during global setup
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Successful database drop when NODE_ENV is 'test' and DB name contains 'test'
2. Refusal to drop when NODE_ENV is 'test' but DB name is a production-like name
3. Refusal to drop when NODE_ENV is set to a project-specific value (e.g., 'ism')
4. Refusal to drop when NODE_ENV is undefined
5. Graceful handling of unreachable MongoDB during global setup
🗒️ Improve review quality by adding custom instructions
Up to standards ✅🟢 Issues
|
Summary
scripts/jest.globalSetup.jssodropDatabase()is never called whenNODE_ENV !== 'test'or when the resolved DB name does not match/test/i.jest.config.jstestMatchto includescripts/tests/**/*.js.Why
Downstream projects (ism_node, trawl_node, comes_node, montaine_node) export
NODE_ENV=<project>in the developer shell so their config resolves to the project DB. Running jest directly (npx jest, IDE runner, editor extension) bypasses thecross-env NODE_ENV=testwrapper and the previous globalSetup happily dropped that project DB.Real incident (2026-04-20, ism_node):
NODE_ENV=ism npx jest modules/appointments --silentwiped 16 collections in 22 ms. No replica set / oplog / dump available, unrecoverable.Behaviour
testNodeTesttestProductionismundefinedTest plan
NODE_ENV=test npm run test:unit— 44 suites / 513 tests passingNODE_ENV=ism node -e "..."— logs refusal, no dropnpm run lint— cleanscripts/tests/jest.globalSetup.unit.tests.js— 5 tests passingCloses #3476
Summary by CodeRabbit
Tests
Bug Fixes