dev: Optimizations / Dead code removal for comparison class #725

ajay-sentry · 2024-07-30T22:37:30Z

Purpose/Motivation

Looking to ease a bit of the comparison class pain that occurs anytime we fetch aspects related to this model, including "ImpactedFiles" and their properties, "compareWithBase" and related relations from the pull model, or aspects from the comparison itself, like "hasDifferentNumberOfHeadAndBaseReports."

This PR does a few things:

Try catch when Github API call fails
Removes the second Github API call when fetching calling the git_comparison fn, as well as related serializer since it looked to be unused
Adds some typing and type casting to make sure we aren't accidentally shooting ourselves in the foot for some cases.

Links to relevant tickets

Notes to Reviewer

Anything to note to the team? Any tips on how to review, or where to start?

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.

codecov-staging · 2024-07-30T22:47:20Z

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
services/comparison.py	77.77%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

codecov-qa · 2024-07-30T22:47:25Z

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.72%. Comparing base (183451b) to head (8f9df8f).

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
- Coverage   91.73%   91.72%   -0.02%     
==========================================
  Files         632      632              
  Lines       17056    17054       -2     
==========================================
- Hits        15646    15642       -4     
- Misses       1410     1412       +2

Flag	Coverage Δ
unit	`91.72% <80.00%> (-0.02%)`	⬇️
unit-latest-uploader	`91.72% <80.00%> (-0.02%)`	⬇️

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

Files	Coverage Δ
api/shared/compare/serializers.py	`100.00% <ø> (ø)`
graphql_api/types/comparison/comparison.py	`100.00% <ø> (ø)`
graphql_api/types/config/config.py	`100.00% <100.00%> (ø)`
services/comparison.py	`98.67% <77.77%> (-0.34%)`	⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

codecov-public-qa · 2024-07-30T22:47:30Z

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.72%. Comparing base (183451b) to head (8f9df8f).

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
- Coverage   91.73%   91.72%   -0.02%     
==========================================
  Files         632      632              
  Lines       17056    17054       -2     
==========================================
- Hits        15646    15642       -4     
- Misses       1410     1412       +2

Flag	Coverage Δ
unit	`91.72% <80.00%> (-0.02%)`	⬇️
unit-latest-uploader	`91.72% <80.00%> (-0.02%)`	⬇️

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

Files	Coverage Δ
api/shared/compare/serializers.py	`100.00% <ø> (ø)`
graphql_api/types/comparison/comparison.py	`100.00% <ø> (ø)`
graphql_api/types/config/config.py	`100.00% <100.00%> (ø)`
services/comparison.py	`98.67% <77.77%> (-0.34%)`	⬇️

codecov · 2024-07-30T22:48:33Z

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.02%. Comparing base (47e6cee) to head (e3ffde6).

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
services/comparison.py	77.77%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##               main       #725        +/-   ##
================================================
- Coverage   96.03000   96.02000   -0.01000     
================================================
  Files           814        814                
  Lines         18430      18428         -2     
================================================
- Hits          17699      17695         -4     
- Misses          731        733         +2

Flag	Coverage Δ
unit	`91.73% <80.00%> (-0.02%)`	⬇️
unit-latest-uploader	`91.73% <80.00%> (-0.02%)`	⬇️

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.

ajay-sentry · 2024-07-31T23:28:18Z

codecov_auth/commands/owner/interactors/tests/test_save_terms_agreement.py

@@ -42,7 +42,7 @@ def test_update_user_when_agreement_is_false(self):
        assert self.current_user.terms_agreement_at == self.updated_at

        self.current_user.refresh_from_db()
-        self.current_user.email == before_refresh_business_email
+        assert self.current_user.email == before_refresh_business_email


some added assertions here

ajay-sentry · 2024-07-31T23:28:28Z

graphql_api/tests/test_config.py

@@ -272,7 +272,7 @@ def test_self_hosted_license_returns_null_if_invalid_license(self, license_mock)
        )
        assert data == {
            "config": {
-                "selfHostedLicense": {"expirationDate": None},
+                "selfHostedLicense": None,


This is the correct test I think

ajay-sentry · 2024-07-31T23:28:59Z

services/tests/test_comparison.py

@@ -1318,22 +1318,6 @@ def setUp(self):
        self.comparison = Comparison(user=owner, base_commit=base, head_commit=head)
        asyncio.set_event_loop(asyncio.new_event_loop())

-    def test_returns_true_if_reverse_comparison_has_commits(self, get_adapter_mock):


These aren't needed anymore since we removed the param

ajay-sentry · 2024-07-31T23:29:04Z

timeseries/tests/test_admin.py

@@ -64,7 +64,7 @@ def test_perform_backfill(self, backfill_dataset):
        )
        assert res.status_code == 302

-        backfill_dataset.call_count == 2
+        assert backfill_dataset.call_count == 2


added assertion

ajay-sentry · 2024-07-31T23:29:11Z

services/tests/test_billing.py

@@ -893,9 +893,11 @@ def test_get_proration_params(self):
            plan=PlanName.CODECOV_PRO_MONTHLY.value, plan_user_count=20
        )
        desired_plan = {"value": PlanName.SENTRY_MONTHLY.value, "quantity": 19}
