Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(jest-core): always use workers in watch mode #14059

Merged
merged 4 commits into from
Apr 15, 2023
Merged

fix(jest-core): always use workers in watch mode #14059

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
537d213
fix(jest-core): always use workers in watch mode
benjaminjkraft Apr 6, 2023
9d42aec
link in changelog
benjaminjkraft Apr 6, 2023
8ce260c
reword comments
benjaminjkraft Apr 14, 2023
22c397a
Merge branch 'main' into watch-worker
SimenB Apr 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ([#14059](https://github.com/facebook/jest/pull/14059)).

### Chore & Maintenance

Expand Down
8 changes: 4 additions & 4 deletions packages/jest-core/src/__tests__/testSchedulerHelper.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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}
Expand Down
22 changes: 11 additions & 11 deletions packages/jest-core/src/testSchedulerHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@mrazauskas mrazauskas Apr 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. Perhaps the comment should say: "and prevents watch mode crashes caused by tests leaking due to improper teardown". Or something that way?

It isn’t that workers do prevent watch mode to crash due to fatal errors in tests, that is more about preventing leaks / open handles or debugging them (as you pointed out in the issue).

There is no mechanism to deal with this problem if tests run in band. Leaks are handled more gracefully in parallel run, because worker farm gets killed after tests run is finished:

https://github.com/facebook/jest/blob/470d72ee74e50291bf8593954f24eb75b5621351/packages/jest-runner/src/index.ts#L180-L194

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions -- took another stab at explaining this.

*/
const isWatchMode = watch || watchAll;
if (isWatchMode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isWatchMode = watch || watchAll;
if (isWatchMode) {
if (watch || watchAll) {

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this comment in place, please. Check that link, the logic there sets maxWorkers to 1 if runInBand is provided. At first it is not clear why runInBand is mentioned here, but it makes sense. Or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, thanks. (It seems with the workerIdleMemoryLimit check that is no longer quite right -- we should pass runInBand in here or more likely reject setting the two together -- but that's probably best left to another PR.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workerIdleMemoryLimit is a special case. It works only if workers are involved. (Feel free to add a comment.) There is no other mechanism to force workers. By the way, workerIdleMemoryLimit could be used to force workers in watch mode ;D

*/
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 &&
Expand Down