SBoudrias / gulp-istanbul

Istanbul unit test coverage plugin for gulp.
MIT License
185 stars 87 forks source link

Files required before instrumentation are loaded from require.cache #42

Closed jpbackman closed 9 years ago

jpbackman commented 9 years ago

We are testing our modules with gulp-istanbul, but in our gulpfile we require() these modules, as some of our gulp tasks use them. This causes require to cache the files before they are instrumented by istanbul, so when the tests require them, the uninstrumented versions are returned from require.cache.

I'm not sure if this is exactly a gulp-istanbul issue.

As a workaround we do:

var through = require('through2')

gulp.task('test', function(cb) {
  gulp.src(['src/**/*.js'])
    .pipe(through.obj(function(file, enc, cb) {  // <-- workaround
      delete require.cache[file.path];
      cb(undefined, file);
    }))
    .pipe(istanbul({ includeUntested: true }))
    .on('finish', function() {
      gulp.src(['test/**/*.js'])
        .pipe(mocha())
        .pipe(istanbul.writeReports())
        .on('end', cb);
    });
});
SBoudrias commented 9 years ago

This is a tricky issue because we cannot invalidate the cache as it contains objects that can hold specific and shared states.

Your use case is actually very weird, and I'm incline to say it is inherently subject to issues. I think you should try to find a way to workaround the need to require these modules inside your build file.

I'll close the issue as there's no possible fixes we can apply on gulp-istanbul to fix your edge case.

robatron commented 9 years ago

I would actually like to second @jpbackman 's issue as we're experiencing it as well, both in the situation s/he's describing, and in a slightly different scenario as well: If the test spec requires the file we're trying to cover, the same thing happens, e.g.,

foo.js:

module.exports = 'bar';

foo-spec.js

describe( 'foo', function () {
    it( 'should return "bar"', function () {
        var foo = require( './foo' ); // <-- Breaks coverage
        assert( foo() === 'bar' );
    } );
} );

We're currently using proxyquire as a workaround, but it's an easy thing to forget.

SBoudrias commented 9 years ago

@robatron Maybe your example is wrong, but in this case, ./foo should be correctly covered.

robatron commented 9 years ago

It may be. That was reduced from a more-complex example. I'll verify in a bit.

SBoudrias commented 9 years ago

@robatron If you're sure this is a bug, please provide a reduce failing test case I can clone and reproduce on my machine.

jpbackman commented 9 years ago

@SBoudrias Yes, our case might be a bit of a corner case, but apparently we're not the only ones facing this problem, so maybe it could at least be mentioned as a side note in the readme.

SBoudrias commented 9 years ago

Yeah, I'll reopen for documentation update.

robatron commented 9 years ago

I'm not able to repro my simple example. Please disregard.

We're still experiencing @jpbackman 's issue, though. His/her workaround worked for us too.

SBoudrias commented 9 years ago

Finally I decided to go with a fix. So each covered file will be removed from cache at the same time it gets covered.

This should fix the issue you had. Please test and let me know how's it been!

jpbackman commented 9 years ago

Works for me, thanks. :)

robatron commented 9 years ago

Just got notification for the 0.4 release. Thank you, @SBoudrias !!

elkorep commented 7 years ago

I'm getting "istanbul is not a function" when doing .pipe(istanbul({ includedUntested: true })