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

Caching #101

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
191 changes: 127 additions & 64 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/* global __coverage__ */
var _ = require('lodash')
var fs = require('fs')
var glob = require('glob')
var micromatch = require('micromatch')
Expand All @@ -9,46 +8,43 @@ var path = require('path')
var rimraf = require('rimraf')
var onExit = require('signal-exit')
var stripBom = require('strip-bom')
var SourceMapCache = require('./lib/source-map-cache')
var resolveFrom = require('resolve-from')
var md5 = require('md5-hex')
var arrify = require('arrify')
var convertSourceMap = require('convert-source-map')
var endsWith = require('ends-with')

/* istanbul ignore next */
if (/index\.covered\.js$/.test(__filename)) {
require('./lib/self-coverage-helper')
}

function NYC (opts) {
_.extend(this, {
subprocessBin: path.resolve(
__dirname,
'./bin/nyc.js'
),
tempDirectory: './.nyc_output',
cwd: process.env.NYC_CWD || process.cwd(),
reporter: 'text',
istanbul: require('istanbul'),
sourceMapCache: new SourceMapCache(),
require: []
}, opts)

if (!Array.isArray(this.reporter)) this.reporter = [this.reporter]
opts = opts || {}

this._istanbul = opts.istanbul
this.subprocessBin = opts.subprocessBin || path.resolve(__dirname, './bin/nyc.js')
this._tempDirectory = opts.tempDirectory || './.nyc_output'
this._cacheDirectory = opts.cacheDirectory || './node_modules/.cache/nyc'
this.cwd = opts.cwd || process.env.NYC_CWD || process.cwd()
this.reporter = arrify(opts.reporter || 'text')

// you can specify config in the nyc stanza of package.json.
var config = require(path.resolve(this.cwd, './package.json')).config || {}
config = config.nyc || {}

// load exclude stanza from config.
this.include = config.include || ['**']
this.include = this._prepGlobPatterns(this.include)
this.include = false
if (config.include) {
this.include = this._prepGlobPatterns(arrify(config.include))
}

this.exclude = ['**/node_modules/**'].concat(config.exclude || ['test/**', 'test{,-*}.js'])
if (!Array.isArray(this.exclude)) this.exclude = [this.exclude]
this.exclude = ['**/node_modules/**'].concat(arrify(config.exclude || ['test/**', 'test{,-*}.js']))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could call this._prepGlobPatterns() here rather than reassigning this.exclude?

this.exclude = this._prepGlobPatterns(this.exclude)

// require extensions can be provided as config in package.json.
this.require = config.require ? config.require : this.require
this.require = arrify(config.require || opts.require)

this.instrumenter = this._createInstrumenter()
this._createOutputDirectory()
}

Expand All @@ -67,14 +63,18 @@ NYC.prototype._loadAdditionalModules = function () {
})
}

NYC.prototype.instrumenter = function () {
return this._instrumenter || (this._instrumenter = this._createInstrumenter())
}

