feat: Create coverage single upload endpoint #962

tony-codecov · 2024-11-05T23:25:41Z

Purpose/Motivation

Closes codecov/engineering-team#2536.

This PR:

Extracts logic from CommitViews, ReportViews and UploadViews into functions.
Creates a new CombinedUploadViews that handles the full upload flow in a single endpoint, utilizing functions extracted from the views previously.
Avoids code duplication in Combined Upload view
Updates URL routing for new combined-upload endpoint

Links to relevant tickets

What does this PR do?

Include a brief description of the changes in this PR. Bullet points are your friend.

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-notifications · 2024-11-05T23:47:46Z

Codecov Report

Attention: Patch coverage is 97.43590% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
upload/views/combined_upload.py	96.61%	2 Missing ⚠️
upload/views/uploads.py	97.50%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

codecov · 2024-11-05T23:47:47Z

Codecov Report

Attention: Patch coverage is 97.43590% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.06%. Comparing base (aa55f93) to head (2100332).
Report is 9 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
upload/views/combined_upload.py	96.61%	2 Missing ⚠️
upload/views/uploads.py	97.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #962      +/-   ##
==========================================
+ Coverage   96.03%   96.06%   +0.02%     
==========================================
  Files         827      828       +1     
  Lines       19071    19140      +69     
==========================================
+ Hits        18315    18386      +71     
+ Misses        756      754       -2

Flag	Coverage Δ
unit	`92.32% <97.43%> (+0.04%)`	⬆️
unit-latest-uploader	`92.32% <97.43%> (+0.04%)`	⬆️

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-qa · 2024-11-07T02:59:43Z

❌ 6 Tests Failed:

Tests completed	Failed	Passed	Skipped
2671	6	2665	6

View the top 3 failed tests by shortest run time

upload/tests/views/test_combined_upload.py::TestCombinedUpload::test_combined_upload_tokenless[False-someone/fork:main]

Stack Traces | 0.017s run time

self = &lt;upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322bb2f0&gt;
db = None, branch = 'someone/fork:main', private = False

    @pytest.mark.parametrize("branch", ["main", "someone:main", "someone/fork:main"])
    @pytest.mark.parametrize("private", [True, False])
    def test_combined_upload_tokenless(self, db, branch, private):
        repository = RepositoryFactory(
            private=private, author__username="codecov", name="the_repo"
        )
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
    
        upload_data = {
            "commit_sha": "abc123",
            "branch": branch,
            "code": "coverage-data",
        }
    
        client = APIClient()
        response = client.post(url, upload_data, format="json")
    
        if ":" in branch and private == False:
&gt;           assert response.status_code == 201
E           assert 401 == 201
E            +  where 401 = &lt;Response status_code=401, "application/json"&gt;.status_code

.../tests/views/test_combined_upload.py:141: AssertionError

upload/tests/views/test_combined_upload.py::TestCombinedUpload::test_get_repo_not_found

Stack Traces | 0.018s run time

self = &lt;upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322b9610&gt;
db = None

    def test_get_repo_not_found(self, db):
        repository = RepositoryFactory(
            name="the_repo", author__username="codecov", author__service="github"
        )
        repo_slug = "codecov::::wrong-repo-name"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
        client = APIClient()
        response = client.post(url, {}, format="json")
&gt;       assert response.status_code == 400
E       assert 401 == 400
E        +  where 401 = &lt;Response status_code=401, "application/json"&gt;.status_code

.../tests/views/test_combined_upload.py:45: AssertionError

upload/tests/views/test_combined_upload.py::TestCombinedUpload::test_combined_upload_tokenless[False-someone:main]

Stack Traces | 0.02s run time

self = &lt;upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322bb200&gt;
db = None, branch = 'someone:main', private = False

    @pytest.mark.parametrize("branch", ["main", "someone:main", "someone/fork:main"])
    @pytest.mark.parametrize("private", [True, False])
    def test_combined_upload_tokenless(self, db, branch, private):
        repository = RepositoryFactory(
            private=private, author__username="codecov", name="the_repo"
        )
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
    
        upload_data = {
            "commit_sha": "abc123",
            "branch": branch,
            "code": "coverage-data",
        }
    
        client = APIClient()
        response = client.post(url, upload_data, format="json")
    
        if ":" in branch and private == False:
