Notify user of errors with uploading in comment #570
Conversation
The BaseNotifier class will be used by the TestResultsNotifier and the ErrorCommentNotifier. It provides a basic structure and utility for sending the comment to Github. The build_message method is not implemented and actual concrete notifier classes are meant to extend this class and implement the build_message method to generate the contents of the comment that will be sent to Github when notify() is called.
This commit changes the TestResultsNotifier to extend the BaseNotifier class from helpers. A change that was made as a side effect is that the BaseNotifier does not accept any arguments in the build_message or notify methods so the class constructor was extended to accept the payload that is used in the build_message method.
This commit changes the functionality of the upload finisher to call the notify task only when all uploads are successful, and when failing uploads are detected to call the NotifyError task instead that will make a comment on the relevant PR notifying the user that upload processing failed.
joseph-sentry
force-pushed
the
joseph/notify-error
branch
from
67f2a1f
to
0339428
Compare
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #570 +/- ##
==========================================
+ Coverage 97.49% 97.52% +0.03%
==========================================
Files 414 417 +3
Lines 34982 35152 +170
==========================================
+ Hits 34105 34282 +177
+ Misses 877 870 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #570 +/- ##
==========================================
+ Coverage 97.49% 97.52% +0.03%
==========================================
Files 414 417 +3
Lines 34982 35152 +170
==========================================
+ Hits 34105 34282 +177
+ Misses 877 870 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #570 +/- ##
==========================================
+ Coverage 97.49% 97.52% +0.03%
==========================================
Files 414 417 +3
Lines 34982 35152 +170
==========================================
+ Hits 34105 34282 +177
+ Misses 877 870 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #570 +/- ##
==========================================
+ Coverage 97.54% 97.61% +0.06%
==========================================
Files 449 452 +3
Lines 36188 37160 +972
==========================================
+ Hits 35299 36272 +973
+ Misses 889 888 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes
|
also adds simple tests for notify error task
|
|
||
| return (True, "comment_posted") | ||
|
|
||
| def insert_breaks(self, table_value): |
There was a problem hiding this comment.
was this in the code but not being used? 💀
| def build_message( | ||
| self, | ||
| ) -> str: | ||
| error_message = f"❗️ We couldn't process [{self.failed_upload}] out of [{self.total_upload}] uploads. Codecov cannot generate a coverage report with partially processed data. Please review the upload errors on the commit page." |
There was a problem hiding this comment.
does this emoji reference work? When I changed notifiers with emojis before I had to use something like :tada: (see _create_welcome_message)
| ) | ||
|
|
||
| # get all upload errors for this commit | ||
| # |
There was a problem hiding this comment.
nit: remove unused line
| ) | ||
|
|
||
| commit: Commit = commits_query.first() | ||
| assert commit, "Commit not found in database." |
There was a problem hiding this comment.
can you explain this line? are you making sure that the commit does not exist? why does the .first() function return as that message?
There was a problem hiding this comment.
it's the opposite, we're making sure that commit exists, and if it doesn't we throw the assertion error with that message
There was a problem hiding this comment.
aaahhhh, i was reading it as though the assert was asserting that element 1 and element 2 were equal, rather than that element 1 is truthy else throw element 2
| num_failed_upload=num_failed_upload, | ||
| num_total_upload=num_total_upload, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
does this throw any errors if any of the extras are null or not found in the transformations above?
There was a problem hiding this comment.
if they're null it'll display null, but if they are undefined it will throw an error, but i don't think it's possible for them to be undefined (or else my ide would probably complain)
joseph-sentry
force-pushed
the
joseph/notify-error
branch
from
caea890
to
a73ff80
Compare
fixes: https://github.com/codecov/internal-issues/issues/517