Assumption that os.cpus() is nonempty - crash

[trenttobler]

running nyc mocha from an npm script results in an error where pLimit throws an exception because concurrency is passed as 0.

Expected `concurrency` to be a number from 1 and up, got `0` (number)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] cover: `nyc mocha`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] cover script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /data/data/com.termux/files/home/.npm/_logs/2020-12-06T14_50_52_546Z-debug.log

I've had issues with code using the os.cpus on Termux before (it is a permissions issue deep in node implementation, as I understand it). Behavior right now appears to be to return an empty array instead of unknown (see termux/termux-packages#1798 for example.). This can be demonstrated from within termux console directly:

$ node
Welcome to Node.js v14.14.0.
Type ".help" for more information.
> os.cpus()
[]
>

I manually edited the file, node_modules/nyc/index.js and replaced every use of os.cpus().length with a hard coded 1 value, and this fixed the error I was getting.

This ticket is to request the index.js use of os.cpus() sanitize the value before using it, to guarantee a positive value, as this appears to fix the issue in termux usage.

You can reproduce the behavior I see by mocking or hard coding a value of [] instead of os.cpus() to observe the behavior on any non-trivial package.

Expected Behavior

concurrency assumes 1 if it cannot determine # of cpus because of environmental protections and security (user space Termux app on android device, for example).

Environment Information

$ npx envinfo@latest --preset nyc
npx: installed 1 in 4.223s

  System:
    OS: android
    CPU: Unknown
    Memory: 263.54 MB / 5.09 GB                           Binaries:
    Node: 14.14.0 - /data/data/com.termux/files/usr/bin/node
    npm: 6.14.8 - /data/data/com.termux/files/usr/bin/npm
  npmPackages:
    nyc: ^15.1.0 => 15.1.0
    typescript: ^4.1.2 => 4.1.2

Thanks!

[coreyfarrell]

I wouldn't object to a PR which changed os.cpus().length to os.cpus().length || 1. Just to warn you you're probably going to see this as a widespread issue, I think everyone assumes that os.cpus() will return an array of no less than 1 element.

[trenttobler]

Agree on that. It has improved in the last two years - when I first encountered this, code similar to this was failing because length isn't defined on undefined.

I'll see if I can make a clean change to fix it and submit a PR.

[coreyfarrell]

Additional comment you will likely need os.cpus().length || /* istanbul ignore next: platform specific for termux */ 1. Otherwise npm test will fail for nyc itself due to coverage enforcement.

[trenttobler]

Looks like there are other problems - not yet sure what they are.

Running npm test on current master for nyc ends with:

Suites:   11 failed, 7 passed, 18 of 18 completed
Asserts:  194 failed, 201 passed, of 395

With a first candidate fix for cpus, it's a little better:

Suites:   7 failed, 11 passed, 18 of 18 completed
Asserts:  77 failed, 340 passed, of 417

Trying to consider if the other errors are significant - I don't think this is the coverage issue you mentioned is present yet. Still looking to determine to see how much effort getting a green test run.

[trenttobler]

Okay, after having taken a look at further errors - it appears termux may have additional behavior preventing nyc from fully measuring coverage, in this environment.

I'll go ahead and close this issue as it's probably going to be non-trivial for me to fix the full list of errors detected in the nyc unit tests after applying a change for os.cpus.