fix(jest-core): don't use workers in watch mode if runInBand specified #14085
Conversation
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.
There was a problem hiding this comment.
Hm.. I am not so sure why --runInBand should not use workers? That is not --doNotUseWorkers option. Performance should be better without workers, so perhaps this is all what shouldRunInBand() should take into account?
Something like --maxWorkers=0 could turn off workers. Just thinking out loud. Not sure if that is needed. I think, if tests run using workers or not is implementation detail and users shouldn’t have to care about that. If tests should run in parallel or in band is different problem already.
|
|
Ah.. That is absolutely fine with me. Less overthinking is always good (; This approach makes the logic of
|
|
It would be wrong to follow my suggestion above, because that is breaking change. @SimenB Looks like everything is in place. This PR is ready to land, if you are happy with it. |
Yeah if for some reason this approach doesn't work that could be a good one. This just seemed like the simplest and most robust approach if it works. |
|
/easycla |
|
Oh, new CLA? Will need to get my company to sign the new one if so. |
|
Yeah, it's Linux/JS Foundation now, sorry 😅 |
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Ok, sorry for the delay, finally got the CLA sorted and merged the changelog, so hopefully this is ready to go now! |
|
Workers are still used (code execution does not break in debugging tool) when It works when I add the option here but I doubt this is the right way to go. |
|
Yeah, I was actually debugging something earlier today and noticed the same thing |
|
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
In #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
--runInBandthat 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.Test plan
Ran unit tests