-
Notifications
You must be signed in to change notification settings - Fork 270
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 notifications - coverage tests #911
Improve notifications - coverage tests #911
Conversation
….com/ochan12/time-to-leave into improve-notifications-coverage-tests
Hey @tupaschoal this is going to be impossible to review. Are you okay with me splitting this into smaller pieces? |
Yes, please do, it will be easier for all :) |
@ochan12 are you willing to continue this PR? You had split it up to another smaller one, not sure how much is left here. |
@tupaschoal Yes, I'll update it |
449840b
to
2e597c6
Compare
2e597c6
to
8c63f4c
Compare
It is ready now @tupaschoal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, here's the summary of coverage change after your PR:
time-to-leave/js 73.13 -> 73.99
main-window.js 67.36 -> 67.70
menus.js 79.06 -> 78.82
time-balance.js 97.56 -> 100.0
time-math.js 78.72 -> 100.0
update-manager.js 00.00 -> 42.85
windows.js 26.92 -> 93.54
time-to-leave/src 78.83 -> 79.52
workday-waiver.js 76.65 -> 77.53
We got some to 100% and a huge increase on some others, which is great!
\changelog-update |
Related issue
Coverage test #906
Context / Background
These were changes I made trying to hit the coverage target, but were completely unrelated to #906 issues. So in order to keep the changes smaller and more focused I created this new PR with tests.
What change is being introduced by this PR?
resetWindows()
to execute on testsHow will this be tested?
With Jest automates test suites