From f90b8314d376a3d6bb652ec1fa58f8b30d800236 Mon Sep 17 00:00:00 2001 From: Nora Shapiro Date: Tue, 21 Jan 2025 11:50:20 -0800 Subject: [PATCH] Revert "Failed check and status messaging in comment, part I" (#1021) --- database/enums.py | 10 --- services/notification/__init__.py | 55 +------------ .../notification/notifiers/checks/patch.py | 81 +++++++------------ .../notification/notifiers/mixins/status.py | 67 ++------------- .../notification/notifiers/status/patch.py | 18 ++--- .../notifiers/tests/unit/test_checks.py | 22 +---- .../notifiers/tests/unit/test_status.py | 16 +--- tasks/tests/integration/test_notify_task.py | 9 +-- 8 files changed, 49 insertions(+), 229 deletions(-) diff --git a/database/enums.py b/database/enums.py index 42b0b9260..d9ac4f4c8 100644 --- a/database/enums.py +++ b/database/enums.py @@ -26,16 +26,6 @@ class Notification(Enum): codecov_slack_app = "codecov_slack_app" -notification_type_status_or_checks = { - Notification.status_changes, - Notification.status_patch, - Notification.status_project, - Notification.checks_changes, - Notification.checks_patch, - Notification.checks_project, -} - - class NotificationState(Enum): pending = "pending" success = "success" diff --git a/services/notification/__init__.py b/services/notification/__init__.py index 650ddfb18..ebe61543e 100644 --- a/services/notification/__init__.py +++ b/services/notification/__init__.py @@ -15,7 +15,6 @@ from shared.torngit.base import TorngitBaseAdapter from shared.yaml import UserYaml -from database.enums import notification_type_status_or_checks from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME, Owner, Repository from services.comparison import ComparisonProxy from services.decoration import Decoration @@ -37,7 +36,6 @@ ChecksWithFallback, ) from services.notification.notifiers.codecov_slack_app import CodecovSlackAppNotifier -from services.notification.notifiers.mixins.status import StatusState from services.yaml import read_yaml_field from services.yaml.reader import get_components_from_yaml @@ -256,62 +254,15 @@ def notify(self, comparison: ComparisonProxy) -> list[IndividualResult]: repoid=comparison.head.commit.repoid, ), ) - - results = [] - status_or_checks_helper_text = {} - notifiers_to_notify = [ - notifier + results = [ + self.notify_individual_notifier(notifier, comparison) for notifier in self.get_notifiers_instances() if notifier.is_enabled() ] - - status_or_checks_notifiers = [ - notifier - for notifier in notifiers_to_notify - if notifier.notification_type in notification_type_status_or_checks - ] - - all_other_notifiers = [ - notifier - for notifier in notifiers_to_notify - if notifier.notification_type not in notification_type_status_or_checks - ] - - status_or_checks_results = [ - self.notify_individual_notifier(notifier, comparison) - for notifier in status_or_checks_notifiers - ] - - if status_or_checks_results and all_other_notifiers: - # if the status/check fails, sometimes we want to add helper text to the message of the other notifications, - # to better surface that the status/check failed. - # so if there are status_and_checks_notifiers and all_other_notifiers, do the status_and_checks_notifiers first, - # look at the results of the checks, if any failed AND they are the type we have helper text for, - # add that text onto the other notifiers messages. - for result in status_or_checks_results: - notification_result = result["result"] - if ( - notification_result is not None - and notification_result.data_sent.get("state") - == StatusState.failure.value - ) and notification_result.data_sent.get("included_helper_text"): - status_or_checks_helper_text.update( - notification_result.data_sent["included_helper_text"] - ) - # TODO: pass status_or_checks_helper_text to all_other_notifiers, - # where they can integrate the helper text into their messages - results = results + status_or_checks_results - - results = results + [ - self.notify_individual_notifier(notifier, comparison) - for notifier in all_other_notifiers - ] return results def notify_individual_notifier( - self, - notifier: AbstractBaseNotifier, - comparison: ComparisonProxy, + self, notifier: AbstractBaseNotifier, comparison: ComparisonProxy ) -> IndividualResult: commit = comparison.head.commit base_commit = comparison.project_coverage_base.commit diff --git a/services/notification/notifiers/checks/patch.py b/services/notification/notifiers/checks/patch.py index fd086aa1b..95b1d5fa6 100644 --- a/services/notification/notifiers/checks/patch.py +++ b/services/notification/notifiers/checks/patch.py @@ -1,35 +1,18 @@ -from typing import Any, TypedDict - from database.enums import Notification from services.comparison import ComparisonProxy, FilteredComparison from services.notification.notifiers.checks.base import ChecksNotifier -from services.notification.notifiers.mixins.status import StatusPatchMixin, StatusState +from services.notification.notifiers.mixins.status import StatusPatchMixin from services.yaml import read_yaml_field -class CheckOutput(TypedDict): - title: str - summary: str - annotations: list[Any] - - -class CheckResult(TypedDict): - state: StatusState - output: CheckOutput - included_helper_text: dict[str, str] - - class PatchChecksNotifier(StatusPatchMixin, ChecksNotifier): context = "patch" - notification_type_display_name = "check" @property def notification_type(self) -> Notification: return Notification.checks_patch - def build_payload( - self, comparison: ComparisonProxy | FilteredComparison - ) -> CheckResult: + def build_payload(self, comparison: ComparisonProxy | FilteredComparison) -> dict: """ This method build the paylod of the patch github checks. @@ -38,23 +21,17 @@ def build_payload( """ if self.is_empty_upload(): state, message = self.get_status_check_for_empty_upload() - result = CheckResult( - state=state, - output=CheckOutput( - title="Empty Upload", - summary=message, - annotations=[], - ), - included_helper_text={}, - ) - return result - status_result = self.get_patch_status( - comparison, notification_type=self.notification_type_display_name - ) + return { + "state": state, + "output": { + "title": "Empty Upload", + "summary": message, + }, + } + state, message = self.get_patch_status(comparison) codecov_link = self.get_codecov_pr_link(comparison) - title = status_result["message"] - message = status_result["message"] + title = message should_use_upgrade = self.should_use_upgrade_decoration() if should_use_upgrade: @@ -77,27 +54,23 @@ def build_payload( or should_use_upgrade or should_annotate is False ): - result = CheckResult( - state=status_result["state"], - output=CheckOutput( - title=title, - summary="\n\n".join([codecov_link, message]), - annotations=[], - ), - included_helper_text=status_result["included_helper_text"], - ) - return result + return { + "state": state, + "output": { + "title": f"{title}", + "summary": "\n\n".join([codecov_link, message]), + }, + } diff = comparison.get_diff(use_original_base=True) # TODO: Look into why the apply diff in get_patch_status is not saving state at this point comparison.head.report.apply_diff(diff) annotations = self.create_annotations(comparison, diff) - result = CheckResult( - state=status_result["state"], - output=CheckOutput( - title=title, - summary="\n\n".join([codecov_link, message]), - annotations=annotations, - ), - included_helper_text=status_result["included_helper_text"], - ) - return result + + return { + "state": state, + "output": { + "title": f"{title}", + "summary": "\n\n".join([codecov_link, message]), + "annotations": annotations, + }, + } diff --git a/services/notification/notifiers/mixins/status.py b/services/notification/notifiers/mixins/status.py index 57c31f845..13ec2cdb5 100644 --- a/services/notification/notifiers/mixins/status.py +++ b/services/notification/notifiers/mixins/status.py @@ -1,7 +1,6 @@ import logging from decimal import Decimal, InvalidOperation from enum import Enum -from typing import Literal, TypedDict from services.comparison import ComparisonProxy, FilteredComparison from services.yaml.reader import round_number @@ -14,35 +13,7 @@ class StatusState(Enum): failure = "failure" -class StatusResult(TypedDict): - """ - The mixins in this file do the calculations and decide the SuccessState for all Status and Checks Notifiers. - Checks have different fields than Statuses, so Checks are converted to the CheckResult type later. - """ - - state: Literal["success", "failure"] # StatusState values - message: str - included_helper_text: dict[str, str] - - -CUSTOM_TARGET_TEXT_PATCH_KEY = "custom_target_helper_text_patch" -CUSTOM_TARGET_TEXT_PROJECT_KEY = "custom_target_helper_text_project" -CUSTOM_TARGET_TEXT_VALUE = ( - "Your {context} {notification_type} has failed because the patch coverage is below the target coverage. " - "You can increase the patch coverage or adjust the " - "[target](https://docs.codecov.com/docs/commit-status#target) coverage." -) - - -HELPER_TEXT_MAP = { - CUSTOM_TARGET_TEXT_PATCH_KEY: CUSTOM_TARGET_TEXT_VALUE, - CUSTOM_TARGET_TEXT_PROJECT_KEY: CUSTOM_TARGET_TEXT_VALUE, -} - - class StatusPatchMixin(object): - context = "patch" - def _get_threshold(self) -> Decimal: """ Threshold can be configured by user, default is 0.0 @@ -58,7 +29,7 @@ def _get_threshold(self) -> Decimal: def _get_target( self, comparison: ComparisonProxy | FilteredComparison - ) -> tuple[Decimal | None, bool]: + ) -> Decimal | None: """ Target can be configured by user, default is auto, which is the coverage level from the base report. Target will be None if no report is found to compare against. @@ -68,7 +39,6 @@ def _get_target( target_coverage = Decimal( str(self.notifier_yaml_settings.get("target")).replace("%", "") ) - is_custom_target = True else: target_coverage = ( Decimal(comparison.project_coverage_base.report.totals.coverage) @@ -76,16 +46,14 @@ def _get_target( and comparison.project_coverage_base.report.totals.coverage is not None else None ) - is_custom_target = False - return target_coverage, is_custom_target + return target_coverage def get_patch_status( - self, comparison: ComparisonProxy | FilteredComparison, notification_type: str - ) -> StatusResult: + self, comparison: ComparisonProxy | FilteredComparison + ) -> tuple[str, str]: threshold = self._get_threshold() - target_coverage, is_custom_target = self._get_target(comparison) + target_coverage = self._get_target(comparison) totals = comparison.get_patch_totals() - included_helper_text = {} # coverage affected if totals and totals.lines > 0: @@ -116,16 +84,7 @@ def get_patch_status( message = ( f"{coverage_rounded}% of diff hit (target {target_rounded}%)" ) - # TODO: - # if state == StatusState.failure.value and is_custom_target: - # helper_text = HELPER_TEXT_MAP[CUSTOM_TARGET_TEXT_PATCH_KEY].format( - # context=self.context, notification_type=notification_type - # ) - # included_helper_text[CUSTOM_TARGET_TEXT_PATCH_KEY] = helper_text - # message = message + " - " + helper_text - return StatusResult( - state=state, message=message, included_helper_text=included_helper_text - ) + return state, message # coverage not affected if comparison.project_coverage_base.commit: @@ -135,16 +94,10 @@ def get_patch_status( ) else: description = "Coverage not affected" - return StatusResult( - state=StatusState.success.value, - message=description, - included_helper_text=included_helper_text, - ) + return StatusState.success.value, description class StatusChangesMixin(object): - context = "changes" - def is_a_change_worth_noting(self, change) -> bool: if not change.new and not change.deleted: # has totals and not -10m => 10h @@ -191,7 +144,6 @@ def get_changes_status( class StatusProjectMixin(object): DEFAULT_REMOVED_CODE_BEHAVIOR = "adjust_base" - context = "project" def _apply_removals_only_behavior( self, comparison: ComparisonProxy | FilteredComparison @@ -468,11 +420,6 @@ def _get_project_status( # use rounded numbers for messages target_rounded = round_number(self.current_yaml, target_coverage) message = f"{head_coverage_rounded}% (target {target_rounded}%)" - # TODO: - # helper_text = HELPER_TEXT_MAP[CUSTOM_TARGET_TEXT].format( - # context=self.context, notification_type=notification_type) - # included_helper_text[CUSTOM_TARGET_TEXT] = helper_text - # message = message + " - " + helper_text return state, message # use rounded numbers for messages diff --git a/services/notification/notifiers/status/patch.py b/services/notification/notifiers/status/patch.py index 10c52ba4e..003b9a2e9 100644 --- a/services/notification/notifiers/status/patch.py +++ b/services/notification/notifiers/status/patch.py @@ -1,6 +1,6 @@ from database.enums import Notification from services.comparison import ComparisonProxy, FilteredComparison -from services.notification.notifiers.mixins.status import StatusPatchMixin, StatusResult +from services.notification.notifiers.mixins.status import StatusPatchMixin from services.notification.notifiers.status.base import StatusNotifier @@ -17,24 +17,18 @@ class PatchStatusNotifier(StatusPatchMixin, StatusNotifier): """ context = "patch" - notification_type_display_name = "status" @property def notification_type(self) -> Notification: return Notification.status_patch - def build_payload( - self, comparison: ComparisonProxy | FilteredComparison - ) -> StatusResult: + def build_payload(self, comparison: ComparisonProxy | FilteredComparison) -> dict: if self.is_empty_upload(): state, message = self.get_status_check_for_empty_upload() - result = StatusResult(state=state, message=message, included_helper_text={}) - return result + return {"state": state, "message": message} - result = self.get_patch_status( - comparison, notification_type=self.notification_type_display_name - ) + state, message = self.get_patch_status(comparison) if self.should_use_upgrade_decoration(): - result["message"] = self.get_upgrade_message() + message = self.get_upgrade_message() - return result + return {"state": state, "message": message} diff --git a/services/notification/notifiers/tests/unit/test_checks.py b/services/notification/notifiers/tests/unit/test_checks.py index 0ebe70e3a..5575f1d3f 100644 --- a/services/notification/notifiers/tests/unit/test_checks.py +++ b/services/notification/notifiers/tests/unit/test_checks.py @@ -703,9 +703,7 @@ def test_build_flag_payload( "output": { "title": "66.67% of diff hit (target 50.00%)", "summary": f"[View this Pull Request on Codecov](test.example.br/gh/test_build_flag_payload/{sample_comparison.head.commit.repository.name}/pull/{sample_comparison.pull.pullid}?dropdown=coverage&src=pr&el=h1)\n\n66.67% of diff hit (target 50.00%)", - "annotations": [], }, - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -731,9 +729,7 @@ def test_build_upgrade_payload( "output": { "title": "Codecov Report", "summary": f"[View this Pull Request on Codecov](test.example.br/gh/test_build_upgrade_payload/{sample_comparison.head.commit.repository.name}/pull/{sample_comparison.pull.pullid}?dropdown=coverage&src=pr&el=h1)\n\nThe author of this PR, codecov-test-user, is not an activated member of this organization on Codecov.\nPlease [activate this user on Codecov](test.example.br/members/gh/test_build_upgrade_payload) to display a detailed status check.\nCoverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.\nPlease don't hesitate to email us at support@codecov.io with any questions.", - "annotations": [], }, - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -765,7 +761,6 @@ def test_build_default_payload( } ], }, - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result["output"]["summary"] == result["output"]["summary"] @@ -788,9 +783,7 @@ def test_build_payload_target_coverage_failure( "output": { "title": "66.67% of diff hit (target 70.00%)", "summary": f"[View this Pull Request on Codecov](test.example.br/gh/test_build_payload_target_coverage_failure/{sample_comparison.head.commit.repository.name}/pull/{sample_comparison.pull.pullid}?dropdown=coverage&src=pr&el=h1)\n\n66.67% of diff hit (target 70.00%)", - "annotations": [], }, - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -826,7 +819,6 @@ def test_build_payload_without_base_report( } ], }, - "included_helper_text": {}, } result = notifier.build_payload(comparison) assert expected_result == result @@ -864,7 +856,6 @@ def test_build_payload_target_coverage_failure_witinh_threshold( } ], }, - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result["state"] == result["state"] @@ -907,7 +898,6 @@ def test_build_payload_with_multiple_changes( } ], }, - "included_helper_text": {}, } result = notifier.build_payload(comparison_with_multiple_changes) assert expected_result["state"] == result["state"] @@ -969,9 +959,7 @@ def test_build_payload_no_diff( "output": { "title": f"Coverage not affected when comparing {base_commit.commitid[:7]}...{head_commit.commitid[:7]}", "summary": f"[View this Pull Request on Codecov](test.example.br/gh/test_build_payload_no_diff/{sample_comparison.head.commit.repository.name}/pull/{sample_comparison.pull.pullid}?dropdown=coverage&src=pr&el=h1)\n\nCoverage not affected when comparing {base_commit.commitid[:7]}...{head_commit.commitid[:7]}", - "annotations": [], }, - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert notifier.notification_type.value == "checks_patch" @@ -1064,6 +1052,10 @@ def test_notify( mock_repo_provider.get_commit_statuses.return_value = Status([]) mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" comparison = sample_comparison + payload = { + "state": "success", + "output": {"title": "Codecov Report", "summary": "Summary"}, + } mock_repo_provider.create_check_run.return_value = 2234563 mock_repo_provider.update_check_run.return_value = "success" notifier = PatchChecksNotifier( @@ -1084,9 +1076,7 @@ def test_notify( "output": { "title": f"Coverage not affected when comparing {base_commit.commitid[:7]}...{head_commit.commitid[:7]}", "summary": f"[View this Pull Request on Codecov](test.example.br/gh/test_notify/{sample_comparison.head.commit.repository.name}/pull/{sample_comparison.pull.pullid}?dropdown=coverage&src=pr&el=h1)\n\nCoverage not affected when comparing {base_commit.commitid[:7]}...{head_commit.commitid[:7]}", - "annotations": [], }, - "included_helper_text": {}, "url": f"test.example.br/gh/test_notify/{sample_comparison.head.commit.repository.name}/pull/{comparison.pull.pullid}", } @@ -1115,9 +1105,7 @@ def test_notify_passing_empty_upload( "output": { "title": "Empty Upload", "summary": "Non-testable files changed.", - "annotations": [], }, - "included_helper_text": {}, "url": f"test.example.br/gh/test_notify_passing_empty_upload/{sample_comparison.head.commit.repository.name}/pull/{comparison.pull.pullid}", } @@ -1146,9 +1134,7 @@ def test_notify_failing_empty_upload( "output": { "title": "Empty Upload", "summary": "Testable files changed", - "annotations": [], }, - "included_helper_text": {}, "url": f"test.example.br/gh/test_notify_failing_empty_upload/{sample_comparison.head.commit.repository.name}/pull/{comparison.pull.pullid}", } diff --git a/services/notification/notifiers/tests/unit/test_status.py b/services/notification/notifiers/tests/unit/test_status.py index f85dbdd5f..79c47672e 100644 --- a/services/notification/notifiers/tests/unit/test_status.py +++ b/services/notification/notifiers/tests/unit/test_status.py @@ -2072,7 +2072,6 @@ def test_build_payload( expected_result = { "message": "66.67% of diff hit (target 50.00%)", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2093,7 +2092,6 @@ def test_build_upgrade_payload( expected_result = { "message": "Please activate this user to display a detailed status check", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2113,7 +2111,6 @@ def test_build_payload_target_coverage_failure( expected_result = { "message": "66.67% of diff hit (target 70.00%)", "state": "failure", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2133,7 +2130,6 @@ def test_build_payload_not_auto_not_string( expected_result = { "message": "66.67% of diff hit (target 57.00%)", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2159,7 +2155,6 @@ def test_build_payload_target_coverage_failure_within_threshold( expected_result = { "message": "66.67% of diff hit (within 5.00% threshold of 70.00%)", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2185,7 +2180,6 @@ def test_get_patch_status_bad_threshold( expected_result = { "message": "66.67% of diff hit (target 70.00%)", "state": "failure", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2213,7 +2207,6 @@ def test_get_patch_status_bad_threshold_fixed( expected_result = { "message": "66.67% of diff hit (within 5.00% threshold of 70.00%)", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2264,7 +2257,6 @@ def test_build_payload_no_diff( expected_result = { "message": f"Coverage not affected when comparing {base_commit.commitid[:7]}...{head_commit.commitid[:7]}", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(sample_comparison) assert expected_result == result @@ -2314,11 +2306,7 @@ def test_build_payload_no_diff_no_base_report( current_yaml=UserYaml({}), repository_service=mock_repo_provider, ) - expected_result = { - "message": "Coverage not affected", - "state": "success", - "included_helper_text": {}, - } + expected_result = {"message": "Coverage not affected", "state": "success"} result = notifier.build_payload(comparison) assert expected_result == result @@ -2341,7 +2329,6 @@ def test_build_payload_without_base_report( expected_result = { "message": "No report found to compare against", "state": "success", - "included_helper_text": {}, } result = notifier.build_payload(comparison) assert expected_result == result @@ -2368,7 +2355,6 @@ def test_build_payload_with_multiple_changes( expected_result = { "message": "50.00% of diff hit (target 76.92%)", "state": "failure", - "included_helper_text": {}, } result = notifier.build_payload(comparison_with_multiple_changes) assert expected_result == result diff --git a/tasks/tests/integration/test_notify_task.py b/tasks/tests/integration/test_notify_task.py index 6840b03c1..407026f53 100644 --- a/tasks/tests/integration/test_notify_task.py +++ b/tasks/tests/integration/test_notify_task.py @@ -1257,16 +1257,9 @@ def test_simple_call_status_and_notifiers( assert expected["result"].data_received == actual["result"].data_received assert expected["result"] == actual["result"] assert expected == actual - - sorted_result = sorted(result["notifications"], key=lambda x: x["notifier"]) - sorted_expected_result = sorted( + assert sorted(result["notifications"], key=lambda x: x["notifier"]) == sorted( expected_result["notifications"], key=lambda x: x["notifier"] ) - assert len(sorted_result) == len(sorted_expected_result) == 7 - assert sorted_result == sorted_expected_result - - result["notifications"] = sorted_result - expected_result["notifications"] = sorted_expected_result assert result == expected_result pull = dbsession.query(Pull).filter_by(pullid=9, repoid=commit.repoid).first()