Fail hard when --concurrency is set to invalid values
#1478
Conversation
novemberborn
left a comment
novemberborn
left a comment
There was a problem hiding this comment.
Thanks @tdeschryver. The error message fix is welcome, and I like the integer parsing stuff too.
I'm wondering though whether the warning when 0 is actually a good idea. We just landed a PR that makes AVA pick a concurrency based on the number of CPUs available. I can see a scenario where "concurrency" is configured in the package.json#ava section, but users may still want to run with the default on their own machines. Supporting --concurrency=0 would allow this.
What do you think?
lib/cli.js
Outdated
| throw new Error(colors.error(figures.cross) + ' The --concurrency and -c flags must be provided.'); | ||
| } | ||
|
|
||
| if (cli.flags.concurrency && isNaN(parseInt(cli.flags.concurrency, 10))) { |
There was a problem hiding this comment.
I think this should be Number.isInteger(Number.parseFloat('cli.flags.concurrency')). That way even values like 9.5 are rejected, which parse to 9 with parseInt().
There was a problem hiding this comment.
Yep! Didn't thought about this case.
lib/cli.js
Outdated
| projectDir, | ||
| timeout: conf.timeout, | ||
| concurrency: conf.concurrency ? parseInt(conf.concurrency, 10) : 0, | ||
| concurrency: conf.concurrency ? parseInt(conf.concurrency, 10) : 1, |
There was a problem hiding this comment.
The default should remain 0. This allows AVA to pick an optimal value for your machine, see https://github.com/avajs/ava/blob/465fcecc9ae0d3274d4d41d3baaca241d6a40130/api.js#L164:L167.
There was a problem hiding this comment.
Oh sorry, I assumed the default had to change, because you wouldn't be able to set it to 0.
|
@novemberborn seems fair to me. I'll remove the |
|
@novemberborn would a check on a negative input be useful here? 🤔 |
novemberborn
left a comment
novemberborn
left a comment
There was a problem hiding this comment.
seems fair to me. I'll remove the 0 check.
Cool.
would a check on a negative input be useful here? 🤔
Heh why not! 😄
lib/cli.js
Outdated
| throw new Error(colors.error(figures.cross) + ' The --concurrency and -c flags must be provided.'); | ||
| } | ||
|
|
||
| if (cli.flags.concurrency && !Number.isInteger(Number.parseFloat(cli.flags.concurrency, 10))) { |
There was a problem hiding this comment.
parseFloat doesn't take a radix.
lib/cli.js
Outdated
|
|
||
| if (cli.flags.concurrency && | ||
| (!Number.isInteger(Number.parseFloat(cli.flags.concurrency)) || parseInt(cli.flags.concurrency, 10) < 0)) { | ||
| throw new Error(colors.error(figures.cross) + ' The --concurrency and -c flags must be a positive integer.'); |
There was a problem hiding this comment.
This should be nonnegative integer, since 0 is allowed. I feel a bit bad about this nitpick
There was a problem hiding this comment.
No problem! I should be the one saying sorry...
novemberborn
changed the title
--concurrency is set to 0--concurrency is set to invalid values
|
Thank you @tdeschryver! |
* Update warning message when the flag is used without a value * Detect non-integer or negative integer values
2 participants
Fixes #1476.
Changes:
0check.concurrencywas set to an empty string, because the error message seemed a bit weird.concurrencyis an integer.concurrencyfrom0to1if noconcurrencyis given.