Bug: Code coverage thresholds not respected

[kunagpal]

Please use the template provided below, when reporting bugs:

Expected Behavior

Programmatic use of NYC with breached code coverage thresholds results in a non-zero exit code.

Observed Behavior

While using NYC programmatically (via the .wrap() interface), coverage threshold breaches result in an exit code of 0.

Bonus Points! Code (or Repository) that Reproduces Issue

https://github.com/kunagpal/express-boilerplate/blob/develop/scripts/test/run.js#L104

Related issues and pull requests:

#384 [This one reports issues for Node v0.10]
#386 [This P.R fixes the above bug]

Suggestion

I feel the code here: https://github.com/istanbuljs/nyc/blob/master/index.js#L459 can be changed to the following:

process.exitCode && process.exit(process.exitCode); // ensures that code coverage is enforced correctly

The existing check on process.version will result in the exit code constraint being satisfied for Node v0.10.x - 0.11.x only.

Forensic Information

Operating System: MacOSX Sierra.
Environment Information: information about your project's environment, see instructions below:

1. run the following script:

sh -c 'node --version; npm --version; npm ls' > output.txt

2. share a gist with the contents of output.txt.
   https://gist.github.com/kunagpal/3f7f7d9c16fddaa8909797c06fec2bc0

PS: I'll be more than happy to send a P.R that fixes this issue.

[bcoe]

@kunagpal would love help digging into this, a good start would be adding a unit test. I'm a bit confused, because newer versions of Node.js should do the right thing when you set:

process.exitCode = 1

I just setup a slack community for the various open-source projects I manage, here:

http://devtoolscommunity.herokuapp.com/

If you have any questions while working on tests.

[kunagpal]

@bcoe I agree that newer versions of Node behave correctly with process.exitCode being set, but the issue lies in the fact that in the code here: https://github.com/istanbuljs/nyc/blob/master/index.js#L459

  var map = this._getCoverageMapFromAllCoverageFiles()
  var nyc = this

  if (perFile) {
    map.files().forEach(function (file) {
      // ERROR: Coverage for lines (90.12%) does not meet threshold (120%) for index.js
      nyc._checkCoverage(map.fileCoverageFor(file).toSummary(), thresholds, file)
    })
  } else {
    // ERROR: Coverage for lines (90.12%) does not meet global threshold (120%)
    nyc._checkCoverage(map.getCoverageSummary(), thresholds)
  }

  // process.exitCode was not implemented until v0.11.8.
  if (/^v0\.(1[0-1]\.|[0-9]\.)/.test(process.version) && process.exitCode !== 0) process.exit(process.exitCode)

From the last line of the snippet, we can see that process.exit will only be called if both conditions are satisfied:

• The current node version is 0.10.x or 0.11.x.
• The currently set exit code (process.exitCode) is non-zero.

The first part of the condition is what is causing the bug, and should be removed. Hope this makes the statement clearer. 😄

[bcoe]

@kunagpal I would start by adding a failing test for the behavior you're seeing, the fact that:

https://github.com/istanbuljs/nyc/blob/master/index.js#L466

Sets the exitCode to 1, should result in $? = 1 once the normal execution of the application completes; I'm not sure that we're really gaining much by not exiting immediately ... but you've tickled my curiosity as to what the underlying issue you're bumping into might be (it's probably something we should fix).

[kunagpal]

@bcoe Thanks for that pointer, I'll add the test.

[kunagpal]

@bcoe To add some more context, my current test suite setup is like this:

1. Run unit tests
2. Using NYC programmatically, generate, report, and enforce code coverage. (https://github.com/kunagpal/express-boilerplate/blob/develop/scripts/test/run.js#L104)
3. If there are no errors in 2, proceed to other tests.

Since the current coverage checking logic is to eagerly exit only for older Node versions, test suites using NYC programmatically on newer Node versions would continue to run even if the code coverage thresholds were not satisfied.

PS: This does not happen when NYC is used via the CLI, or when just the unit tests are run in isolation.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[JaKXz]

I don't think this is an issue with the latest versions of nyc so I'm going to close this for now. Please let me know if not!