&gt;           assert response.status_code == 201
E           assert 401 == 201
E            +  where 401 = &lt;Response status_code=401, "application/json"&gt;.status_code

.../tests/views/test_combined_upload.py:141: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

codecov-public-qa · 2024-11-07T02:59:46Z

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 2677 tests with 6 failed, 2665 passed and 6 skipped.

View the full list of failed tests

pytest

Class name: upload.tests.views.test_combined_upload.TestCombinedUpload
Test name: test_combined_upload_tokenless[False-someone/fork:main]

self = <upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322bb2f0>
db = None, branch = 'someone/fork:main', private = False

    @pytest.mark.parametrize("branch", ["main", "someone:main", "someone/fork:main"])
    @pytest.mark.parametrize("private", [True, False])
    def test_combined_upload_tokenless(self, db, branch, private):
        repository = RepositoryFactory(
            private=private, author__username="codecov", name="the_repo"
        )
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
    
        upload_data = {
            "commit_sha": "abc123",
            "branch": branch,
            "code": "coverage-data",
        }
    
        client = APIClient()
        response = client.post(url, upload_data, format="json")
    
        if ":" in branch and private == False:
>           assert response.status_code == 201
E           assert 401 == 201
E            +  where 401 = <Response status_code=401, "application/json">.status_code

.../tests/views/test_combined_upload.py:141: AssertionError

Class name: upload.tests.views.test_combined_upload.TestCombinedUpload
Test name: test_combined_upload_tokenless[False-someone:main]

self = <upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322bb200>
db = None, branch = 'someone:main', private = False

    @pytest.mark.parametrize("branch", ["main", "someone:main", "someone/fork:main"])
    @pytest.mark.parametrize("private", [True, False])
    def test_combined_upload_tokenless(self, db, branch, private):
        repository = RepositoryFactory(
            private=private, author__username="codecov", name="the_repo"
        )
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
    
        upload_data = {
            "commit_sha": "abc123",
            "branch": branch,
            "code": "coverage-data",
        }
    
        client = APIClient()
        response = client.post(url, upload_data, format="json")
    
        if ":" in branch and private == False:
>           assert response.status_code == 201
E           assert 401 == 201
E            +  where 401 = <Response status_code=401, "application/json">.status_code

.../tests/views/test_combined_upload.py:141: AssertionError

Class name: upload.tests.views.test_combined_upload.TestCombinedUpload
Test name: test_combined_upload_with_errors

self = <upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322bb9b0>
db = None

    def test_combined_upload_with_errors(self, db):
        repository = RepositoryFactory()
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
    
        client = APIClient()
        client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token)
    
        # Missing required fields
        response = client.post(url, {}, format="json")
        assert response.status_code == 400
>       assert "commit_sha" in response.json()
E       assert 'commit_sha' in {'commitid': ['This field may not be null.']}
E        +  where {'commitid': ['This field may not be null.']} = functools.partial(<bound method ClientMixin._parse_json of <rest_framework.test.APIClient object at 0x7f09b4dae510>>, <Response status_code=400, "application/json">)()
E        +    where functools.partial(<bound method ClientMixin._parse_json of <rest_framework.test.APIClient object at 0x7f09b4dae510>>, <Response status_code=400, "application/json">) = <Response status_code=400, "application/json">.json

.../tests/views/test_combined_upload.py:162: AssertionError

Class name: upload.tests.views.test_combined_upload.TestCombinedUpload
Test name: test_get_repo

self = <upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322bbc50>
db = None

    def test_get_repo(self, db):
        repository = RepositoryFactory(
            name="the_repo", author__username="codecov", author__service="github"
        )
        repository.save()
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
        client = APIClient()
        client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token)
        response = client.post(url, {}, format="json")
        assert response.status_code == 400  # Bad request due to missing required fields
>       assert "commit_sha" in response.json()
E       assert 'commit_sha' in {'commitid': ['This field may not be null.']}
E        +  where {'commitid': ['This field may not be null.']} = functools.partial(<bound method ClientMixin._parse_json of <rest_framework.test.APIClient object at 0x7f09c44f5970>>, <Response status_code=400, "application/json">)()
E        +    where functools.partial(<bound method ClientMixin._parse_json of <rest_framework.test.APIClient object at 0x7f09c44f5970>>, <Response status_code=400, "application/json">) = <Response status_code=400, "application/json">.json

