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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Oct 30, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f060d9
Read initial data from pre-instrumented files in "noop" instrumenter
motiz88 Oct 19, 2016
f658352
Update istanbul-lib-instrument to v1.2.0
motiz88 Oct 25, 2016
5f7fac1
Fix style
motiz88 Oct 25, 2016
06262f6
Use lastCoverage.path over lastCoverage.data.path
motiz88 Oct 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ function NYC (config) {

this.processInfo = new ProcessInfo(config && config._processInfo)
this.rootId = this.processInfo.root || this.generateUniqueID()
this.instrument = config.instrument
this.all = config.all
}

NYC.prototype._createTransform = function (ext) {
Expand Down Expand Up @@ -166,6 +168,9 @@ NYC.prototype.addAllFiles = function () {
_this.addFile(filename)
var coverage = coverageFinder()
var lastCoverage = _this.instrumenter().lastFileCoverage()
if (lastCoverage) {
filename = lastCoverage.path
}
if (lastCoverage && _this.exclude.shouldInstrument(filename)) {
coverage[filename] = lastCoverage
}
Expand Down Expand Up @@ -233,7 +238,7 @@ NYC.prototype.walkAllFiles = function (dir, visitor) {
}

NYC.prototype._maybeInstrumentSource = function (code, filename, relFile) {
var instrument = this.exclude.shouldInstrument(filename, relFile)
var instrument = (!this.instrument && this.all) || this.exclude.shouldInstrument(filename, relFile)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bcoe bcoe Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!instrument) {
return null
}
Expand Down
11 changes: 10 additions & 1 deletion lib/instrumenters/noop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
var FileCoverage = require('istanbul-lib-coverage').classes.FileCoverage
var readInitialCoverage = require('istanbul-lib-instrument').readInitialCoverage

function NOOP () {
return {
instrumentSync: function (code) {
var extracted = readInitialCoverage(code)
if (extracted) {
this.fileCoverage = new FileCoverage(extracted.coverageData)
} else {
this.fileCoverage = null
}
return code
},
lastFileCoverage: function () {
return null
return this.fileCoverage
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
"glob": "^7.0.6",
"istanbul-lib-coverage": "^1.0.0",
"istanbul-lib-hook": "^1.0.0-alpha.4",
"istanbul-lib-instrument": "^1.1.3",
"istanbul-lib-instrument": "^1.2.0",
"istanbul-lib-report": "^1.0.0-alpha.3",
"istanbul-lib-source-maps": "^1.0.2",
"istanbul-reports": "^1.0.0-alpha.8",
Expand Down