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 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 4 additions & 17 deletions index.js
View file
Open in desktop
Expand Up @@ -195,26 +195,13 @@ NYC.prototype.instrumentAllFiles = function (input, output, cb) {
try {
const stats = fs.lstatSync(input)
if (stats.isDirectory()) {
const globOptions = { dot: true, mark: true, ignore: ['**/.git', '**/.git/**/*'] }
const outputPaths = (output) ? glob.sync(`${path.resolve(input)}/**/*`, globOptions) : []

const partition = (universal, subsetFilter) => {
return universal.reduce(([a, aDash], member) => {
return subsetFilter(member) ? [[...a, member], aDash] : [a, [...aDash, member]]
}, [[], []])
}

const [dirs, files] = partition(outputPaths, filename => filename.endsWith('/'))

inputDir = input
this.walkAllFiles(input, visitor)

if (files.length || dirs.length) {
dirs.map(file => path.resolve(output, path.relative(input, file)))
.forEach(dir => mkdirp.sync(dir))

files.map(file => path.relative(input, file))
.forEach(file => { copySync(path.join(input, file), path.join(output, file), { overwrite: false }) })
if (output) {
const globOptions = { dot: true, ignore: ['**/.git', `**/${path.basename(output)}`] }
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is a problem for nyc instrument . output if ./lib/output/data.json exists (that output folder should be copied). Instead of ignoring **/${path.basename(output)} couldn't we just ignore output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only produces a list of top level files using glob.sync(`${path.resolve(input)}/*`, ...), so the inclusion of **/${path.basename(output)} won't exclude any further sub-dirs. The recursive copy of each top level sub-dir is then handled by fs-extra.copySync. I agree it looks a little silly and should really be just output, but I couldn't get it to work with glob.sync. I should point out that this also means we're only ignoring .git directories at the top level in this implementation.

Anyway I'll see if I can come up with a nicer solution here, in the meantime I'll drop in the change below and rebase

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 someone does nyc instrument . subdir/output? In that case wouldn't we need to ignore subdir, not output? I ask due to the use of path.basename.

glob.sync(`${path.resolve(input)}/*`, globOptions)
.forEach(src => copySync(src, path.join(output, path.relative(input, src)), { overwrite: false }))
}
} else {
visitor(input)
Expand Down
19 changes: 18 additions & 1 deletion test/nyc-integration.js
View file
Open in desktop
Expand Up @@ -693,6 +693,23 @@ describe('the nyc cli', function () {
})
})

it('can instrument the root directory', function (done) {
coreyfarrell marked this conversation as resolved.
Show resolved Hide resolved
const args = [bin, 'instrument', '.', './output']

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

proc.on('close', function (code) {
code.should.equal(0)
const files = fs.readdirSync(path.resolve(fixturesCLI, './output'))
files.should.include('args.js')
files.should.include('subdir')
done()
})
})

it('allows a sub-directory of files to be instrumented', function (done) {
const args = [bin, 'instrument', './subdir/input-dir', './output']

Expand All @@ -709,7 +726,7 @@ describe('the nyc cli', function () {
})
})

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

const proc = spawn(process.execPath, args, {
Expand Down