fix: UI and status report diff patch numbers #679
Conversation
related: codecov/internal-issues#721 related: codecov/engineering-team#2387 We have a problem that the UI and the statuses are reporting different numbers for patch. Part of this issue is that the `NotifyTask` (calculates patch for status) has access to the `Pull` while the `ComputeComparison` (calculates for the UI) doesn't. This is a problem because the base for patch coverage might be different, so numbers would be different (see [here](https://github.com/codecov/worker/blob/9078de24b4dbed1bb64250b2b3864336954f310c/tasks/compute_comparison.py#L334) that we assume no action is needed cause there's nothing we can do). So the 1st change here is to have the `NotifyTask` calculate and save patch coverage. The second change is to cache the patch calculation so we don't compute it more than once.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #679 +/- ##
========================================
Coverage 98.06% 98.07%
========================================
Files 434 433 -1
Lines 36626 36759 +133
========================================
+ Hits 35919 36051 +132
- Misses 707 708 +1
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 #679 +/- ##
========================================
Coverage 98.06% 98.07%
========================================
Files 434 433 -1
Lines 36626 36759 +133
========================================
+ Hits 35919 36051 +132
- Misses 707 708 +1
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 #679 +/- ##
========================================
Coverage 98.06% 98.07%
========================================
Files 434 433 -1
Lines 36626 36759 +133
========================================
+ Hits 35919 36051 +132
- Misses 707 708 +1
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 #679 +/- ##
==========================================
+ Coverage 98.11% 98.13% +0.02%
==========================================
Files 474 475 +1
Lines 37952 38655 +703
==========================================
+ Hits 37237 37936 +699
- Misses 715 719 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 23 files with indirect coverage changes
|
There was a problem hiding this comment.
i think this is good? i feel weird about save_patch_totals() using the project coverage base, but i don't think there is a patch coverage base field on the model. and i don't think we expect there to be multiple CommitComparisons with the same head and project base but different patch base. so i think the logic is right?
tasks/notify.py
Outdated
| if comparison.project_coverage_base.commit is None: | ||
| # This is the base that will be saved in the CommitComparison | ||
| # Even if the patch coverage could come from a different commit | ||
| return |
There was a problem hiding this comment.
in other words, we identify a CommitComparison by its project coverage base irrespective of whether its patch coverage base is different?
There was a problem hiding this comment.
Yes, that's right... but this was a good callout.
The Comparison is created by the UploadFinisher task here. I thought that Pull.compared_to was the project base. But maybe that's not the case... I'll review before merging
There was a problem hiding this comment.
Tracking the code to this (where we decide what is the base_commit for the comparison) it seems that it's always the project base, unless there is no pull, then we use the commit's parent
So I guess the logic is fine. If we have a project base we would have created a comparison. Otherwise there is no Pull object and so there's no Comparison.
But this also means we might emit patch coverage in the UI 🤔
Should we then get_or_create a comparison instead of only updating if it exists... it might be a better idea
|
i think codecov/engineering-team#884 describes a different approach for the same problem. so it can be closed, but take a peek at the |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
related: https://github.com/codecov/internal-issues/issues/721
related: codecov/engineering-team#2387
We have a problem that the UI and the statuses are reporting different numbers for patch.
Part of this issue is that the
NotifyTask(calculates patch for status) has access to thePullwhile theComputeComparison(calculates for the UI) doesn't.This is a problem because the base for patch coverage might be different, so numbers would be different (see here that we assume no action is needed cause there's nothing we can do).
So the 1st change here is to have the
NotifyTaskcalculate and save patch coverage.The second change is to cache the patch calculation so we don't compute it more than once.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.