-        self.stripe._get_proration_params(owner, desired_plan) == "none"
+        assert self.stripe._get_proration_params(owner, desired_plan) == "none"


added assertion

ajay-sentry · 2024-07-31T23:29:43Z

services/comparison.py

@@ -953,7 +940,10 @@ def change_coverage(self) -> Optional[float]:
            and self.head_coverage
            and self.head_coverage.coverage
        ):
-            return float(self.head_coverage.coverage - self.base_coverage.coverage)
+            return float(


This could result in some weird behavior if head_coverage.coverage was a non-float/int value

Since you can't subtract 2 strings

ajay-sentry · 2024-07-31T23:33:24Z

services/comparison.py

@@ -774,12 +777,8 @@ def _fetch_comparison_and_reverse_comparison(self):
            self.base_commit.commitid, self.head_commit.commitid
        )

-        reverse_comparison_coro = adapter.get_compare(


This other routine was doing an extra Github API call every time we needed a fresh version of this property (assuming the caching worked to begin with)

Hmmm how do we know we don't need this still?

The return value from this thing was only being used in "has_unmerged_base_commits" which didn't seem to be used anywhere

ajay-sentry · 2024-07-31T23:33:41Z

services/comparison.py

@@ -791,18 +790,6 @@ def non_carried_forward_flags(self):
        flags_dict = self.head_report.flags
        return [flag for flag, vals in flags_dict.items() if not vals.carriedforward]

-    @cached_property
-    def has_unmerged_base_commits(self):


This didn't seem to be used anywhere across gazebo, worker, api, or shared

sentry-io · 2024-08-05T17:04:26Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ SystemExit: 1 GraphQL [Pull] View Issue
‼️ SystemExit: 1 GraphQL [Commit] View Issue

_{Did you find this useful? React with a 👍 or 👎}

ajay-sentry added 3 commits July 30, 2024 14:51

add try pass when doing git comparison

c640fbd

Merge branch 'main' into Ajay/exception-handling-git-diff

1f6881e

remove unused serializer and coroutine, some type updates

ba6d9f8

ajay-sentry requested a review from a team as a code owner July 30, 2024 22:37

ajay-sentry added 7 commits July 31, 2024 14:05

update tests

0c8f239

index off 0

f02ce9e

Merge branch 'main' into Ajay/comparison-gql-optimizations

7c6e95b

change to float

d66ac68

fix assertions in other tests, remove print

54adf0e

change back the non-trivial one

cd1d314

remove xmls

8f9df8f

ajay-sentry commented Jul 31, 2024

View reviewed changes

ajay-sentry changed the title ~~dev: Optimizations for comparison class~~ dev: Optimizations / Dead code removal for comparison class Jul 31, 2024

ajay-sentry commented Jul 31, 2024

View reviewed changes

Merge branch 'main' into Ajay/comparison-gql-optimizations

e3ffde6

ajay-sentry temporarily deployed to staging August 2, 2024 19:39 — with GitHub Actions Inactive

adrian-codecov approved these changes Aug 2, 2024

View reviewed changes

ajay-sentry added this pull request to the merge queue Aug 2, 2024

Merged via the queue into main with commit e245d32 Aug 2, 2024
26 of 31 checks passed

ajay-sentry deleted the Ajay/comparison-gql-optimizations branch August 2, 2024 20:22

ajay-sentry mentioned this pull request Aug 5, 2024

Comparison.py Enhancements codecov/engineering-team#2202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev: Optimizations / Dead code removal for comparison class #725

dev: Optimizations / Dead code removal for comparison class #725

ajay-sentry commented Jul 30, 2024

codecov-staging bot commented Jul 30, 2024 •

edited by codecov-notifications bot

Loading

codecov-qa bot commented Jul 30, 2024 •

edited

Loading

codecov-public-qa bot commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

ajay-sentry Jul 31, 2024

adrian-codecov Aug 1, 2024

ajay-sentry Aug 2, 2024

ajay-sentry Jul 31, 2024

sentry-io bot commented Aug 5, 2024 •

edited

Loading

dev: Optimizations / Dead code removal for comparison class #725

dev: Optimizations / Dead code removal for comparison class #725

Conversation

ajay-sentry commented Jul 30, 2024

Purpose/Motivation

Links to relevant tickets

Notes to Reviewer

Legal Boilerplate

codecov-staging bot commented Jul 30, 2024 • edited by codecov-notifications bot Loading

Codecov Report

codecov-qa bot commented Jul 30, 2024 • edited Loading

Codecov Report

codecov-public-qa bot commented Jul 30, 2024 • edited Loading

Codecov Report

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Aug 5, 2024 • edited Loading

Suspect Issues

codecov-staging bot commented Jul 30, 2024 •

edited by codecov-notifications bot

Loading

codecov-qa bot commented Jul 30, 2024 •

edited

Loading

codecov-public-qa bot commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

sentry-io bot commented Aug 5, 2024 •

edited

Loading