Use at least one worker by default (fixes #7181)

[biomedia-thomas]

Fixes #7181 use at least 1 CPU worker instead of 0 by default when number of CPUs is 1.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[thymikee]

Can we also fix this in jest-runtime?
https://github.com/facebook/jest/blob/081e155587503e64703d4673b687f2b92390296b/packages/jest-runtime/src/cli/index.js#L79

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[thymikee]

Thinking about jest-runtime, shouldn't we use globalConfig.maxWorkers there? cc @SimenB @mjesun @rubennorte

[mjesun]

Absolutely. This code probably comes from some time ago where the maximum amount of workers could not be configured.

[rubennorte]

@thymikee I don't have all the context but I'd say we don't use the value in the config there because that call is to generate the haste map, whose workers are mainly used to synchronously read files from disk, which isn't CPU intensive anyway.

Edit: @mjesun I just saw your answer. Do you think what I said could have something to do with it?

[mjesun]

Even if Haste processes are not CPU consuming, they still consume some CPU and they might force the kernel to switch to them; so it's legit to say that they should be bound too.

What's however right is that if we say the limit is 8, we'll end up with 8 processes for haste + 8 processes for executing tests, which is not very nice... Either way, limiting it (even if you end up with 2x processes) is way better than just letting it go wild.

[rubennorte]

What's however right is that if we say the limit is 8, we'll end up with 8 processes for haste + 8 processes for executing tests, which is not very nice...

We wouldn't be having 16 processes because haste processes execute and finish before test execution. So it's 8 + 8 serially, which is fine.

[cpojer]

I agree everything should use the same configuration for workers. We just need to make sure we still properly run things in band when specified.

[SimenB]

Yeah, using config.maxWorkers should be right thing to do here, we've normalized for runInBand, n etc. being passed in jest-config: https://github.com/facebook/jest/blob/081e155587503e64703d4673b687f2b92390296b/packages/jest-config/src/normalize.js#L691

---

This also needs a changelog entry 🙂

[rickhanlonii]

Will this mean we'll use a single worker for runInBand (so it would fix #5731)?

[SimenB]

No (I think?). This is related to having a minimum >0, and normalizing the behavior. runInBand should still mean no worker, and no work on isolating it is proposed here (I think?)

[rubennorte]

Thanks @biomedia-thomas!

[rubennorte]

Oops, I merged too soon. I'm add the changelog entry myself 😅

[github-actions]

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.