feat: count difference of uploaded sessions #454
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #454 +/- ##
==========================================
- Coverage 97.34% 97.34% -0.01%
==========================================
Files 399 402 +3
Lines 33616 33740 +124
==========================================
+ Hits 32724 32844 +120
- Misses 892 896 +4
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 #454 +/- ##
==========================================
- Coverage 97.34% 97.25% -0.09%
==========================================
Files 399 410 +11
Lines 33616 34070 +454
==========================================
+ Hits 32724 33136 +412
- Misses 892 934 +42
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 #454 +/- ##
==========================================
- Coverage 97.34% 97.25% -0.09%
==========================================
Files 399 410 +11
Lines 33616 34070 +454
==========================================
+ Hits 32724 33136 +412
- Misses 892 934 +42
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 ✅
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 #454 +/- ##
==========================================
- Coverage 97.37% 97.33% -0.04%
==========================================
Files 430 442 +12
Lines 34306 35883 +1577
==========================================
+ Hits 33405 34928 +1523
- Misses 901 955 +54
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 43 files with indirect coverage changes This change has been scanned for critical changes. Learn more |
giovanni-guidini
force-pushed
the
gio/count-report-uploaded-diff
branch
from
f3842bd
to
d5ca70f
Compare
ticket: codecov/engineering-team#1622 These changes introduce a new capabiliy to `ComparisonProxy`. Namely, to get the difference in the number of uploads reported per flag between HEAD and BASE. The idea is to be able to give more actionable message in the PR comment on the following scenario: 1. I have uploaded 5 reports to my base commit 2. I upload 1 report to head commit 3. I have NOT covered all lines of my patch 4. I'm seeing project coverage go WAY down Because typically coverage is uploaded via CI, and CI doesn't change often, a different number of reports present could indicate issues in the coverage results. We have to ignore carryforward sessions because different commits might upload different sets of reports. There are 2 functions so that we can also get the _total_ number of reports uploaded per flag, not only the diff
giovanni-guidini
force-pushed
the
gio/count-report-uploaded-diff
branch
from
d5ca70f
to
6c87069
Compare
services/comparison/__init__.py
Outdated
| # We ignore carryforward sessions | ||
| # Because not all commits would upload all flags (potentially) | ||
| # But they are still carried forward | ||
| if session.session_type == SessionType.uploaded: |
There was a problem hiding this comment.
This matches the comment a bit better.
| if session.session_type == SessionType.uploaded: | |
| if session.session_type != SessionType.carriedforward: |
| # But they are still carried forward | ||
| if session.session_type == SessionType.uploaded: | ||
| if session.flags == []: | ||
| session.flags = [""] |
There was a problem hiding this comment.
What does a session with no flags mean? Is there a possibility for "" to be a session flag? And if it can be an existing string, then do we want to differentiate no flags with the actual "" flag?
There was a problem hiding this comment.
A session with no flag is one that is uploaded without using the (optional) --flag argument in the CLI or uploader. They are not incommon if you have only a simple project and don't separates your tests.
I think it's extremely unlikely that "" is used as an actual flag because it doesn't carry any information in it. In essence there's little difference uploading with a "" flag and no flag. I don't think we need to handle it separately.
A good callout though.
services/comparison/__init__.py
Outdated
| existing = per_flag_dict.get(flag) | ||
| if existing: | ||
| existing[curr_counter] += 1 | ||
| else: | ||
| new_entry = ReportUploadedCount( | ||
| flag=flag, base_count=0, head_count=0 | ||
| ) | ||
| new_entry[curr_counter] += 1 | ||
| per_flag_dict[flag] = new_entry |
There was a problem hiding this comment.
The Python collections module contains a Counter class that does something similar. One advantage of counter is that it doesn't require the existing check (for both adding and retrieving data). The main downside of using a counter is that we would have to recast it into List[ReportUploadedCount] if that's the type we want we want to return.
There was a problem hiding this comment.
After some reflecting I decided to not use Counters... The main reason is that I think we should keep the ReportUploadedCount type.
idk exactly what you had in mind, but the way I see it we'd need 2 counters (one for head_count, one for base_count). The main loop wouldn't change too much, and the casting to ReportUploadedCount would be an extra process. So I justed reworked the if / else but of this main loop a little bit.
Open to suggestions tho, of course
There was a problem hiding this comment.
That sounds good! I was also thinking of keeping 2 counters, but as mentioned, I wasn't sure about recasting it into ReportUploadedCount. I liked the newly added simplification! 👍
| def test_get_reports_uploaded_count_per_flag_diff( | ||
| head_sessions, base_sessions, expected_diff | ||
| ): | ||
| head_report = Report() | ||
| head_report.sessions = head_sessions | ||
| base_report = Report() | ||
| base_report.sessions = base_sessions | ||
| comparison_proxy = ComparisonProxy( | ||
| comparison=Comparison( | ||
| head=FullCommit(report=head_report, commit=None), | ||
| project_coverage_base=FullCommit(report=base_report, commit=None), | ||
| patch_coverage_base_commitid=None, | ||
| enriched_pull=None, | ||
| ) | ||
| ) | ||
| # Python Dicts preserve order, so we can actually test this equality | ||
| # See more https://stackoverflow.com/a/39537308 | ||
| assert comparison_proxy.get_reports_uploaded_count_per_flag_diff() == expected_diff |
There was a problem hiding this comment.
These tests almost look the same as the ones on line 65 (including the parametrized valuses). Can we just add a the results and the extra assert call for get_reports_uploaded_count_per_flag_diff?
| from services.comparison.types import Comparison, FullCommit, ReportUploadedCount | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
When adding parametrize tests, I like to add ids for each test case to more clearly communicate what the test is checking.
There was a problem hiding this comment.
this is so cool... I didn't know about that. Thanks for sharing, will add and ID on those
bundle tests together. slight refactor of dict buildup
ticket: codecov/engineering-team#1622
These changes introduce a new capabiliy to
ComparisonProxy. Namely, to get thedifference in the number of uploads reported per flag between HEAD and BASE.
The idea is to be able to give more actionable message in the PR comment on the following scenario:
Because typically coverage is uploaded via CI, and CI doesn't change often, a different
number of reports present could indicate issues in the coverage results.
We have to ignore carryforward sessions because different commits might upload different
sets of reports.
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.