-
Notifications
You must be signed in to change notification settings - Fork 277
Raise test coverage to 100% #403
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
Conversation
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 stuff! Thank you very much for your PR @philihp - I've left two comments - not 100% related to this PR but would be good if we can sort these out. That 100% badge in the README will look good!
src/js/store/configureStore.ts
Outdated
storageMiddleware, | ||
]; | ||
|
||
/* istanbul ignore next */ |
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.
Might not be related to this PR - but shall we remove this block completely (along with the isDev
variable)? I guess we can also remove redux-logger
from package.json
.
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.
Awesome! Even better 😎, I'll make this change.
I didn't want to overstep and really tried to avoid changing non-test files, but if you'd be okay with refactors, I'd be happy to go deeper and propose some cleanup changes.
|
||
exports[`./utils/github-api.ts should format the notification reason 12`] = ` | ||
Object { | ||
"description": undefined, |
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.
Just realised that this is broken in the source file - shall we update the actual file https://github.com/manosim/gitify/blob/master/src/js/utils/github-api.ts#L48 to use DESCRIPTIONS['CI_ACTIVITY']
which will remove that undefined
.
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.
Cheer for making the changes @philihp 🚀
|
This raises the line-by-line test coverage from 98% to 100%. I tried to keep my edits limited to only the tests, with the exception of
src/js/store/configureStore.ts
having a block of code that looks like it exists for debugging/development.