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.

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

--maxWorkers only allows numbers and no % of cores #8559

Closed
philiiiiiipp opened this issue Jun 13, 2019 · 4 comments · Fixed by #8565
Closed

--maxWorkers only allows numbers and no % of cores #8559

philiiiiiipp opened this issue Jun 13, 2019 · 4 comments · Fixed by #8565

Comments

@philiiiiiipp
Copy link
Contributor

🐛 Bug Report

Giving --maxWorkers a % instead of a fixed core number does not seem to work.

I looked into it, I found that it seems to expect a number and parsing it sets it to NaN

maxWorkers: NaN,
  w: NaN,
  'max-workers': NaN,
  all: undefined,
  bail: undefined,

in jest-cli it checks the args only for a number

https://github.com/facebook/jest/blob/master/packages/jest-cli/src/cli/args.ts#L345

maxWorkers: {
    alias: 'w',
    description:
      'Specifies the maximum number of workers the worker-pool ' +
      'will spawn for running tests. This defaults to the number of the ' +
      'cores available on your machine. (its usually best not to override ' +
      'this default)',
    type: 'number',
  },

I quickly tried setting it to a string and it works as expected. I am not sure of all the ramifications of this change but it might be the correct type for it.

To Reproduce

run jest with --maxWorkers=20% and it will default to 50%

Expected behavior

It should use only 20% of my cpus

Run npx envinfo --preset jest

npx: installed 1 in 1.01s

  System:
    OS: macOS 10.14.5
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
  Binaries:
    Node: 8.12.0 - /usr/local/opt/node@8/bin/node
    Yarn: 1.12.3 - ~/.npm-packages/bin/yarn
    npm: 6.9.0 - ~/.npm-packages/bin/npm
  npmPackages:
    jest: ^24.8.0 => 24.8.0 
@philiiiiiipp philiiiiiipp changed the title --maxWorkers=20% --maxWorkers only allows numbers and no % Jun 13, 2019
@philiiiiiipp philiiiiiipp changed the title --maxWorkers only allows numbers and no % --maxWorkers only allows numbers and no % of cores Jun 13, 2019
@thymikee
Copy link
Collaborator

It should be a string. Can you send a PR fixing this? :)
We'll likely be able to remove the casting after this change:
https://github.com/facebook/jest/blob/43d1cf6ad49b00c077c01560bbedc9fc8c61311d/packages/jest-config/src/getMaxWorkers.ts#L17-L19

@philiiiiiipp
Copy link
Contributor Author

Ok will do :-)

@philiiiiiipp
Copy link
Contributor Author

It should be a string. Can you send a PR fixing this? :)
We'll likely be able to remove the casting after this change:

https://github.com/facebook/jest/blob/43d1cf6ad49b00c077c01560bbedc9fc8c61311d/packages/jest-config/src/getMaxWorkers.ts#L17-L19

I looked into that. I think in principle we would actually need to make it a string e.g. parseInt(maxWorkers + '', 10). Js is just not really harsh on us and working even if maxWorkers is a number.

I traced this variable around a bit and it seems as if there are multiple definitions depending on where this is coming from

https://github.com/facebook/jest/blob/43d1cf6ad49b00c077c01560bbedc9fc8c61311d/packages/jest-types/src/Config.ts#L325

and

https://github.com/facebook/jest/blob/43d1cf6ad49b00c077c01560bbedc9fc8c61311d/packages/jest-types/src/Config.ts#L457

I am not sure if we can savely change this to a string everywhere rather than a string | number. I suppose this could be a seperate pull request.

@github-actions
Copy link

This issue 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants