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

feat: add message when test_result is setup #331

Merged
merged 2 commits into from
Mar 15, 2024
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
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 @@


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
Expand All @@ -126,6 +133,10 @@

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 @@
"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 @@
"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 - 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 Public QA / codecov/patch

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

Added lines #L321 - L322 were not covered by tests
":white_check_mark: All tests successful. No failed tests found :relaxed:"
)
Copy link

@rohan-at-sentry rohan-at-sentry Mar 15, 2024

Choose a reason for hiding this comment

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

Looks like this message will fire every time the all_test_passed bool is true. @giovanni-guidini can you confirm?

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 is accurate.
Keep in mind that if the setup for test_results is complete but not all tests passed then we don't notify with this notifier.

Copy link

@rohan-at-sentry rohan-at-sentry Mar 15, 2024

Choose a reason for hiding this comment

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

in that case... after first time setup every time all test pass, we'll show a message titled "Test ingestion set up successfully..."

Which is a little weird..

Maybe we alter that message to say "All tests successful... ☺️ " instead. wdyt @giovanni-guidini @Adal3n3 . That way it gives an indication that test result ingestion setup was successful and provides a meaningful update on subsequent runs, without extra conditional checks

Choose a reason for hiding this comment

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

@Adal3n3 @giovanni-guidini and @rohan-at-sentry aligned

Tweak the message to

✅ All tests successful. No failed tests found ☺️



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.",
"",
":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)",
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).",
"",
":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
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:",
"",
":white_check_mark: All tests successful. 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
Loading