.../tests/views/test_combined_upload.py:32: AssertionError

Class name: upload.tests.views.test_combined_upload.TestCombinedUpload
Test name: test_get_repo_not_found

self = <upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322b9610>
db = None

    def test_get_repo_not_found(self, db):
        repository = RepositoryFactory(
            name="the_repo", author__username="codecov", author__service="github"
        )
        repo_slug = "codecov::::wrong-repo-name"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
        client = APIClient()
        response = client.post(url, {}, format="json")
>       assert response.status_code == 400
E       assert 401 == 400
E        +  where 401 = <Response status_code=401, "application/json">.status_code

.../tests/views/test_combined_upload.py:45: AssertionError

Class name: upload.tests.views.test_combined_upload.TestCombinedUpload
Test name: test_successful_combined_upload

self = <upload.tests.views.test_combined_upload.TestCombinedUpload object at 0x7f0a322ba330>
mock_update_commit = <MagicMock name='update_commit' id='139679667421648'>
db = None

    @patch("services.task.TaskService.update_commit")
    def test_successful_combined_upload(self, mock_update_commit, db):
        repository = RepositoryFactory(
            name="the_repo", author__username="codecov", author__service="github"
        )
        repository.save()
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.combined_upload",
            args=[repository.author.service, repo_slug],
        )
    
        upload_data = {
            "commit_sha": "abc123",
            "branch": "main",
            "pull_request_number": "42",
            "code": "coverage-data",
            "build_code": "build-1",
            "build_url": "http://ci.test/build/1",
            "job_code": "job-1",
            "flags": ["unit", "integration"],
            "name": "Upload 1",
        }
    
        client = APIClient()
        client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token)
        response = client.post(url, upload_data, format="json")
    
>       assert response.status_code == 201
E       assert 400 == 201
E        +  where 400 = <Response status_code=400, "application/json">.status_code

.../tests/views/test_combined_upload.py:96: AssertionError

github-actions · 2024-11-08T18:04:38Z

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

Swatinem

the tests so far only cover error cases. it would be nice to actually test the whole logic as well :-)

otherwise, I personally dislike mixins, but if that fits well within the codestyle of the repo, thats fine as well.

reducing the metrics signal/noise ratio would also be nice to make the actual code more readable.

upload/tests/views/test_combined_upload.py

Swatinem · 2024-11-11T13:33:28Z

upload/tests/views/test_combined_upload.py

+        client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token)
+
+        # Missing required fields
+        response = client.post(url, {}, format="json")


this is the same as test_get_repo, isn’t it?

I’m fine with checking multiple edge cases in a single test, though the other opinion to only test a single thing in one test is also fine.
Either way, you shouldn’t have the worst of both worlds :-D

Some of the tests are reused from test_uploads. I have added 2 more tests for the successful case, one with and one without shelter headers in the API request.

upload/views/commits.py

upload/views/uploads.py

upload/views/combined_upload.py

Swatinem

nice, this looks good, though probably someone else from @codecov/api should also review/approve this.

I have two smaller improvements, otherwise this could sure use some type annotations. And the PR description still mentions mixins, so you might want to update that.

upload/views/uploads.py

Swatinem · 2024-11-12T14:09:41Z

upload/tests/views/test_combined_upload.py

+    assert RepositoryFlag.objects.filter(
+        repository_id=repository.repoid, flag_name="flag1"
+    ).exists()
+    assert RepositoryFlag.objects.filter(
+        repository_id=repository.repoid, flag_name="flag2"
+    ).exists()
+    flag1 = RepositoryFlag.objects.filter(
+        repository_id=repository.repoid, flag_name="flag1"
+    ).first()
+    flag2 = RepositoryFlag.objects.filter(
+        repository_id=repository.repoid, flag_name="flag2"
+    ).first()
+    assert UploadFlagMembership.objects.filter(
+        report_session_id=upload.id, flag_id=flag1.id
+    ).exists()
+    assert UploadFlagMembership.objects.filter(
+        report_session_id=upload.id, flag_id=flag2.id
+    ).exists()
+    assert [flag for flag in upload.flags.all()] == [flag1, flag2]


instead of these exists calls, I would just assert that the flags are properly attached:

