-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix: parameter filtering takes branch into account #913
Conversation
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 #913 +/- ##
==========================================
+ Coverage 96.25% 96.32% +0.06%
==========================================
Files 823 823
Lines 18999 19644 +645
==========================================
+ Hits 18288 18922 +634
- Misses 711 722 +11
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
📢 Thoughts on this report? Let us know! |
@@ -298,6 +299,17 @@ def compare(row: TestResultsRow) -> int: | |||
return rows[left:] | |||
|
|||
|
|||
def get_relevant_totals( |
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.
love this refactor!
|
||
flaky_test_ids = set([flake.test_id for flake in flakes]) | ||
flaky_test_ids = ( |
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.
being able to have a generalized pattern for all these makes it so much easier to read
utils/test_results.py
Outdated
Q(repository_id=repoid) | ||
& (Q(end_date__date__isnull=True) | Q(end_date__date__gt=since)) | ||
) | ||
totals = get_relevant_totals(repoid, branch, since) |
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.
Reading it this way now, we could also bump totals up to line 372 and use it for each case now right?
Vs. having it be the "starting line" in each of our case statements below
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.
couple comments, nothing blocking
No description provided.