feat: add include and exclude options to instrument command
#1007
Conversation
This allows you to better select which files are instrumented when running the instrument command. It's a pretty minimal implementation to make it as easy as possible to integrate with other proposed changes. Test support is included with this change.
4 similar comments
This comment has been minimized.
This comment has been minimized.
# Conflicts: # lib/commands/instrument.js
|
Hold off a minute, I may have noticed something. |
index.js
Outdated
| if (!_this.exclude.shouldInstrument(filename)) { | ||
| return | ||
| } | ||
|
|
||
| var code = fs.readFileSync(inFile, 'utf-8') | ||
|
|
||
| for (ext in _this.transforms) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/excluderules. - 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.
There was a problem hiding this comment.
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.
# Conflicts: # lib/commands/instrument.js
AndrewFinlay
changed the title
include and exclude options to instrument commandinclude and exclude options to instrument command
# Conflicts: # index.js # lib/commands/instrument.js # test/nyc-integration.js
…nstrumented versions where needed So I've implemented this using `fs-extra`, not sure if that's the module you wish to go with here. This is now relying on `testExclude.globSync` to get the correct files for instrumentation, so no more `shouldInstrument` There's quite a few test changes in here, as `fs-extra.copySync` won't let you copy from `./` to `./outputMkdirHere`, but they're probably for the best anyway. This also means that any attempt to store the instrumentation result in a subdir of the input dir will fail.
index.js
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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 tmpThis 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 tmpThis 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.
There was a problem hiding this comment.
A potential issue with this approach:
nyc instrument --delete . self-coverageThis 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
One other though, should the copy skip the .git/ folder?
There was a problem hiding this comment.
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.
…o `output` dir It ignores files in the `.git` folder and copies empty directories Unfortunately I had to keep `fs-extra`, although this should be easy to drop when the project moves to node 8+ when `fs.copyFileSync` is made available. We need to use `fs.chmodSync` to set the mode as any `write` mode flags get filtered when trying to set them through `fs.writeFileSync`. What's missing? * No check to see where the working directory is relative to the project root directory * Tests confirming includes/excludes can be set in a config file or stanza and are observed by the instrument command * Tests confirming file permissions match between files in the input and output directories * Tests confirming we can't instrument outside of the project root directory
…tside of cwd Tests include * using a `.nycrc` file to specify excludes * checking that file mode is preserved after instrumentation, * copying all uninstrumented files across to the output directory * Ensure that we can't instrument in place, overwriting the existing source * Ensure that we can't instrument source from outside the project root directory
A file `node_modules` file involved in the tests was being git ignored
|
@AndrewFinlay can we get these changes rebased and merged? :) cc @coreyfarrell @bcoe |
# Conflicts: # index.js
In the last attempt at this I was jumping through hoops trying to maintain empty folders and structure etc. Turns out none of that is necessary. So this change runs the instrumentation process over the included files, then copies across anything remaining excluding freshly created output directory.
AndrewFinlay
changed the title
include and exclude options to instrument commandinclude and exclude options to instrument command
And a test to prove it!
index.js
Outdated
| 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)}`] } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
@coreyfarrell I'm going to defer my review to @bcoe, I'm not quite familiar enough atm and don't have the cycles this week |
…ude` Previously this was adding `nyc instrument` full copy of `src` to `dst` as well as `include`/`exclude` The full copy work was proving troublesome so it's been moved to it's own branch/PR This commit removes the full copy, removes now unused dependencies, and updates tests accordingly
|
This PR has been split up so that it only adds support for |
|
@AndrewFinlay Thanks for working with me to figure out scope of this change and how to deal with it. |
This allows you to better select which files are instrumented when running the instrument command. It's a pretty minimal implementation to make it as easy as possible to integrate with other proposed changes. Test support is included with this change.
Fixes #596, fixes #890