assert {flag.name for flag in upload.flags.all()} == {"flag1", "flag2"}

We shouldn’t care about the implementation details of how the database manages its relations and joins. We care that our upload has the proper flags.

tony-codecov added 2 commits November 5, 2024 18:16

Initial commit

d58c40e

Add back validate_activated_repo

af557db

Combined Upload v1

8e1560b

tony-codecov mentioned this pull request Nov 6, 2024

Upload-process uses upload-coverage endpoint codecov/codecov-cli#551

Open

Linting

541451a

tony-codecov added 4 commits November 6, 2024 22:42

Fix tests

a0a5bc4

Lints

c5ce5d3

Add throttling for combined-upload

3f74fa4

Merge branch 'main' into tony/coverage-single-upload

a4a5466

tony-codecov added 4 commits November 8, 2024 13:06

Better code

87e2a2b

Add metrics

7987e60

Use updated repository obj

deb6e15

Lints

fd7dcf4

tony-codecov force-pushed the tony/coverage-single-upload branch from 2f70222 to fd7dcf4 Compare November 8, 2024 18:36

tony-codecov marked this pull request as ready for review November 8, 2024 18:43

tony-codecov requested a review from a team as a code owner November 8, 2024 18:43

Swatinem reviewed Nov 11, 2024

View reviewed changes

thomasrockhu-codecov mentioned this pull request Nov 11, 2024

Inconsistent - Token required because branch is protected codecov/codecov-action#1580

Closed

tony-codecov added 6 commits November 12, 2024 03:28

Use emit_metrics for better readability

001c993

Return early if broken upload

24f9c94

Add external_id and created_at outputs for combined_upload

b527507

Add successful combined_upload test for normal use case and shelter

93b0f48

Merge branch 'main' into tony/coverage-single-upload

a99b9f2

Use functions instead of Mix ins

52ce8a4

tony-codecov requested a review from Swatinem November 12, 2024 11:46

Swatinem approved these changes Nov 12, 2024

View reviewed changes

tony-codecov added 2 commits November 12, 2024 10:24

Merge branch 'main' into tony/coverage-single-upload

be72579

Use analytics_token instead of token

6becd24

tony-codecov requested a review from a team November 12, 2024 15:30

Lints

2100332

tony-codecov added this pull request to the merge queue Nov 13, 2024

Merged via the queue into main with commit 6fa33eb Nov 13, 2024
18 of 19 checks passed

tony-codecov deleted the tony/coverage-single-upload branch November 13, 2024 17:28

drazisil-codecov mentioned this pull request Nov 21, 2024

[CLI/API] When the CLI command empty-upload --force is used on a repo with carry forward flags, the notifications fail codecov/engineering-team#2365

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create coverage single upload endpoint #962

feat: Create coverage single upload endpoint #962

tony-codecov commented Nov 5, 2024 •

edited

Loading

codecov-notifications bot commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

codecov-qa bot commented Nov 7, 2024

codecov-public-qa bot commented Nov 7, 2024

pytest

github-actions bot commented Nov 8, 2024

Swatinem left a comment

Swatinem Nov 11, 2024

tony-codecov Nov 12, 2024

Swatinem left a comment

Swatinem Nov 12, 2024

feat: Create coverage single upload endpoint #962

feat: Create coverage single upload endpoint #962

Conversation

tony-codecov commented Nov 5, 2024 • edited Loading

Purpose/Motivation

Links to relevant tickets

What does this PR do?

Notes to Reviewer

Legal Boilerplate

codecov-notifications bot commented Nov 5, 2024 • edited Loading

Codecov Report

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

codecov-qa bot commented Nov 7, 2024

❌ 6 Tests Failed:

codecov-public-qa bot commented Nov 7, 2024

❌ Failed Test Results:

pytest

github-actions bot commented Nov 8, 2024

Swatinem left a comment

Choose a reason for hiding this comment

Swatinem Nov 11, 2024

Choose a reason for hiding this comment

tony-codecov Nov 12, 2024

Choose a reason for hiding this comment

Swatinem left a comment

Choose a reason for hiding this comment

Swatinem Nov 12, 2024

Choose a reason for hiding this comment

tony-codecov commented Nov 5, 2024 •

edited

Loading

codecov-notifications bot commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading