Skip to content

Commit

Permalink
Fix notifications with an empty HEAD report
Browse files Browse the repository at this point in the history
There were a couple of errors spread around the notification code assuming that a `head.report` always exists, which is not necessarily the case.

In particular that would be the case when using `empty-upload` or `manual_trigger`.
  • Loading branch information
Swatinem committed Oct 24, 2024
1 parent 02c2e2b commit 48ae32b
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 5 deletions.
3 changes: 2 additions & 1 deletion services/notification/notifiers/codecov_slack_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ def build_payload(self, comparison: ComparisonProxy) -> dict:
message = "unknown"
notation = ""
comparison_url = None
head_report = comparison.head.report
return {
"url": comparison_url,
"message": message,
Expand All @@ -99,7 +100,7 @@ def build_payload(self, comparison: ComparisonProxy) -> dict:
if comparison.project_coverage_base
else None
),
"head_totals_c": str(comparison.head.report.totals.coverage),
"head_totals_c": str(head_report.totals.coverage) if head_report else "0",
}

def notify(self, comparison: ComparisonProxy) -> NotificationResult:
Expand Down
7 changes: 5 additions & 2 deletions services/notification/notifiers/mixins/status.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,11 @@ def get_project_status(
def _get_project_status(
self, comparison: ComparisonProxy | FilteredComparison
) -> tuple[str, str]:
head_report_totals = comparison.head.report.totals
if head_report_totals.coverage is None:
if (
not comparison.head.report
or (head_report_totals := comparison.head.report.totals) is None
or head_report_totals.coverage is None
):
state = self.notifier_yaml_settings.get("if_not_found", "success")
message = "No coverage information found on head"
return (state, message)
Expand Down
6 changes: 5 additions & 1 deletion tasks/notify.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ def run_impl_within_lock(
and not self.send_notifications_if_commit_differs_from_pulls_head(
commit, enriched_pull, current_yaml
)
and empty_upload is None
):
log.info(
"Not sending notifications for commit when it differs from pull's most recent head",
Expand Down Expand Up @@ -531,7 +532,10 @@ def save_patch_totals(self, comparison: ComparisonProxy) -> None:
This is done to make sure the patch coverage reported by notifications and UI is the same
(because they come from the same source)
"""
if comparison.comparison.patch_coverage_base_commitid is None:
if (
comparison.comparison.patch_coverage_base_commitid is None
or not comparison.has_head_report()
):
# Can't get diff to calculate patch totals
return
head_commit = comparison.head.commit
Expand Down
82 changes: 81 additions & 1 deletion tasks/tests/integration/test_notify_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from unittest.mock import patch

import pytest
from mock import PropertyMock
from mock import AsyncMock, PropertyMock
from shared.validation.types import CoverageCommentRequiredChanges

from database.models import Pull
Expand All @@ -11,6 +11,7 @@
from services.archive import ArchiveService
from services.comparison import get_or_create_comparison
from services.notification.notifiers.base import NotificationResult
from services.repository import EnrichedPull
from tasks.notify import NotifyTask

sample_token = "ghp_test6ldgmyaglf73gcnbi0kprz7dyjz6nzgn"
Expand Down Expand Up @@ -1314,3 +1315,82 @@ def test_notifier_call_no_head_commit_report(
"reason": "no_head_report",
}
assert result == expected_result

@patch("requests.post")
def test_notifier_call_no_head_commit_report_empty_upload(
self,
mock_post_request,
dbsession,
mocker,
codecov_vcr,
mock_storage,
mock_configuration,
mock_repo_provider,
):
mock_configuration.params["setup"]["codecov_dashboard_url"] = (
"https://codecov.io"
)
mocked_app = mocker.patch.object(NotifyTask, "app")
repository = RepositoryFactory.create(
owner__unencrypted_oauth_token=sample_token,
owner__username="ThiagoCodecov",
owner__service="github",
owner__service_id="44376991",
name="example-python",
)
dbsession.add(repository)
dbsession.flush()
master_commit = CommitFactory.create(
message="",
pullid=None,
branch="master",
commitid="17a71a9a2f5335ed4d00496c7bbc6405f547a527",
repository=repository,
)
commit = CommitFactory.create(
message="",
pullid=1234,
branch="test-branch-1",
commitid="649eaaf2924e92dc7fd8d370ddb857033231e67a",
repository=repository,
_report_json=None,
)
dbsession.add(commit)
dbsession.add(master_commit)
dbsession.flush()
pull = PullFactory.create(
repository=repository,
pullid=1234,
head=commit.commitid,
base=master_commit.commitid,
)
dbsession.add(pull)
dbsession.flush()
task = NotifyTask()
with open("tasks/tests/samples/sample_chunks_1.txt") as f:
content = f.read().encode()
archive_hash = ArchiveService.get_archive_hash(commit.repository)
master_chunks_url = (
f"v4/repos/{archive_hash}/commits/{master_commit.commitid}/chunks.txt"
)
mock_storage.write_file("archive", master_chunks_url, content)

mocker.patch("tasks.notify.NotifyTask.fetch_and_update_whether_ci_passed")
mocker.patch(
"tasks.notify.fetch_and_update_pull_request_information_from_commit",
return_value=EnrichedPull(database_pull=pull, provider_pull=None),
)
mock_repo_provider.get_commit_statuses = AsyncMock(return_value=None)

result = task.run_impl_within_lock(
dbsession,
repoid=commit.repoid,
commitid=commit.commitid,
current_yaml={"coverage": {"status": {"project": True}}},
empty_upload="pass",
)

assert result["notified"]
assert len(result["notifications"]) == 2
assert result["notifications"][0]["result"] is not None
assert result["notifications"][1]["result"] is not None

0 comments on commit 48ae32b

Please sign in to comment.