From 5e92d543b74f0ace1b4c86b7af66b4319d11fd03 Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Wed, 29 Jan 2025 14:56:34 -0800 Subject: [PATCH] Failed check and status messaging in comment, part V (#1044) --- .../notification/notifiers/mixins/status.py | 51 +++++++++++--- .../notifiers/tests/unit/test_status.py | 70 +++++++++++++++++++ 2 files changed, 110 insertions(+), 11 deletions(-) diff --git a/services/notification/notifiers/mixins/status.py b/services/notification/notifiers/mixins/status.py index ef8a165d6..3d6e0beae 100644 --- a/services/notification/notifiers/mixins/status.py +++ b/services/notification/notifiers/mixins/status.py @@ -27,16 +27,23 @@ class StatusResult(TypedDict): CUSTOM_TARGET_TEXT_PATCH_KEY = "custom_target_helper_text_patch" CUSTOM_TARGET_TEXT_PROJECT_KEY = "custom_target_helper_text_project" +CUSTOM_RCB_INDIRECT_CHANGES_KEY = "custom_rcb_indirect_changes_helper_text" CUSTOM_TARGET_TEXT_VALUE = ( "Your {context} {notification_type} has failed because the {point_of_comparison} coverage ({coverage}%) is below the target coverage ({target}%). " "You can increase the {point_of_comparison} coverage or adjust the " "[target](https://docs.codecov.com/docs/commit-status#target) coverage." ) +CUSTOM_RCB_INDIRECT_CHANGES_VALUE = ( + "Your {context} {notification_type} has failed because you have indirect coverage changes. " + "Learn more about [Unexpected Coverage Changes](https://docs.codecov.com/docs/unexpected-coverage-changes) " + "and [reasons for indirect coverage changes](https://docs.codecov.com/docs/unexpected-coverage-changes#reasons-for-indirect-changes)." +) HELPER_TEXT_MAP = { CUSTOM_TARGET_TEXT_PATCH_KEY: CUSTOM_TARGET_TEXT_VALUE, CUSTOM_TARGET_TEXT_PROJECT_KEY: CUSTOM_TARGET_TEXT_VALUE, + CUSTOM_RCB_INDIRECT_CHANGES_KEY: CUSTOM_RCB_INDIRECT_CHANGES_VALUE, } @@ -311,12 +318,15 @@ def _apply_adjust_base_behavior( return None def _apply_fully_covered_patch_behavior( - self, comparison: ComparisonProxy | FilteredComparison - ) -> tuple[str, str] | None: + self, + comparison: ComparisonProxy | FilteredComparison, + notification_type: str, + ) -> tuple[tuple[str, str] | None, dict]: """ Rule for passing project status on fully_covered_patch behavior: - Pass if patch coverage is 100% and there are no unexpected changes + Pass if patch coverage is 100% and there are no indirect changes """ + helper_text = {} log.info( "Applying fully_covered_patch behavior to project status", extra=dict(commit=comparison.head.commit.commitid), @@ -332,23 +342,37 @@ def _apply_fully_covered_patch_behavior( "Unexpected changes when applying patch_100 behavior", extra=dict(commit=comparison.head.commit.commitid), ) - return None + + # their comparison failed because of unexpected/indirect changes, give them helper text about it + helper_text[CUSTOM_RCB_INDIRECT_CHANGES_KEY] = HELPER_TEXT_MAP[ + CUSTOM_RCB_INDIRECT_CHANGES_KEY + ].format( + context=self.context, + notification_type=notification_type, + ) + return None, helper_text diff = comparison.get_diff(use_original_base=True) patch_totals = comparison.head.report.apply_diff(diff) if patch_totals is None or patch_totals.lines == 0: # Coverage was not changed by patch return ( - StatusState.success.value, - ", passed because coverage was not affected by patch", + ( + StatusState.success.value, + ", passed because coverage was not affected by patch", + ), + helper_text, ) coverage = Decimal(patch_totals.coverage) if coverage == 100.0: return ( - StatusState.success.value, - ", passed because patch was fully covered by tests, and no indirect coverage changes", + ( + StatusState.success.value, + ", passed because patch was fully covered by tests, and no indirect coverage changes", + ), + helper_text, ) - return None + return None, helper_text def get_project_status( self, comparison: ComparisonProxy | FilteredComparison, notification_type: str @@ -373,9 +397,14 @@ def get_project_status( elif removed_code_behavior == "adjust_base": removed_code_result = self._apply_adjust_base_behavior(comparison) elif removed_code_behavior == "fully_covered_patch": - removed_code_result = self._apply_fully_covered_patch_behavior( - comparison + removed_code_result, helper_text = ( + self._apply_fully_covered_patch_behavior( + comparison, + notification_type=notification_type, + ) ) + # if user set this in their yaml, give them helper text related to it + result["included_helper_text"].update(helper_text) else: if removed_code_behavior not in [False, "off"]: log.warning( diff --git a/services/notification/notifiers/tests/unit/test_status.py b/services/notification/notifiers/tests/unit/test_status.py index 21a0425ba..8fc2d0491 100644 --- a/services/notification/notifiers/tests/unit/test_status.py +++ b/services/notification/notifiers/tests/unit/test_status.py @@ -21,9 +21,11 @@ from services.decoration import Decoration from services.notification.notifiers.base import NotificationResult from services.notification.notifiers.mixins.status import ( + CUSTOM_RCB_INDIRECT_CHANGES_KEY, CUSTOM_TARGET_TEXT_PATCH_KEY, CUSTOM_TARGET_TEXT_PROJECT_KEY, CUSTOM_TARGET_TEXT_VALUE, + HELPER_TEXT_MAP, ) from services.notification.notifiers.status import ( ChangesStatusNotifier, @@ -1909,6 +1911,74 @@ def test_notify_fully_covered_patch_behavior_fail( assert result == expected_result mock_get_impacted_files.assert_called() + def test_notify_fully_covered_patch_behavior_fail_indirect_changes( + self, + comparison_with_multiple_changes, + mock_repo_provider, + mock_configuration, + multiple_diff_changes, + mocker, + ): + json_diff = multiple_diff_changes + mock_repo_provider.get_compare.return_value = {"diff": json_diff} + mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" + mock_get_impacted_files = mocker.patch.object( + ComparisonProxy, + "get_impacted_files", + return_value={ + "files": [ + { + "base_name": "tests/file1.py", + "head_name": "tests/file1.py", + # Not complete, but we only care about these fields + "removed_diff_coverage": [[1, "h"]], + "added_diff_coverage": [[2, "h"], [3, "h"]], + "unexpected_line_changes": "any value in this field", + }, + { + "base_name": "tests/file2.go", + "head_name": "tests/file2.go", + "removed_diff_coverage": [[1, "h"], [3, None]], + "added_diff_coverage": [], + "unexpected_line_changes": [], + }, + ], + }, + ) + notifier = ProjectStatusNotifier( + repository=comparison_with_multiple_changes.head.commit.repository, + title="title", + notifier_yaml_settings={ + "target": "70%", + "removed_code_behavior": "fully_covered_patch", + }, + notifier_site_settings=True, + current_yaml=UserYaml({}), + repository_service=mock_repo_provider, + ) + expected_result = { + "message": "50.00% (target 70.00%)", + "state": "failure", + "included_helper_text": { + CUSTOM_TARGET_TEXT_PROJECT_KEY: CUSTOM_TARGET_TEXT_VALUE.format( + context="project", + notification_type="status", + point_of_comparison="head", + coverage="50.00", + target="70.00", + ), + CUSTOM_RCB_INDIRECT_CHANGES_KEY: HELPER_TEXT_MAP[ + CUSTOM_RCB_INDIRECT_CHANGES_KEY + ].format( + context="project", + notification_type="status", + ), + }, + } + result = notifier.build_payload(comparison_with_multiple_changes) + assert result == expected_result + mock_get_impacted_files.assert_called() + def test_notify_fully_covered_patch_behavior_success( self, comparison_100_percent_patch,