-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add sync test results task #501
base: main
Are you sure you want to change the base?
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: tasks/test_results_processor.py
Did you find this useful? React with a 👍 or 👎 |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #501 +/- ##
==========================================
+ Coverage 97.49% 97.55% +0.06%
==========================================
Files 418 421 +3
Lines 35009 35125 +116
==========================================
+ Hits 34131 34266 +135
+ Misses 878 859 -19
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 #501 +/- ##
==========================================
+ Coverage 97.49% 97.55% +0.06%
==========================================
Files 418 421 +3
Lines 35009 35125 +116
==========================================
+ Hits 34131 34266 +135
+ Misses 878 859 -19
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. Additional details and impacted files@@ Coverage Diff @@
## main #501 +/- ##
==========================================
+ Coverage 97.51% 97.57% +0.06%
==========================================
Files 449 452 +3
Lines 35732 35848 +116
==========================================
+ Hits 34844 34979 +135
+ Misses 888 869 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
This change has been scanned for critical changes. Learn more |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #501 +/- ##
==========================================
+ Coverage 97.49% 97.55% +0.06%
==========================================
Files 418 421 +3
Lines 35009 35125 +116
==========================================
+ Hits 34131 34266 +135
+ Misses 878 859 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
tasks/backfill_test_instances.py
Outdated
branch=None, | ||
commitid=None, | ||
test_instance_list = ( | ||
TestInstance.objects.select_related("upload__report__commit") |
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.
Good to see we're using the django models! 🥇
tasks/backfill_test_instances.py
Outdated
for test_instance in chunk: | ||
for i in range(0, test_instance_list.count(), 1000): | ||
updates = [] | ||
thing = ( |
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.
Could we rename this to something more meaningful 😅, seems to me this is something like "test_instances_without_identifiers" or something like that?
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.
oops 😅, test_instances_without_identifiers
sounds good
tasks/sync_test_results.py
Outdated
|
||
log = logging.getLogger(__name__) | ||
|
||
# TODO: turn these into Django ORM calls |
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.
I think it should be this or close to this
from django.db.models import Avg, Case, When, Value
from django.db.models.functions import Now
from django.utils import timezone
from datetime import timedelta
... some code
thirty_days_ago = timezone.now() - timedelta(days=30)
failure_rate_queryset = TestInstance.objects.filter(
repoid=<repoid>,
created_at__gt=thirty_days_ago,
outcome__in=['pass', 'failure', 'error']
).values('test_id').annotate(
failure_rate=Avg(
Case(
When(outcome='pass', then=Value(0.0)),
When(outcome__in=['failure', 'error'], then=Value(1.0)),
output_field=FloatField()
)
)
)
commit_agg_queryset = TestInstance.objects.filter(
repoid=<repoid>,
created_at__gt=thirty_days_ago,
outcome__in=['failure', 'error']
).values('test_id').annotate(
commits=ArrayAgg('commitid', distinct=True)
)
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.
Have we tested this in staging? Idk if we have test instances there though
I think we do have test instances on there, I will see if I can test it out on there |
53d3d60
to
bf10be5
Compare
8c41bce
to
034fd8d
Compare
failure rate and commits where failed will be populated by the sync test resuls task we are denormalizing the repoid onto the test instance model for perf reasons
This task should be run when we want to update the failure rate and commits where failed in the Test model.
this task will populate the fields of test instances that are missing information since they were created before we added the fields
Signed-off-by: joseph-sentry <[email protected]>
Signed-off-by: joseph-sentry <[email protected]>
440f47f
to
d6d892b
Compare
Signed-off-by: joseph-sentry <[email protected]>
Depends on: codecov/shared#247
Fixes: codecov/engineering-team#1648 and codecov/engineering-team#1647
Requires risky migration from codecov/shared@05641f5 to be run