Fix race condition with --coverage and babel-jest identical file contents edge case #4432
Conversation
packages/babel-jest/src/index.js
Outdated
| @@ -90,6 +90,8 @@ const createTransformer = (options: any) => { | |||
| .update('\0', 'utf8') | |||
| .update(fileData) | |||
| .update('\0', 'utf8') | |||
| .update(filename) | |||
There was a problem hiding this comment.
Can we make this relative to the rootDir (if it isn't already)?
There was a problem hiding this comment.
It's not, checking 😄
There was a problem hiding this comment.
In order to make it relative to rootDir we'll have to pass it here first:
https://github.com/facebook/jest/blob/0cb03f6474f52964ed74eadcde2ae3e219b4d04e/packages/jest-runtime/src/script_transformer.js#L77-L80
There was a problem hiding this comment.
Alright I'm on it.
There was a problem hiding this comment.
Pushed it, I also changed a flowtype hint as it appeared to be incorrect: 69456ca#diff-066bbe3aae5baf14d3b62017f0dc86d4R85
According to the definition here: https://github.com/facebook/jest/blob/0cb03f6474f52964ed74eadcde2ae3e219b4d04e/types/Transform.js#L37-L41
getCacheKey gets CacheKeyOptions not TransformOptions .
|
@cpojer pushed, looks like CIs are happy 😃 |
Codecov Report
@@ Coverage Diff @@
## master #4432 +/- ##
==========================================
+ Coverage 56.66% 56.92% +0.26%
==========================================
Files 191 189 -2
Lines 6496 6461 -35
Branches 6 3 -3
==========================================
- Hits 3681 3678 -3
+ Misses 2812 2782 -30
+ Partials 3 1 -2
Continue to review full report at Codecov.
|
|
Very much appreciate the thorough summary, added tests and the fix. Thank you so much for your contribution to Jest, this is awesome. |
|
cc @aaronabramov check this PR out! |
|
Thanks @cpojer! I really appreciate the time all the maintainers and contributors spend making jest great. To me that makes it a no-brainer to spend extra time making the PR easier to review 😄 |
…ents edge case (jestjs#4432) * Add test case * Fix cache race condition by including filename in cache key * make filename cache key relative to rootDir * Update snapshot
|
I'm still having this problem with 21.2.1, 22.2.0, and 22.2.2 (I'm getting 0 coverage reported for all but one of the files that are identical except for their path) unless I run with --no-cache, which is less than ideal. If I have two files that are identical except for their paths, it's more or less random which one will show the proper coverage and which one will show 0. |
|
@gdmitchell can you make a repo to reproduce this? It sounds like you're having a similar problem, but it if it's a regression directly related to this PR then I would've experienced it in my own projects as well as you. But in my case it's been reliable for months now 😮 |
|
@stipsan Unfortunately, I haven't been able to repro it on anything but a work repo that I can't share. I tried making a minimal repo of just the files that exhibited the problem but when I did so, clearing the cache made the problem go away, which it doesn't in the original repo. 🤔 |
|
I understand. An alternative could be that you redact any sensitive information from your jest config file and share that and I might be able to help you see if we can reproduce it somehow 🙂 Do you use any setup like lerna, yarn workspaces or NODE_PATH or similar btw? |
|
Here's my package.json with a few redactions, which includes my Jest config: Note that in scripts I would normally have just the Jest command instead of jest --no-cache, but I've added --no-cache for the time being as a workaround for this problem. We do use Yarn, though in my testing I've also been running Jest directly and it doesn't seem to make any difference. |
|
Ok thanks! This is interesting Transformers are affected by wether you run with Btw I meant the yarn monorepo feature they call workspaces: https://yarnpkg.com/lang/en/docs/workspaces/ |
|
Ahh, yeah. We don't use workspaces as far as I'm aware. Here's preprocessor.js. Just changed the names of a couple variables but the idea is that it does babel.transform on anything not in node_modules or that is one of our own dependencies (library1/library2). |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
There's a super edge case race condition in the way babel-jest generates its cache key that affects code coverage reports on files that are identical except for their path.
It only affects cases where you have the same file contents in separate files that have different dirnames. This is not uncommon in large codebases though where some use cases encourage code duplication over KISS for various reasons. However when it does happen it can be very frustrating as most devs won't even know where to start trying to figure out why some coverage tests suddenly start failing.
There's at least one reported case related to this: #3968
There may be more, I'll have another go at finding them after this PR is merged or closed.
I started with creating a repo that shows how to reproduce the problem: https://github.com/stipsan/jest-identical-files-coverage-bug
But I decided to attempt fixing the problem, and do a bug report if I failed to find and resolve the issue in jest itself.
Test plan
The fix itself is very simple, just make the filename part of the cache key (which jest-runtime is doing), writing a test though is super hard because it does not happen if you run tests with
--no-cache(which most integration tests in jest is doing, to avoid false passing tests, if my understanding is correct).But a wise coder once said that if it's a super edge case then testing it is super important.
The attached test makes sure to cover the edge case by running the relevant test twice, once to prime the cache and the second time to test if it reports coverage correctly.
In my testing I noticed several cases where the race condition would not occur if the cache directory were empty. But if it primes the cache first it consistently fails on the second test run.
I tried my best to follow existing testing conventions and live up to the coding standard of the project codebase. Since I am new to the codebase however I am eager to adjust the PR to your feedback.