Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed check and status messaging in comment, part I #1014

Merged
merged 2 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions database/enums.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ 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"
Expand Down
55 changes: 52 additions & 3 deletions services/notification/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
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
Expand All @@ -36,6 +37,7 @@
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

Expand Down Expand Up @@ -254,15 +256,62 @@
repoid=comparison.head.commit.repoid,
),
)
results = [
self.notify_individual_notifier(notifier, comparison)

results = []
status_or_checks_helper_text = {}
notifiers_to_notify = [
notifier
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(

Check warning on line 298 in services/notification/__init__.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/notification/__init__.py#L298

Added line #L298 was not covered by tests
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
Expand Down
81 changes: 54 additions & 27 deletions services/notification/notifiers/checks/patch.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,35 @@
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
from services.notification.notifiers.mixins.status import StatusPatchMixin, StatusState
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) -> dict:
def build_payload(
self, comparison: ComparisonProxy | FilteredComparison
) -> CheckResult:
"""
This method build the paylod of the patch github checks.

Expand All @@ -21,17 +38,23 @@ def build_payload(self, comparison: ComparisonProxy | FilteredComparison) -> dic
"""
if self.is_empty_upload():
state, message = self.get_status_check_for_empty_upload()
return {
"state": state,
"output": {
"title": "Empty Upload",
"summary": message,
},
}
state, message = self.get_patch_status(comparison)
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
)
codecov_link = self.get_codecov_pr_link(comparison)

title = message
title = status_result["message"]
message = status_result["message"]

should_use_upgrade = self.should_use_upgrade_decoration()
if should_use_upgrade:
Expand All @@ -54,23 +77,27 @@ def build_payload(self, comparison: ComparisonProxy | FilteredComparison) -> dic
or should_use_upgrade
or should_annotate is False
):
return {
"state": state,
"output": {
"title": f"{title}",
"summary": "\n\n".join([codecov_link, message]),
},
}
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
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)

return {
"state": state,
"output": {
"title": f"{title}",
"summary": "\n\n".join([codecov_link, message]),
"annotations": annotations,
},
}
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
67 changes: 60 additions & 7 deletions services/notification/notifiers/mixins/status.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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
Expand All @@ -13,7 +14,35 @@ 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. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also looking at the mockup for this, is it reasonable to include the numbers in this message as well, like:

… has failed because the patch coverage (X%) is below the target coverage (Y%).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not in the design, but I think it makes sense - having the %s makes the message better, will add

"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
Expand All @@ -29,7 +58,7 @@ def _get_threshold(self) -> Decimal:

def _get_target(
self, comparison: ComparisonProxy | FilteredComparison
) -> Decimal | None:
) -> tuple[Decimal | None, bool]:
"""
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.
Expand All @@ -39,21 +68,24 @@ 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)
if comparison.has_project_coverage_base_report()
and comparison.project_coverage_base.report.totals.coverage is not None
else None
)
return target_coverage
is_custom_target = False
return target_coverage, is_custom_target

def get_patch_status(
self, comparison: ComparisonProxy | FilteredComparison
) -> tuple[str, str]:
self, comparison: ComparisonProxy | FilteredComparison, notification_type: str
) -> StatusResult:
threshold = self._get_threshold()
target_coverage = self._get_target(comparison)
target_coverage, is_custom_target = self._get_target(comparison)
totals = comparison.get_patch_totals()
included_helper_text = {}

# coverage affected
if totals and totals.lines > 0:
Expand Down Expand Up @@ -84,7 +116,16 @@ def get_patch_status(
message = (
f"{coverage_rounded}% of diff hit (target {target_rounded}%)"
)
return state, message
# 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also decided I'm not going to add the helper text to this message, so the resulting message for check or status will be unchanged

return StatusResult(
state=state, message=message, included_helper_text=included_helper_text
)

# coverage not affected
if comparison.project_coverage_base.commit:
Expand All @@ -94,10 +135,16 @@ def get_patch_status(
)
else:
description = "Coverage not affected"
return StatusState.success.value, description
return StatusResult(
state=StatusState.success.value,
message=description,
included_helper_text=included_helper_text,
)


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
Expand Down Expand Up @@ -144,6 +191,7 @@ def get_changes_status(

class StatusProjectMixin(object):
DEFAULT_REMOVED_CODE_BEHAVIOR = "adjust_base"
context = "project"

def _apply_removals_only_behavior(
self, comparison: ComparisonProxy | FilteredComparison
Expand Down Expand Up @@ -420,6 +468,11 @@ 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
Expand Down
18 changes: 12 additions & 6 deletions services/notification/notifiers/status/patch.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from database.enums import Notification
from services.comparison import ComparisonProxy, FilteredComparison
from services.notification.notifiers.mixins.status import StatusPatchMixin
from services.notification.notifiers.mixins.status import StatusPatchMixin, StatusResult
from services.notification.notifiers.status.base import StatusNotifier


Expand All @@ -17,18 +17,24 @@
"""

context = "patch"
notification_type_display_name = "status"

@property
def notification_type(self) -> Notification:
return Notification.status_patch

def build_payload(self, comparison: ComparisonProxy | FilteredComparison) -> dict:
def build_payload(
self, comparison: ComparisonProxy | FilteredComparison
) -> StatusResult:
if self.is_empty_upload():
state, message = self.get_status_check_for_empty_upload()
return {"state": state, "message": message}
result = StatusResult(state=state, message=message, included_helper_text={})
return result

Check warning on line 32 in services/notification/notifiers/status/patch.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/notification/notifiers/status/patch.py#L31-L32

Added lines #L31 - L32 were not covered by tests

state, message = self.get_patch_status(comparison)
result = self.get_patch_status(
comparison, notification_type=self.notification_type_display_name
)
if self.should_use_upgrade_decoration():
message = self.get_upgrade_message()
result["message"] = self.get_upgrade_message()

return {"state": state, "message": message}
return result
Loading
Loading