fix: respect changes of cwd in options

[jure]

Well, this one took a while.

Long story short, sometimes during runtime, the cwd in opts for shouldSkip does change (e.g. a multi-project Jest setup) through babel-jest https://github.com/facebook/jest/blob/master/packages/babel-jest/src/index.js#L125-L134, and if exclude https://github.com/istanbuljs/babel-plugin-istanbul/blob/master/src/index.js#L19 isn't 'redone' at that point, it will falsely exclude files from instrumentation (as it's using the previous/initial cwd).

I believe this is the cause for numerous missing coverage issues in multi-project setups. I'm only aware of Jest issues jestjs/jest#5417 jestjs/jest#5457 jestjs/jest#6483, which is what we're using, but there might be others.

In any case, it probably makes sense to rebuild exclude if cwd, its crucial option, changes, so that's what I've done. Also added a test for it.

Let me know if this feels ok!

[jure]

Hm, this passes locally. Let me see what's up.

Edit: Fails locally on a fresh clone 🤷‍♂️ , I'll fix it and in the mean time, mark this one as a WIP, if that's alright with folks.

Edit: 🤦‍♂️ That's what I get for staying up all night crawling the depths of Jest and Babel's dependencies. Fixing right now.

[coveralls]

Coverage remained the same at 97.059% when pulling 5da8435 on jure:respect_cwd into ad1177e on istanbuljs:master.

[jure]

Just a small update: We're now successfully using this in production in a multi-project Jest setup.

[nrobertdehault]

@jure You made my day with your fix, but I just realized version 5.0.1 also fixed the issue for me.

[jure]

@nrobertdehault Interesting! Glad 5.0.1 works for you. I was doubtful it would work for us, given that the new test covers our issue, but I did try it, and unfortunately it doesn't work, as we still have missing coverage. Maybe yours was a different issue?

[jure]

Any chance this gets looked at by someone up the chain? @coreyfarrell?

[coreyfarrell]

Sorry for the delay I've been very busy the past couple months.

[coreyfarrell]

Thanks for the fix, merged.

[ptitjes]

@coreyfarrell Will there be a patch release soon ?

[jure]

Good to have another confirmation, that this might be the source of missing coverage for many users of jest with projects: jestjs/jest#5457 (comment).

[SimenB]

@bcoe would it be possible to get a release of this? Seems to affect quite a few Jest users 🙂

[coreyfarrell]

@SimenB v5.1.1 is now published to npm under the next tag. Please let me know when you've had a chance to test it against some real projects and I'll tag it latest.

[SimenB]

Thanks! I'm unable to reproduce using the latest version of Jest (I wonder if it's because babel is better at loading plugins now in v7?). But I can verify that the new version doesn't break, at least 🙂

[jure]

Not sure I follow, @SimenB, are you saying that it works fine with 5.1.0 now, without this patch? I can give it a shot on Monday in our project, see if it’s fixed.

[SimenB]

@jure yeah, I can't reproduce after upgrading to jest's beta, which includes 5.1.0. Would be awesome if you could verify!

[jure]

Sorry for the delay. I'm now using jest 24 with babel-plugin-istanbul 5.1.0, and that seems to collect coverage correctly across multiple jest projects. I'll keep an eye out for any weirdness, but this has likely been resolved upstream, somewhere. I'd rather not go digging where exactly, it's a wild world up/down there :)

Maybe it's OK to be vigilant about cwd changes for the same instance though (this MR, that is), given that strange things have happened before.