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.

Sign up for GitHub

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

Jump to bottom

Add call to avoid status + checks at the same time #756

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Add call to avoid status + checks at the same time #756

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50832dc
Add call to avoid status + checks at the same time
adrian-codecov Oct 2, 2024
f22847c
get rid of prints
adrian-codecov Oct 3, 2024
901e799
Merge branch 'main' into 1967-adjust-checks-with-fallbacks
adrian-codecov Oct 4, 2024
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
26 changes: 24 additions & 2 deletions services/notification/notifiers/checks/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def notify(self, comparison: Comparison):
or comparison.enriched_pull.provider_pull is None
):
log.debug(
"Faling back to commit_status: Pull request not in provider",
"Falling back to commit_status: Pull request not in provider",
extra=dict(
notifier=self.name,
repoid=comparison.head.commit.repoid,
Expand All @@ -111,7 +111,7 @@ def notify(self, comparison: Comparison):
)
if comparison.pull.state != "open":
log.debug(
"Faling back to commit_status: Pull request closed",
"Falling back to commit_status: Pull request closed",
extra=dict(
notifier=self.name,
repoid=comparison.head.commit.repoid,
Expand Down Expand Up @@ -142,6 +142,28 @@ def notify(self, comparison: Comparison):
data_sent=None,
data_received=None,
)
# Check for existing statuses for this commit. If so, retain
# statuses and don't do a check as well
statuses = comparison.get_existing_statuses()
status_title = self.get_status_external_name()
if statuses and statuses.get(status_title):
log.debug(
"Falling back to commit_status: Status already exists for this commit",
extra=dict(
notifier=self.name,
repoid=comparison.head.commit.repoid,
notifier_title=self.title,
status_title=status_title,
commit=comparison.head.commit,
),
)
return NotificationResult(
notification_attempted=False,
notification_successful=None,
explanation="preexisting_commit_status",
data_sent=None,
data_received=None,
)
payload = None
try:
with nullcontext():
Expand Down
1 change: 1 addition & 0 deletions services/notification/notifiers/checks/checks_with_fallback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def notify(self, comparison):
res.explanation == "no_pull_request"
or res.explanation == "pull_request_not_in_provider"
or res.explanation == "pull_request_closed"
or res.explanation == "preexisting_commit_status"
):
log.info(
"Couldn't use checks notifier, falling back to status notifiers",
Expand Down
45 changes: 45 additions & 0 deletions services/notification/notifiers/tests/unit/test_checks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from shared.reports.readonly import ReadOnlyReport
from shared.reports.resources import Report, ReportFile, ReportLine
from shared.torngit.exceptions import TorngitClientGeneralError, TorngitError
from shared.torngit.status import Status
from shared.yaml.user_yaml import UserYaml

from services.decoration import Decoration
Expand Down Expand Up @@ -307,6 +308,7 @@ def test_checks_403_failure(self, sample_comparison, mocker, mock_repo_provider)
assert res == "success"

def test_checks_failure(self, sample_comparison, mocker, mock_repo_provider):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_repo_provider.create_check_run = Mock(
side_effect=TorngitClientGeneralError(
409, response_data="No Access", message="No Access"
Expand Down Expand Up @@ -1022,6 +1024,7 @@ def test_send_notification_annotations_paginations(
def test_notify(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
payload = {
Expand Down Expand Up @@ -1054,6 +1057,7 @@ def test_notify(
def test_notify_passing_empty_upload(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
mock_repo_provider.create_check_run.return_value = 2234563
Expand Down Expand Up @@ -1081,6 +1085,7 @@ def test_notify_passing_empty_upload(
def test_notify_failing_empty_upload(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
mock_repo_provider.create_check_run.return_value = 2234563
Expand Down Expand Up @@ -1108,6 +1113,7 @@ def test_notify_failing_empty_upload(
def test_notification_exception(
self, sample_comparison, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
notifier = PatchChecksNotifier(
repository=sample_comparison.head.commit.repository,
Expand Down Expand Up @@ -1153,6 +1159,34 @@ def test_notification_exception_not_fit(self, sample_comparison, mocker):
assert result.data_sent is None
assert result.data_received is None

def test_notification_exception_preexisting_commit_status(
self, sample_comparison, mocker, mock_repo_provider
):
comparison = sample_comparison
notifier = ProjectChecksNotifier(
repository=comparison.head.commit.repository,
title="title",
notifier_yaml_settings={"paths": ["file_1.go"]},
notifier_site_settings=True,
current_yaml=UserYaml({}),
)
mock_repo_provider.get_commit_statuses.return_value = Status(
[
{
"time": "2024-10-01T22:34:52Z",
"state": "success",
"description": "42.85% (+0.00%) compared to 36be7f3",
"context": "codecov/project/title",
}
]
)
result = notifier.notify(sample_comparison)
assert not result.notification_attempted
assert result.notification_successful is None
assert result.explanation == "preexisting_commit_status"
assert result.data_sent is None
assert result.data_received is None

def test_checks_with_after_n_builds(self, sample_comparison, mocker):
notifier = ChecksNotifier(
repository=sample_comparison.head.commit.repository,
Expand Down Expand Up @@ -1690,6 +1724,7 @@ def test_build_payload_no_base_report(
def test_check_notify_no_path_match(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
payload = {
Expand Down Expand Up @@ -1723,6 +1758,7 @@ def test_check_notify_no_path_match(
def test_check_notify_single_path_match(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
payload = {
Expand Down Expand Up @@ -1763,6 +1799,7 @@ def test_check_notify_single_path_match(
def test_check_notify_multiple_path_match(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
payload = {
Expand Down Expand Up @@ -1797,6 +1834,7 @@ def test_check_notify_multiple_path_match(
def test_check_notify_with_paths(
self, sample_comparison, mocker, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
comparison = sample_comparison
payload = {
Expand Down Expand Up @@ -1833,6 +1871,7 @@ def test_notify_pass_behavior_when_coverage_not_uploaded(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down Expand Up @@ -1878,6 +1917,7 @@ def test_notify_pass_behavior_when_coverage_uploaded(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down Expand Up @@ -1917,6 +1957,7 @@ def test_notify_include_behavior_when_coverage_not_uploaded(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down Expand Up @@ -1957,6 +1998,7 @@ def test_notify_exclude_behavior_when_coverage_not_uploaded(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down Expand Up @@ -1986,6 +2028,7 @@ def test_notify_exclude_behavior_when_coverage_uploaded(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down Expand Up @@ -2026,6 +2069,7 @@ def test_notify_exclude_behavior_when_some_coverage_uploaded(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down Expand Up @@ -2070,6 +2114,7 @@ def test_notify_exclude_behavior_no_flags(
mock_repo_provider,
mock_configuration,
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
mock_repo_provider.create_check_run.return_value = 2234563
mock_repo_provider.update_check_run.return_value = "success"
Expand Down
2 changes: 2 additions & 0 deletions services/notification/tests/unit/test_notification_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from celery.exceptions import SoftTimeLimitExceeded
from shared.plan.constants import PlanName
from shared.reports.resources import Report, ReportFile, ReportLine
from shared.torngit.status import Status
from shared.yaml import UserYaml

from database.enums import Decoration, Notification, NotificationState
Expand Down Expand Up @@ -461,6 +462,7 @@ def test_notify_individual_notifier_timeout(self, mocker, sample_comparison):
def test_notify_individual_checks_notifier(
self, mocker, sample_comparison, mock_repo_provider, mock_configuration
):
mock_repo_provider.get_commit_statuses.return_value = Status([])
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of defining this in every test thats using the function, how about adding this as a default to the mock_repo_provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a bad thought, but the mock_repo_provider is used in other files/tests so it may lead to unnecessary calls/undesired side effects. I also like it this way cause it makes it clearer to me what each test does, I find it better for readability :). I'll merge this tmrw since it's kinda late now

mock_configuration._params["setup"] = {"codecov_dashboard_url": "test"}
current_yaml = {}
commit = sample_comparison.head.commit
Expand Down
Loading