Refactor -o & --coverage implementation | Fixes #6859

[MarcoScabbiolo]

Summary

Added DependencyResolver#resolveInverseModuleMap that exposes all the module map (the files and their dependencies). resolveInverse was kept in its original form for compatibility.

The changed files are no longer attached to collectCoverageFrom. Now they're passed through runners, schedulers and workers to Runtime.shouldInstrument, the proper method to exclude files from coverage.

Includes e2e regression tests.

Fixes #6859, fixes #7101, closes #7153

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[codecov-io]

Codecov Report

Merging #7611 into master will decrease coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7611      +/-   ##
==========================================
- Coverage   68.26%   68.04%   -0.22%     
==========================================
  Files         249      249              
  Lines        9623     9538      -85     
  Branches        6        6              
==========================================
- Hits         6569     6490      -79     
+ Misses       3052     3046       -6     
  Partials        2        2

Impacted Files	Coverage Δ
packages/jest-cli/src/lib/watch_plugins_helpers.js	94.44% <0%> (-0.56%)	⬇️
packages/jest-runtime/src/index.js	76.92% <0%> (-0.07%)	⬇️
packages/expect/src/matchers.js	99.41% <0%> (-0.02%)	⬇️
packages/expect/src/index.js	94.01% <0%> (ø)	⬆️
packages/jest-diff/src/diffStrings.js	100% <0%> (+1.47%)	⬆️
packages/jest-matcher-utils/src/index.js	100% <0%> (+5.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f425bd4...f12facb. Read the comment docs.

[SimenB]

Thank you so much! A failing test is the best sort of bug report ❤️

[MarcoScabbiolo]

@SimenB, @stipsan I ended up fixing it.

[MarcoScabbiolo]

Looks like the Windows job had a blue screen :(

[SimenB]

That's awesome @MarcoScabbiolo, thanks! I added a changelog entry to this.

However, I'm not entirely sure this is the correct fix. For instance, if I add this diff the the integration test, it fails again:

diff --git i/e2e/__tests__/onlyChanged.test.js w/e2e/__tests__/onlyChanged.test.js
index 8f85d0dd9..9aa64f785 100644
--- i/e2e/__tests__/onlyChanged.test.js
+++ w/e2e/__tests__/onlyChanged.test.js
@@ -151,7 +151,7 @@ test('do not pickup non-tested files when reporting coverage on only changed fil
   run(`${GIT} commit --no-gpg-sign -m "first"`, DIR);
 
   writeFiles(DIR, {
-    'b.test.js': 'it("passes", () => {})',
+    'b.test.js': 'require("./package.json");\nit("passes", () => {})',
     'package.json': JSON.stringify({name: 'new name'}),
   });
 

That will not fail if we remove -o. We're missing some filter that would remove files we never collect coverage from

---

Looks like the Windows job had a blue screen :(

Ignore it, see #7760

[MarcoScabbiolo]

@SimenB What you point out is correct, coverage collection behaves differently when using -o, but I think this is a good starting point to implement a feature that was honestly missing. I've added the scenario to the test, thanks.

There are some candidates to these "default coverage filter" we're looking for.Runtime.shouldInstrument is a good one, however I've tried it and it didn't work. Anyways it is used because the coverage for each file is initialized using generateEmptyCoverage which correctly implements Runtime.shouldInstrument.

I think the main issue right now is that the way to filter the coverage for only changed files is to set collectCoverageFrom in the globalConfig, and alters some aspects of the coverage collection behavior as if it was the user who is forcing the collection of coverage from these files.

jest-cli/runJest.js

let collectCoverageFrom = [];

...

if (matches.collectCoverageFrom) {
        collectCoverageFrom = ...
}

...

if (collectCoverageFrom.length) {
    const newConfig: GlobalConfig = {...globalConfig, collectCoverageFrom};
    globalConfig = Object.freeze(newConfig);
}

Maybe using a different option to filter coverage for only changed files would fix the problem and differentiate between user and internal filters.

[MarcoScabbiolo]

Indeed:

jest-cli/reporters/coverage_reporter#_addUntestedFiles

if (
  globalConfig.collectCoverageFrom &&
  globalConfig.collectCoverageFrom.length
) {
  context.hasteFS
    .matchFilesWithGlob(globalConfig.collectCoverageFrom, config.rootDir)
    .forEach(filePath =>
      files.push({
        config,
        path: filePath,
      }),
    );
}

[stipsan]

I'm sorry that I've not checked up on this until now, given that it's 20 days since I said I would look into it… But I'm really happy to see a bug report turned into a first time PR! 🎉

Maybe using a different option to filter coverage for only changed files would fix the problem and differentiate between user and internal filters.

I ran into this as well, it was one of the most tricky parts of implementing this to begin with 🤔

[MarcoScabbiolo]

I finally got it, most of the previous code was deleted as it was unnecessary, I added changedFiles to globalConfig and then use it to exclude files from coverage, much clean and simpler.

I've deleted the runJestWithCoverage unit tests file as it tested only the previous behavior, using collectCoverageFrom for this same purpose. The e2e tests should cover these changes, but it would be nice to add new unit tests if somebody feels like it.

@stipsan

[SimenB]

I'm really excited about the direction you took, it feels more correct than the current implementation! Only real feedback I have is that using config to propagate this value between jest-cli and jest-runtime is not correct (it's not configuration). We'll have to thread the value through manually

[MarcoScabbiolo]

@SimenB I thought it wasn't a nice thing to do to add it to the globalConfig but since that was being done in the previous implementation I figured it would be ok, but you're right the value should be passed through. I'll try to find some time to properly finish it.

I was using path.resolve to make sure filenames are normalized to absolute ones. I'll try removing all the resolutions and see how it works, the paths may already be absolute or compatible between each other.

[SimenB]

since that was being done in the previous implementation I figured it would be ok

That was done mostly since collectCoverageFrom is a config option already. It was a hacky solution when added and we should have caught it then - the implementation shouldn't have piggy-backed on it. So this is a really good cleanup 🙂

I was using path.resolve to make sure filenames are normalized to absolute ones.

That seems fine, but they should resolve from something, e.g. rootDir. We don't want to use cwd, which is what path.resolve falls back to. If we don't need the resolve call though, that'd be the best

[MarcoScabbiolo]

I've refactored the -o/--coverage implementation based on feedback by @SimenB
Thank you for your assistance and for maintaining this awesome project.

[SimenB]

This is really starting to shape up, thanks! I'd like to us pass changedFiles inside of an object to reporters and runners - other than that this is awesome!

[jeysal]

Thanks, I think this part really needed some cleaning up to avoid obscure behavior 👍 left some additional comments

[SimenB]

Will this fix #7153?

[MarcoScabbiolo]

Yes @SimenB, that PR aims to tackle the same underlying issue

[SimenB]

Thank you so much!

[github-actions]

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.