diff --git a/services/comparison/__init__.py b/services/comparison/__init__.py index 7dcc7f40f..368201fe4 100644 --- a/services/comparison/__init__.py +++ b/services/comparison/__init__.py @@ -1,5 +1,6 @@ import asyncio import logging +from dataclasses import dataclass from typing import List, Optional from shared.reports.changes import get_changes_using_rust, run_comparison_using_rust @@ -23,6 +24,13 @@ log = logging.getLogger(__name__) +@dataclass +class NotificationContext(object): + """Extra information not necessarily related to coverage that may affect notifications""" + + all_tests_passed: bool + + class ComparisonProxy(object): """The idea of this class is to produce a wrapper around Comparison with functionalities that @@ -42,7 +50,9 @@ class ComparisonProxy(object): comparison (Comparison): The original comparison we want to wrap and proxy """ - def __init__(self, comparison: Comparison): + def __init__( + self, comparison: Comparison, context: Optional[NotificationContext] = None + ): self.comparison = comparison self._repository_service = None self._adjusted_base_diff = None @@ -57,6 +67,7 @@ def __init__(self, comparison: Comparison): self._behind_by_lock = asyncio.Lock() self._archive_service = None self._overlays = {} + self.context = context def get_archive_service(self): if self._archive_service is None: @@ -241,6 +252,9 @@ async def get_behind_by(self): ] return self._behind_by + def all_tests_passed(self): + return self.context is not None and self.context.all_tests_passed + async def get_existing_statuses(self): async with self._existing_statuses_lock: if self._existing_statuses is None: diff --git a/services/notification/__init__.py b/services/notification/__init__.py index 4e2561a87..3e3255e62 100644 --- a/services/notification/__init__.py +++ b/services/notification/__init__.py @@ -16,7 +16,7 @@ from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME from helpers.metrics import metrics -from services.comparison.types import Comparison +from services.comparison import ComparisonProxy from services.decoration import Decoration from services.license import is_properly_licensed from services.notification.commit_notifications import ( @@ -186,7 +186,7 @@ def get_statuses(self, current_flags: List[str]): for component_status in self._get_component_statuses(current_flags): yield component_status - async def notify(self, comparison: Comparison) -> List[NotificationResult]: + async def notify(self, comparison: ComparisonProxy) -> List[NotificationResult]: if not is_properly_licensed(comparison.head.commit.get_db_session()): log.warning( "Not sending notifications because the system is not properly licensed" @@ -218,7 +218,7 @@ async def notify(self, comparison: Comparison) -> List[NotificationResult]: return results async def notify_individual_notifier( - self, notifier, comparison + self, notifier: AbstractBaseNotifier, comparison: ComparisonProxy ) -> NotificationResult: commit = comparison.head.commit base_commit = comparison.project_coverage_base.commit diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index 5d1bfb033..419a88f63 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -103,6 +103,13 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non class NewHeaderSectionWriter(BaseSectionWriter): + def _possibly_include_test_result_setup_confirmation(self, comparison): + if comparison.all_tests_passed(): + yield ("") + yield ( + ":white_check_mark: All tests successful. No failed tests found :relaxed:" + ) + async def do_write_section(self, comparison, diff, changes, links, behind_by=None): yaml = self.current_yaml base_report = comparison.project_coverage_base.report @@ -126,6 +133,10 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non hide_project_coverage = self.settings.get("hide_project_coverage", False) if hide_project_coverage: + for msg in self._possibly_include_test_result_setup_confirmation( + comparison + ): + yield msg return if base_report and head_report: @@ -197,6 +208,9 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non "Changes have been made to critical files, which contain lines commonly executed in production. [Learn more](https://docs.codecov.com/docs/impact-analysis)" ) + for msg in self._possibly_include_test_result_setup_confirmation(comparison): + yield msg + class HeaderSectionWriter(BaseSectionWriter): async def do_write_section(self, comparison, diff, changes, links, behind_by=None): @@ -303,6 +317,12 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non "Changes have been made to critical files, which contain lines commonly executed in production. [Learn more](https://docs.codecov.com/docs/impact-analysis)" ) + if comparison.all_tests_passed(): + yield ("") + yield ( + ":white_check_mark: All tests successful. No failed tests found :relaxed:" + ) + class AnnouncementSectionWriter(BaseSectionWriter): ats_message = ( diff --git a/services/notification/notifiers/tests/integration/test_comment.py b/services/notification/notifiers/tests/integration/test_comment.py index 1060cfb87..b07a53aa5 100644 --- a/services/notification/notifiers/tests/integration/test_comment.py +++ b/services/notification/notifiers/tests/integration/test_comment.py @@ -4,7 +4,7 @@ from shared.reports.readonly import ReadOnlyReport from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory -from services.comparison import ComparisonProxy +from services.comparison import ComparisonProxy, NotificationContext from services.comparison.types import Comparison, EnrichedPull, FullCommit from services.decoration import Decoration from services.notification.notifiers.comment import CommentNotifier @@ -337,6 +337,7 @@ def sample_comparison_for_limited_upload( class TestCommentNotifierIntegration(object): @pytest.mark.asyncio async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration): + sample_comparison.context = NotificationContext(all_tests_passed=True) mock_configuration._params["setup"] = { "codecov_url": None, "codecov_dashboard_url": None, @@ -359,6 +360,8 @@ async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration): "> Project coverage is 60.00%. Comparing base [(`5b174c2`)](https://app.codecov.io/gh/joseph-sentry/codecov-demo/commit/5b174c2b40d501a70c479e91025d5109b1ad5c1b?dropdown=coverage&el=desc) to head [(`5601846`)](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?dropdown=coverage&src=pr&el=desc).", "> Report is 2 commits behind head on main.", "", + ":white_check_mark: All tests successful. No failed tests found :relaxed:", + "", ":exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.", "", "[![Impacted file tree graph](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9/graphs/tree.svg?width=650&height=150&src=pr&token=abcdefghij)](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=tree)", diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index 51bb6a19e..9ae3474f9 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -16,6 +16,7 @@ from database.models.core import GithubAppInstallation from database.tests.factories import RepositoryFactory +from services.comparison import NotificationContext from services.comparison.overlays.critical_path import CriticalPathOverlay from services.decoration import Decoration from services.notification.notifiers.base import NotificationResult @@ -3991,6 +3992,37 @@ async def test_new_header_section_writer_with_behind_by( "> Report is 3 commits behind head on master.", ] + @pytest.mark.asyncio + async def test_new_header_section_writer_test_results_setup( + self, mocker, sample_comparison + ): + sample_comparison.context = NotificationContext(all_tests_passed=True) + writer = NewHeaderSectionWriter( + mocker.MagicMock(), + mocker.MagicMock(), + show_complexity=mocker.MagicMock(), + settings={}, + current_yaml=mocker.MagicMock(), + ) + mocker.patch( + "services.notification.notifiers.mixins.message.sections.round_number", + return_value=Decimal(0), + ) + res = list( + await writer.write_section( + sample_comparison, + None, + None, + links={"pull": "urlurl", "base": "urlurl"}, + ) + ) + assert res == [ + "All modified and coverable lines are covered by tests :white_check_mark:", + f"> Project coverage is 0%. Comparing base [(`{sample_comparison.project_coverage_base.commit.commitid[:7]}`)](urlurl?dropdown=coverage&el=desc) to head [(`{sample_comparison.head.commit.commitid[:7]}`)](urlurl?dropdown=coverage&src=pr&el=desc).", + "", + ":white_check_mark: All tests successful. No failed tests found :relaxed:", + ] + @pytest.mark.asyncio async def test_new_header_section_writer_no_project_coverage( self, mocker, sample_comparison @@ -4018,6 +4050,36 @@ async def test_new_header_section_writer_no_project_coverage( "All modified and coverable lines are covered by tests :white_check_mark:", ] + @pytest.mark.asyncio + async def test_new_header_section_writer_no_project_coverage_test_results_setup( + self, mocker, sample_comparison + ): + sample_comparison.context = NotificationContext(all_tests_passed=True) + writer = NewHeaderSectionWriter( + mocker.MagicMock(), + mocker.MagicMock(), + show_complexity=mocker.MagicMock(), + settings={"hide_project_coverage": True}, + current_yaml=mocker.MagicMock(), + ) + mocker.patch( + "services.notification.notifiers.mixins.message.sections.round_number", + return_value=Decimal(0), + ) + res = list( + await writer.write_section( + sample_comparison, + None, + None, + links={"pull": "urlurl", "base": "urlurl"}, + ) + ) + assert res == [ + "All modified and coverable lines are covered by tests :white_check_mark:", + "", + ":white_check_mark: All tests successful. No failed tests found :relaxed:", + ] + class TestAnnouncementsSectionWriter(object): @pytest.mark.asyncio diff --git a/tasks/notify.py b/tasks/notify.py index bd90395fa..b72427bdc 100644 --- a/tasks/notify.py +++ b/tasks/notify.py @@ -27,7 +27,7 @@ from services.activation import activate_user from services.billing import BillingPlan from services.commit_status import RepositoryCIFilter -from services.comparison import ComparisonProxy +from services.comparison import ComparisonProxy, NotificationContext from services.comparison.types import Comparison, FullCommit from services.decoration import determine_decoration_details from services.lock_manager import LockManager, LockType @@ -325,6 +325,10 @@ def run_impl_within_lock( head_report, enriched_pull, empty_upload, + all_tests_passed=( + test_result_commit_report is not None + and test_result_commit_report.test_result_totals is not None + ), ) self.log_checkpoint(kwargs, UploadFlow.NOTIFIED) log.info( @@ -378,6 +382,9 @@ def submit_third_party_notifications( head_report, enriched_pull: EnrichedPull, empty_upload=None, + # It's only true if the test_result processing is setup + # And all tests indeed passed + all_tests_passed: bool = False, ): # base_commit is an "adjusted" base commit; for project coverage, we # compare a PR head's report against its base's report, or if the base @@ -405,7 +412,8 @@ def submit_third_party_notifications( ), patch_coverage_base_commitid=patch_coverage_base_commitid, current_yaml=current_yaml, - ) + ), + context=NotificationContext(all_tests_passed=all_tests_passed), ) decoration_type = self.determine_decoration_type_from_pull(