-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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.
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
fix(jest-core): always use workers in watch mode #14059
Conversation
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 jestjs#13996.
* 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. |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isWatchMode = watch || watchAll; | |
if (isWatchMode) { | |
if (watch || watchAll) { |
* 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase please? Otherwise all looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm down with this 👍
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.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
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.
Test plan
Updated existing unit tests. Let me know if it makes sense to add something e2e for the specific crash case in #13996.