fix(jest-core): don't use workers in watch mode if runInBand specified

In jestjs#14059, I added code to say that if you use watch mode, even if you only run one test and one worker, we should still not run in band, because then a hard-crash in the test will crash the entire watcher. But the way the logic is written, this overrides even an explicit `--runInBand`, which can be useful if you really want to, say, attach a debugger to watch-mode; it's not the smoothest experience but it's better than nothing. In this commit I make it so that if you explicitly say `--runInBand` that overrides everything else. (But if you just say `--maxWorkers=1`, or you just only have one test to run, we still use workers.) This required some replumbing; let me know if there's a better way.
benjaminjkraft · Apr 18, 2023 · 38cdf0c · 38cdf0c
1 parent 15af9b3
commit 38cdf0c
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,7 +7,7 @@
 ### Fixes
 
 - `[jest-config]` Handle frozen config object ([#14054](https://github.com/facebook/jest/pull/14054))
-- `[jest-core]` Always use workers in watch mode to avoid crashes ([#14059](https://github.com/facebook/jest/pull/14059)).
+- `[jest-core]` Use workers in watch mode by default to avoid crashes ([#14059](https://github.com/facebook/jest/pull/14059) & TODO).
 - `[jest-environment-jsdom, jest-environment-node]` Fix assignment of `customExportConditions` via `testEnvironmentOptions` when custom env subclass defines a default value ([#13989](https://github.com/facebook/jest/pull/13989))
 - `[jest-matcher-utils]` Fix copying value of inherited getters ([#14007](https://github.com/facebook/jest/pull/14007))
 - `[jest-mock]` Tweak typings to allow `jest.replaceProperty()` replace methods ([#14008](https://github.com/facebook/jest/pull/14008))

diff --git a/packages/jest-config/src/index.ts b/packages/jest-config/src/index.ts
@@ -121,6 +121,7 @@ const groupOptions = (
     replname: options.replname,
     reporters: options.reporters,
     rootDir: options.rootDir,
+    runInBand: options.runInBand,
     runTestsByPath: options.runTestsByPath,
     seed: options.seed,
     shard: options.shard,

diff --git a/packages/jest-core/src/__tests__/testSchedulerHelper.test.js b/packages/jest-core/src/__tests__/testSchedulerHelper.test.js
@@ -23,32 +23,34 @@ const getTestMock = () => ({
 const getTestsMock = () => [getTestMock(), getTestMock()];
 
 test.each`
-  tests              | timings        | detectOpenHandles | maxWorkers   | watch    | workerIdleMemoryLimit | expectedResult
-  ${[getTestMock()]} | ${[500, 500]}  | ${false}          | ${undefined} | ${true}  | ${undefined}          | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${1}         | ${true}  | ${undefined}          | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${2}         | ${true}  | ${undefined}          | ${false}
-  ${[getTestMock()]} | ${[2000, 500]} | ${false}          | ${undefined} | ${true}  | ${undefined}          | ${false}
-  ${getTestMock()}   | ${[500, 500]}  | ${false}          | ${undefined} | ${true}  | ${undefined}          | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${1}         | ${false} | ${undefined}          | ${true}
-  ${getTestMock()}   | ${[2000, 500]} | ${false}          | ${2}         | ${false} | ${undefined}          | ${false}
-  ${[getTestMock()]} | ${[2000]}      | ${false}          | ${undefined} | ${false} | ${undefined}          | ${true}
-  ${getTestsMock()}  | ${[500, 500]}  | ${false}          | ${undefined} | ${false} | ${undefined}          | ${true}
-  ${new Array(45)}   | ${[500]}       | ${false}          | ${undefined} | ${false} | ${undefined}          | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${undefined} | ${false} | ${undefined}          | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${true}           | ${undefined} | ${false} | ${undefined}          | ${true}
-  ${[getTestMock()]} | ${[500, 500]}  | ${false}          | ${undefined} | ${true}  | ${'500MB'}            | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${1}         | ${true}  | ${'500MB'}            | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${1}         | ${false} | ${'500MB'}            | ${false}
-  ${[getTestMock()]} | ${[2000]}      | ${false}          | ${undefined} | ${false} | ${'500MB'}            | ${false}
-  ${getTestsMock()}  | ${[500, 500]}  | ${false}          | ${undefined} | ${false} | ${'500MB'}            | ${false}
-  ${getTestsMock()}  | ${[2000, 500]} | ${true}           | ${undefined} | ${false} | ${'500MB'}            | ${true}
+  tests              | timings        | detectOpenHandles | runInBand | maxWorkers   | watch    | workerIdleMemoryLimit | expectedResult
+  ${[getTestMock()]} | ${[500, 500]}  | ${false}          | ${false}  | ${undefined} | ${true}  | ${undefined}          | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${false}  | ${1}         | ${true}  | ${undefined}          | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${false}  | ${2}         | ${true}  | ${undefined}          | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${true}   | ${1}         | ${true}  | ${undefined}          | ${true}
+  ${[getTestMock()]} | ${[2000, 500]} | ${false}          | ${false}  | ${undefined} | ${true}  | ${undefined}          | ${false}
+  ${getTestMock()}   | ${[500, 500]}  | ${false}          | ${false}  | ${undefined} | ${true}  | ${undefined}          | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${false}  | ${1}         | ${false} | ${undefined}          | ${true}
+  ${getTestMock()}   | ${[2000, 500]} | ${false}          | ${false}  | ${2}         | ${false} | ${undefined}          | ${false}
+  ${[getTestMock()]} | ${[2000]}      | ${false}          | ${false}  | ${undefined} | ${false} | ${undefined}          | ${true}
+  ${getTestsMock()}  | ${[500, 500]}  | ${false}          | ${false}  | ${undefined} | ${false} | ${undefined}          | ${true}
+  ${new Array(45)}   | ${[500]}       | ${false}          | ${false}  | ${undefined} | ${false} | ${undefined}          | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${false}  | ${undefined} | ${false} | ${undefined}          | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${true}           | ${false}  | ${undefined} | ${false} | ${undefined}          | ${true}
+  ${[getTestMock()]} | ${[500, 500]}  | ${false}          | ${false}  | ${undefined} | ${true}  | ${'500MB'}            | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${false}  | ${1}         | ${true}  | ${'500MB'}            | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${false}          | ${false}  | ${1}         | ${false} | ${'500MB'}            | ${false}
+  ${[getTestMock()]} | ${[2000]}      | ${false}          | ${false}  | ${undefined} | ${false} | ${'500MB'}            | ${false}
+  ${getTestsMock()}  | ${[500, 500]}  | ${false}          | ${false}  | ${undefined} | ${false} | ${'500MB'}            | ${false}
+  ${getTestsMock()}  | ${[2000, 500]} | ${true}           | ${false}  | ${undefined} | ${false} | ${'500MB'}            | ${true}
 `(
   'shouldRunInBand() - should return $expectedResult for runInBand mode',
   ({
     tests,
     timings,
     detectOpenHandles,
     maxWorkers,
+    runInBand,
     watch,
     workerIdleMemoryLimit,
     expectedResult,
@@ -57,6 +59,7 @@ test.each`
       shouldRunInBand(tests, timings, {
         detectOpenHandles,
         maxWorkers,
+        runInBand,
         watch,
         workerIdleMemoryLimit,
       }),

diff --git a/packages/jest-core/src/testSchedulerHelper.ts b/packages/jest-core/src/testSchedulerHelper.ts
@@ -16,13 +16,15 @@ export function shouldRunInBand(
   {
     detectOpenHandles,
     maxWorkers,
+    runInBand,
     watch,
     watchAll,
     workerIdleMemoryLimit,
   }: Config.GlobalConfig,
 ): boolean {
+  // If user asked for run in band, respect that.
   // detectOpenHandles makes no sense without runInBand, because it cannot detect leaks in workers
-  if (detectOpenHandles) {
+  if (runInBand || detectOpenHandles) {
     return true;
   }
 
@@ -39,9 +41,6 @@ export function shouldRunInBand(
    * Otherwise, run in band if we only have one test or one worker available.
    * Also, if we are confident from previous runs that the tests will finish
    * quickly we also run in band to reduce the overhead of spawning workers.
-   * Finally, the user can provide the runInBand argument in the CLI to
-   * force running in band, which sets maxWorkers to 1 here:
-   * https://github.com/facebook/jest/blob/d106643a8ee0ffa9c2f11c6bb2d12094e99135aa/packages/jest-config/src/getMaxWorkers.ts#L27-L28
    */
   const areFastTests = timings.every(timing => timing < SLOW_TEST_TIME);
   const oneWorkerOrLess = maxWorkers <= 1;

diff --git a/packages/jest-types/src/Config.ts b/packages/jest-types/src/Config.ts
@@ -400,6 +400,7 @@ export type GlobalConfig = {
   randomize?: boolean;
   replname?: string;
   reporters?: Array<ReporterConfig>;
+  runInBand: boolean;
   runTestsByPath: boolean;
   rootDir: string;
   seed: number;

diff --git a/packages/test-utils/src/config.ts b/packages/test-utils/src/config.ts
@@ -47,6 +47,7 @@ const DEFAULT_GLOBAL_CONFIG: Config.GlobalConfig = {
   replname: undefined,
   reporters: [],
   rootDir: '/test_root_dir/',
+  runInBand: false,
   runTestsByPath: false,
   seed: 1234,
   silent: false,