From 537d2138b81ad8ccaaad9750385f7929971bd3c4 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Thu, 6 Apr 2023 14:27:58 -0700 Subject: [PATCH 1/3] fix(jest-core): always use workers in watch mode The comment here already said one good reason to do this; another is that if the test hard-crashes (e.g. an async error after it completes) then using workers allows us to still watch for changes (perhaps to fix that crash). So now we just always use workers in watch mode; this probably worsens startup time slightly but for watch mode that's hopefully not as much of a problem. Fixes #13996. --- CHANGELOG.md | 1 + .../src/__tests__/testSchedulerHelper.test.js | 8 +++---- packages/jest-core/src/testSchedulerHelper.ts | 22 +++++++++---------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ad8312f260d..a61b91a1be2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - `[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-snapshot]` Fix a potential bug when not using prettier and improve performance ([#14036](https://github.com/facebook/jest/pull/14036)) +- `[jest-core]` Always use workers in watch mode to avoid crashes (TODO). ### Chore & Maintenance diff --git a/packages/jest-core/src/__tests__/testSchedulerHelper.test.js b/packages/jest-core/src/__tests__/testSchedulerHelper.test.js index dc5cc6620bda..d4fbc2756750 100644 --- a/packages/jest-core/src/__tests__/testSchedulerHelper.test.js +++ b/packages/jest-core/src/__tests__/testSchedulerHelper.test.js @@ -24,8 +24,8 @@ const getTestsMock = () => [getTestMock(), getTestMock()]; test.each` tests | timings | detectOpenHandles | maxWorkers | watch | workerIdleMemoryLimit | expectedResult - ${[getTestMock()]} | ${[500, 500]} | ${false} | ${undefined} | ${true} | ${undefined} | ${true} - ${getTestsMock()} | ${[2000, 500]} | ${false} | ${1} | ${true} | ${undefined} | ${true} + ${[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} @@ -36,8 +36,8 @@ test.each` ${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'} | ${true} - ${getTestsMock()} | ${[2000, 500]} | ${false} | ${1} | ${true} | ${'500MB'} | ${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} diff --git a/packages/jest-core/src/testSchedulerHelper.ts b/packages/jest-core/src/testSchedulerHelper.ts index 434e903e2118..0fdb0b15182d 100644 --- a/packages/jest-core/src/testSchedulerHelper.ts +++ b/packages/jest-core/src/testSchedulerHelper.ts @@ -27,25 +27,25 @@ export function shouldRunInBand( } /* - * Run in band if we only have one test or one worker available, unless we - * are using the watch mode, in which case the TTY has to be responsive and - * we cannot schedule anything in the main thread. Same logic applies to - * watchAll. + * If we are using watch/watchAll mode, don't schedule anything in the main + * thread to keep the TTY responsive and to keep the watcher running even if + * the test crashes. + */ + const isWatchMode = watch || watchAll; + if (isWatchMode) { + return false; + } + + /* + * 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. * https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17 */ - const isWatchMode = watch || watchAll; const areFastTests = timings.every(timing => timing < SLOW_TEST_TIME); const oneWorkerOrLess = maxWorkers <= 1; const oneTestOrLess = tests.length <= 1; - if (isWatchMode) { - return oneWorkerOrLess || (oneTestOrLess && areFastTests); - } - return ( // When specifying a memory limit, workers should be used !workerIdleMemoryLimit && From 9d42aec6220d6abdf3aea9904337dc603dcb2fd9 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Thu, 6 Apr 2023 14:36:31 -0700 Subject: [PATCH 2/3] link in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a61b91a1be2d..065e933170b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ - `[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-snapshot]` Fix a potential bug when not using prettier and improve performance ([#14036](https://github.com/facebook/jest/pull/14036)) -- `[jest-core]` Always use workers in watch mode to avoid crashes (TODO). +- `[jest-core]` Always use workers in watch mode to avoid crashes ([#14059](https://github.com/facebook/jest/pull/14059)). ### Chore & Maintenance From 8ce260c0a3183d29bf32c6c13b55a81df021d0f5 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Fri, 14 Apr 2023 16:04:57 -0700 Subject: [PATCH 3/3] reword comments --- packages/jest-core/src/testSchedulerHelper.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/jest-core/src/testSchedulerHelper.ts b/packages/jest-core/src/testSchedulerHelper.ts index 0fdb0b15182d..6582415ac5d5 100644 --- a/packages/jest-core/src/testSchedulerHelper.ts +++ b/packages/jest-core/src/testSchedulerHelper.ts @@ -28,11 +28,10 @@ export function shouldRunInBand( /* * If we are using watch/watchAll mode, don't schedule anything in the main - * thread to keep the TTY responsive and to keep the watcher running even if - * the test crashes. + * thread to keep the TTY responsive and to prevent watch mode crashes caused + * by leaks (improper test teardown). */ - const isWatchMode = watch || watchAll; - if (isWatchMode) { + if (watch || watchAll) { return false; } @@ -40,7 +39,9 @@ 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. - * https://github.com/facebook/jest/blob/700e0dadb85f5dc8ff5dac6c7e98956690049734/packages/jest-config/src/getMaxWorkers.js#L14-L17 + * 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;