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

Do not trigger Notify twice from TestResultsFinisher #900

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Do not trigger Notify twice from TestResultsFinisher #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff1cfc5
Do not trigger `Notify` twice from `TestResultsFinisher`
Swatinem Nov 18, 2024
bc5307f
aaaaand fix the tests
Swatinem Nov 18, 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
51 changes: 15 additions & 36 deletions tasks/test_results_finisher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import logging
from dataclasses import dataclass
from typing import Any, Dict
from typing import Any

from asgiref.sync import async_to_sync
from shared.yaml import UserYaml
Expand Down Expand Up @@ -42,7 +42,6 @@
ESCAPE_FAILURE_MESSAGE_DEFN = [
Replacement(["\r"], "", EscapeEnum.REPLACE),
]
QUEUE_NOTIFY_KEY = "queue_notify"


@dataclass
Expand All @@ -56,7 +55,7 @@ class TestResultsFinisherTask(BaseCodecovTask, name=test_results_finisher_task_n
def run_impl(
self,
db_session: Session,
chord_result: Dict[str, Any],
chord_result: dict[str, Any],
*,
repoid: int,
commitid: str,
Expand Down Expand Up @@ -99,7 +98,7 @@ def run_impl(
previous_result=chord_result,
**kwargs,
)
if finisher_result[QUEUE_NOTIFY_KEY]:
if finisher_result["queue_notify"]:
self.app.tasks[notify_task_name].apply_async(
args=None,
kwargs=dict(
Expand All @@ -121,7 +120,7 @@ def process_impl_within_lock(
repoid: int,
commitid: str,
commit_yaml: UserYaml,
previous_result: Dict[str, Any],
previous_result: dict[str, Any],
**kwargs,
):
log.info(
Expand Down Expand Up @@ -176,9 +175,7 @@ def process_impl_within_lock(

if self.check_if_no_success(previous_result):
# every processor errored, nothing to notify on
metrics.incr(
"test_results.finisher.failure.no_successful_processing",
)
metrics.incr("test_results.finisher.failure.no_successful_processing")

queue_notify = False

Expand All @@ -199,7 +196,7 @@ def process_impl_within_lock(
return {
"notify_attempted": False,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: queue_notify,
"queue_notify": queue_notify,
}

# if we succeed once, error should be None for this commit forever
Expand Down Expand Up @@ -258,24 +255,14 @@ def process_impl_within_lock(
db_session.flush()

if failed_tests == 0:
metrics.incr(
"test_results.finisher.normal_notify_called.all_tests_passed",
)
self.app.tasks[notify_task_name].apply_async(
args=None,
kwargs=dict(
repoid=repoid, commitid=commitid, current_yaml=commit_yaml.to_dict()
),
)
metrics.incr("test_results.finisher.normal_notify_called.all_tests_passed")
return {
"notify_attempted": False,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: True,
"queue_notify": True,
}

metrics.incr(
"test_results.finisher.success.tests_failed",
)
metrics.incr("test_results.finisher.success.tests_failed")
repo_service = get_repo_provider_service(repo)
pull = async_to_sync(fetch_and_update_pull_request_information_from_commit)(
repo_service, commit, commit_yaml
Expand Down Expand Up @@ -320,17 +307,15 @@ def process_impl_within_lock(
log.info("Made upgrade comment", extra=self.extra_dict)

return {
"notify_attempted": False,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: False,
"notify_attempted": True,
"notify_succeeded": success,
"queue_notify": False,
}

flaky_tests = dict()

if should_do_flaky_detection(repo, commit_yaml):
flaky_tests = self.get_flaky_tests(
db_session, commit_yaml, repoid, failures
)
flaky_tests = self.get_flaky_tests(db_session, repoid, failures)

failures = sorted(failures, key=lambda x: x.duration_seconds)[:3]

Expand Down Expand Up @@ -369,19 +354,14 @@ def process_impl_within_lock(
extra=dict(
success=success,
notifier_result=notifier_result.value,
repoid=repoid,
commitid=commit.commitid,
test_ids=list(flaky_tests.keys()),
),
)
metrics.incr("test_results.finisher.detected_flaky_test")

self.extra_dict["success"] = success
self.extra_dict["notifier_result"] = notifier_result.value
log.info(
"Finished test results notify",
extra=self.extra_dict,
)
log.info("Finished test results notify", extra=self.extra_dict)

# using a var as a tag here will be fine as it's a boolean
metrics.incr(
Expand All @@ -391,13 +371,12 @@ def process_impl_within_lock(
return {
"notify_attempted": True,
"notify_succeeded": success,
QUEUE_NOTIFY_KEY: False,
"queue_notify": False,
}

def get_flaky_tests(
self,
db_session: Session,
commit_yaml: UserYaml,
repoid: int,
failures: list[TestResultsNotificationFailure],
) -> dict[str, FlakeInfo]:
Expand Down
28 changes: 12 additions & 16 deletions tasks/tests/unit/test_test_results_finisher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from services.repository import EnrichedPull
from services.test_results import generate_test_id
from services.urls import get_members_url
from tasks.test_results_finisher import QUEUE_NOTIFY_KEY, TestResultsFinisherTask
from tasks.test_results_finisher import TestResultsFinisherTask

here = Path(__file__)

Expand Down Expand Up @@ -358,7 +358,7 @@ def test_upload_finisher_task_call(
expected_result = {
"notify_attempted": True,
"notify_succeeded": True,
QUEUE_NOTIFY_KEY: False,
"queue_notify": False,
}

test_results_mock_app.tasks[
Expand Down Expand Up @@ -485,7 +485,7 @@ def test_upload_finisher_task_call_no_failures(
expected_result = {
"notify_attempted": False,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: True,
"queue_notify": True,
}
test_results_mock_app.tasks[
"app.tasks.notify.Notify"
Expand Down Expand Up @@ -542,7 +542,7 @@ def test_upload_finisher_task_call_no_success(
expected_result = {
"notify_attempted": False,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: True,
"queue_notify": True,
}

assert expected_result == result
Expand Down Expand Up @@ -625,14 +625,12 @@ def test_upload_finisher_task_call_upgrade_comment(
commit_yaml={"codecov": {"max_report_age": False}},
)

expected_result = {
"notify_attempted": False,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: False,
assert result == {
"notify_attempted": True,
"notify_succeeded": True,
"queue_notify": False,
}

assert expected_result == result

mock_repo_provider_comments.post_comment.assert_called_with(
pull.pullid,
f"The author of this PR, test_username, is not an activated member of this organization on Codecov.\nPlease [activate this user on Codecov]({get_members_url(pull)}) to display this PR comment.\nCoverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.\nPlease don't hesitate to email us at support@codecov.io with any questions.",
Expand Down Expand Up @@ -685,7 +683,7 @@ def test_upload_finisher_task_call_existing_comment(
expected_result = {
"notify_attempted": True,
"notify_succeeded": True,
QUEUE_NOTIFY_KEY: False,
"queue_notify": False,
}

test_results_mock_app.tasks[
Expand Down Expand Up @@ -812,7 +810,7 @@ def test_upload_finisher_task_call_comment_fails(
expected_result = {
"notify_attempted": True,
"notify_succeeded": False,
QUEUE_NOTIFY_KEY: False,
"queue_notify": False,
}

test_results_mock_app.tasks[
Expand Down Expand Up @@ -1041,7 +1039,7 @@ def test_upload_finisher_task_call_computed_name(
expected_result = {
"notify_attempted": True,
"notify_succeeded": True,
QUEUE_NOTIFY_KEY: False,
"queue_notify": False,
}

assert expected_result == result
Expand Down Expand Up @@ -1138,9 +1136,7 @@ def test_upload_finisher_task_call_main_with_plan(
test_results_setup,
plan,
):
mocked_get_flaky_tests = mocker.patch.object(
TestResultsFinisherTask, "get_flaky_tests"
)
mocker.patch.object(TestResultsFinisherTask, "get_flaky_tests")
mock_feature = mocker.patch("services.test_results.FLAKY_TEST_DETECTION")
mock_feature.check_value.return_value = True
commit_yaml = {
Expand Down
Loading