we should still run transpilers on source files included via --all

[bcoe]

@jwhitfieldseed could you give this branch a shot and let me know if it does the trick for you?

Reviewers: @jamestalmage @novemberborn

[joews]

That works in my repro example and a real project. Thanks! 👏

[jamestalmage]

My only thought would be to do this conditionally if we don't already have coverage data on that particular file. This feels like a performance hit.

[novemberborn]

Looks good. Only thing I can think of is maybe to gitignore ./test/fixtures/needs-transpile.js in case a test crashes and it's left behind.

[bcoe]

@albertogasparin @jwhitfieldseed could you give the next tag of nyc a spin and let me know if things are fixed for you:

npm i nyc@next

[albertogasparin]

Not there yet. Now I got a different error:

/react-starterkit/node_modules/nyc/node_modules/source-map/lib/source-map-consumer.js:542
        throw new TypeError('Column must be greater than or equal to 0, got '
              ^
TypeError: Column must be greater than or equal to 0, got -2
    at SourceMapConsumer_findMapping [as _findMapping] (/react-starterkit/node_modules/nyc/node_modules/source-map/lib/source-map-consumer.js:542:15)
    at SourceMapConsumer_originalPositionFor [as originalPositionFor] (/react-starterkit/node_modules/nyc/node_modules/source-map/lib/source-map-consumer.js:603:24)
    at mapLocation (/react-starterkit/node_modules/nyc/lib/source-map-cache.js:33:25)
    at /react-starterkit/node_modules/nyc/lib/source-map-cache.js:93:18
    at Array.forEach (native)
    at SourceMapCache._rewriteStatements (/react-starterkit/node_modules/nyc/lib/source-map-cache.js:92:40)
    at /react-starterkit/node_modules/nyc/lib/source-map-cache.js:23:11
    at Array.forEach (native)
    at SourceMapCache.applySourceMaps (/react-starterkit/node_modules/nyc/lib/source-map-cache.js:15:23)
    at NYC.writeCoverageFile (/react-starterkit/node_modules/nyc/index.js:296:25)

This is the project I'm testing it on: https://github.com/albertogasparin/react-starterkit/tree/nyc-coverage

[albertogasparin]

By looking at the source code, I've also tried adding --cache. However it looks like the option gets ignored, as in NYC.prototype.writeCoverageFile this.enableCache is always false when --all is also defined.

[joews]

@bcoe nyc@next works for me on https://github.com/jwhitfieldseed/prequel. Thanks!

[novemberborn]

I've reproduced @albertogasparin's problem (don't forget to install nyc@next in the checkout anybody else wants to try). Looks like the coverage report for the ./api/todos/index.js file has an illegal statement location:

statementMap:
   { '1': { start: { line: 1, column: -2 }, end: { line: 1, column: 60 } },

Seems like an Istanbul bug to me so perhaps @albertogasparin you could report it upstream?.

Meanwhile I did some hacks to normalize the positions, see this branch: https://github.com/novemberborn/nyc/tree/clamp-positions.

@bcoe do you think this is something we should fix on our end? I can turn that branch into a PR with some tests.

[albertogasparin]

Thanks for pointing out the statementMap!
Tried removing "retainLines": true in .babelrc and it worked!
I'm wondering if it should be reported to istambul or if it is a problem on source map creation.

[novemberborn]

Applying the source map to the report that was created by Istanbul is what fails. retainLines would create some rather long lines I suppose, maybe that's tripping up Istanbul. I don't quite know enough about the Istanbul internals to reason about it further though.

[bcoe]

@novemberborn great debugging; I would definitely be interested in patching this on our end, but I think we should also open an issue with Istanbul, and be prepared to pull our code out (mind doing this).

@novemberborn want to open a pull request with your patch, and we can see if it fixes @albertogasparin's problem?

[novemberborn]

@bcoe done the PR now (#150).

I've tried reproducing the issue with @albertogasparin's code by transpiling the api/todos/index.js file, then covering it (like build-self-coverage does for NYC) and writing the report, but the positions are valid. That's awfully generic to report upstream 😞

[bcoe]

@albertogasparin @novemberborn give this a try:

npm i nyc@next

If things are looking good, I'd like to promote next to latest tomorrow.

[albertogasparin]

Tested nyc@next with both retainLines flag on and off, and both reports are generated as expected.
Thanks a lot guys for fixing it so quickly 👍

On a side note, it is interesting that, given the same source, I get two slightly different reports:

All files  | 42.03 | 36.21 | 30.19 | 41.61  # retainLines: true
All files  | 43.55 | 42.59 | 26.83 | 42.37  # retainLines: false

But I assume this is expected as the instrumented code is different.

[novemberborn]

On a side note, it is interesting that, given the same source, I get two slightly different reports:

All files | 42.03 | 36.21 | 30.19 | 41.61 # retainLines: true
All files | 43.55 | 42.59 | 26.83 | 42.37 # retainLines: false
But I assume this is expected as the instrumented code is different.

Given the illegal coverage positions returned by Istanbul we may not be able to map coverage to the correct source lines, hence the difference. But coverage is pretty far from 100% it seems, so relatively speaking it's not much of a concern. Would be interesting to see if the underlying error stops you from hitting 100%.

[albertogasparin]

Well, it looks like that by approaching 100% coverage the difference disappears. Files with 100% coverage are 100% regardless of the flag, so it is a non issue unless you care about 81% vs 82% 😉

[boneskull]

regarding retainLines, FWIW, I'm getting a fairly significant discrepancy:

All files | 72.7  | 51.94 | 49.83 | 95.31 |  # retainLines: true
All files | 96.13 | 75.61 | 83.62 | 96.13 |  # retainLines: false

Don't know if this is really worth pursuing anyway; I had it on to assist WebStorm with debugging.

[novemberborn]

@boneskull I wonder if the source map is less accurate when retainLines is on, or maybe it surfaces issues when we map the coverage report. Seems like an edge case either way.