dylanb / gulp-coverage

Gulp coverage reporting for Node.js that is independent of the test runner
MIT License
60 stars 12 forks source link

Instrument cover option #3

Closed p3drosola closed 10 years ago

p3drosola commented 10 years ago

The instrument cover option seems to having some problems.

src/**/*.js doesn't work even though the src directory is present . **/src/**/*.js works, however.

The problem is that the full file path will be present in the coverage report, which is not good. I noticed you also had this issue in the screenshot.

I believe the multi globbing is not picking up the right base dir.

dylanb commented 10 years ago

Would it be possible for you to illustrate the exact directory structure you are working with?

p3drosola commented 10 years ago

The directory looks like

src/
    foo.js
    bar.js
test/
   foo.js
   bar.js

when my config looks like this it won't work

gulp.task('coverage', function () {
  gulp.src(["test/**/*.js"])
    .pipe(cover.instrument({
      pattern: ["src/**/*.js"],
      debugDirectory: '.debug'
    }))
    .pipe(mocha({
      reporter: 'dot'
    }))
    .pipe(cover.report({
      outFile: 'coverage.html'
    }))
    .pipe(cover.enforce({}));
});

I have to prepend "**/" to the pattern option so it picks up the src files.

But then I see the full path in the sidebar, which is incorrect.

screen shot 2014-02-06 at 18 16 42

dylanb commented 10 years ago

thanks for the comprehensive data, In your case, I cannot see a downside to using the \ but I can see a downside when you might have multiple src directories and only want to instrument some of them.

p3drosola commented 10 years ago

The point is it's wrong. src/**/*.js should pick up my files.

On Fri, Feb 7, 2014 at 12:01 AM, Dylan Barrell notifications@github.comwrote:

thanks for the comprehensive data, In your case, I cannot see a downside to using the \ but I can see a downside when you might have multiple src directories and only want to instrument some of them.

Reply to this email directly or view it on GitHubhttps://github.com/dylanb/gulp-coverage/issues/3#issuecomment-34384879 .

dylanb commented 10 years ago

I designed it specifically this way. So that you could easily instrument files no matter where they are. For example, if your file src/foo.js included another file with require('../../otherdir/otherfile.js'), then you could simply instrument that with **/otherfile.js regardless of where it lives.

It is wrong in the sense that for you (who did not design the system), it seems wrong - which is very strong evidence that it needs to be changed :-)

p3drosola commented 10 years ago

Sorry, I didn't mean to sound too critical.

I think it should be consistent with all the other gulp plugins though.

I think it your scenario it would make more sense to do something like this.

gulp.src(["test/**/*.js"])
    .pipe(cover.instrument({
      pattern: ['src/foo.js', '../otherdir/otherfile.js'],
      debugDirectory: '.debug'
    }))

If the full path is visible in the output, then it's not "portable" in the sense that the exact same script will produce different outputs in each person's machine.

dylanb commented 10 years ago

can you look at the fixed version and confirm that it fixes your issue? If you confirm, I will bump the version and publish.

Thanks for supporting gulp-coverage :-)

p3drosola commented 10 years ago

It's throwing some errors when I instrument files deeper than the first layer of src.

src/*.js works fine but src/**/*.js throws

TypeError: Cannot read property '__instrumented_miss' of null

You can see the project right here, if you want to reproduce it:

dylanb commented 10 years ago

Ok, I have pushed a fix for that issue

p3drosola commented 10 years ago

Hey nice work! It works great.

The only thing missing now is to use relative paths in the coverage report. you can use path.relative.

current: screen shot 2014-02-10 at 11 48 36

ideal: screen shot 2014-02-10 at 11 50 13