-
Notifications
You must be signed in to change notification settings - Fork 10
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
redo: Failed check and status messaging in comment, part I #1022
Conversation
notification_result = result["result"] | ||
if ( | ||
notification_result is not None | ||
and notification_result.data_sent is not None |
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.
this check notification_result.data_sent is not None
is added since the reverted version #1014
] | ||
res = notifications_service.notify(sample_comparison) | ||
assert expected_result == res | ||
|
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.
added this test to cover the case that failed in the now reverted version #1014
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1022 +/- ##
=======================================
Coverage 97.71% 97.71%
=======================================
Files 458 458
Lines 37006 37072 +66
=======================================
+ Hits 36159 36224 +65
- Misses 847 848 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
9d9d277
to
07be913
Compare
…o other notifiers
7c99a5a
to
9634837
Compare
this PR was missing a check and caused an error.
the check has been added and a unit test to cover the condition that broke when it was deployed.
perviously deployed and reverted version: #1014