Implement function to extract empty coverage data from an instrumented file

[motiz88]

This is in support of fixing istanbuljs/babel-plugin-istanbul#4. Next step would be to consume this new function in nyc when not instrumenting.

This won't build until a babel-traverse release is made with babel/babel#4746. PRing early so the code can be reviewed until then.

[yahoocla]

CLA is valid!

[bcoe]

This is looking really slick, however I'm not quite picturing how we'll handle walking every file yet.

Is the idea that nyc would apply its all logic, read in the raw un-compiled file, and then invoke readInitialCoverage and merge this onto its __coverage__ object?

[bcoe]

I'm I'm reading this correctly:

• you stop the parse early as soon as you see an assignment with the variable matching genVar.
• you're updating babel-traverse such that it now evaluates the object on the fly, allowing you to pluck this off the node as we traverse the tree.

This is quite clever and I dig it 👍

[motiz88]

😀

Just to be more precise here:

We parse the entire file, then stop traversing the AST the moment we find a variable declaration whose name is equal to genVar. Then we use babel-traverse's robust understanding of scopes and ability to statically evaluate nodes (much the opposite of valueToNode used here) to extract the value of coverageData from within that IIFE.

The change to babel-traverse merely adds support for evaluating object literals, which was a long-standing TODO in Babel and of course is essential here.

[motiz88]

So my plan is for nyc to read the compiled file. This minimizes duplicated work and the risk of breaking something if for some reason (babel config, version conflicts, the weather...) we don't duplicate it exactly right. That is, we defer entirely to whoever instrumented the file in the first place, and try to assume very little about that step - except that it was done by a version of visitor.js we're compatible with, and that the names of "our" variables have not been mangled by e.g. a minifier (this is actually a problem but I'm leaving it open atm).

[motiz88]

This actually gives me an idea: How about, instead of looking for the genVar hash (computed from sourceFilePath which we actually might not know when we first look at the file! As in the Babel codebase - source files are in src/ and transpiled to lib/; the hashes are for src/) we do this:

1. Have visitor insert a magic constant as an extra field in the initial coverage object - say, a salted hash of the current istanbul-lib-instrument major version.
2. Have read-coverage search directly for an object expression with this magic constant. This doesn't require any prior knowledge of the source file's location (or looking at the source map if any), and is robust in the face of minification & name mangling and most other transforms that could happen after instrumentation.

@bcoe I would love feedback on this direction as I now prefer it to what I implemented yesterday 😄

[motiz88]

This just leaves nyc: --all is is implemented using addAllFiles(), which attempts to grab instrumenter.lastFileCoverage(). With instrument: false, instrumenter is the noop instrumenter, for which lastFileCoverage() returns null. I would actually just use the noop instrumenter as a hook for this functionality - invoke readInitialCoverage inside its instrumentSync() method and use the result for lastFileCoverage().

I'll sketch out a PR to nyc and see what needs to be changed here, if anything. (* ahem MONOREPO ahem * 😉 )

EDIT: nyc will also need to retrieve sourceFilePath from the instrumented file - if any - and use that instead of filename in both the shouldInstrument() call and when storing the data in the global coverage map.

[bySabi]

I'm currently working on a new Karma Coverage Reporter plugin base on new Istanbul API, say v1?
Yesterday I was trying to implement the 'includeAllSource' feature ripping some code from nyc.
The current flow is:

• build code with babel-plugin-istanbul
• parse .babelrc and extract include/exclude istanbul plugin file rules
• Instrument this files with empty objects using

libInstrumenter.createInstrumenter({
    esModules: true     // Enable instrument ES6 modules
  });

Is a little hacky ...
I don´t understand everything your doing here but please take in consideration this use-case.

I stop add includeAllFiles feature until this is finnish.

[motiz88]

@bySabi sounds like the perfect use case for this. You're doing essentially the duplicated/brittle work I was worried about - having to look at .babelrc, instrumenting twice, etc. With this new feature you should be able to just readInitialCoverage from the final transpiled code.

You would probably still need to grab hold of nyc's include/exclude config somehow, to best emulate nyc's part of the task.

[bySabi]

Ah! fine this is wonderful @motiz88 ... I´m still trying to figure out the "correct way" to grab hold include/exclude rules settings in order of not duplicate it all over the place.

Parsing include/exclude from babelrc need to take in consideration env: { "test": { like keys.
Ideally we can have a status object with include/exclude rules, probably this is not posible, or extend test-exclude package to handle babelrc parsing or is anything better ...

[bySabi]

@motiz88 now all included files will be instrumented by default. Thats right?
Then options like includeAllFiles or --all not make sense anymore.

[motiz88]

No, that's not exactly right. The exact same files will be instrumented as
before, only now --all will work as intended with babel-plugin-istanbul
and correctly report 0% coverage for source files that are never required
in a coverage test run.

[bySabi]

But babel-plugin-istanbul will instrument not required modules always or how?

[motiz88]

That depends on how you run the plugin. If you compile an entire directory
with babel-cli and enable the plugin, it will instrument all of the files
you compile. (Then --all will be able to report on all of them, which
this PR will help accomplish.) But if you rely on a bundler to pull in and
transpile files, you may need to explicitly tell it to process any extra
files not found in your require() graph.

However, bundler integrations are out of scope for what I'm personally
doing here and in nyc. They certainly should be able to put this new API
to good use, though.

[bySabi]

Ok, I got it now! ... Let this is finnished/merged to see what more is needed to do with bundlers. I will be watching it.

Thanks.

[motiz88]

So this essentially works - see this test repo I made and the caveats on istanbuljs/nyc#420.

As for this specific PR, the new implementation is still not entirely robust - it expects certain local variable names to be used in the coverage initialization function, which won't survive e.g. minification. This can definitely be fixed if we identify the variables by order and/or content shape instead (at least if they're not found by name).

(Note: I've accidentally pulled in #29 but I'm going to leave it in unless you object, @bcoe, on the assumption that #29 will get merged soon anyhow)

[bcoe]

it expects certain local variable names to be used in the coverage initialization function, which won't survive e.g. minification.

I wonder is there any convention for making that a line should be skipped by a minifier, e.g., `/* skipNextLine */.

This is looking good, are we still waiting on the changes to babel-traverse to land, curious why we're seeing the failures.

[elodszopos]

Anxiously looking forward to this, boys. Keep up the good work.

[motiz88]

@bcoe Yeah, there's a Babel release being worked on right now which should include the updated babel-traverse (the PR has already been accepted and merged).

[bcoe]

@motiz88 just happened to notice a new babel release go out, does that mean we can rebase this and it will work :D