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

Jump to bottom

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 6 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion index.js
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@ const arrify = require('arrify')
const cachingTransform = require('caching-transform')
const util = require('util')
const findCacheDir = require('find-cache-dir')
const fs = require('fs')
const fs = require('fs-extra')
coreyfarrell marked this conversation as resolved.
Show resolved Hide resolved
const Hash = require('./lib/hash')
const libCoverage = require('istanbul-lib-coverage')
const libHook = require('istanbul-lib-hook')
Expand Down Expand Up @@ -200,6 +200,10 @@ NYC.prototype.instrumentAllFiles = function (input, output, cb) {
try {
const stats = fs.lstatSync(input)
if (stats.isDirectory()) {
if (output) {
fs.copySync(input, output)
Copy link
Member

Choose a reason for hiding this comment

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

With the current nyc if I just did the following:

mkdir -p tmp/bin
touch tmp/bin/nyc.js
chmod a+x tmp/bin/nyc.js
./bin/nyc.js instrument bin/nyc.js tmp

This resulted in tmp/bin/nyc.js containing the instrumented script with chmod a+x. In theory fs.copySync should copy the chmod flags from input to output. I'd like to see a test to verify this (probably have to skip that test for Windows).

Oddly enough I also tried:

./bin/nyc.js instrument bin tmp

This resulted in creation of tmp/nyc.js and tmp/wrap.js, though I expected it to create tmp/bin/*.js. Probably deal with this inconsistency in a follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

A potential issue with this approach:

nyc instrument --delete . self-coverage

This would result in copying the entire project into ./self-coverage, which would be correct. The issue is that this.walkAllFiles happens after these files are created. I think we need to stop using this.walkAllFiles, instead perform:

const files = this.exclude.globSync(input);
if (output) {
  fs.copySync(input, output);
}
files.forEach(relFile => {
  visitor(relFile);
})

This way testExclude.globSync is run before we create any files. I know this shouldn't matter as someone following the above example should add self-coverage to the exclude list, but I think it will turn into an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fs-extra.copySync currently prevents this from happening as it won't let you copy . into ./dest. It seems a more custom solution without fs-extra.copySync may be a better approach.

Copy link
Member

Choose a reason for hiding this comment

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

One other though, should the copy skip the .git/ folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely, I can't think of a use case where you'd need to copy the .git/ folder from . to ./dest while instrumenting files.

}

inputDir = input
this.walkAllFiles(input, visitor)
} else {
Expand Down
13 changes: 13 additions & 0 deletions lib/commands/instrument.js
View file
Open in desktop
@@ -1,6 +1,7 @@
const NYC = require('../../index.js')
const path = require('path')
const rimraf = require('rimraf')
const testExclude = require('test-exclude')

exports.command = 'instrument <input> [output]'

Expand Down Expand Up @@ -48,6 +49,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'
})
.option('es-modules', {
default: true,
type: 'boolean',
Expand Down Expand Up @@ -76,6 +87,8 @@ exports.handler = function (argv) {
cwd: argv.cwd,
compact: argv.compact,
preserveComments: argv.preserveComments,
include: argv.include,
exclude: argv.exclude,
esModules: argv.esModules,
exitOnError: argv.exitOnError
})
Expand Down
61 changes: 42 additions & 19 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -74,6 +74,7 @@
"find-cache-dir": "^2.0.0",
"find-up": "^3.0.0",
"foreground-child": "^1.5.6",
"fs-extra": "^7.0.1",
"istanbul-lib-coverage": "^2.0.4",
"istanbul-lib-hook": "^2.0.4",
"istanbul-lib-instrument": "^3.1.1",
Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/cli/subdir/input-dir/exclude-me/index.js
View file
Open in desktop
@@ -0,0 +1,2 @@
'use strict';
console.log('Hello, World!')