NYC.prototype._createInstrumenter = function () {
var configFile = path.resolve(this.cwd, './.istanbul.yml')

if (!fs.existsSync(configFile)) configFile = undefined

var instrumenterConfig = this.istanbul.config.loadFile(configFile).instrumentation.config
var instrumenterConfig = this.istanbul().config.loadFile(configFile).instrumentation.config

return new this.istanbul.Instrumenter({
return new (this.istanbul()).Instrumenter({
Copy link
Contributor

Choose a reason for hiding this comment

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

Assign this.istanbul() within this function?

coverageVariable: '__coverage__',
embedSource: instrumenterConfig['embed-source'],
noCompact: !instrumenterConfig.compact,
Expand All @@ -85,41 +85,41 @@ NYC.prototype._createInstrumenter = function () {
NYC.prototype._prepGlobPatterns = function (patterns) {
if (!patterns) return patterns

var directories = []
patterns = _.map(patterns, function (pattern) {
var result = []

function add (pattern) {
if (result.indexOf(pattern) === -1) {
result.push(pattern)
}
}

patterns.forEach(function (pattern) {
// Allow gitignore style of directory exclusion
if (!_.endsWith(pattern, '/**')) {
directories.push(pattern.replace(/\/$/, '').concat('/**'))
if (!endsWith(pattern, '/**')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not !/\/\*\*$/.test(pattern) and do away with the ends-with dependency?

add(pattern.replace(/\/$/, '').concat('/**'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently String#concat() is slow.

Also, apparently it exists! 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

(And yes you didn't write concat(). But since we're speading things up 😜 )

}

return pattern
add(pattern)
})
return _.union(patterns, directories)

return result
}

NYC.prototype.addFile = function (filename, returnImmediately) {
NYC.prototype.addFile = function (filename) {
var relFile = path.relative(this.cwd, filename)
var instrument = this.shouldInstrumentFile(filename, relFile)
var content = stripBom(fs.readFileSync(filename, 'utf8'))

if (instrument) {
this.sourceMapCache.add(filename, content)
content = this.instrumenter.instrumentSync(content, './' + relFile)
} else if (returnImmediately) {
return {}
}

var source = stripBom(fs.readFileSync(filename, 'utf8'))
var content = this._addSource(source, filename, relFile)
return {
instrument: instrument,
content: content,
relFile: relFile
instrument: !!content,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty subtle (as is content || source below).

relFile: relFile,
content: content || source
}
}

NYC.prototype.shouldInstrumentFile = function (filename, relFile) {
relFile = relFile.replace(/^\.\//, '') // remove leading './'.

return (micromatch.any(filename, this.include) || micromatch.any(relFile, this.include)) &&
return (!this.include || micromatch.any(filename, this.include) || micromatch.any(relFile, this.include)) &&
!(micromatch.any(filename, this.exclude) || micromatch.any(relFile, this.exclude))
}

Expand All @@ -132,7 +132,7 @@ NYC.prototype.addAllFiles = function () {
var obj = _this.addFile(filename, true)
if (obj.instrument) {
module._compile(
_this.instrumenter.getPreamble(obj.content, obj.relFile),
_this.instrumenter().getPreamble(obj.content, obj.relFile),
filename
)
}
Expand All @@ -141,20 +141,38 @@ NYC.prototype.addAllFiles = function () {
this.writeCoverageFile()
}

var hashCache = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it doesn't matter in practice but may be better to have this on the instance?


NYC.prototype._addSource = function (code, filename, relFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rename this to _maybeInstrumentSource() it's easier to see why it may return null. Then you could assign to a instrumentedSource variable and the logic in addFile() becomes more clear.

var instrument = this.shouldInstrumentFile(filename, relFile)

if (!instrument) {
return null
}

var hash = md5(code)
hashCache['./' + relFile] = hash
var cacheFilePath = path.join(this.cacheDirectory(), hash + '.js')

try {
return fs.readFileSync(cacheFilePath, 'utf8')
} catch (e) {
var sourceMap = convertSourceMap.fromSource(code) || convertSourceMap.fromMapFileSource(code, path.dirname(filename))
if (sourceMap) {
var mapPath = path.join(this.cacheDirectory(), hash + '.map')
fs.writeFileSync(mapPath, sourceMap.toJSON())
}
var instrumented = this.instrumenter().instrumentSync(code, './' + relFile)
fs.writeFileSync(cacheFilePath, instrumented)
return instrumented
}
}

NYC.prototype._wrapRequire = function () {
var _this = this
appendTransform(function (code, filename) {
var relFile = path.relative(_this.cwd, filename)
var instrument = _this.shouldInstrumentFile(filename, relFile)

if (!instrument) {
return code
}

_this.sourceMapCache.add(filename, code)

// now instrument the compiled code.
return _this.instrumenter.instrumentSync(code, './' + relFile)
return _this._addSource(code, filename, relFile) || code
})
}

Expand All @@ -163,7 +181,8 @@ NYC.prototype.cleanup = function () {
}

NYC.prototype._createOutputDirectory = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a misnomer now.

mkdirp.sync(this.tmpDirectory())
mkdirp.sync(this.tempDirectory())
mkdirp.sync(this.cacheDirectory())
}

NYC.prototype._wrapExit = function () {
Expand All @@ -188,18 +207,28 @@ NYC.prototype.writeCoverageFile = function () {
if (typeof __coverage__ === 'object') coverage = __coverage__
if (!coverage) return

Object.keys(coverage).forEach(function (relFile) {
if (hashCache[relFile] && coverage[relFile]) {
coverage[relFile].contentHash = hashCache[relFile]
}
})

fs.writeFileSync(
path.resolve(this.tmpDirectory(), './', process.pid + '.json'),
JSON.stringify(this.sourceMapCache.applySourceMaps(coverage)),
JSON.stringify(coverage),
'utf-8'
)
}

NYC.prototype.istanbul = function () {
return this._istanbul || (this._istanbul = require('istanbul'))
}

NYC.prototype.report = function (cb, _collector, _reporter) {
cb = cb || function () {}

var collector = _collector || new this.istanbul.Collector()
var reporter = _reporter || new this.istanbul.Reporter()
var collector = _collector || new (this.istanbul()).Collector()
var reporter = _reporter || new (this.istanbul()).Reporter()

this._loadReports().forEach(function (report) {
collector.add(report)
Expand All @@ -212,24 +241,58 @@ NYC.prototype.report = function (cb, _collector, _reporter) {
reporter.write(collector, true, cb)
}

var loadedMaps = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concern here as for hashCache.


NYC.prototype._loadReports = function () {
var _this = this
var files = fs.readdirSync(this.tmpDirectory())

return _.map(files, function (f) {
var SourceMapCache = require('./lib/source-map-cache')
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you move the require statement down here because source-map-cache pulls in lodash? Because we could probably remove it there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

var sourceMapCache = new SourceMapCache()

var cacheDir = _this.cacheDirectory()

return files.map(function (f) {
var report
try {
return JSON.parse(fs.readFileSync(
report = JSON.parse(fs.readFileSync(
path.resolve(_this.tmpDirectory(), './', f),
'utf-8'
))
} catch (e) { // handle corrupt JSON output.
return {}
}

if (report) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition seems unnecessary. If the file no longer exists, or did not contain valid JSON, we'll already have returned. The other option is that it contains valid JSON but it's not an object. There's plenty other ways the report could be invalid for the code that follows.

Object.keys(report).forEach(function (relFile) {
var fileReport = report[relFile]
if (fileReport && fileReport.contentHash) {
var hash = fileReport.contentHash
if (!(loadedMaps[hash] || (loadedMaps[hash] === false))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the second === false comparison?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait that prevents multiple attempts to read the source map. Maybe rewrite to if (loadedMaps[hash] !== false && !loadedMaps[hash]) so that's a bit more obvious?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just !(hash in loadedMaps).

try {
var mapPath = path.join(cacheDir, hash + '.map')
loadedMaps[hash] = fs.readFileSync(mapPath, 'utf8')
} catch (e) {
loadedMaps[hash] = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add a comment "set to false to avoid repeatedly trying to load the map"

}
}
if (loadedMaps[hash]) {
sourceMapCache.addMap(relFile, loadedMaps[hash])
}
}
})
report = sourceMapCache.applySourceMaps(report)
}
return report
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just return the result of applySourceMaps(report)

})
}

NYC.prototype.tmpDirectory = function () {
return path.resolve(this.cwd, './', this.tempDirectory)
NYC.prototype.tmpDirectory = NYC.prototype.tempDirectory = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

tempDirectory() is now preferred, right? Worth deprecating tmpDirectory()? Regardless would be good to add a comment as to why there's two options. Digging through the commit history to find out later will be painful.

Copy link
Member Author

Choose a reason for hiding this comment

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

tmpDirectory is there for backwards compatibility. I don't think anybody is using that outside us, and it certainly is not documented. I think it's safe just to drop the other one.

return path.resolve(this.cwd, './', this._tempDirectory)
}

NYC.prototype.cacheDirectory = function () {
return path.resolve(this.cwd, './', this._cacheDirectory)
}

NYC.prototype.mungeArgs = function (yargv) {
Expand Down
4 changes: 4 additions & 0 deletions lib/source-map-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ SourceMapCache.prototype.add = function (filename, source) {
if (sourceMap) this.cache['./' + path.relative(this.cwd, filename)] = new SourceMapConsumer(sourceMap.sourcemap)
}

SourceMapCache.prototype.addMap = function (relFile, mapJson) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer need SourceMapCache#add() right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Except in your tests I think.

this.cache[relFile] = new SourceMapConsumer(JSON.parse(mapJson))
}

SourceMapCache.prototype.applySourceMaps = function (coverage) {
var _this = this
var mappedCoverage = _.cloneDeep(coverage)
Expand Down
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"scripts": {
"pretest": "standard",
"test": "npm run cover",
"clean": "rm -rf ./.nyc_output ./.self_coverage ./test/fixtures/.nyc_output ./test/build && rm -f *covered.js ./lib/*covered.js",
"clean": "rm -rf ./.nyc_output ./node_modules/.cache ./.self_coverage ./test/fixtures/.nyc_output ./test/fixtures/node_modules/.cache ./test/build && rm -f *covered.js ./lib/*covered.js",
"build": "node ./build-tests",
"instrument": "node ./build-self-coverage.js",
"run-tests": "tap --no-cov -b ./test/build/*.js ./test/src/source-map-cache.js",
Expand Down Expand Up @@ -64,11 +64,14 @@
"license": "ISC",
"dependencies": {
"append-transform": "^0.2.0",
"arrify": "^1.0.1",
"convert-source-map": "^1.1.2",
"ends-with": "^0.2.0",
"foreground-child": "^1.3.0",
"glob": "^5.0.14",
"istanbul": "^0.4.1",
"lodash": "^3.10.0",
"md5-hex": "^1.1.0",
"micromatch": "~2.1.6",
"mkdirp": "^0.5.0",
"resolve-from": "^2.0.0",
Expand Down
Loading