No coverage analysis when running sub-command on Windows

[mewdriller]

So I've got an issue that I think I can isolate down to these two scenarios:

1: Running my tests directly from within the nyc command; e.g.: nyc ava --verbose or an npm script called cover with the same contents (nyc ava --verbose).

  - sagas » index » fetchRepo (success)
  - services » api » fetchAssignees
  √ actions » index » repo.request
  - sagas » index » fetchRepo (failure)
  - services » api » fetchLabels
  √ actions » index » repo.success
  √ actions » index » repo.failure
  - services » api » fetchMilestones
  - services » api » fetchRepos
  - services » api » fetchRepos
  √ services » api » fetchIssues catches errors with a single page (102ms)
  √ services » api » fetchIssues handles a single page (152ms)
  √ services » api » fetchIssues catches errors for multiple pages
  √ services » api » fetchIssues handles multiple pages

  7 tests passed [11:20:08]
  2 tests skipped
  5 tests todo

-----------|----------|----------|----------|----------|----------------|
File       |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-----------|----------|----------|----------|----------|----------------|
 actions\  |      100 |    66.67 |      100 |      100 |                |
  index.js |      100 |    66.67 |      100 |      100 |                |
 sagas\    |        0 |        0 |      100 |        0 |                |
  index.js |        0 |        0 |      100 |        0 |              8 |
 services\ |      100 |    76.92 |    58.33 |      100 |                |
  api.js   |      100 |    76.92 |    58.33 |      100 |                |
  index.js |      100 |      100 |      100 |      100 |                |
-----------|----------|----------|----------|----------|----------------|
All files  |    85.71 |    66.67 |    66.67 |    92.31 |                |
-----------|----------|----------|----------|----------|----------------|

2: Running my tests indirectly via an npm script (the test script contains ava --verbose); e.g.: nyc npm test or an npm script called cover with the same contents (nyc npm test).

  - sagas » index » fetchRepo (success)
  - sagas » index » fetchRepo (failure)
  - services » api » fetchAssignees
  √ actions » index » repo.request
  - services » api » fetchLabels
  √ actions » index » repo.success
  √ actions » index » repo.failure
  - services » api » fetchMilestones
  - services » api » fetchRepos
  - services » api » fetchRepos
  √ services » api » fetchIssues catches errors with a single page
  √ services » api » fetchIssues handles a single page
  √ services » api » fetchIssues catches errors for multiple pages
  √ services » api » fetchIssues handles multiple pages

  7 tests passed [11:23:23]
  2 tests skipped
  5 tests todo

----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
----------|----------|----------|----------|----------|----------------|
All files |      100 |      100 |      100 |      100 |                |
----------|----------|----------|----------|----------|----------------|

It seems like the nyc process isn't picking up the coverage information when there's another level of indirection.

System Specs

OS: Windows 10 Pro N
Node: 6.2.2
npm: 3.8.3
nyc: 6.6.1
ava: 0.15.2

[addaleax]

Do you have some kind of public reproduction of your problem? Usually nyc should be able to handle this situation.

[mewdriller]

@addaleax: I just pushed a repo to reproduce the issue. There are two npm scripts to reproduce the cases:

1: npm run cover-direct calls nyc ava --verbose

  √ repo.request
  √ repo.success
  √ repo.failure

  3 tests passed [13:13:23]

-------------|----------|----------|----------|----------|----------------|
File         |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-------------|----------|----------|----------|----------|----------------|
 src\        |      100 |    66.67 |      100 |      100 |                |
  actions.js |      100 |    66.67 |      100 |      100 |                |
-------------|----------|----------|----------|----------|----------------|
All files    |      100 |    66.67 |      100 |      100 |                |
-------------|----------|----------|----------|----------|----------------|

2: npm run cover calls nyc npm test, which in turn calls ava --verbose

  √ repo.request
  √ repo.success
  √ repo.failure

  3 tests passed [13:13:44]

----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
----------|----------|----------|----------|----------|----------------|
All files |      100 |      100 |      100 |      100 |                |
----------|----------|----------|----------|----------|----------------|

[addaleax]

Could you try this patch in node_modules/nyc/node_modules/spawn-wrap?

diff --git a/lib/win-rebase.js b/lib/win-rebase.js
index 3436c28327b2..0242b5f4a014 100644
--- a/lib/win-rebase.js
+++ b/lib/win-rebase.js
@@ -1,8 +1,18 @@
 var re = /^\s*("*)([^"]*?\b(?:node|iojs)(?:\.exe)?)("*)( |$)/
+var npmre = /^\s*("*)([^"]*?\b(?:npm))("*)( |$)/
+var which = require('which')
+var path_ = require('path')

 module.exports = function (path, rebase) {
   var m = path.match(re)
-  if (!m) return path
+  if (!m) {
+    m = path.match(npmre)
+    if (!m) return path
+    var npmPath = 'npm'
+    try { npmPath = which.sync('npm') } catch(e) {}
+    npmPath = path_.dirname(npmPath) + '\\node_modules\\npm\\bin\\npm-cli.js'
+    return path.replace(npmre, m[1] + rebase + ' "' + npmPath + '"' + m[3] + m[4])
+  }
   // preserve the quotes
   var replace = m[1] + rebase + m[3] + m[4]
   return path.replace(re, replace)

This seems to make your reproduction repository work for me.

[mewdriller]

Nice! With that patch applied to win-rebase.js I get the expected output from npm run cover.

Let me know if I can be of any help with testing/reproduce anything else.

[bcoe]

great find @addaleax! was this a regression? or an edge-case we'd missed?

[addaleax]

@bcoe This kind of looks like a slightly different variant of #190 for Windows, nothing actually new. You did the spawn-wrap tests for that one with { skip: winNoShebang } set, any chance you’d find the time to generalize them so they work on Windows, too?

I’m doing all the Windows stuff through a VM and it’s been a while since I did a lot of Windows work.

Also, I’m not sure… I’m feeling like my patch above would definitely mean relying on npm implementation details. That shouldn’t stop it from making it into spawn-wrap, but I think at some point I’d like to take the time to work out a more reliable solution. I know there’s been some back and forth on the npm cli issue tracker about these $PATH/$NODE things with which I’d like to catch up first, though.

[bcoe]

@addaleax we already have this logic in spawn-wrap:

https://github.com/tapjs/spawn-wrap/blob/master/index.js#L211

I think it would be reasonable to add your patch, but we should probably try to share the logic? what do you think. I agree let's get tests running on AppVeyor for as much of this functionality as possible, I also only run a VM.

[addaleax]

@bcoe Yes, that would be share-able… I’ll try to get some tests for this together later today if you don’t beat me to it. ;)

[bcoe]

@addaleax that would be amazing, I'm in the midst of marching istanbul/nyc towards a merger so don't think I'll be able to get to this issue as quickly as I'd like.

oh, speaking of which, if you have a few moments, would love to have you run as many of your projects as possible on this branch:

#303