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

Use istanbul-lib-hook to wrap require and support vm hooks #308

Merged
merged 3 commits into from
Jul 14, 2016
Merged
Show file tree
Hide file tree
Changes from 2 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
23 changes: 21 additions & 2 deletions bin/nyc.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ var yargs = require('yargs/yargs')(process.argv.slice(2))
describe: 'directory to output coverage reports in',
default: 'coverage'
})
.option('temp-directory', {
describe: 'directory from which coverage JSON files are read',
default: './.nyc_output'
})
.example('$0 report --reporter=lcov', 'output an HTML lcov report to ./coverage')
})
.command('check-coverage', 'check whether coverage is within thresholds provided', function (yargs) {
Expand Down Expand Up @@ -127,6 +131,16 @@ var yargs = require('yargs/yargs')(process.argv.slice(2))
type: 'boolean',
description: 'should nyc handle instrumentation?'
})
.option('hook-run-in-context', {
default: false,
type: 'boolean',
description: 'should nyc wrap vm.runInThisContext?'
})
.option('hook-create-script', {
default: false,
Copy link
Member

Choose a reason for hiding this comment

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

we've tried to be fairly batteries included so far, for instance we already instrument subprocesses right out of the gate. I might be tempted to default this to true. I'm happy with the argument name 👍

type: 'boolean',
description: 'should nyc wrap vm.createScript?'
})
.help('h')
.alias('h', 'help')
.version()
Expand Down Expand Up @@ -164,7 +178,9 @@ if (argv._[0] === 'report') {
include: argv.include,
exclude: argv.exclude,
sourceMap: !!argv.sourceMap,
instrumenter: argv.instrumenter
instrumenter: argv.instrumenter,
hookRunInContext: argv.hookRunInContext,
hookCreateScript: argv.hookCreateScript
}))
nyc.reset()

Expand All @@ -175,6 +191,8 @@ if (argv._[0] === 'report') {
NYC_CACHE: argv.cache ? 'enable' : 'disable',
NYC_SOURCE_MAP: argv.sourceMap ? 'enable' : 'disable',
NYC_INSTRUMENTER: argv.instrumenter,
NYC_HOOK_RUN_IN_CONTEXT: argv.hookRunInContext,
NYC_HOOK_CREATE_SCRIPT: argv.hookCreateScript,
BABEL_DISABLE_CACHE: 1
}
if (argv.require.length) {
Expand Down Expand Up @@ -218,7 +236,8 @@ function report (argv) {

;(new NYC({
reporter: argv.reporter,
reportDir: argv.reportDir
reportDir: argv.reportDir,
tempDirectory: argv.tempDirectory
})).report()
}

Expand Down
4 changes: 3 additions & 1 deletion bin/wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ try {
include: process.env.NYC_INCLUDE ? process.env.NYC_INCLUDE.split(',') : [],
enableCache: process.env.NYC_CACHE === 'enable',
sourceMap: process.env.NYC_SOURCE_MAP === 'enable',
instrumenter: process.env.NYC_INSTRUMENTER
instrumenter: process.env.NYC_INSTRUMENTER,
hookRunInContext: process.env.NYC_HOOK_RUN_IN_CONTEXT === 'true',
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, I might stick with setting the environment variable to enable; I think this approach originated from not wanting to get confused by variable type (!!'false' === true).

hookCreateScript: process.env.NYC_HOOK_CREATE_SCRIPT === 'true'
})).wrap()

sw.runMain()
7 changes: 3 additions & 4 deletions build-self-coverage.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var istanbul = require('istanbul')
var istanbul = require('istanbul-lib-instrument')
Copy link
Member

Choose a reason for hiding this comment

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

I really like this fix. it felt weird leaving in the Istanbul dependency.

var fs = require('fs')
var path = require('path')

Expand All @@ -8,10 +8,9 @@ var path = require('path')
var indexPath = path.join(__dirname, name)
var source = fs.readFileSync(indexPath, 'utf8')

