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

feat: add include and exclude options to instrument command #1007

Merged
merged 17 commits into from
Apr 4, 2019
Merged

feat: add include and exclude options to instrument command #1007

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d576e44
Added `include` and `exclude` options for instrument command
Mar 1, 2019
39db2cc
Merge branch 'master' into instrument-include-exclude-option
Mar 7, 2019
880ccde
Merge branch 'master' into instrument-include-exclude-option
Mar 11, 2019
5f76838
Merge branch 'master' into instrument-include-exclude-option
Mar 20, 2019
5174d12
`nyc instrument` copies all files to an output directory, then adds i…
Mar 20, 2019
bd25a38
Use `var cov_` rather than `_cov` in tests
Mar 20, 2019
02bc210
nyc instrument now copies all files, with permissions, from `input` t…
Mar 22, 2019
b2eb670
Update tests, prevent in place instrumentation and instrumentation ou…
Mar 25, 2019
995aa1a
Include missing test files
Mar 25, 2019
5a28941
Ignore file permissions test if running on Windows
Mar 25, 2019
d22c585
Use process.platform instead of os.platform
Mar 25, 2019
fd69f9a
Merge branch 'master' into instrument-include-exclude-option
Mar 28, 2019
1fbc204
Clean up copy remaining files for nyc instrument command
Apr 2, 2019
e1a1b94
Make sure we can instrument the root directory
Apr 2, 2019
17e4736
Merge in master, update test title
Apr 2, 2019
6852bbf
Merge branch 'master' into instrument-include-exclude-option
Apr 3, 2019
8903365
Split PR such that this only handles `nyc instrument --include --excl…
Apr 4, 2019
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
5 changes: 5 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ NYC.prototype.instrumentAllFiles = function (input, output, cb) {
var ext
var transform
var inFile = path.resolve(inputDir, filename)

if (!_this.exclude.shouldInstrument(filename)) {
return
}

var code = fs.readFileSync(inFile, 'utf-8')

for (ext in _this.transforms) {
Copy link
Member

Choose a reason for hiding this comment

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

This results in no output for files that are not instrumented. But if I have:

src/index.js
src/no-coverage.js

With src/no-coverage.js excluded, I think I still need both dest/index.js and dest/no-coverage.js to be created, just dest/no-coverage.js should be unmodified. Same if I run nyc instrument src/no-coverage.js - that should print the original source.

So I'm thinking:

if (_this.exclude.shouldInstrument) {
  for (ext in ...) { ... }
}

I suspect this will change some of the tests, you will have to look for cov_ or not in the output files for example instead of checking if the file exists/doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, you're thinking your point is that dest/ should have a fully functional codebase, just some files will be instrumented and some won't. I agree, it would be a bit shocking to be missing files, it makes it impossible to import the library.

I believe we check for the cov__ global in a few other tests, I'd look for an existing test using this approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, though this idea might require bypassing walkAllFiles entirely, just using:

glob.sync('**/*', { cwd: input, nodir: true }).forEach(visitor)

Specifically walkAllFiles uses the extension and exclude filters which we don't really want if we're attempting to copy everything from src to dest. This could be problematic though when using npx nyc instrument . dest as that would include node_modules. Maybe nyc instrument needs a --full-copy option? If enabled we directly use glob.sync to capture all files, if not then we use testExclude.globSync. I'm leaning towards leaving that option disabled by default.

Possibly future enhancement: when nyc instrument copies files which have chmod +x enabled should we do the same for the copied file?

Copy link
Member

Choose a reason for hiding this comment

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

Let's create an issue for keeping the perms the same; I think it would potentially even be worth splitting this into its own library -- it sounds like we want something like cpr that:

  • allows us to apply our own include/exclude rules.
  • maintains file permissions.

Definitely beyond the scope of this pull however; although I think maintaining the existing behavior of copying over the whole directory is smart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a new commit that addresses some of these concerns, namely copying all existing files in input to output before instrumentation and replacing output files in place. To do this I've brought in fs-extra as it seems like a popular and maintained module, but I'm not particularly attached to it so if it needs to change let me know of your preferred replacement.

Expand Down
13 changes: 13 additions & 0 deletions lib/commands/instrument.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const testExclude = require('test-exclude')
const NYC = require('../../index.js')

exports.command = 'instrument <input> [output]'
Expand Down Expand Up @@ -46,6 +47,16 @@ exports.builder = function (yargs) {
type: 'boolean',
description: 'should nyc exit when an instrumentation failure occurs?'
})
.option('include', {
alias: 'n',
default: [],
describe: 'a list of specific files and directories that should be instrumented, glob patterns are supported'
})
.option('exclude', {
alias: 'x',
default: testExclude.defaultExclude,
coreyfarrell marked this conversation as resolved.
Show resolved Hide resolved
describe: 'a list of specific files and directories that should not be instrumented, glob patterns are supported'
})
.example('$0 instrument ./lib ./output', 'instrument all .js files in ./lib with coverage and output in ./output')
}

Expand All @@ -63,6 +74,8 @@ exports.handler = function (argv) {
require: argv.require,
compact: argv.compact,
preserveComments: argv.preserveComments,
include: argv.include,
exclude: argv.exclude,
exitOnError: argv.exitOnError
})

Expand Down
87 changes: 83 additions & 4 deletions test/nyc-integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* global describe, it, beforeEach */
/* global describe, it, beforeEach, afterEach */

const _ = require('lodash')
const path = require('path')
Expand Down Expand Up @@ -548,6 +548,10 @@ describe('the nyc cli', function () {
})

describe('output folder specified', function () {
afterEach(function () {
rimraf.sync(path.resolve(fixturesCLI, 'output'))
})

it('allows a single file to be instrumented', function (done) {
var args = [bin, 'instrument', './half-covered.js', './output']

Expand All @@ -561,7 +565,6 @@ describe('the nyc cli', function () {
var files = fs.readdirSync(path.resolve(fixturesCLI, './output'))
files.length.should.equal(1)
files.should.include('half-covered.js')
rimraf.sync(path.resolve(fixturesCLI, 'output'))
done()
})
})
Expand All @@ -579,7 +582,6 @@ describe('the nyc cli', function () {
var files = fs.readdirSync(path.resolve(fixturesCLI, './output'))
files.should.include('env.js')
files.should.include('es6.js')
rimraf.sync(path.resolve(fixturesCLI, 'output'))
done()
})
})
Expand All @@ -596,7 +598,84 @@ describe('the nyc cli', function () {
code.should.equal(0)
var files = fs.readdirSync(path.resolve(fixturesCLI, './output'))
files.should.include('index.js')
rimraf.sync(path.resolve(fixturesCLI, 'output'))
done()
})
})

it('allows a subdirectory to be excluded', function (done) {
const args = [bin, 'instrument', '--exclude', '**/subdir/**', './', './output']

const proc = spawn(process.execPath, args, {
cwd: fixturesCLI,
env: env
})

proc.on('close', function (code) {
code.should.equal(0)
const subdirExists = fs.existsSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
subdirExists.should.equal(false)
const files = fs.readdirSync(path.resolve(fixturesCLI, './output'))
files.length.should.not.equal(0)
files.should.not.include('subdir')
done()
})
})

it('allows a file to be excluded', function (done) {
const args = [bin, 'instrument', '--exclude', 'subdir/input-dir/bad.js', './', './output']

const proc = spawn(process.execPath, args, {
cwd: fixturesCLI,
env: env
})

proc.on('close', function (code) {
code.should.equal(0)
const subdirExists = fs.existsSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
subdirExists.should.equal(true)
const files = fs.readdirSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
files.should.include('index.js')
files.should.not.include('bad.js')
done()
})
})

it('allows specifying a single sub-directory to be included', function (done) {
const args = [bin, 'instrument', '--include', '**/subdir/input-dir/bad.js', './', './output']

const proc = spawn(process.execPath, args, {
cwd: fixturesCLI,
env: env
})

proc.on('close', function (code) {
code.should.equal(0)
let files = fs.readdirSync(path.resolve(fixturesCLI, './output'))
files.length.should.equal(1)
const subdirExists = fs.existsSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
subdirExists.should.equal(true)
files = fs.readdirSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
files.should.include('bad.js')
files.should.not.include('index.js')
done()
})
})

it('allows a file to be excluded from an included directory', function (done) {
const args = [bin, 'instrument', '--exclude', '**/bad.js', '--include', '**/subdir/input-dir/**', './', './output']

const proc = spawn(process.execPath, args, {
cwd: fixturesCLI,
env: env
})

proc.on('close', function (code) {
code.should.equal(0)
const subdirExists = fs.existsSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
subdirExists.should.equal(true)
const files = fs.readdirSync(path.resolve(fixturesCLI, './output/subdir/input-dir'))
files.should.include('index.js')
files.should.not.include('bad.js')
done()
})
})
Expand Down