Fix notifications with an empty HEAD report #816
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #816 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 446 446
Lines 36619 36649 +30
=======================================
+ Hits 35892 35923 +31
+ Misses 727 726 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #816 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 446 446
Lines 36619 36649 +30
=======================================
+ Hits 35892 35923 +31
+ Misses 727 726 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #816 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 446 446
Lines 36619 36649 +30
=======================================
+ Hits 35892 35923 +31
+ Misses 727 726 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #816 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 446 446
Lines 36619 36649 +30
=======================================
+ Hits 35892 35923 +31
+ Misses 727 726 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Swatinem
force-pushed
the
swatinem/fix-notify-no-report
branch
from
c68f8fc
to
d1d8d3a
Compare
There were a couple of errors spread around the notification code assuming that a `head.report` always exists, which is not necessarily the case. In particular that would be the case when using `empty-upload` or `manual_trigger`.
Swatinem
force-pushed
the
swatinem/fix-notify-no-report
branch
from
d1d8d3a
to
48ae32b
Compare
| @@ -84,6 +84,7 @@ def build_payload(self, comparison: ComparisonProxy) -> dict: | |||
| message = "unknown" | |||
| notation = "" | |||
| comparison_url = None | |||
| head_report = comparison.head.report | |||
There was a problem hiding this comment.
Is comparison.head typed as a FullCommit in the code? Maybe it would be a good idea to update the type to account for that. I've seen examples where the commit might also be None and have a half-done PR to update it
There was a problem hiding this comment.
I’m unsure about that one. I had the problem that comparison might be wrongly typed, as some tests were failing because it was actually a FilteredComparison, though I’m not sure if that only happened because of tests, or if that can also happen in real world code. In other parts of the notification infra, I have updated the types to take that into account.
There were a couple of errors spread around the notification code assuming that a
head.reportalways exists, which is not necessarily the case.In particular that would be the case when using
empty-uploadormanual_trigger.Fixes WORKER-P6A