Skip to content

Conversation

@duszans
Copy link

@duszans duszans commented Oct 12, 2018

Summary

Fix for #7101

Test plan

  1. Checked it solves the problem described in Watch mode filename/test name filtering is not taken into account for coverage calculation with collectCoverageFrom set #7101
  2. Checked that also without watch and without collectCoverageFrom it still works correct

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rickhanlonii rickhanlonii requested a review from SimenB October 13, 2018 13:49
@rickhanlonii
Copy link
Member

@duszans can you fix the conflict?

@SimenB
Copy link
Member

SimenB commented Oct 13, 2018

@stipsan thoughts on this?

Copy link
Contributor

@rogeliog rogeliog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add tests for this?

if (
globalConfig.collectCoverageFrom &&
globalConfig.collectCoverageFrom.length
globalConfig.collectCoverageFrom.length &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check for !config.watchAll?

Copy link
Author

@duszans duszans Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rogeliog I don't think so - the watchAll is true when all files are watched in watch mode.
watch is true when there is some filtering applied, e.g. by test file name (at least in jest-cli/src/watch.js)

@stipsan
Copy link
Contributor

stipsan commented Oct 13, 2018

What will happen if you have negated patterns as well?

{
  "collectCoverageFrom": [
    "**/*.{js,jsx}",
    "!**/node_modules/**",
    "!**/vendor/**"
  ]
}

Will files from vendor suddenly show up? There is also another use case: —findRelatedTests. It uses collectCoverageFrom under the hood and it seems like this PR would break commands like: jest --watch --coverage —findRelatedTests file1.js?

A quick way to fix this could be to check for wether filename or testname filtering is applied in addition to the watch mode check in this PR.
A longer but perhaps safer fix could be to apply the filters on top of collectCoverageFrom instead of ignoring it, so that negative patterns are still in effect 🤔

@duszans
Copy link
Author

duszans commented Oct 16, 2018

@stipsan Could you provide reproduction steps for what could be broke in this PR related to --findRelatedTests? I checked that in my case it works fine with these changes.
In my sample repository which is mentioned in the related issue when I do with changes from this PR:

jest --watch --coverage --findRelatedTests src/add.js

then related add-test.js is run and coverage calculated only for add.js as expected.

@duszans
Copy link
Author

duszans commented Oct 16, 2018

@stipsan I also checked jest --watch --coverage with a file src/square.js which uses vendor/multiply.js, and a src/square-test.js and with the config you mentioned:

  "collectCoverageFrom": [
    "**/*.{js,jsx}",
    "!**/node_modules/**",
    "!**/vendor/**"
  ]

and with these changes it works just fine (e.g. it takes "!**/vendor/** into account). When filtering by square test file name no files from vendor are shown in the coverage. If I remove the vendor exclusion from collectCoverageFrom then vendor/multiply.js is taken into account so negations from collectCoverageFrom are clearly still working correctly.

@duszans
Copy link
Author

duszans commented Oct 16, 2018

Are there any other thoughts/issues to consider or can this be merged?

@codecov-io
Copy link

Codecov Report

Merging #7153 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7153   +/-   ##
=======================================
  Coverage   67.31%   67.31%           
=======================================
  Files         248      248           
  Lines        9615     9615           
  Branches        3        4    +1     
=======================================
  Hits         6472     6472           
  Misses       3142     3142           
  Partials        1        1
Impacted Files Coverage Δ
...ckages/jest-cli/src/reporters/coverage_reporter.js 67.71% <ø> (ø) ⬆️

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 ee9fc73...ed4a1bf. Read the comment docs.

@rickhanlonii
Copy link
Member

Did you see @rogeliog's comment about adding a test?

@stipsan
Copy link
Contributor

stipsan commented Oct 16, 2018

@duszans Awesome, thanks for taking the time to check those use cases 👍😃

@SimenB
Copy link
Member

SimenB commented Nov 19, 2018

@duszans mind adding a test so we can land this?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants