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

fix: coverage summary reporting #7058

Merged
merged 1 commit into from
Oct 2, 2018
Merged

fix: coverage summary reporting #7058

merged 1 commit into from
Oct 2, 2018

Conversation

eddiemonge
Copy link
Contributor

Summary

Prevent text-summary from always being displayed when covereage is
configured. The reporters the configuration specifies should be used.
If no reporters are specified, then add the default text-summary.

Fixes #7048

Test plan

Not really sure how to write tests for this.

@codecov-io
Copy link

codecov-io commented Sep 27, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7058      +/-   ##
==========================================
+ Coverage   66.66%   66.67%   +<.01%     
==========================================
  Files         253      253              
  Lines       10597    10597              
  Branches        4        3       -1     
==========================================
+ Hits         7065     7066       +1     
+ Misses       3531     3530       -1     
  Partials        1        1
Impacted Files Coverage Δ
...ckages/jest-cli/src/reporters/coverage_reporter.js 67.71% <100%> (+0.78%) ⬆️

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 78e0893...999e55c. Read the comment docs.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Can we add a test for that?

) {
coverageReporters = coverageReporters.concat(['text-summary']);
if (!this._globalConfig.useStderr && coverageReporters.length < 1) {
coverageReporters = coverageReporters.push('text-summary');
Copy link
Collaborator

Choose a reason for hiding this comment

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

assignment is not necessary as push mutates the array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Forgot to delete the assignment when I changed to push instead of concat

@natealcedo
Copy link

You could probably write an integration test for this and do a snapshot on stdout.

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

Yeah, an integration test would be nice 🙂

@@ -94,12 +94,8 @@ export default class CoverageReporter extends BaseReporter {
}

let coverageReporters = this._globalConfig.coverageReporters || [];
Copy link
Member

Choose a reason for hiding this comment

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

CI wants this to be const now

@eddiemonge
Copy link
Contributor Author

You could probably write an integration test for this and do a snapshot on stdout.

How do I do that? There doesn't seem to be an easy to follow guide for that. (also, the wiki is turned on in this repo but unused. maybe it should be turned off?)

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

How do I do that?

If you look in the e2e directory, we have a bunch of directories. They're all integration tests. In the __tests__ directory we're launching Jest on this tests, and asserting on the output.

Contributions to CONTRIBUTING.md with details would be greatly appreciated

@eddiemonge
Copy link
Contributor Author

updated with more testing and snapshots

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.

Thank you! Can you update the changelog as well?

@eddiemonge
Copy link
Contributor Author

That's not automatically done? how do I know what version this change will go in as?

@SimenB
Copy link
Member

SimenB commented Oct 2, 2018

That's not automatically done?

Nope

how do I know what version this change will go in as?

Put it under master at the top

Prevent text-summary from always being displayed when covereage is
configured. The reporters the configuration specifies should be used.
If no reporters are specified, then add the default text-summary.
@eddiemonge
Copy link
Contributor Author

Updated

@SimenB SimenB merged commit ec382f5 into jestjs:master Oct 2, 2018
@eddiemonge eddiemonge deleted the issue_7048 branch October 2, 2018 21:52
@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.

Coverage always reports summary
6 participants