Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support different coverage options in projects #9633

Closed
wants to merge 3 commits into from

Conversation

royra
Copy link

@royra royra commented Mar 4, 2020

Summary

Having the coverage configuration at the global config is not consistent with the way it runs. The globs and paths specified at collectCoverageFrom, collectCoverageOnlyFrom, coverageThreshold are relative to the projects' <rootDir> (i.e, do not include the project path), but there is no way to specify options for a specific project.

You can create a union of the configuration for all projects and put it in the global config, but this has the side effect of creating ambiguities. For example, consider the following configuration:

{
  collectCoverageFrom: [
    "index.js",  // package1 wants index.js covered
    "!index.js",  // package2 wants it excluded
  ]
}

This PR moves the following config props from globalConfig to projectConfig:
collectCoverageFrom, collectCoverageOnlyFrom, coverageThreshold

Those properties can now be specified per project.
(Of course, you can still specify them at the global config if projects are not used)

As a side effect, they were removed from the whitelist of watch-able properties.

Fixes the following issues:
#9628 (opened by me)
#6998
#7733 (partial solution)
#8722
#8793
#6483
#6143
#5457
#4255

Test plan

Added an e2e test from the case described at #9628.

@facebook-github-bot
Copy link
Contributor

Hi @royra!

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 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 to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

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

@drew-gross
Copy link

drew-gross commented Mar 4, 2020

Codecov Report

Merging #9633 into master will increase coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9633      +/-   ##
==========================================
+ Coverage   65.09%   65.29%   +0.20%     
==========================================
  Files         287      312      +25     
  Lines       12144    12226      +82     
  Branches     3009     3008       -1     
==========================================
+ Hits         7905     7983      +78     
- Misses       3604     3606       +2     
- Partials      635      637       +2     
Impacted Files Coverage Δ
examples/manual-mocks/models/user.js 100.00% <0.00%> (ø)
examples/react/CheckboxWithLabel.js 100.00% <0.00%> (ø)
examples/angular/shared/sub.service.ts 0.00% <0.00%> (ø)
examples/jquery/displayUser.js 100.00% <0.00%> (ø)
examples/typescript/sum.js 100.00% <0.00%> (ø)
examples/angular/shared/data.service.ts 100.00% <0.00%> (ø)
examples/timer/timerGame.js 100.00% <0.00%> (ø)
e2e/coverage-projects/packages/mul/other_file.js 100.00% <0.00%> (ø)
examples/automatic-mocks/utils.js 50.00% <0.00%> (ø)
examples/timer/infiniteTimerGame.js 85.71% <0.00%> (ø)
... and 15 more

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 e3aa106...e5b4db1. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Mar 4, 2020

Codecov Report

Merging #9633 into master will increase coverage by 0.20%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9633      +/-   ##
==========================================
+ Coverage   64.92%   65.13%   +0.20%     
==========================================
  Files         288      313      +25     
  Lines       12184    12266      +82     
  Branches     3022     3019       -3     
==========================================
+ Hits         7911     7989      +78     
- Misses       3635     3637       +2     
- Partials      638      640       +2     
Impacted Files Coverage Δ
packages/jest-config/src/index.ts 11.59% <ø> (ø)
packages/jest-core/src/lib/update_global_config.ts 80.00% <ø> (-1.64%) ⬇️
packages/jest-core/src/watch.ts 78.53% <ø> (ø)
...ckages/jest-reporters/src/generateEmptyCoverage.ts 66.66% <ø> (ø)
packages/jest-runner/src/runTest.ts 2.17% <ø> (ø)
packages/jest-runtime/src/index.ts 64.75% <ø> (ø)
packages/jest-reporters/src/coverage_reporter.ts 54.49% <83.33%> (+0.59%) ⬆️
e2e/coverage-projects/packages/add/index.js 100.00% <100.00%> (ø)
e2e/coverage-projects/packages/mul/other_file.js 100.00% <100.00%> (ø)
packages/jest-reporters/src/get_watermarks.ts 100.00% <100.00%> (ø)
... and 26 more

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 cd98198...e30321d. Read the comment docs.

@royra
Copy link
Author

royra commented Mar 24, 2020

@SimenB @thymikee @jeysal any chance to get this reviewed?

@jeysal
Copy link
Contributor

jeysal commented Mar 24, 2020

Hi, at some point it surely will be, it's just that all maintainers currently have very little time, sorry about that 😐

Roy Razon added 3 commits April 12, 2020 20:52
Moved the following config props from globalConfig to projectConfig:
collectCoverageFrom, collectCoverageOnlyFrom, coverageThreshold

