From 93ee90fe2ad43f02be9ebac5bc903a1798a834c3 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 6 Dec 2024 11:10:16 +0100 Subject: [PATCH] Resolve notifier `pull` on demand A recent change to load the `repo_provider` and `pull` ahead of time regressed the happy path of test results when no test failures are present. In that case, no notification has to be performed, so no `repo_provider` or `pull` is needed. Additionally to fixing this regression, this also fixes the case when the `pull` is legitimately `None`, but the on-demand loading was trying to fetch it once again. --- helpers/notifier.py | 21 +++++++++++---------- services/test_results.py | 8 -------- tasks/test_results_finisher.py | 22 +++++++--------------- 3 files changed, 18 insertions(+), 33 deletions(-) diff --git a/helpers/notifier.py b/helpers/notifier.py index f9b78ca61..fed700e9b 100644 --- a/helpers/notifier.py +++ b/helpers/notifier.py @@ -1,6 +1,7 @@ import logging from dataclasses import dataclass from enum import Enum +from typing import Literal from asgiref.sync import async_to_sync from shared.torngit.base import TorngitBaseAdapter @@ -27,17 +28,23 @@ class NotifierResult(Enum): class BaseNotifier: commit: Commit commit_yaml: UserYaml | None - _pull: EnrichedPull | None = None + _pull: EnrichedPull | None | Literal[False] = False _repo_service: TorngitBaseAdapter | None = None - def get_pull(self): + def get_pull(self, do_log=True) -> EnrichedPull | None: repo_service = self.get_repo_service() - if self._pull is None: + if self._pull is False: self._pull = async_to_sync( fetch_and_update_pull_request_information_from_commit )(repo_service, self.commit, self.commit_yaml) + if self._pull is None and do_log: + log.info( + "Not notifying since there is no pull request associated with this commit", + extra=dict(commitid=self.commit.commitid), + ) + return self._pull def get_repo_service(self) -> TorngitBaseAdapter: @@ -46,7 +53,7 @@ def get_repo_service(self) -> TorngitBaseAdapter: return self._repo_service - def send_to_provider(self, pull, message): + def send_to_provider(self, pull: EnrichedPull, message: str) -> bool: repo_service = self.get_repo_service() assert repo_service @@ -75,12 +82,6 @@ def build_message(self) -> str: def notify(self) -> NotifierResult: pull = self.get_pull() if pull is None: - log.info( - "Not notifying since there is no pull request associated with this commit", - extra=dict( - commitid=self.commit.commitid, - ), - ) return NotifierResult.NO_PULL message = self.build_message() diff --git a/services/test_results.py b/services/test_results.py index 51408444a..18ee6f4ff 100644 --- a/services/test_results.py +++ b/services/test_results.py @@ -296,10 +296,6 @@ def build_message(self) -> str: def error_comment(self): pull = self.get_pull() if pull is None: - log.info( - "Not notifying since there is no pull request associated with this commit", - extra=dict(commitid=self.commit.commitid), - ) return False, "no_pull" message = ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format." @@ -313,10 +309,6 @@ def error_comment(self): def upgrade_comment(self): pull = self.get_pull() if pull is None: - log.info( - "Not notifying since there is no pull request associated with this commit", - extra=dict(commitid=self.commit.commitid), - ) return False, "no_pull" db_pull = pull.database_pull diff --git a/tasks/test_results_finisher.py b/tasks/test_results_finisher.py index bff8845ab..1c6c7050c 100644 --- a/tasks/test_results_finisher.py +++ b/tasks/test_results_finisher.py @@ -149,11 +149,6 @@ def process_impl_within_lock( db_session.add(totals) db_session.flush() - repo_service = get_repo_provider_service(repo) - pull = async_to_sync(fetch_and_update_pull_request_information_from_commit)( - repo_service, commit, commit_yaml - ) - if self.check_if_no_success(previous_result): # every processor errored, nothing to notify on queue_notify = False @@ -161,9 +156,7 @@ def process_impl_within_lock( # if error is None this whole process should be a noop if totals.error is not None: # make an attempt to make test results comment - notifier = TestResultsNotifier( - commit, commit_yaml, _pull=pull, _repo_service=repo_service - ) + notifier = TestResultsNotifier(commit, commit_yaml) success, reason = notifier.error_comment() # also make attempt to make coverage comment @@ -237,6 +230,11 @@ def process_impl_within_lock( "queue_notify": True, } + repo_service = get_repo_provider_service(repo) + pull = async_to_sync(fetch_and_update_pull_request_information_from_commit)( + repo_service, commit, commit_yaml + ) + if pull is not None: activate_seat_info = determine_seat_activation(pull) @@ -283,7 +281,6 @@ def process_impl_within_lock( flaky_tests = self.get_flaky_tests(db_session, repoid, failures) failures = sorted(failures, key=lambda x: x.duration_seconds)[:3] - payload = TestResultsNotificationPayload( failed_tests, passed_tests, skipped_tests, failures, flaky_tests ) @@ -291,14 +288,9 @@ def process_impl_within_lock( commit, commit_yaml, payload=payload, _pull=pull, _repo_service=repo_service ) notifier_result = notifier.notify() + success = True if notifier_result is NotifierResult.COMMENT_POSTED else False TestResultsFlow.log(TestResultsFlow.TEST_RESULTS_NOTIFY) - match notifier_result: - case NotifierResult.COMMENT_POSTED: - success = True - case _: - success = False - if len(flaky_tests): log.info( "Detected failure on test that has been identified as flaky",