Skip to content

Commit

Permalink
feat: add message when test_result is setup
Browse files Browse the repository at this point in the history
To help users confirm that test_results is setup correctly we want to display
a message near the header saying that that is the case and all tests passed.

These changes accomplish that by creating a new `NotificationContext` in the
`ComparisonProxy` and carrying the info to the notifier.
  • Loading branch information
giovanni-guidini committed Mar 15, 2024
1 parent 600e403 commit 7e662d2
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 7 deletions.
16 changes: 15 additions & 1 deletion services/comparison/__init__.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions services/notification/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions services/notification/notifiers/mixins/message/sections.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
":heavy_check_mark: Test ingestion set up successfully. 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
Expand All @@ -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:
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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 (

Check warning on line 322 in services/notification/notifiers/mixins/message/sections.py

View check run for this annotation

Codecov Public QA / codecov/patch

services/notification/notifiers/mixins/message/sections.py#L321-L322

Added lines #L321 - L322 were not covered by tests

Check warning on line 322 in services/notification/notifiers/mixins/message/sections.py

View check run for this annotation

Codecov - QA / codecov/patch

services/notification/notifiers/mixins/message/sections.py#L321-L322

Added lines #L321 - L322 were not covered by tests

Check warning on line 322 in services/notification/notifiers/mixins/message/sections.py

View check run for this annotation

Codecov / codecov/patch

services/notification/notifiers/mixins/message/sections.py#L321-L322

Added lines #L321 - L322 were not covered by tests
":heavy_check_mark: Test ingestion set up successfully. No failed tests found. :relaxed:"
)


class AnnouncementSectionWriter(BaseSectionWriter):
ats_message = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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.",
"",
":heavy_check_mark: Test ingestion set up successfully. 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)",
Expand Down
62 changes: 62 additions & 0 deletions services/notification/notifiers/tests/unit/test_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).",
"",
":heavy_check_mark: Test ingestion set up successfully. No failed tests found. :relaxed:",
]

@pytest.mark.asyncio
async def test_new_header_section_writer_no_project_coverage(
self, mocker, sample_comparison
Expand Down Expand Up @@ -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:",
"",
":heavy_check_mark: Test ingestion set up successfully. No failed tests found. :relaxed:",
]


class TestAnnouncementsSectionWriter(object):
@pytest.mark.asyncio
Expand Down
12 changes: 10 additions & 2 deletions tasks/notify.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 7e662d2

Please sign in to comment.