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

Improve the message when run coverage while there are no tests #6334

Merged
merged 13 commits into from
Aug 9, 2018

Conversation

rhysawilliams2010
Copy link
Contributor

@rhysawilliams2010 rhysawilliams2010 commented May 29, 2018

Resolves #6141

Summary

I wanted to be able to have a global threshold group (for any new files) even though I have path and glob threshold groups for all the existing files in my project.

At the moment, if you have a global threshold and glob or path matchers for all the files in the coverage data, jest throws an exception: Jest: Coverage data for global was not found.

I also wanted to be able to have a file be matched by more than one path or glob threshold group. So I can target specific file paths with precise thresholds but still have those files included in a path threshold group targeting the file's directory.

I updated the coverage reporting so that it still conforms to the
documentation but doesn't throw an error when there are no files matching
"global" threshold group (maybe because they are already matched with a path or glob).
I also made sure that a file is matched against all matching path and glob threshold groups instead of just one.

Test plan

image

@rhysawilliams2010 rhysawilliams2010 force-pushed the coverage-no-tests-warning branch 2 times, most recently from d7b3765 to 12e2f38 Compare May 29, 2018 03:27
const file = path.resolve(f[0]);
const override = f[1];
c[file] = new CoverageSummary({
...covSummary,
Copy link
Member

Choose a reason for hiding this comment

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

Please use Object.assign

@SimenB
Copy link
Member

SimenB commented May 29, 2018

@aaronabramov coverage collection is still pretty magical to me, does this change make sense?

@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #6334 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6334      +/-   ##
==========================================
+ Coverage   63.55%   63.56%   +<.01%     
==========================================
  Files         235      235              
  Lines        9030     9032       +2     
  Branches        3        4       +1     
==========================================
+ Hits         5739     5741       +2     
  Misses       3290     3290              
  Partials        1        1
Impacted Files Coverage Δ
...ckages/jest-cli/src/reporters/coverage_reporter.js 66.92% <100%> (+0.52%) ⬆️

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 0591f22...66798eb. Read the comment docs.

@@ -16,6 +16,7 @@ let libSourceMaps;
let CoverageReporter;
let istanbulApi;

import {CoverageSummary} from 'istanbul-lib-coverage/lib/file';
Copy link
Member

Choose a reason for hiding this comment

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

is it supported to do a deep import like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@aaronabramov aaronabramov left a comment

Choose a reason for hiding this comment

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

i'm actually really struggling to understand what's going on here :)
having an e2e test for this would be amazing

@@ -16,6 +16,7 @@ let libSourceMaps;
let CoverageReporter;
let istanbulApi;

import {CoverageSummary} from 'istanbul-lib-coverage/lib/file';
Copy link
Contributor

Choose a reason for hiding this comment

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

@SimenB
Copy link
Member

SimenB commented May 29, 2018

An e2e test would be great, yeah. Can probably extend https://github.com/facebook/jest/tree/master/e2e/coverage-report to encompass the fix here. Or use it as basis to setup a new dedicated e2e test

@rhysawilliams2010
Copy link
Contributor Author

rhysawilliams2010 commented May 29, 2018

@SimenB is this the test I should add to?

rhys williams added 3 commits May 30, 2018 10:21
fixes jestjs#6141
Update the coverage reporting so that it still conforms to the
documentation but doesn't throw an error when there are no files matching
"global" threshold group (maybe because they are already matched with a path or glob).
Also make sure that a file is matched against all matching path and glob threshold groups instead of just one.
(instead of the whole module).
So that I can use the original createCoverageSummary function to
create test data.
@SimenB
Copy link
Member

SimenB commented May 30, 2018

Yeah, that seems reasonable! 🙂

rhys williams and others added 7 commits May 31, 2018 10:56
I added snapshots for stderr so that I could test the Jest errors raised for each failed threshold group.
stderr snapshots.
Do a replace on the stderr which has a full path to a failed
glob threshold file which won't match when the test runs on windows
(appveyor)
@thymikee
Copy link
Collaborator

thymikee commented Jul 7, 2018

@aaronabramov might taking a look once again? Test was added :)

Copy link
Contributor

@aaronabramov aaronabramov left a comment

Choose a reason for hiding this comment

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

yeah! e2e describes what happened much better :)

* upstream/master: (122 commits)
  fix: don't report promises as open handles (jestjs#6716)
  support serializing `DocumentFragment` (jestjs#6705)
  Allow test titles to include array index (jestjs#6414)
  fix `toContain` suggest to contain equal message (jestjs#6810)
  fix: testMatch not working with negations (jestjs#6648)
  Add test cases for jestjs#6744 (jestjs#6772)
  print stack trace on calls to process.exit (jestjs#6714)
  Updates SnapshotTesting.md to provide more info. on snapshot scope (jestjs#6735)
  Mark snapshots as obsolete when moved to an inline snapshot (jestjs#6773)
  [Docs] Clarified the use of literal values as property matchers in toMatchSnapshot() (jestjs#6807)
  Update CHANGELOG.md (jestjs#6799)
  fix changelog entry that is not in 23.4.2 (jestjs#6796)
  Fix --coverage with --findRelatedTests overwriting collectCoverageFrom options (jestjs#6736)
  Update testURL default value from about:blank to localhost (jestjs#6792)
  fix: `matchInlineSnapshot` when prettier dependencies are mocked (jestjs#6776)
  Fix retryTimes and add e2e regression test (jestjs#6762)
  Release v23.4.2
  Docs/ExpectAPI: Correct docs for `objectContaining` (jestjs#6754)
  chore(packages/babel-jest) readme (jestjs#6746)
  docs: noted --coverage aliased by --collectCoverage (jestjs#6741)
  ...
@thymikee thymikee merged commit 08cb885 into jestjs:master Aug 9, 2018
@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.

Improve the message when run coverage while there are no tests
6 participants