fix(test): skip per-suite migration re-runs via globalSetup pre-warm (#3549)

PierreBrisorgueil · web-flow · commit eae9410dc864 · 2026-05-01T08:36:43.000+02:00
* fix(test): skip per-suite migration re-runs via globalSetup pre-warm 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). * fix(test): address CR/Copilot/Codacy review findings on globalSetup - 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 * test(migrations): cover claim/unclaim/error branches via repo mocks 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%.
diff --git a/lib/app.js b/lib/app.js
@@ -39,7 +39,26 @@ const startExpress = async () => {
 
 /**
  * Bootstrap the required services
- * @returns {Object} db and app instances
+ *
+ * In test mode (--runInBand, single process), subsequent calls skip the
+ * migrations step by checking the `DEVKIT_MIGRATIONS_RAN` environment variable.
+ * That variable is set by `scripts/jest.globalSetup.js` which runs ONCE before
+ * any test vm context is created. Keys set on process.env in globalSetup are
+ * included in jest's protectProperties() sweep and survive per-suite vm context
+ * teardown unchanged — unlike module-scope vars or globalThis which are reset
+ * per test file under --experimental-vm-modules.
+ *
+ * Migrations are DB-backed and idempotent; skipping the glob scan + DB
+ * round-trip on 2nd+ suite bootstraps saves ≈ 100 ms × 18 suites ≈ 1.8 s
+ * and avoids redundant file-read accumulation under --experimental-vm-modules.
+ *
+ * Each test suite still gets a fresh Express app and mongoose connection so
+ * per-suite module state (e.g. config mutations in tests) is correctly isolated.
+ *
+ * In production / non-test environments the env flag is never set, so behaviour
+ * is unchanged.
+ *
+ * @returns {Promise<{db: object, app: object}>} Resolves with db and app instances
  */
 const bootstrap = async () => {
   let db;
@@ -48,7 +67,16 @@ const bootstrap = async () => {
   try {
     await SentryService.init();
     db = await startMongoose();
-    await migrations.run();
+    // DEVKIT_MIGRATIONS_RAN is set by jest.globalSetup.js before any vm context
+    // is created, so it persists across all test suite vm context teardown cycles.
+    // Falls through to migrations.run() when globalSetup failed or flag is absent.
+    // Scoped to NODE_ENV=test so the flag cannot accidentally skip migrations in
+    // production or dev (e.g. when --env-file-if-exists=.env injects it).
+    const migrationsAlreadyRan =
+      process.env.NODE_ENV === 'test' && process.env.DEVKIT_MIGRATIONS_RAN === '1';
+    if (!migrationsAlreadyRan) {
+      await migrations.run();
+    }
     app = await startExpress();
   } catch (e) {
     throw new Error(`unable to initialize Mongoose or ExpressJS : ${e}`);
diff --git a/modules/core/tests/migrations.unit.tests.js b/modules/core/tests/migrations.unit.tests.js
@@ -1,13 +1,15 @@
 /**
  * Module dependencies.
  */
+import { jest } from '@jest/globals';
 import path from 'path';
 import mongoose from 'mongoose';
 
 // Ensure the Migration model is registered before tests
 import '../models/migration.model.mongoose.js';
 
 import migrations from '../../../lib/services/migrations.js';
+import migrationRepository from '../repositories/migration.repository.js';
 
 /**
  * Unit tests for the migration system (no DB connection required)
@@ -80,4 +82,78 @@ describe('Migrations unit tests:', () => {
       expect(namePath.options.unique).toBe(true);
     });
   });
+
+  // The following tests stub migrationRepository directly so they exercise the
+  // claim / unclaim / error branches without needing a live MongoDB. Bootstrap
+  // no longer re-runs migrations.run() per suite (it sets DEVKIT_MIGRATIONS_RAN
+  // in globalSetup), so these branches must be covered explicitly.
+  describe('repository-mocked branches', () => {
+    let createSpy;
+    let deleteByNameSpy;
+    let listExecutedSpy;
+    let syncIndexesSpy;
+
+    beforeEach(() => {
+      createSpy = jest.spyOn(migrationRepository, 'create');
+      deleteByNameSpy = jest.spyOn(migrationRepository, 'deleteByName').mockResolvedValue({ acknowledged: true, deletedCount: 1 });
+      listExecutedSpy = jest.spyOn(migrationRepository, 'listExecuted');
+      syncIndexesSpy = jest.spyOn(migrationRepository, 'syncIndexes').mockResolvedValue([]);
+    });
+
+    afterEach(() => {
+      createSpy.mockRestore();
+      deleteByNameSpy.mockRestore();
+      listExecutedSpy.mockRestore();
+      syncIndexesSpy.mockRestore();
+    });
+
+    describe('runMigration claim path', () => {
+      it('returns false when another runner already claimed the migration (E11000)', async () => {
+        // Simulate the unique-index duplicate-key error from a concurrent runner
+        const dup = Object.assign(new Error('E11000 duplicate key'), { code: 11000 });
+        createSpy.mockRejectedValueOnce(dup);
+        const result = await migrations.runMigration('modules/core/migrations/__never-run-claim-fail.js', new Set());
+        expect(result).toBe(false);
+        expect(createSpy).toHaveBeenCalledTimes(1);
+      });
+
+      it('rethrows non-duplicate errors from the repository', async () => {
+        createSpy.mockRejectedValueOnce(new Error('boom'));
+        await expect(
+          migrations.runMigration('modules/core/migrations/__never-run-other-error.js', new Set()),
+        ).rejects.toThrow('boom');
+      });
+
+      it('unclaims when the migration file fails to import', async () => {
+        createSpy.mockResolvedValueOnce({ name: 'doesNotExist' });
+        await expect(
+          migrations.runMigration('modules/core/migrations/__file-does-not-exist.js', new Set()),
+        ).rejects.toThrow();
+        expect(deleteByNameSpy).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    describe('run() summary branches', () => {
+      it('returns total/executed counts when all migrations are already executed', async () => {
+        // listExecuted returns every discovered file → executedCount stays 0
+        const files = await migrations.discoverMigrationFiles();
+        const executedNames = files.map((f) => path.relative(process.cwd(), f).replace(/\\/g, '/'));
+        listExecutedSpy.mockResolvedValueOnce(executedNames.map((name) => ({ name })));
+        const result = await migrations.run();
+        expect(result.total).toBe(files.length);
+        expect(result.executed).toBe(0);
+      });
+
+      it('unclaims and rethrows when the imported migration has no up() export', async () => {
+        // claim succeeds; the file we point at exists but exports no up()
+        createSpy.mockResolvedValueOnce({ name: 'no-up' });
+        // This very test file is a real ESM module that does not export up()
+        const realFileWithoutUp = path.resolve('modules/core/tests/migrations.unit.tests.js');
+        await expect(
+          migrations.runMigration(realFileWithoutUp, new Set()),
+        ).rejects.toThrow(/does not export an up\(\) function/);
+        expect(deleteByNameSpy).toHaveBeenCalled();
+      });
+    });
+  });
 });
diff --git a/scripts/jest.globalSetup.js b/scripts/jest.globalSetup.js
@@ -1,7 +1,19 @@
 /**
- * Jest global setup - drops the test database before the test suite runs.
- * Replaces the gulp dropDB task that previously ran before jest.
- * Silently skips if MongoDB is unreachable (e.g. fresh CI environment).
+ * Jest global setup — runs once in the jest main process before any test file.
+ *
+ * Responsibilities:
+ *  1. Drop the test database (clean-slate guard, original behaviour).
+ *  2. Run migrations once and set `process.env.DEVKIT_MIGRATIONS_RAN = '1'`
+ *     so that per-suite bootstrap() calls can skip the migrations step.
+ *
+ * Why process.env and not process properties or globalThis?
+ *   Jest --experimental-vm-modules creates a fresh vm.createContext() for each
+ *   test file. Both globalThis and arbitrary process properties set INSIDE a vm
+ *   context are cleaned up by jest's GlobalProxy on teardown.
+ *   process.env keys set IN GLOBALSETUP (before any vm context is created) are
+ *   present when jest calls protectProperties(process) during vm context
+ *   initialisation — they land in the "protected" set and survive all
+ *   per-suite teardown cycles unchanged.
  *
  * Safety guards (#3476):
  *   1. Refuse to drop anything when `NODE_ENV !== 'test'`. Protects downstream
@@ -15,14 +27,12 @@
  * Both checks log a clear refusal reason and exit without connecting.
  */
 import mongoose from 'mongoose';
+import path from 'path';
+import { pathToFileURL } from 'url';
 
 /**
- * Jest global setup entry point — drops the test database before the suite runs.
- * Enforces the NODE_ENV + DB-name guards described at the top of this file.
- * @returns {Promise<void>} Resolves once the guard check completes and, when
- * guards pass and Mongo is reachable, once the database has been dropped and
- * the connection closed. Never rejects — a Mongo failure is swallowed so tests
- * can still run against existing state (e.g. fresh CI without mongo).
+ * Jest global setup entry point.
+ * @returns {Promise<void>}
  */
 export default async () => {
   if (process.env.NODE_ENV !== 'test') {
@@ -31,6 +41,8 @@ export default async () => {
     );
     return;
   }
+
+  // 1. Drop the test database (clean-slate guard)
   try {
     const config = (await import('../config/index.js')).default;
     const dbName = new URL(config.db.uri).pathname.replace(/^\//, '');
@@ -40,7 +52,7 @@ export default async () => {
       );
       return;
     }
-    await mongoose.connect(config.db.uri);
+    await mongoose.connect(config.db.uri, config.db.options);
     try {
       await mongoose.connection.dropDatabase();
     } finally {
@@ -49,4 +61,38 @@ export default async () => {
   } catch {
     // MongoDB unreachable or drop failed — tests will run against existing state
   }
+
+  // 2. Run migrations once before any test vm context is created.
+  //    Set process.env.DEVKIT_MIGRATIONS_RAN = '1' so per-suite bootstrap() calls
+  //    can skip migrations.run() (saves ~100ms × 18 suites ≈ 1.8s).
+  //    Keys set on process.env HERE (before vm contexts) are included in jest's
+  //    protectProperties() pass and survive per-suite GlobalProxy teardown.
+  try {
+    const config = (await import('../config/index.js')).default;
+    if (!Array.isArray(config.files?.mongooseModels)) {
+      console.warn('[jest.globalSetup] migrations pre-run skipped: config.files.mongooseModels is absent or not an array.');
+      return;
+    }
+    await mongoose.connect(config.db.uri, config.db.options);
+    try {
+      // Load mongoose models (needed by migration model registry)
+      await Promise.all(
+        config.files.mongooseModels.map(async (modelPath) => {
+          await import(pathToFileURL(path.resolve(modelPath)).href);
+        }),
+      );
+
+      // Run pending migrations
+      const migrations = (await import('../lib/services/migrations.js')).default;
+      await migrations.run();
+
+      // Mark migrations as run so test vm contexts skip migrations.run()
+      process.env.DEVKIT_MIGRATIONS_RAN = '1';
+    } finally {
+      await mongoose.disconnect();
+    }
+  } catch (err) {
+    // Non-fatal: test suites will run migrations themselves (pre-existing behaviour)
+    console.warn('[jest.globalSetup] migrations pre-run failed — suites run individually:', err.message);
+  }
 };
diff --git a/scripts/tests/jest.globalSetup.unit.tests.js b/scripts/tests/jest.globalSetup.unit.tests.js
@@ -5,6 +5,12 @@
  * observed from inside a test. These tests exercise the module as a plain
  * async function — the same entry point jest uses — while mocking mongoose
  * and config to avoid touching a real database.
+ *
+ * globalSetup has two sequential phases:
+ *  Phase 1: connect → dropDatabase → disconnect (DB drop)
+ *  Phase 2: connect → loadModels → migrations.run → disconnect (migrations pre-run)
+ * Phase 2 uses an explicit guard — tests that mock config without
+ * `files.mongooseModels` skip the migrations phase (with a console.warn).
  */
 import { jest, describe, test, expect, beforeEach, afterEach } from '@jest/globals';
 
@@ -80,40 +86,50 @@ describe('scripts/jest.globalSetup safety guards', () => {
   test('drops database when NODE_ENV=test and DB name contains "test" (case-insensitive)', async () => {
     process.env.NODE_ENV = 'test';
     jest.unstable_mockModule('../../config/index.js', () => ({
-      default: { db: { uri: 'mongodb://localhost:27017/NodeTest' } },
+      default: { db: { uri: 'mongodb://localhost:27017/NodeTest' }, files: { mongooseModels: [] } },
     }));
     const { default: globalSetup } = await import('../jest.globalSetup.js');
 
     await globalSetup();
 
-    expect(connect).toHaveBeenCalledWith('mongodb://localhost:27017/NodeTest');
+    // Phase 1 (drop): connect → dropDatabase → disconnect
+    expect(connect).toHaveBeenCalledWith('mongodb://localhost:27017/NodeTest', undefined);
     expect(dropDatabase).toHaveBeenCalledTimes(1);
-    expect(disconnect).toHaveBeenCalledTimes(1);
+    // Phase 2 (migrations): connects again with mongooseModels: [] → no models to load,
+    // migrations.run skipped, disconnect called via finally block
+    // Minimum: both phases connected + disconnected at least once each
+    expect(connect.mock.calls.length).toBeGreaterThanOrEqual(2);
+    expect(disconnect.mock.calls.length).toBeGreaterThanOrEqual(2);
   });
 
   test('swallows errors when MongoDB is unreachable', async () => {
     process.env.NODE_ENV = 'test';
     connect.mockRejectedValueOnce(new Error('ECONNREFUSED'));
     jest.unstable_mockModule('../../config/index.js', () => ({
-      default: { db: { uri: 'mongodb://localhost:27017/NodeTest' } },
+      default: { db: { uri: 'mongodb://localhost:27017/NodeTest' }, files: { mongooseModels: [] } },
     }));
     const { default: globalSetup } = await import('../jest.globalSetup.js');
 
     await expect(globalSetup()).resolves.toBeUndefined();
+    // Phase 1 connect threw → dropDatabase never reached
     expect(dropDatabase).not.toHaveBeenCalled();
+    // Phase 2 still tries to connect after the Phase 1 failure (second call)
+    expect(connect.mock.calls.length).toBeGreaterThanOrEqual(2);
+    expect(disconnect).toHaveBeenCalledTimes(1);
   });
 
   test('disconnects even when dropDatabase() fails (no dangling connection)', async () => {
     process.env.NODE_ENV = 'test';
     dropDatabase.mockRejectedValueOnce(new Error('drop failed'));
     jest.unstable_mockModule('../../config/index.js', () => ({
-      default: { db: { uri: 'mongodb://localhost:27017/NodeTest' } },
+      default: { db: { uri: 'mongodb://localhost:27017/NodeTest' }, files: { mongooseModels: [] } },
     }));
     const { default: globalSetup } = await import('../jest.globalSetup.js');
 
     await expect(globalSetup()).resolves.toBeUndefined();
-    expect(connect).toHaveBeenCalledTimes(1);
+    // Phase 1: dropDatabase failed but disconnect is always called (finally block)
     expect(dropDatabase).toHaveBeenCalledTimes(1);
-    expect(disconnect).toHaveBeenCalledTimes(1);
+    // Both phases call disconnect via finally blocks — no dangling connections
+    expect(disconnect.mock.calls.length).toBeGreaterThanOrEqual(2);
   });
 });
