Skip to content
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

Improve aggregate test results function #813

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

joseph-sentry
Copy link
Contributor

@joseph-sentry joseph-sentry commented Sep 10, 2024

This PR does 2 things:

  • add the lastDuration field to the TestResult GQL model
  • use the DailyTestRollup object to query for aggregates, this is being done for perf reasons

depends on: codecov/worker#699

this commit also adds the LastDuration field to the TestResult
GQL model. The reason we're using latest_run and not the updated_at
field of the object is because in the future we want to parse the
timestamp from the JUnit XML file so latest_run will be tied to that
and not the time at which we process the test results.
Copy link
Contributor

This PR includes changes to shared. Please review them here: codecov/shared@d4ceb0e...5cc5f48

@codecov-notifications
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.24%. Comparing base (c2a3149) to head (5f3decc).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@               Coverage Diff                @@
##               main       #813        +/-   ##
================================================
+ Coverage   96.23000   96.24000   +0.01000     
================================================
  Files           812        812                
  Lines         18567      18596        +29     
================================================
+ Hits          17868      17897        +29     
  Misses          699        699                
Flag Coverage Δ
unit 92.47% <100.00%> (+0.01%) ⬆️
unit-latest-uploader 92.47% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -893,14 +892,15 @@ def test_test_results_no_tests(self) -> None:
def test_branch_filter_on_test_results(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am I missing something or are there no assertions on this test and the next two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the github file viewer is just not showing them, they are there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh weird, yeah I see them now

requirements.in Outdated
@@ -45,7 +45,7 @@ pytz
redis
regex
requests
sentry-sdk>=2.13.0
sentry-sdk>=1.40.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intended? :O

class Array(Aggregate):
function = "array"


def aggregate_test_results(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to update the doc for this fn

When(outcome__in=["failure", "error"], then=Value(1.0)),
totals = DailyTestRollup.objects.filter(repoid=repoid, date__gt=time_ago)

print([(t.pass_count, t.fail_count) for t in totals], branch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still want this? :P

)

return failure_rates_queryset
print(aggregation_of_test_results.query)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still need this? :P


aggregation_of_test_results = totals.values("test").annotate(
failure_rate=(
Cast(Sum(F("fail_count")), output_field=FloatField())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Cast(Sum(F("fail_count"))) get cached when we do this calc twice? I don't have an idea of how we'd simplify, mostly curious if you knew

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would expect postgres to take care of this in a performant way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough

Copy link
Contributor

@ajay-sentry ajay-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants