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

added --all flag for indicating that all files should have coverage applied #44

Merged
merged 6 commits into from
Sep 9, 2015

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Sep 7, 2015

I opted for the flag --all rather than --complete and added a some tests.

@ronkorving mind trying out this branch and letting me know if it does the trick for you?

@ronkorving
Copy link

The rename and implementation changes are fine with me 👍

@bcoe
Copy link
Member Author

bcoe commented Sep 7, 2015

@ronkorving awesome, did you try it out on a few of your modules? does it do the trick -- if so I'll get this merged tonight.

@ronkorving
Copy link

I'm having issues, will report back once I find out what it is.

@ronkorving
Copy link

By the way, running nyc with an unknown option (I first used --complete) gives a very cryptic error:

internal/child_process.js:298
    throw errnoException(err, 'spawn');
    ^

Error: spawn EACCES
    at exports._errnoException (util.js:837:11)
etc...

content,
'./' + relFile
glob.sync('**/*.js').forEach(function (filename) {
var obj = _this.addFile(filename)

Choose a reason for hiding this comment

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

We are now reading full file contents of files that should be excluded. Potentially quite a painful performance hit.

@ronkorving
Copy link

So for some reason, I'm getting this error on this file (a few thousand files up to this point did not give an error):

...
READING node_modules/express/node_modules/proxy-addr/node_modules/ipaddr.js
fs.js:603
  var r = binding.read(fd, buffer, offset, length, position);
                  ^

Error: EISDIR: illegal operation on a directory, read
    at Error (native)
    at Object.fs.readSync (fs.js:603:19)
    at Object.fs.readFileSync (fs.js:438:24)
    at NYC.addFile (CENSORED/node_modules/nyc/index.js:66:29)
etc

While running:

./node_modules/.bin/nyc --all ./node_modules/.bin/tape test/*.js

And while having added a console.log statement to see which file was blowing up:

 65   console.log('READING', filename);
 66   var content = stripBom(fs.readFileSync(filename, 'utf8'))

@ronkorving
Copy link

I understand the problem now. Before, node_modules was being ignored and we didn't read anything. ipaddr.js which you see there is the name of an NPM module, and is therefore a directory, not a file. We're now doing readFileSync on everything, even when in an exclude path. My PR didn't do that and it used findFile which doesn't return folders, unlike glob, which is why it didn't blow up before.

I would recommend some refactoring here to:

  • stop reading files we exclude.
  • run fs.statSync on what glob returns to make sure it's actually a file we can read.

@bcoe
Copy link
Member Author

bcoe commented Sep 7, 2015

@ronkorving give that update a shot.

With regards to the error you're seeing passing --complete, unfortunately there's no quick fix that I can think of. Since there's no way to know that --complete is a boolean argument (since it's an unknown flag), it's hard for the argument parser to know not to consume the next token.

@ronkorving
Copy link

Looks good, let me try again 👍

@ronkorving
Copy link

\o/ victory!
I'm happy if you are, merge away :)

@ronkorving
Copy link

Good to go? :) Sorry for the impatience, but I can't wait to integrate this into my projects.

bcoe added a commit that referenced this pull request Sep 9, 2015
added --all flag for indicating that all files should have coverage applied
@bcoe bcoe merged commit c7921c6 into master Sep 9, 2015
@bcoe bcoe deleted the complete branch September 9, 2015 04:54
@ronkorving
Copy link

Awesome :) If you could follow that up with an NPM release, you'll have made my day ;)

@bcoe
Copy link
Member Author

bcoe commented Sep 9, 2015

@ronkorving go ahead and try:

npm install nyc@next

I'm going to test out the changes on a few of my libraries, and if everything is looking good will move the the tag next to latest.

@ronkorving
Copy link

I have no idea how to update my package.json file to use @next (this concept of next/latest is new to me). But with a manual install as you showed, it all works fine for me 👍

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.

2 participants