API Improvements: Fix typings for various files pt5 #1087

JerrySentry · 2025-01-09T20:00:49Z

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 · 2025-01-09T20:44:47Z

Codecov Report

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

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
upload/helpers.py	89.65%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

codecov-qa · 2025-01-09T20:44:53Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2673	1	2672	7

View the top 1 failed tests by shortest run time

upload/tests/test_upload.py::UploadHandlerHelpersTest::test_determine_upload_commit_to_use

Stack Traces | 0.01s run time

self = <upload.tests.test_upload.UploadHandlerHelpersTest testMethod=test_determine_upload_commit_to_use>
mock_repo_provider_service = <MagicMock name='_get_git_commit_data' id='139687214715616'>
mock_async = <MagicMock name='RepoProviderService' id='139687014279376'>

    @patch("upload.helpers.RepoProviderService")
    @patch("upload.helpers._get_git_commit_data")
    def test_determine_upload_commit_to_use(
        self, mock_repo_provider_service, mock_async
    ):
        mock_repo_provider_service.return_value = {
            "message": "Merge 1c78206f1a46dc6db8412a491fc770eb7d0f8a47 into 261aa931e8e3801ad95a31bbc3529de2bba436c8"
        }
    
        with self.subTest("not a github commit"):
            org = G(
                Owner,
                service="bitbucket",
                oauth_token=encryptor.encode("hahahahaha").decode(),
            )
            repo = G(Repository, author=org)
            upload_params = {
                "service": "bitbucket",
                "commit": "3be5c52bd748c508a7e96993c02cf3518c816e84",
            }
>           assert (
                "3be5c52bd748c508a7e96993c02cf3518c816e84"
                == determine_upload_commit_to_use(upload_params, repo)
            )

upload/tests/test_upload.py:511: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

upload_params = {'commit': '3be5c52bd748c508a7e96993c02cf3518c816e84', 'service': 'bitbucket'}
repository = <Repository: Repo<Owner<bitbucket/None>/6>>

    def determine_upload_commit_to_use(
        upload_params: Dict[str, Any], repository: Repository
    ) -> str:
        """
        Do processing on the upload request parameters to determine which commit to use for the upload:
        - If this is a merge commit on github, use the first commit SHA in the merge commit message.
        - Otherwise, use the value provided in the request parameters.
        """
        # Check if this is a merge commit and, if so, use the commitid of the commit being merged into per the merge commit message.
        # See https://docs.codecov.io/docs/merge-commits for more context.
        service = repository.author.service
        if service.startswith("github") and not upload_params.get(
            "_did_change_merge_commit"
        ):
            token = try_to_get_best_possible_bot_token(repository)
            commitid = upload_params.get("commit", "")
            if token is None:
                return commitid
            # Get the commit message from the git provider and check if it's structured like a merge commit message
            try:
                adapter = RepoProviderService().get_adapter(
                    repository.author, repository, use_ssl=True, token=token
                )
                git_commit_data = _get_git_commit_data(adapter, commitid, token)
            except TorngitObjectNotFoundError:
                log.warning(
                    "Unable to fetch commit. Not found",
                    extra=dict(commit=commitid),
                )
                return commitid
            except TorngitClientError:
                log.warning("Unable to fetch commit", extra=dict(commit=commitid))
                return commitid
    
            git_commit_message = git_commit_data.get("message", "").strip()
            is_merge_commit = re.match(r"^Merge\s\w{40}\sinto\s\w{40}$", git_commit_message)
    
            if is_merge_commit:
                # If the commit message says "Merge A into B", we'll extract A and use that as the commitid for this upload
                new_commit_id = git_commit_message.split(" ")[1]
                log.info(
                    "Upload is for a merge commit, updating commit id for upload",
                    extra=dict(
                        commit=commitid,
                        commit_message=git_commit_message,
                        new_commit=new_commit_id,
                    ),
                )
                return new_commit_id
    
        # If it's not a merge commit we'll just use the commitid provided in the upload parameters
>       return commitid
E       UnboundLocalError: cannot access local variable 'commitid' where it is not associated with a value

upload/helpers.py:492: UnboundLocalError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

