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

Separated the snapshot summary creation from the printing to improve testability. #4373

Merged
merged 1 commit into from
Aug 27, 2017

Conversation

excitement-engineer
Copy link
Contributor

I noticed that the coverage for the jest-cli reporters was not very high so I decided I could add some value by writing some tests for it:)

I split the creation of the snapshot summary to a separate file (get_snapshot_summary.js).

I wanted to write a test for the snapshot summary text printed in the console at the end of a text. However, the summary_reporter.js in which the snapshot summary creation is contained requires a lot of overhead to set up making focused testing difficult. This is why I split the creation of the snapshot summary from the reporting about it.

I am hoping that test coverage of the reporters can be gradually improved by splitting them up into smaller units with well defined and focused responsibilities that are easily testable.

Let me know what you think!

@codecov-io
Copy link

Codecov Report

Merging #4373 into master will increase coverage by 0.19%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4373      +/-   ##
==========================================
+ Coverage   55.85%   56.04%   +0.19%     
==========================================
  Files         189      190       +1     
  Lines        6383     6388       +5     
  Branches        6        6              
==========================================
+ Hits         3565     3580      +15     
+ Misses       2815     2805      -10     
  Partials        3        3
Impacted Files Coverage Δ
...ackages/jest-cli/src/reporters/summary_reporter.js 12% <0%> (-7.7%) ⬇️
...ges/jest-cli/src/reporters/get_snapshot_summary.js 100% <100%> (ø)
packages/jest-cli/src/reporters/utils.js 63.54% <0%> (+1.04%) ⬆️

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 e4278b3...ce09cba. Read the comment docs.

@cpojer cpojer merged commit 39f83a9 into jestjs:master Aug 27, 2017
@cpojer
Copy link
Member

cpojer commented Aug 27, 2017

Woohoo, more code coverage :) Thanks again @excitement-engineer. Looks good, nice diff.

@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 13, 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.

4 participants