var instrumentor = new istanbul.Instrumenter({
var instrumentor = istanbul.createInstrumenter({
coverageVariable: '___NYC_SELF_COVERAGE___',
esModules: true,
noAutoWrap: true
esModules: true
})

var instrumentedSource = instrumentor.instrumentSync(source, indexPath)
Expand Down
23 changes: 20 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
var fs = require('fs')
var glob = require('glob')
var libCoverage = require('istanbul-lib-coverage')
var libHook = require('istanbul-lib-hook')
var libReport = require('istanbul-lib-report')
var libSourceMaps = require('istanbul-lib-source-maps')
var reports = require('istanbul-reports')
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')
Expand Down Expand Up @@ -67,6 +67,9 @@ function NYC (opts) {

this.sourceMapCache = libSourceMaps.createSourceMapStore()

this.hookRunInContext = config.hookRunInContext
this.hookCreateScript = config.hookCreateScript

this.hashCache = {}
this.loadedMaps = null
this.fakeRequire = null
Expand Down Expand Up @@ -289,13 +292,26 @@ NYC.prototype._handleJs = function (code, filename) {
return this._maybeInstrumentSource(code, filename, relFile) || code
}

NYC.prototype._wrapRequire = function () {
NYC.prototype._addHook = function (type) {
var handleJs = this._handleJs.bind(this)
var dummyMatcher = function () { return true } // we do all processing in transformer
libHook['hook' + type](dummyMatcher, handleJs, { extensions: this.extensions })
}

NYC.prototype._wrapRequire = function () {
this.extensions.forEach(function (ext) {
require.extensions[ext] = js
appendTransform(handleJs, ext)
})
this._addHook('Require')
}

NYC.prototype._addOtherHooks = function () {
if (this.hookRunInContext) {
this._addHook('RunInThisContext')
}
if (this.hookCreateScript) {
this._addHook('CreateScript')
}
}

NYC.prototype.cleanup = function () {
Expand Down Expand Up @@ -329,6 +345,7 @@ NYC.prototype._wrapExit = function () {

NYC.prototype.wrap = function (bin) {
this._wrapRequire()
this._addOtherHooks()
this._wrapExit()
this._loadAdditionalModules()
return this
Expand Down
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"build": "node ./build-tests",
"instrument": "node ./build-self-coverage.js",
"run-tests": "tap -t120 --no-cov -b ./test/build/*.js ./test/src/nyc-bin.js",
"report": "istanbul report --include=./.self_coverage/*.json lcov text",
"report": "node ./bin/nyc --temp-directory ./.self_coverage/ -r text -r lcov report",
"cover": "npm run clean && npm run build && npm run instrument && npm run run-tests && npm run report",
"dev": "npm run clean && npm run build && npm run run-tests",
"version": "standard-version"
Expand Down Expand Up @@ -72,7 +72,6 @@
"author": "Ben Coe <[email protected]>",
"license": "ISC",
"dependencies": {
"append-transform": "^0.4.0",
"arrify": "^1.0.1",
"caching-transform": "^1.0.0",
"convert-source-map": "^1.1.2",
Expand All @@ -82,6 +81,7 @@
"foreground-child": "^1.5.3",
"glob": "^7.0.3",
"istanbul-lib-coverage": "^1.0.0-alpha.4",
"istanbul-lib-hook": "^1.0.0-alpha.4",
"istanbul-lib-instrument": "^1.1.0-alpha.1",
"istanbul-lib-report": "^1.0.0-alpha.3",
"istanbul-lib-source-maps": "^1.0.0-alpha.10",
Expand All @@ -105,9 +105,9 @@
"exists-sync": "0.0.3",
"forking-tap": "^0.1.1",
"is-windows": "^0.2.0",
"istanbul": "^0.4.4",
"lodash": "^4.12.0",
"newline-regex": "^0.2.1",
"requirejs": "^2.2.0",
"sanitize-filename": "^1.5.3",
"sinon": "^1.15.3",
"source-map-support": "^0.4.1",
Expand All @@ -123,7 +123,6 @@
"url": "[email protected]:bcoe/nyc.git"
},
"bundledDependencies": [
"append-transform",
"arrify",
"caching-transform",
"convert-source-map",
Expand All @@ -134,6 +133,7 @@
"glob",
"istanbul-lib-coverage",
"istanbul-lib-instrument",
"istanbul-lib-hook",
"istanbul-lib-report",
"istanbul-lib-source-maps",
"istanbul-reports",
Expand Down
16 changes: 16 additions & 0 deletions test/fixtures/hooks/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// RequireJS uses `vm.runInThisContext`
// make sure we add hooks for it as well

var rjs = require('requirejs'),
assert = require('assert');

rjs.config({
baseUrl : __dirname,
nodeRequire : require
});

rjs(['./lib/lorem'], function(lorem){
var result = lorem(1, 2, 3);
assert.equal(9, result);
});

11 changes: 11 additions & 0 deletions test/fixtures/hooks/lib/ipsum.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
define(function(){

function sum(a, b) {
return a + b;
}

return {
sum : sum
};

});
7 changes: 7 additions & 0 deletions test/fixtures/hooks/lib/lorem.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
define(['./ipsum'], function (ipsum) {

return function exec(a, b, c){
return ipsum.sum(a, b) * c;
};

});
10 changes: 10 additions & 0 deletions test/fixtures/hooks/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "hooktest",
"version": "1.1.1",
"description": "AMD/ requirejs test project",
"main": "index.js",
"dependencies": {
"requirejs": "^2.2.0"
},
"private": true
}
22 changes: 22 additions & 0 deletions test/src/nyc-bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var fs = require('fs')
var spawn = require('child_process').spawn
var isWindows = require('is-windows')()
var fixturesCLI = path.resolve(__dirname, '../fixtures/cli')
var fixturesHooks = path.resolve(__dirname, '../fixtures/hooks')
var fakebin = path.resolve(fixturesCLI, 'fakebin')
var bin = path.resolve(__dirname, '../../bin/nyc')
var rimraf = require('rimraf')
Expand Down Expand Up @@ -388,4 +389,25 @@ describe('the nyc cli', function () {
})
})
})

describe('hooks', function () {
it('provides coverage for requireJS and AMD modules', function (done) {
var args = [bin, '--hook-run-in-context', process.execPath, './index.js']
Copy link
Member

Choose a reason for hiding this comment

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

lovely! I'm definitely finding myself writing more and more tests that directly exercise the bin which I don't think is a bad thing -- for one catches regressions I've introduced in the actual command line parsing.


var proc = spawn(process.execPath, args, {
cwd: fixturesHooks,
env: process.env
})
var stdout = ''
proc.stdout.on('data', function (chunk) {
stdout += chunk
})
proc.on('close', function (code) {
code.should.equal(0)
stdout.should.match(/ipsum\.js/)
stdout.should.match(/lorem\.js/)
done()
})
})
})
})