codecov-public-qa · 2025-01-09T20:45:01Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2674	1	2673	6

View the top 1 failed tests by shortest run time

upload/tests/test_upload.py::UploadHandlerHelpersTest::test_determine_upload_commit_to_use

Stack Traces | 0.01s run time

self = <upload.tests.test_upload.UploadHandlerHelpersTest testMethod=test_determine_upload_commit_to_use>
mock_repo_provider_service = <MagicMock name='_get_git_commit_data' id='139687214715616'>
mock_async = <MagicMock name='RepoProviderService' id='139687014279376'>

    @patch("upload.helpers.RepoProviderService")
    @patch("upload.helpers._get_git_commit_data")
    def test_determine_upload_commit_to_use(
        self, mock_repo_provider_service, mock_async
    ):
        mock_repo_provider_service.return_value = {
            "message": "Merge 1c78206f1a46dc6db8412a491fc770eb7d0f8a47 into 261aa931e8e3801ad95a31bbc3529de2bba436c8"
        }
    
        with self.subTest("not a github commit"):
            org = G(
                Owner,
                service="bitbucket",
                oauth_token=encryptor.encode("hahahahaha").decode(),
            )
            repo = G(Repository, author=org)
            upload_params = {
                "service": "bitbucket",
                "commit": "3be5c52bd748c508a7e96993c02cf3518c816e84",
            }
>           assert (
                "3be5c52bd748c508a7e96993c02cf3518c816e84"
                == determine_upload_commit_to_use(upload_params, repo)
            )

upload/tests/test_upload.py:511: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

upload_params = {'commit': '3be5c52bd748c508a7e96993c02cf3518c816e84', 'service': 'bitbucket'}
repository = <Repository: Repo<Owner<bitbucket/None>/6>>

    def determine_upload_commit_to_use(
        upload_params: Dict[str, Any], repository: Repository
    ) -> str:
        """
        Do processing on the upload request parameters to determine which commit to use for the upload:
        - If this is a merge commit on github, use the first commit SHA in the merge commit message.
        - Otherwise, use the value provided in the request parameters.
        """
        # Check if this is a merge commit and, if so, use the commitid of the commit being merged into per the merge commit message.
        # See https://docs.codecov.io/docs/merge-commits for more context.
        service = repository.author.service
        if service.startswith("github") and not upload_params.get(
            "_did_change_merge_commit"
        ):
            token = try_to_get_best_possible_bot_token(repository)
            commitid = upload_params.get("commit", "")
            if token is None:
                return commitid
            # Get the commit message from the git provider and check if it's structured like a merge commit message
            try:
                adapter = RepoProviderService().get_adapter(
                    repository.author, repository, use_ssl=True, token=token
                )
                git_commit_data = _get_git_commit_data(adapter, commitid, token)
            except TorngitObjectNotFoundError:
                log.warning(
                    "Unable to fetch commit. Not found",
                    extra=dict(commit=commitid),
                )
                return commitid
            except TorngitClientError:
                log.warning("Unable to fetch commit", extra=dict(commit=commitid))
                return commitid
    
            git_commit_message = git_commit_data.get("message", "").strip()
            is_merge_commit = re.match(r"^Merge\s\w{40}\sinto\s\w{40}$", git_commit_message)
    
            if is_merge_commit:
                # If the commit message says "Merge A into B", we'll extract A and use that as the commitid for this upload
                new_commit_id = git_commit_message.split(" ")[1]
                log.info(
                    "Upload is for a merge commit, updating commit id for upload",
                    extra=dict(
                        commit=commitid,
                        commit_message=git_commit_message,
                        new_commit=new_commit_id,
                    ),
                )
                return new_commit_id
    
        # If it's not a merge commit we'll just use the commitid provided in the upload parameters
>       return commitid
E       UnboundLocalError: cannot access local variable 'commitid' where it is not associated with a value

upload/helpers.py:492: UnboundLocalError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

github-actions · 2025-01-09T20:45:02Z

✅ All tests successful. No failed tests were found.

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

codecov · 2025-01-09T20:57:27Z

Codecov Report

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

