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

fix: Exclude negated not working with '--all' switch #977

Merged
merged 13 commits into from
Feb 22, 2019

Conversation

AndrewFinlay
Copy link
Contributor

While I've been tinkering with the instrument command I noticed that the exclude negated functionality provided by the test-exclude package wasn't working for 'nyc --all' and in the future for 'nyc instrument'.

This PR includes a set of tests and a fix for that behaviour.

Note that it does promote an existing sub-dependency, 'array-uniq' to package.json, and at a higher major version than before. So if this is a problem let me know and we'll try and find another way to do it.

Also I've added a couple of files and folders to the cli test fixtures, and added an exclude negation to .gitignore to allow testing exclude negation on a file in node_modules.

AndrewFinlay and others added 8 commits October 20, 2018 08:17
test: stop using LAZY_LOAD_COUNT (istanbuljs#960)
Test that we can include excluded files using negated excludes
Test that we can include files under 'node_modules' using negated excludes

All tests currently fail
Previously the call to glob.sync was knocking out all files in the exclude patterns,
this would also knock out any files that were intended to be restored by the exclude negated patterns.
This would prevent node_modules exclude negated files from being covered when run with --all.

Notes:

I've promoted 'array-uniq' to a top level dependency, I figured I'd point out that it uses es6 under the hood as I don't see if used often in this project
I'm putting this one down to being a 43C day and the air conditioning being broken.
@AndrewFinlay
Copy link
Contributor Author

Looks like this is failing tests needing a new lock-file, are you fine with me updating this?

@coveralls
Copy link

coveralls commented Jan 25, 2019

Coverage Status

Coverage increased (+0.02%) to 96.419% when pulling d5d9b64 on AndrewFinlay:exclude-negated-change into 12b8986 on istanbuljs:master.

@AndrewFinlay AndrewFinlay changed the title Exclude negated change Fix: Exclude negated not working with '--all' switch Feb 19, 2019
@AndrewFinlay AndrewFinlay changed the title Fix: Exclude negated not working with '--all' switch fix: Exclude negated not working with '--all' switch Feb 19, 2019
Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

One minor nit/style thing about the added dependency, but otherwise this is great. Thanks @AndrewFinlay!

index.js Show resolved Hide resolved
@AndrewFinlay
Copy link
Contributor Author

Wow, that's got to be the worst case of 'it works on my machine' I've seen in a while. I'll do some digging to see what's going on.

@AndrewFinlay
Copy link
Contributor Author

AndrewFinlay commented Feb 21, 2019

So I can't get this to fail the should include 'node_modules' using exclude patterns test on my local machine. Running with node 8.15.0, npm 6.4.1 on osx, and no modifications to the source that aren't part of the PR.

Strangely, I can get the Travis CI should include 'node_modules' using exclude patterns test to pass if a console.log is left in the middle of the walkAllFiles function, see commit 82ed06e. Now I'm not suggesting this as a solution, but it's an interesting result that's got me wondering if there's some timing issues in here.

Anyway I figured I'd see if this rung any bells before I dive a little deeper into this.

@coreyfarrell
Copy link
Member

I just pushed eac3110 to my own fork of nyc and it looks like it's going to pass (just waiting on a couple OSX builds).

https://travis-ci.org/coreyfarrell/nyc/builds/496549022

Do you mind if I do a force-push to your nyc branch? What I'd like to from your branch is:

git reset eac3110 --hard
git commit --amend --no-edit
git push --force

The result will be to delete commits that came after eac3110 and replace eac3110 with another commit that has the same exact content (just force generating a new SHA).

@AndrewFinlay
Copy link
Contributor Author

AndrewFinlay commented Feb 21, 2019

Can do, give me a minute. Done

@JaKXz
Copy link
Member

JaKXz commented Feb 21, 2019

If my suggestion is causing this much trouble I don't think it's worth it for now... It was just a minor nit anyway

@AndrewFinlay
Copy link
Contributor Author

@JaKXz To my eye the code change really seemed to be a minor refactor, aside from the return statement. At the moment I can't explain the discrepancy, and changing the code back makes me feel I'm leaving a landmine behind.

So my plan is to do a little more digging, I'll keep you up to informed as to where the PR is at.

@AndrewFinlay AndrewFinlay changed the title fix: Exclude negated not working with '--all' switch wip: fix: Exclude negated not working with '--all' switch Feb 21, 2019
@AndrewFinlay
Copy link
Contributor Author

So it looks like this might fix #833

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

This looks great, I think it'll resolve a bunch of issues related to getting coverage of node_modules.

})
})

it('should include \'node_modules\' using exclude patterns', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets add another test (in a follow-up) to do nearly the same thing as this, just without the --all argument and with a test script that actually has require('./include-exclude/node_modules/cover-me.js');? This will prove that --exclude '!**/node_modules/**' works again. I've already done this basic test at the command-line and it did what I expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll put something together after lunch

@coreyfarrell coreyfarrell changed the title wip: fix: Exclude negated not working with '--all' switch fix: Exclude negated not working with '--all' switch Feb 22, 2019
@coreyfarrell coreyfarrell merged commit 91de23c into istanbuljs:master Feb 22, 2019
coreyfarrell added a commit that referenced this pull request Feb 22, 2019
This was referenced Apr 3, 2019
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