fix: Exclude negated not working with '--all' switch #977
Conversation
Sync repo
Bring fork up to date
test: stop using LAZY_LOAD_COUNT (istanbuljs#960)
Get latest
Get latest
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.
|
Looks like this is failing tests needing a new lock-file, are you fine with me updating this? |
Get Latest
# Conflicts: # index.js
AndrewFinlay
changed the title
AndrewFinlay
changed the title
There was a problem hiding this comment.
One minor nit/style thing about the added dependency, but otherwise this is great. Thanks @AndrewFinlay!
|
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. |
|
So I can't get this to fail the Strangely, I can get the Travis CI Anyway I figured I'd see if this rung any bells before I dive a little deeper into this. |
|
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 --forceThe 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). |
|
Can do, give me a minute. Done |
AndrewFinlay
force-pushed
the
exclude-negated-change
branch
from
68edea3
to
0202ca7
Compare
|
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 |
|
@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
changed the title
AndrewFinlay
force-pushed
the
exclude-negated-change
branch
from
0202ca7
to
d5d9b64
Compare
|
So it looks like this might fix #833 |
| }) | ||
| }) | ||
|
|
||
| it('should include \'node_modules\' using exclude patterns', function (done) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool, I'll put something together after lunch
coreyfarrell
changed the title
This reverts commit 91de23c.
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.