Project coverage is 96.02%. Comparing base (8660eab) to head (1376bc7).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
upload/helpers.py	89.65%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1087   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files         828      828           
  Lines       19485    19503   +18     
=======================================
+ Hits        18710    18728   +18     
  Misses        775      775

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

suejung-sentry

This is awesome!! Thank you for doing it!
Ideally we'd have fewer generic Any or Dict, but I know that gets messy, so this is a great step forward for now.

suejung-sentry · 2025-01-09T22:58:57Z

upload/helpers.py

@@ -437,31 +449,28 @@ def determine_upload_commit_to_use(upload_params, repository):
    # Check if this is a merge commit and, if so, use the commitid of the commit being merged into per the merge commit message.
    # See https://docs.codecov.io/docs/merge-commits for more context.
    service = repository.author.service
+    commitid = upload_params.get("commit", "")


Our past default return was None not empty string "" - just making sure does anything downstream care about this distinction?

From what I can tell it shouldn't, if anything a None would actually cause an error in some cases downstream or upstream.

suejung-sentry · 2025-01-09T23:03:13Z

upload/helpers.py

-            if headers.get("X_Content_Type") in (None, "text/html")
-            else headers.get("X_Content_Type")
+            if headers.get("X_Content_Type", "") in (None, "text/html")
+            else headers.get("X_Content_Type", "")


This change means that where we used to default to X_Content_Type: text/plain when the header is None, now we'd end up with X_Content_Type: "" empty string. So I think that means we need to revert to the prior?

Either that or

if headers.get("X_Content_Type", "") in ("", "text/html")

Ah good point!

JerrySentry added 3 commits January 9, 2025 15:00

API Improvements: Fix typings for various files pt5

0c294c4

fix 1

4b84456

fix 3

843adb1

JerrySentry and others added 3 commits January 9, 2025 16:14

fix 4

baf4a5e

Merge branch 'main' into jan_09_mypy

e7f22be

fix 5

ae91725

JerrySentry marked this pull request as ready for review January 9, 2025 22:21

JerrySentry requested review from a team as code owners January 9, 2025 22:21

suejung-sentry approved these changes Jan 9, 2025

View reviewed changes

fix 6

1376bc7

adrian-codecov approved these changes Jan 10, 2025

View reviewed changes

JerrySentry added this pull request to the merge queue Jan 10, 2025

Merged via the queue into main with commit 47a0933 Jan 10, 2025
19 checks passed

JerrySentry deleted the jan_09_mypy branch January 10, 2025 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Improvements: Fix typings for various files pt5 #1087

API Improvements: Fix typings for various files pt5 #1087

JerrySentry commented Jan 9, 2025

codecov-staging bot commented Jan 9, 2025 •

edited by codecov-notifications bot

Loading

codecov-qa bot commented Jan 9, 2025 •

edited

Loading

codecov-public-qa bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

suejung-sentry left a comment

suejung-sentry Jan 9, 2025

JerrySentry Jan 10, 2025

suejung-sentry Jan 9, 2025

JerrySentry Jan 10, 2025

API Improvements: Fix typings for various files pt5 #1087

API Improvements: Fix typings for various files pt5 #1087

Conversation

JerrySentry commented Jan 9, 2025

Legal Boilerplate

codecov-staging bot commented Jan 9, 2025 • edited by codecov-notifications bot Loading

Codecov Report

codecov-qa bot commented Jan 9, 2025 • edited Loading

❌ 1 Tests Failed:

codecov-public-qa bot commented Jan 9, 2025 • edited Loading

❌ 1 Tests Failed:

github-actions bot commented Jan 9, 2025

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

suejung-sentry left a comment

Choose a reason for hiding this comment

suejung-sentry Jan 9, 2025

Choose a reason for hiding this comment

JerrySentry Jan 10, 2025

Choose a reason for hiding this comment

suejung-sentry Jan 9, 2025

Choose a reason for hiding this comment

JerrySentry Jan 10, 2025

Choose a reason for hiding this comment

codecov-staging bot commented Jan 9, 2025 •

edited by codecov-notifications bot

Loading

codecov-qa bot commented Jan 9, 2025 •

edited

Loading

codecov-public-qa bot commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading