Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NYC v11.2.0 exclude file with sourcemap from coverage #666

Closed
pvdlg opened this issue Sep 5, 2017 · 9 comments
Closed

NYC v11.2.0 exclude file with sourcemap from coverage #666

pvdlg opened this issue Sep 5, 2017 · 9 comments

Comments

@pvdlg
Copy link
Contributor

pvdlg commented Sep 5, 2017

Expected Behavior

As previous version, the expected behavior is to include in the coverage all the files in include property whether they have a sourcemap or not.
It seems to be a regression due to #649.

Observed Behavior

When a file referenced in include has a sourcemap it's excluded from the coverage report.

Bonus Points! Code (or Repository) that Reproduces Issue

  • Create src/index.js with some sample code in it
  • Run babel src --source-maps --out-dir lib to create a compiled with a sourcemap in lib
  • Write a test file that import lib/index.js and test it
  • run nyc ava -v

NYC config in package.json:

  "nyc": {
    "lines": 100,
    "statements": 100,
    "functions": 100,
    "branches": 100,
    "include": [
      "lib/*.js"
    ],
    "reporter": [
      "lcov",
      "text"
    ],
    "all": true
  }

NYC will report:
----------|----------|----------|----------|----------|----------------|
File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
All files | Unknown | Unknown | Unknown | Unknown | |
----------|----------|----------|----------|----------|----------------|

Forensic Information

Operating System: OS X
Environment Information: https://gist.github.com/vanduynslagerp/8ff68570c269ed0f11403c77b128ad88

@bcoe
Copy link
Member

bcoe commented Sep 5, 2017

@princjef any thoughts about a compromise that would prevent this bug and also address #581?

@vanduynslagerp would it be sufficient to move the this.exclude.shouldInstrument logic to after the coverage remapping has been applied?

@pvdlg
Copy link
Contributor Author

pvdlg commented Sep 5, 2017

@bcoe turns out I was wrong, #649 is not the cause of this issue. It's #637.
When I comment those 3 lines it works as expected.

I guess that

map.filter(function (filename) {
    return _this.exclude.shouldInstrument(filename)
  })

should happen before

map.data = this.sourceMaps.remapCoverage(map.data)

But I'm not really sure as I don't fully understand #637.

@bcoe
Copy link
Member

bcoe commented Sep 5, 2017

@vanduynslagerp hmm, this is pertinent to my interests because I specifically wanted this feature landed for Node.js any thoughts on how we can have our cake and eat it too? (CC: @schutm).

Shouldn't the fact that we've already remapped our coverage (map.data = this.sourceMaps.remapCoverage(map.data)) be doing the trick for us (obviously it's not, but I'm curious why not!).

@pvdlg
Copy link
Contributor Author

pvdlg commented Sep 5, 2017

What I'm doing is something similar to what is described in #637:

  • I have my es6 sources in src
  • I transpile them with babel in lib
  • I deliverer the content of lib therefore this is what I want to test, so my tests import what's in lib

In order to make that works (in 11.1.0) I have to add in package.json: "nyc": {"include": ["lib/*.js"]}. If I don't do that then the content of lib is not instrumented and not included in the coverage report.

With 1.12.0, because of "nyc": {"include": ["lib/*.js"]} the content of src is excluded and therefore filtered out by map.filter(function (filename) { return _this.exclude.shouldInstrument(filename)}).

I tried to add the content of src to the include array. But if I do so nyc attempt to instrument it and fail as it contains es6 import statement. And anyway there is no reason to instrument the content of src as I test with the content of lib.

I didn't dug really deep so far, but I think the problem is that when nyc determines what has to be instrumented it doesn't do a remap but it does before filtering the file to exclude from the report.

So in the scenario of src containing the sources, lib containing the transpiled sources and the sourcemaps and the tests being ran with lib we one of this 2 situation:

  1. include src
  • Instrument src => fails if you an es6 import
  • Test with lib
  • Probably include src in report
  1. include lib
  • Instrument lib
  • Test with lib
  • Include src only in report as it is remapped, and report that nothing was executed as src was indeed not executed in the tests

So my understanding (and I might miss something here) is that the scenario described in #637 was already working as long as you add the actual files you are testing on to the include option and make it restrictive enough to exclude the dependencies you do not want to report on.

@bcoe
Copy link
Member

bcoe commented Sep 6, 2017

@vanduynslagerp out of curiosity, do things work for you if you do move the _this.exclude.shouldInstrument logic to above the coverage remapping? seems like this might be the quick fix.

@pvdlg
Copy link
Contributor Author

pvdlg commented Sep 6, 2017

Do you mean here ?

That was my intuition as well.

So, to be clear, that works:

map.filter(function (filename) {
  return _this.exclude.shouldInstrument(filename)
})
map.data = this.sourceMaps.remapCoverage(map.data)

That does not work:

map.data = this.sourceMaps.remapCoverage(map.data)
map.filter(function (filename) {
  return _this.exclude.shouldInstrument(filename)
})

@bcoe
Copy link
Member

bcoe commented Sep 6, 2017

@vanduynslagerp 👍 cool, if you submit a pull request with that change I'll try to get a patch out the door ASAP -- @schutm would you be able to confirm whether this modified implementation would continue to work for your use-case?

@pvdlg
Copy link
Contributor Author

pvdlg commented Sep 6, 2017

Sure ! PR done #667

@bcoe bcoe closed this as completed in #667 Sep 6, 2017
@schutm
Copy link
Contributor

schutm commented Sep 8, 2017

@vanduynslagerp I think my use case is slightly different. You have your sources and transpiled files in the same root folder (project folder). I have my transpiled files in folder next to my project folder. E.g. your structure looks like:

Projects
    Project A
        src
        lib
    Project B
        src
        lib

Mine would like:

Projects
    Project A
        src
    Project B
        src
    Outputs
        lib A
        lib B

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants