fix: Remove duplicate names for resolvers and tests #527

ajay-sentry · 2024-04-29T20:46:09Z

Purpose/Motivation

When investigating another ticket, I saw that in our repo we sometimes used the same function name for multiple resolvers. This seemed like a clear code error, but then I realized that the resolver "bindable_field" decorator was added to things to maintain that uniqueness.

However, one place this wasn't accounted for was in tests. And I found some surprising results post fixing the duplicate test names. That the tests with duplicate names were not running. This seemed pretty concerning, since after fixing all the duplicate test names I saw there were 8 failing tests; ranging from a few months to a few years old.

This also made me realize that we don't have a linter on API (or worker / shared if I was looking at the code correctly), and might be something we want to look into adding sooner rather than later.

Anyway, this PR aims to fix all the duplicate function name errors in API, though I may need a bit of help with the remaining failing tests to see if they're still needed or not. My thinking is 3 of the 5 aren't needed anymore? In test_branch.py, and test_pull_comparison.py

Screenshots

AFTER Removing the duplicate function names

BEFORE

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.

ajay-sentry · 2024-04-29T20:53:31Z

api/internal/tests/views/test_coverage_viewset.py

@@ -333,24 +333,6 @@ def test_tree_not_found_for_components(
        res = self._tree(components="Does_not_exist")
        assert res.status_code == 404

-    @patch("shared.reports.api_report_service.build_report_from_commit")
-    @patch("services.components.commit_components")
-    def test_tree_not_found_for_components(


This test looked to be an exact copy of the one above it

ajay-sentry · 2024-04-29T20:53:45Z

codecov_auth/commands/owner/tests/test_owner.py

@@ -56,7 +56,7 @@ def test_save_terms_agreement_delegate_to_interactor(self, interactor_mock):
        interactor_mock.assert_called_once_with(input_dict)

    @patch("codecov_auth.commands.owner.owner.StartTrialInteractor.execute")
-    def test_cancel_trial_delegate_to_interactor(self, interactor_mock):


For all the resolvers, I just named them what the property was

These changes weren't necessary though; just a cleanliness thing

ajay-sentry · 2024-04-29T21:29:59Z

graphql_api/tests/test_branch.py

@@ -628,36 +628,7 @@ def test_fetch_path_contents_unknown_path(
        }

    @patch("services.report.build_report_from_commit")
-    def test_fetch_path_contents_unknown_flags(self, report_mock):


this looked to be a duplicate of the below test that was never updated

ajay-sentry · 2024-04-29T21:30:17Z

graphql_api/tests/test_pull_comparison.py

@@ -979,68 +979,6 @@ def test_pull_comparison_no_comparison(self, compute_comparisons_mock):

        compute_comparisons_mock.assert_called_once

-    def test_pull_comparison_missing_head_report(self):


This and the following test are using deprecated patterns

ajay-sentry · 2024-04-29T21:30:30Z

upload/tests/test_upload.py

-        expected_response = {"vcs_revision": "c739768fcac68144a3a6d82305b9c4106934d31a"}
+        expected_response = {
+            "vcs_revision": "c739768fcac68144a3a6d82305b9c4106934d31a",
+            "stop_time": "",


test was missing stop_time

ajay-sentry · 2024-04-29T21:30:49Z

services/tests/test_components.py

@@ -153,9 +153,9 @@ def test_head_report(self, base_report_mock):
            }
        )
        component_comparison = ComponentComparison(self.comparison, component_go)
-        assert component_comparison.head_report.files == ["file_1.go"]
+        assert component_comparison.base_report.files == ["file_1.go"]


test wasn't actually looking at base_report

codecov-qa · 2024-04-29T21:39:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.50%. Comparing base (9d70756) to head (fd6a77a).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
+ Coverage   91.48%   91.50%   +0.01%     
==========================================
  Files         599      599              
  Lines       16271    16271              
==========================================
+ Hits        14885    14888       +3     
+ Misses       1386     1383       -3

Flag	Coverage Δ
unit	`91.50% <100.00%> (+0.01%)`	⬆️
unit-latest-uploader	`91.50% <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.

codecov-public-qa · 2024-04-29T21:39:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.50%. Comparing base (9d70756) to head (fd6a77a).

✅ All tests successful. No failed tests found ☺️

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
+ Coverage   91.48%   91.50%   +0.01%     
==========================================
  Files         599      599              
  Lines       16271    16271              
==========================================
+ Hits        14885    14888       +3     
+ Misses       1386     1383       -3

Flag	Coverage Δ
unit	`91.50% <100.00%> (+0.01%)`	⬆️
unit-latest-uploader	`91.50% <100.00%> (+0.01%)`	⬆️

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

Files	Coverage Δ
graphql_api/types/bundle_analysis/base.py	`100.00% <100.00%> (ø)`
graphql_api/types/bundle_analysis/comparison.py	`100.00% <100.00%> (ø)`
graphql_api/types/comparison/comparison.py	`100.00% <100.00%> (ø)`
graphql_api/types/file/file.py	`100.00% <100.00%> (ø)`
graphql_api/types/me/me.py	`100.00% <100.00%> (ø)`
graphql_api/types/plan/plan.py	`98.33% <100.00%> (ø)`
services/bundle_analysis.py	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

codecov · 2024-04-29T21:43:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (9d70756) to head (fd6a77a).

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    #527     +/-   ##
=======================================
+ Coverage   95.78   95.80   +0.02     
=======================================
  Files        774     774             
  Lines      17127   17127             
=======================================
+ Hits       16404   16407      +3     
+ Misses       723     720      -3

Flag	Coverage Δ
unit	`91.50% <100.00%> (+0.01%)`	⬆️
unit-latest-uploader	`91.50% <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.

nora-codecov

I agree with the linter issue you raised in the description of this pr - it's standard to have one, we should have one.

nora-codecov · 2024-04-30T00:37:17Z

api/internal/tests/views/test_coverage_viewset.py

@@ -333,24 +333,6 @@ def test_tree_not_found_for_components(
        res = self._tree(components="Does_not_exist")
        assert res.status_code == 404

-    @patch("shared.reports.api_report_service.build_report_from_commit")
-    @patch("services.components.commit_components")
-    def test_tree_not_found_for_components(


fix most of tests, put up for review and enlist some help

abeed3c

ajay-sentry requested review from a team as code owners April 29, 2024 20:46

ajay-sentry commented Apr 29, 2024

View reviewed changes

fix rest of tests

fd6a77a

ajay-sentry commented Apr 29, 2024

View reviewed changes

nora-codecov approved these changes Apr 30, 2024

View reviewed changes

JerrySentry approved these changes May 1, 2024

View reviewed changes

ajay-sentry added this pull request to the merge queue May 1, 2024

Merged via the queue into main with commit 7433b28 May 1, 2024
19 checks passed

ajay-sentry deleted the Ajay/duplicate-fn-fixes branch May 1, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove duplicate names for resolvers and tests #527

fix: Remove duplicate names for resolvers and tests #527

ajay-sentry commented Apr 29, 2024 •

edited

Loading

ajay-sentry Apr 29, 2024

nora-codecov Apr 30, 2024

ajay-sentry Apr 29, 2024

ajay-sentry Apr 29, 2024

ajay-sentry Apr 29, 2024

ajay-sentry Apr 29, 2024

ajay-sentry Apr 29, 2024

ajay-sentry Apr 29, 2024

codecov-qa bot commented Apr 29, 2024

codecov-public-qa bot commented Apr 29, 2024

codecov bot commented Apr 29, 2024

nora-codecov left a comment

nora-codecov Apr 30, 2024

		@@ -979,68 +979,6 @@ def test_pull_comparison_no_comparison(self, compute_comparisons_mock):

		compute_comparisons_mock.assert_called_once

		def test_pull_comparison_missing_head_report(self):

fix: Remove duplicate names for resolvers and tests #527

fix: Remove duplicate names for resolvers and tests #527

Conversation

ajay-sentry commented Apr 29, 2024 • edited Loading

Purpose/Motivation

Screenshots

Legal Boilerplate

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

codecov-qa bot commented Apr 29, 2024

Codecov Report

codecov-public-qa bot commented Apr 29, 2024

Codecov Report

codecov bot commented Apr 29, 2024

Codecov Report

nora-codecov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajay-sentry commented Apr 29, 2024 •

edited

Loading