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

Don't instrument files outside of the current working directory #160

Merged
merged 5 commits into from
Feb 4, 2016

Conversation

novemberborn
Copy link
Contributor

Files outside of the current working directory (meaning higher up in the file hierarchy) should not be instrumented.

Fixes #157 where nyc is run from inside a node_modules directory.

Fixes #154 where 'npm link' is used to fulfil dependencies.

This removes matching the include/exclude patterns to the absolute filename, the patterns are now applied only to the relative portion within the current working directory.

addAllFiles() has been changed to explicitly add files in the current working directory only.

Some other tests related to include/exclude behavior have been cleaned up.


Note that I'm not entirely certain filename in shouldInstrumentFile() is always absolute. Review on that front would be especially appreciated.

This changes the include/exclude matching behavior, but restricting it to the cwd makes more sense to me.

The test is superfluous as the presence of node_modules in the filename causes
it to be excluded. There are also repeat assertions for the same values.
Clarify what's being tested. Remove misleading comment. Explicitly test for
exclude overriding include.
This was referenced Feb 1, 2016
@bcoe
Copy link
Member

bcoe commented Feb 2, 2016

@novemberborn great work; I'll do a thorough test on a few platforms/projects and we can release this to a next tag ASAP.

@novemberborn
Copy link
Contributor Author

I'll do a thorough test on a few platforms/projects and we can release this to a next tag ASAP.

Great!

@bcoe
Copy link
Member

bcoe commented Feb 3, 2016

@novemberborn starting doing a little bit of testing, this patch doesn't seem to be working for a linked module:

screen shot 2016-02-02 at 10 58 56 pm

One other thing we might want to be mindful of is this logic:

relFile = relFile.replace(/^\.\//, '') // remove leading './'.

We should probably be using path.sep for the benefit of Windows.

@bcoe
Copy link
Member

bcoe commented Feb 3, 2016

@novemberborn the good news, is I saw that this patch solves the issues @micnic was having \o/

@jamestalmage
Copy link
Member

We should probably be using path.sep

Or just account for either slash in the regexp

str.replace(/\.[\\\/]/, '')

Using path.sep in a regexp is painfull

@sindresorhus
Copy link
Member

Or just account for either slash in the regexp

👍

Files outside of the current working directory (meaning higher up in the file
hierarchy) should not be instrumented.

Fixes istanbuljs#157 where nyc is run from inside a node_modules directory.

Fixes istanbuljs#154 where 'npm link' is used to fulfill dependencies.

This removes matching the include/exclude patterns to the absolute filename, the
patterns are now applied only to the relative portion within the current working
directory.

addAllFiles() has been changed to explicitly add files in the current working
directory only.
There is no specific test for this, I've simply repeated the assertions that
started with './'.
@novemberborn
Copy link
Contributor Author

@bcoe

starting doing a little bit of testing, this patch doesn't seem to be working for a linked module

Ha, this is a nice one! I reproduced this by cloning https://github.com/bcoe/yargs/tree/split-out-parser and https://github.com/yargs/yargs-parser/tree/require.main.filename into sibling directories, then npm linking yargs-parser into yargs.

Turns out when you have paths like these:

/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T/tmp.NOCrQG1U/yargs
/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T/tmp.NOCrQG1U/yargs-parser

'yars-parser'.indexOf('yars') does return 0, so yargs-parser was included. I've (force-)pushed a fix which checks if path.relative(this.cwd, filename) returns a string that starts with ... I was being lazy with indexOf.

We should probably be using path.sep for the benefit of Windows.

Pushed a commit which fixes that, using @jamestalmage's method.

@bcoe
Copy link
Member

bcoe commented Feb 3, 2016

lol @novemberborn that's great, as usual I manage to be a nightmare user of our own library and find a crazy edge-case:

golden

bcoe added a commit that referenced this pull request Feb 4, 2016
Don't instrument files outside of the current working directory
@bcoe bcoe merged commit 0535137 into istanbuljs:master Feb 4, 2016
@novemberborn novemberborn deleted the improve-excludes branch February 4, 2016 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants