Read initial data from pre-instrumented files in "noop" instrumenter

[motiz88]

This is part of my fix for using --all on externally instrumented files, as when using babel-plugin-istanbul (istanbuljs/babel-plugin-istanbul#4). I laid out the idea for this part here.

Requires istanbuljs-archived-repos/istanbul-lib-instrument#28 and babel/babel#4746 (update: now both released, in istanbul-lib-instrument v1.2.0 and Babel v6.18.0 respectively).

To see the results and/or test this yourself, see the repo I've put up at https://github.com/motiz88/nyc-all-demo.

Known problems/kludges:

• The code makes a special case in _maybeInstrumentSource for !this.instrument && this.all, in which case it will always go on to "instrument" the file (actually read the existing data in the noop instrumenter). This is because we can't know until we've read the file whether it is a transpiled version of a file we're interested in.
• Instrumenting/testing when process.cwd() is a symlink causes problems at the moment, such as covered files being reported twice with different paths: one with 0% coverage and one with the actual coverage.

[bcoe]

@motiz88 slick, I like that we ultimately take advantage of nyc already having logic for walking the directory structure, rather than moving this logic into babel-plugin-istanbul like I was originally thinking.

Instrumenting/testing when process.cwd() is a symlink causes problems at the moment

This isn't a blocker, but I bet this is a bug we could put some tests around and squash in the near future.

[bcoe]

@motiz88 this will work fine if my project doesn't use babel, and we use --all, I'm sure our existing tests will catch this case.

We should also try to add a test for the new --all behavior; I'd be fine with doing it as a cli integration tests, and adding a new fixtures directory with babel config.

[motiz88]

One thing to think about is that all the parsing and traversing is costly. And with this PR we walk the entire directory structure and parse every bit of JS we find (though actually I think node_modules is blacklisted somewhere - right?), all without knowing whether the user has even instrumented anything. So from a perf/least surprise standpoint, maybe we should keep the noop instrumenter the way it is, and let the user opt-in to looking for externally instrumented files. Like maybe change the instrument option to true/false/'external'.

[bcoe]

@motiz88 it would be neat if we could get the best of both worlds; turn on the instrumenter hack for --all perhaps only if we detect we're in a babel environment. Perhaps there's a global set we can check?

[motiz88]

@bcoe Hmm. I can't think of anything that I would want nyc to strictly assume about the way Babel is being used. It feels cleanest when we frame it as "loading stuff instrumented separately".

I kind of take back my concern about doing completely unintended work: If this applies only to !instrument && all, then the intent to load instrumented files from a different source is pretty clear, and so is the intent to report on files that have not been loaded in the coverage run.

I can imagine, however, still wanting to limit that crawl to a known location. If my sources are in src/ and my transpiled+instrumented files are in dist/, I want to be able to say so via something like include and exclude, rather than let nyc loose on my entire directory tree.

[bcoe]

@motiz88 this line of reasoning seems reasonable; If I've gone to the trouble of turning off the instrumenter, there's about a 99% chance I'm using babel 👍

Let's try to avoid adding more configuration dials and knobs if possible -- would love for things to feel magic.

One thing I've been meaning to ask; this approach will work well with babel-register?

[motiz88]

@bcoe That's an interesting question.

Due to how babel-register works, files that are not required are never compiled. So nyc would have to instrument them to get the empty coverage object. Doesn't this already basically work when the config is all && instrument? (Haven't tested yet)

The one thing I can see being a problem is the babylon.parse call in istanbul-lib-instrument complaining on experimental syntax, which can be worked around by parsing with plugins: ["*"].

[motiz88]

Okay, so with the two blocking PRs merged and released, this just needs tests, right? @bcoe I won't get around to it until at least tomorrow, sadly.

[motiz88]

I am having multiple issues trying to set up a working test env on my Windows box. I don't believe I'll be able to write tests here any time soon. Maybe someone else can step in and complete this PR? (It's now functionally complete)

[coveralls]

Changes Unknown when pulling 06262f6 on motiz88:read-initial-coverage into * on istanbuljs:master*.

[bcoe]

@motiz88 I can step up to the plate this weekend; I was hoping to test behavior with babel-register any ways.

[bcoe]

I don't think we want this bit of logic (!this.instrument && this.all), we still want to avoid instrumenting files that are hit by our exclude rule, otherwise we'll end up instrumenting a huge chunk of the node_modules folder, which slows coverage to a crawl.

[motiz88]

Yeah, I mentioned this above in a comment:

I can imagine, however, still wanting to limit that crawl to a known location. If my sources are in src/ and my transpiled+instrumented files are in dist/, I want to be able to say so via something like include and exclude, rather than let nyc loose on my entire directory tree.

[motiz88]

But users may be surprised/confused at the need to include their generated files, especially if the mechanism for doing so is conflated with the one for their source files. Anyway, for a "magic" workflow, node_modules should probably be blacklisted by default no matter what the fine-tuning options are.

[bcoe]

But users may be surprised/confused at the need to include their generated files

I think the logic is correct if we just call this.exclude.shouldInstrument(filename, relFile) without any changes. Keep in mind that when we run nyc --all we iterate over all the files in the project using glob:

 glob.sync(pattern, {cwd: dir, nodir: true, ignore: this.exclude.exclude}).forEach(function (filename) {
    visitor(filename)
  })

As long as folks setup their include/exlude rules appropriately, this means that we'll pull in the coverage headers regardless of whether they've been pre-generated and stored to disk, or in the case of babel-register are being plopped in on the fly.

So far folks have been having good luck playing with this new feature:

istanbuljs/babel-plugin-istanbul#4

Great work @motiz88

[bcoe]

@motiz88 released in:

npm cache clear; npm i nyc@next

This is a pretty big change, so would love the extra help testing against a few of your libraries.