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

we should still run transpilers on source files included via --all #143

Merged
merged 2 commits into from
Jan 21, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
7 changes: 5 additions & 2 deletions bin/nyc.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,14 @@ if (argv._[0] === 'report') {
checkCoverage(argv)
} else if (argv._.length) {
// wrap subprocesses and execute argv[1]
var nyc = (new NYC())
if (!Array.isArray(argv.require)) argv.require = [argv.require]

var nyc = (new NYC({
require: argv.require
}))
nyc.reset()

if (argv.all) nyc.addAllFiles()
if (!Array.isArray(argv.require)) argv.require = [argv.require]

var env = {
NYC_CWD: process.cwd(),
Expand Down
19 changes: 16 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ var fs = require('fs')
var glob = require('glob')
var micromatch = require('micromatch')
var mkdirp = require('mkdirp')
var Module = require('module')
var appendTransform = require('append-transform')
var cachingTransform = require('caching-transform')
var path = require('path')
var rimraf = require('rimraf')
var onExit = require('signal-exit')
var stripBom = require('strip-bom')
var resolveFrom = require('resolve-from')
var arrify = require('arrify')
var SourceMapCache = require('./lib/source-map-cache')
Expand Down Expand Up @@ -158,15 +158,26 @@ NYC.prototype._prepGlobPatterns = function (patterns) {

NYC.prototype.addFile = function (filename) {
var relFile = path.relative(this.cwd, filename)
var source = stripBom(fs.readFileSync(filename, 'utf8'))
var source = this._readTranspiledSource(path.resolve(this.cwd, filename))
Copy link
Member

Choose a reason for hiding this comment

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

My only thought would be to do this conditionally if we don't already have coverage data on that particular file. This feels like a performance hit.

var instrumentedSource = this._maybeInstrumentSource(source, 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.

What if we've already wrapped require? Then the result of _readTranspiledSource will already be instrumented. Is addFile only called for reports?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jamestalmage reading through the codebase I think that addAllFiles is the only method that calls addFile -- given this, I'm pretty sure we don't need to worry about a performance hit, and this shouldn't cause trouble for reports.

Mind running this branch against one of your larger codebases and making sure there's no performance hit?

Copy link
Member

Choose a reason for hiding this comment

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

Better yet, use the profiling docs:

screenshot 2016-01-18 19 44 34

Fears alleviated! 🚢


return {
instrument: !!instrumentedSource,
relFile: relFile,
content: instrumentedSource || source
}
}

NYC.prototype._readTranspiledSource = function (path) {
var source = null
Module._extensions['.js']({
_compile: function (content, filename) {
source = content
}
}, path)
return source
}

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

Expand All @@ -177,8 +188,10 @@ NYC.prototype.shouldInstrumentFile = function (filename, relFile) {
NYC.prototype.addAllFiles = function () {
var _this = this

this._loadAdditionalModules()

glob.sync('**/*.js', {nodir: true, ignore: this.exclude}).forEach(function (filename) {
var obj = _this.addFile(filename, true)
var obj = _this.addFile(filename)
if (obj.instrument) {
module._compile(
_this.instrumenter().getPreamble(obj.content, obj.relFile),
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/transpile-hook.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
var fs = require('fs')

require.extensions['.js'] = function (module, filename) {
var content = fs.readFileSync(filename, 'utf8');
module._compile(content.replace('--> pork chop sandwiches <--', ''), filename);
}
26 changes: 26 additions & 0 deletions test/src/nyc-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,32 @@ describe('nyc', function () {

return done()
})

it('transpiles files added via addAllFiles', function (done) {
fs.writeFileSync(
'./test/fixtures/needs-transpile.js',
'--> pork chop sandwiches <--\nvar a = 99',
'utf-8'
)

var nyc = (new NYC({
require: './test/fixtures/transpile-hook'
}))

nyc.reset()
nyc.addAllFiles()

var reports = _.filter(nyc._loadReports(), function (report) {
return ap(report)['./test/fixtures/needs-transpile.js']
})
var report = reports[0]['./test/fixtures/needs-transpile.js']

reports.length.should.equal(1)
report.s['1'].should.equal(0)

fs.unlinkSync('./test/fixtures/needs-transpile.js')
return done()
})
})

describe('cache', function () {
Expand Down