You can now specify those properties per project.

As a side effect, they were removed from the whitelist of watch-able properties.
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.

Sorry about the slow review 😬 The code itself looks solid, but I think we might need to support changing these in watch plugins. Not 100% though. @rogeliog @thymikee @jeysal thoughts?

@@ -12,12 +12,9 @@ const DEFAULT_GLOBAL_CONFIG: Config.GlobalConfig = {
changedFilesWithAncestor: false,
changedSince: '',
collectCoverage: false,
Copy link
Member

Choose a reason for hiding this comment

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

why not all the coverage* options?

@@ -157,8 +157,6 @@ For stability and safety reasons, only part of the global configuration keys can
- [`bail`](configuration.html#bail-number--boolean)
- [`changedSince`](cli.html#--changedsince)
- [`collectCoverage`](configuration.html#collectcoverage-boolean)
- [`collectCoverageFrom`](configuration.html#collectcoveragefrom-array)
Copy link
Member

Choose a reason for hiding this comment

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

I think watch plugins should still be able to set these, somehow... Might have to solve watchplugins setting project options before we can land this

@@ -61,6 +61,7 @@ module.exports = {
'/packages/jest-worker/src/__performance_tests__',
'/packages/pretty-format/perf/test.js',
'/e2e/__tests__/iterator-to-null-test.ts',
'/.history',
Copy link
Member

Choose a reason for hiding this comment

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

what's this directory?

@codecov-commenter
Copy link

Codecov Report

Merging #9633 into master will increase coverage by 0.20%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9633      +/-   ##
==========================================
+ Coverage   64.88%   65.08%   +0.20%     
==========================================
  Files         289      314      +25     
  Lines       12229    12311      +82     
  Branches     3030     3027       -3     
==========================================
+ Hits         7935     8013      +78     
- Misses       3654     3656       +2     
- Partials      640      642       +2     
Impacted Files Coverage Δ
packages/jest-config/src/index.ts 11.59% <ø> (ø)
packages/jest-core/src/lib/update_global_config.ts 80.00% <ø> (-1.64%) ⬇️
packages/jest-core/src/watch.ts 78.53% <ø> (ø)
...ckages/jest-reporters/src/generateEmptyCoverage.ts 66.66% <ø> (ø)
packages/jest-runner/src/runTest.ts 2.22% <ø> (ø)
packages/jest-runtime/src/index.ts 64.68% <ø> (ø)
packages/jest-reporters/src/coverage_reporter.ts 54.49% <83.33%> (+0.59%) ⬆️
e2e/coverage-projects/packages/add/index.js 100.00% <100.00%> (ø)
e2e/coverage-projects/packages/mul/other_file.js 100.00% <100.00%> (ø)
packages/jest-reporters/src/get_watermarks.ts 100.00% <100.00%> (ø)
... and 26 more

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 c830517...e44574f. Read the comment docs.

@justinbayctxs
Copy link

Got pretty far in switching a monorepo to use Jest's multi-project feature and had to stop when I hit these coverage issues. Would love to see this landed 💯

@ziofat
Copy link

ziofat commented Nov 18, 2020

Well, any updates on this? We are facing the same issue and will be glad to have this feature.

@starikcetin
Copy link

This PR needs some love, my coverage report only shows the covered files and nothing about the uncovered files.

starikcetin added a commit to ctisbtes/btes that referenced this pull request Feb 28, 2021
this is a workaround until this PR gets merged: jestjs/jest#9633
starikcetin added a commit to ctisbtes/btes that referenced this pull request Feb 28, 2021
this is a workaround until this PR gets merged: jestjs/jest#9633
@piranna
Copy link
Contributor

piranna commented Dec 22, 2021

This PR needs some love, my coverage report only shows the covered files and nothing about the uncovered files.

Can this PR get merged? I'm getting the same issues than @starikcetin.

@piranna
Copy link
Contributor

piranna commented Dec 23, 2021

How can we get this to move forward and get it merged?

@SimenB
Copy link
Member

SimenB commented Jan 5, 2022

How can we get this to move forward and get it merged?

Nothing has changed since my review in 2020 (and I've honestly lost all context here, so once those comments are resolved (mainly we need to figure out how this interacts with watch plugins)) I'll need to take a new look at this

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jan 5, 2023
@github-actions
Copy link

github-actions bot commented Feb 4, 2023

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Feb 4, 2023
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

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 Mar 7, 2023
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.