From 0098f86f4fef92395d3df03153e39e7bee5e9b02 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 20 Sep 2024 15:12:56 +0200 Subject: [PATCH] WIP: Make `ComparisonProxy` sync --- services/comparison/__init__.py | 322 ++++++++---------- services/comparison/overlays/critical_path.py | 4 +- .../tests/unit/test_comparison_proxy.py | 25 +- services/comparison/types.py | 6 +- services/notification/notifiers/base.py | 6 +- .../notification/notifiers/checks/base.py | 58 ++-- .../notification/notifiers/checks/patch.py | 74 ++-- .../notification/notifiers/checks/project.py | 79 ++--- .../notifiers/comment/__init__.py | 70 ++-- .../notifiers/comment/conditions.py | 52 ++- .../notifiers/mixins/message/__init__.py | 24 +- .../notifiers/mixins/message/sections.py | 48 ++- .../notification/notifiers/mixins/status.py | 109 +++--- .../notification/notifiers/status/base.py | 38 +-- .../notification/notifiers/status/changes.py | 5 +- .../notification/notifiers/status/patch.py | 4 +- .../notification/notifiers/status/project.py | 4 +- .../notifiers/tests/unit/test_status.py | 148 ++++---- .../tests/unit/test_comparison.py | 11 +- tasks/compute_comparison.py | 11 +- 20 files changed, 486 insertions(+), 612 deletions(-) diff --git a/services/comparison/__init__.py b/services/comparison/__init__.py index c18300077..d43b02ece 100644 --- a/services/comparison/__init__.py +++ b/services/comparison/__init__.py @@ -1,19 +1,16 @@ import asyncio import logging from dataclasses import dataclass -from typing import Dict, List, Optional import sentry_sdk +from asgiref.sync import async_to_sync from shared.reports.changes import get_changes_using_rust, run_comparison_using_rust from shared.reports.types import Change, ReportTotals -from shared.torngit.exceptions import ( - TorngitClientGeneralError, -) +from shared.torngit.exceptions import TorngitClientGeneralError from shared.utils.sessions import SessionType from database.enums import CompareCommitState, TestResultsProcessingError from database.models import CompareCommit -from helpers.metrics import metrics from services.archive import ArchiveService from services.comparison.changes import get_changes from services.comparison.overlays import get_overlay @@ -58,7 +55,7 @@ class ComparisonProxy(object): """ def __init__( - self, comparison: Comparison, context: Optional[ComparisonContext] = None + self, comparison: Comparison, context: ComparisonContext | None = None ): self.comparison = comparison self._repository_service = None @@ -69,14 +66,11 @@ def __init__( self._existing_statuses = None self._behind_by = None self._branch = None - self._diff_lock = asyncio.Lock() - self._changes_lock = asyncio.Lock() - self._existing_statuses_lock = asyncio.Lock() self._behind_by_lock = asyncio.Lock() self._archive_service = None self._overlays = {} self.context = context or ComparisonContext() - self._cached_reports_uploaded_per_flag: List[ReportUploadedCount] | None = None + self._cached_reports_uploaded_per_flag: list[ReportUploadedCount] | None = None def get_archive_service(self): if self._archive_service is None: @@ -85,7 +79,6 @@ def get_archive_service(self): ) return self._archive_service - @metrics.timer("internal.services.comparison.get_filtered_comparison") def get_filtered_comparison(self, flags, path_patterns): if not flags and not path_patterns: return self @@ -122,156 +115,144 @@ def enriched_pull(self): def pull(self): return self.comparison.pull - async def get_diff(self, use_original_base=False): - async with self._diff_lock: - head = self.comparison.head.commit - base = self.comparison.project_coverage_base.commit - patch_coverage_base_commitid = self.comparison.patch_coverage_base_commitid + def get_diff(self, use_original_base=False): + head = self.comparison.head.commit + base = self.comparison.project_coverage_base.commit + patch_coverage_base_commitid = self.comparison.patch_coverage_base_commitid - # If the original and adjusted bases are the same commit, then if we - # already fetched the diff for one we can return it for the other. - bases_match = patch_coverage_base_commitid == ( - base.commitid if base else "" - ) + # If the original and adjusted bases are the same commit, then if we + # already fetched the diff for one we can return it for the other. + bases_match = patch_coverage_base_commitid == (base.commitid if base else "") - populate_original_base_diff = use_original_base and ( - not self._original_base_diff - ) - populate_adjusted_base_diff = (not use_original_base) and ( - not self._adjusted_base_diff + populate_original_base_diff = use_original_base and ( + not self._original_base_diff + ) + populate_adjusted_base_diff = (not use_original_base) and ( + not self._adjusted_base_diff + ) + if populate_original_base_diff: + if bases_match and self._adjusted_base_diff: + self._original_base_diff = self._adjusted_base_diff + elif patch_coverage_base_commitid is not None: + pull_diff = async_to_sync(self.repository_service.get_compare)( + patch_coverage_base_commitid, head.commitid, with_commits=False + ) + self._original_base_diff = pull_diff["diff"] + else: + return None + elif populate_adjusted_base_diff: + if bases_match and self._original_base_diff: + self._adjusted_base_diff = self._original_base_diff + elif base is not None: + pull_diff = async_to_sync(self.repository_service.get_compare)( + base.commitid, head.commitid, with_commits=False + ) + self._adjusted_base_diff = pull_diff["diff"] + else: + return None + + if use_original_base: + return self._original_base_diff + else: + return self._adjusted_base_diff + + def get_changes(self) -> list[Change] | None: + if self._changes is None: + diff = self.get_diff() + self._changes = get_changes( + self.comparison.project_coverage_base.report, + self.comparison.head.report, + diff, ) - if populate_original_base_diff: - if bases_match and self._adjusted_base_diff: - self._original_base_diff = self._adjusted_base_diff - elif patch_coverage_base_commitid is not None: - pull_diff = await self.repository_service.get_compare( - patch_coverage_base_commitid, head.commitid, with_commits=False - ) - self._original_base_diff = pull_diff["diff"] - else: - return None - elif populate_adjusted_base_diff: - if bases_match and self._original_base_diff: - self._adjusted_base_diff = self._original_base_diff - elif base is not None: - pull_diff = await self.repository_service.get_compare( - base.commitid, head.commitid, with_commits=False + if ( + self.comparison.project_coverage_base.report is not None + and self.comparison.head.report is not None + and self.comparison.project_coverage_base.report.rust_report is not None + and self.comparison.head.report.rust_report is not None + ): + rust_changes = get_changes_using_rust( + self.comparison.project_coverage_base.report, + self.comparison.head.report, + diff, + ) + original_paths = set([c.path for c in self._changes]) + new_paths = set([c.path for c in rust_changes]) + if original_paths != new_paths: + only_on_new = sorted(new_paths - original_paths) + only_on_original = sorted(original_paths - new_paths) + log.info( + "There are differences between python changes and rust changes", + extra=dict( + only_on_new=only_on_new[:100], + only_on_original=only_on_original[:100], + repoid=self.head.commit.repoid, + ), ) - self._adjusted_base_diff = pull_diff["diff"] - else: - return None - if use_original_base: - return self._original_base_diff - else: - return self._adjusted_base_diff - - async def get_changes(self) -> Optional[List[Change]]: - # Just make sure to not cause a deadlock between this and get_diff - async with self._changes_lock: - if self._changes is None: - diff = await self.get_diff() - with metrics.timer( - "internal.worker.services.comparison.changes.get_changes_python" - ): - self._changes = get_changes( - self.comparison.project_coverage_base.report, - self.comparison.head.report, - diff, - ) - if ( - self.comparison.project_coverage_base.report is not None - and self.comparison.head.report is not None - and self.comparison.project_coverage_base.report.rust_report - is not None - and self.comparison.head.report.rust_report is not None - ): - with metrics.timer( - "internal.worker.services.comparison.changes.get_changes_rust" - ): - rust_changes = get_changes_using_rust( - self.comparison.project_coverage_base.report, - self.comparison.head.report, - diff, - ) - original_paths = set([c.path for c in self._changes]) - new_paths = set([c.path for c in rust_changes]) - if original_paths != new_paths: - only_on_new = sorted(new_paths - original_paths) - only_on_original = sorted(original_paths - new_paths) - log.info( - "There are differences between python changes and rust changes", - extra=dict( - only_on_new=only_on_new[:100], - only_on_original=only_on_original[:100], - repoid=self.head.commit.repoid, - ), - ) - return self._changes + return self._changes @sentry_sdk.trace - async def get_patch_totals(self) -> ReportTotals | None: + def get_patch_totals(self) -> ReportTotals | None: """Returns the patch coverage for the comparison. Patch coverage refers to looking at the coverage in HEAD report filtered by the git diff HEAD..BASE. """ if self._patch_totals: return self._patch_totals - diff = await self.get_diff(use_original_base=True) + diff = self.get_diff(use_original_base=True) self._patch_totals = self.head.report.apply_diff(diff) return self._patch_totals - async def get_behind_by(self): - async with self._behind_by_lock: - if self._behind_by is None: - if not getattr( - self.comparison.project_coverage_base.commit, "commitid", None - ): - log.info( - "Comparison base commit does not have commitid, unable to get behind_by" + def get_behind_by(self): + if self._behind_by is None: + if not getattr( + self.comparison.project_coverage_base.commit, "commitid", None + ): + log.info( + "Comparison base commit does not have commitid, unable to get behind_by" + ) + return None + + provider_pull = self.comparison.enriched_pull.provider_pull + if provider_pull is None: + log.info( + "Comparison does not have provider pull request information, unable to get behind_by" + ) + return None + branch_to_get = provider_pull["base"]["branch"] + if self._branch is None: + try: + branch_response = async_to_sync(self.repository_service.get_branch)( + branch_to_get + ) + except TorngitClientGeneralError: + log.warning( + "Unable to fetch base branch from Git provider", + extra=dict( + branch=branch_to_get, + ), ) return None - - provider_pull = self.comparison.enriched_pull.provider_pull - if provider_pull is None: - log.info( - "Comparison does not have provider pull request information, unable to get behind_by" + except KeyError: + log.warning( + "Error fetching base branch from Git provider", + extra=dict( + branch=branch_to_get, + ), ) return None - branch_to_get = provider_pull["base"]["branch"] - if self._branch is None: - try: - branch_response = await self.repository_service.get_branch( - branch_to_get - ) - except TorngitClientGeneralError: - log.warning( - "Unable to fetch base branch from Git provider", - extra=dict( - branch=branch_to_get, - ), - ) - return None - except KeyError: - log.warning( - "Error fetching base branch from Git provider", - extra=dict( - branch=branch_to_get, - ), - ) - return None - self._branch = branch_response - - distance = await self.repository_service.get_distance_in_commits( - self._branch["sha"], - self.comparison.project_coverage_base.commit.commitid, - with_commits=False, - ) - self._behind_by = distance["behind_by"] - self.enriched_pull.database_pull.behind_by = distance["behind_by"] - self.enriched_pull.database_pull.behind_by_commit = distance[ - "behind_by_commit" - ] + self._branch = branch_response + + distance = async_to_sync(self.repository_service.get_distance_in_commits)( + self._branch["sha"], + self.comparison.project_coverage_base.commit.commitid, + with_commits=False, + ) + self._behind_by = distance["behind_by"] + self.enriched_pull.database_pull.behind_by = distance["behind_by"] + self.enriched_pull.database_pull.behind_by_commit = distance[ + "behind_by_commit" + ] return self._behind_by def all_tests_passed(self): @@ -280,30 +261,28 @@ def all_tests_passed(self): def test_results_error(self): return self.context is not None and self.context.test_results_error - async def get_existing_statuses(self): - async with self._existing_statuses_lock: - if self._existing_statuses is None: - self._existing_statuses = ( - await self.repository_service.get_commit_statuses( - self.head.commit.commitid - ) - ) - return self._existing_statuses + def get_existing_statuses(self): + if self._existing_statuses is None: + self._existing_statuses = async_to_sync( + self.repository_service.get_commit_statuses + )(self.head.commit.commitid) + return self._existing_statuses def get_overlay(self, overlay_type, **kwargs): if overlay_type not in self._overlays: self._overlays[overlay_type] = get_overlay(overlay_type, self, **kwargs) return self._overlays[overlay_type] - async def get_impacted_files(self): - files_in_diff = await self.get_diff() + @sentry_sdk.trace + def get_impacted_files(self) -> dict: + files_in_diff = self.get_diff() return run_comparison_using_rust( self.comparison.project_coverage_base.report, self.comparison.head.report, files_in_diff, ) - def get_reports_uploaded_count_per_flag(self) -> List[ReportUploadedCount]: + def get_reports_uploaded_count_per_flag(self) -> list[ReportUploadedCount]: """This function counts how many reports (by flag) the BASE and HEAD commit have.""" if self._cached_reports_uploaded_per_flag: # Reports may have many sessions, so it's useful to memoize this function @@ -317,7 +296,7 @@ def get_reports_uploaded_count_per_flag(self) -> List[ReportUploadedCount]: ), ) return [] - per_flag_dict: Dict[str, ReportUploadedCount] = dict() + per_flag_dict: dict[str, ReportUploadedCount] = dict() base_report = self.comparison.project_coverage_base.report head_report = self.comparison.head.report ops = [(base_report, "base_count"), (head_report, "head_count")] @@ -341,7 +320,7 @@ def get_reports_uploaded_count_per_flag(self) -> List[ReportUploadedCount]: self._cached_reports_uploaded_per_flag = list(per_flag_dict.values()) return self._cached_reports_uploaded_per_flag - def get_reports_uploaded_count_per_flag_diff(self) -> List[ReportUploadedCount]: + def get_reports_uploaded_count_per_flag_diff(self) -> list[ReportUploadedCount]: """ Returns the difference, per flag, or reports uploaded in BASE and HEAD @@ -395,28 +374,27 @@ def __init__(self, real_comparison: ComparisonProxy, *, flags, path_patterns): commit=real_comparison.head.commit, report=real_comparison.head.report.filter(flags=flags, paths=path_patterns), ) - self._changes_lock = asyncio.Lock() - async def get_impacted_files(self): - return await self.real_comparison.get_impacted_files() + def get_impacted_files(self) -> dict: + return self.real_comparison.get_impacted_files() - async def get_diff(self, use_original_base=False): - return await self.real_comparison.get_diff(use_original_base=use_original_base) + def get_diff(self, use_original_base=False): + return self.real_comparison.get_diff(use_original_base=use_original_base) @sentry_sdk.trace - async def get_patch_totals(self) -> ReportTotals | None: + def get_patch_totals(self) -> ReportTotals | None: """Returns the patch coverage for the comparison. Patch coverage refers to looking at the coverage in HEAD report filtered by the git diff HEAD..BASE. """ if self._patch_totals: return self._patch_totals - diff = await self.get_diff(use_original_base=True) + diff = self.get_diff(use_original_base=True) self._patch_totals = self.head.report.apply_diff(diff) return self._patch_totals - async def get_existing_statuses(self): - return await self.real_comparison.get_existing_statuses() + def get_existing_statuses(self): + return self.real_comparison.get_existing_statuses() def has_project_coverage_base_report(self): return self.real_comparison.has_project_coverage_base_report() @@ -425,15 +403,13 @@ def has_project_coverage_base_report(self): def enriched_pull(self): return self.real_comparison.enriched_pull - async def get_changes(self) -> Optional[List[Change]]: - # Just make sure to not cause a deadlock between this and get_diff - async with self._changes_lock: - if self._changes is None: - diff = await self.get_diff() - self._changes = get_changes( - self.project_coverage_base.report, self.head.report, diff - ) - return self._changes + def get_changes(self) -> list[Change] | None: + if self._changes is None: + diff = self.get_diff() + self._changes = get_changes( + self.project_coverage_base.report, self.head.report, diff + ) + return self._changes @property def pull(self): diff --git a/services/comparison/overlays/critical_path.py b/services/comparison/overlays/critical_path.py index dcf1d6358..1d1f40ed8 100644 --- a/services/comparison/overlays/critical_path.py +++ b/services/comparison/overlays/critical_path.py @@ -122,11 +122,11 @@ async def search_files_for_critical_changes( ) return list(critical_files_from_profiling | critical_files_from_yaml) - async def find_impacted_endpoints(self): + def find_impacted_endpoints(self): analyzer = self.full_analyzer if analyzer is None: return None - diff = rustify_diff(await self._comparison.get_diff()) + diff = rustify_diff(self._comparison.get_diff()) return self.full_analyzer.find_impacted_endpoints( self._comparison.project_coverage_base.report.rust_report.get_report(), self._comparison.head.report.rust_report.get_report(), diff --git a/services/comparison/tests/unit/test_comparison_proxy.py b/services/comparison/tests/unit/test_comparison_proxy.py index d4ab458aa..399d0bade 100644 --- a/services/comparison/tests/unit/test_comparison_proxy.py +++ b/services/comparison/tests/unit/test_comparison_proxy.py @@ -1,4 +1,3 @@ -import pytest from mock import call, patch from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory @@ -44,12 +43,11 @@ def make_sample_comparison(adjusted_base=False): class TestComparisonProxy(object): compare_url = "https://api.github.com/repos/{}/compare/{}...{}" - @pytest.mark.asyncio @patch("shared.torngit.github.Github.get_compare") - async def test_get_diff_adjusted_base(self, mock_get_compare): + def test_get_diff_adjusted_base(self, mock_get_compare): comparison = make_sample_comparison(adjusted_base=True) mock_get_compare.return_value = {"diff": "magic string"} - result = await comparison.get_diff(use_original_base=False) + result = comparison.get_diff(use_original_base=False) assert result == "magic string" assert comparison._adjusted_base_diff == "magic string" @@ -67,12 +65,11 @@ async def test_get_diff_adjusted_base(self, mock_get_compare): ), ] - @pytest.mark.asyncio @patch("shared.torngit.github.Github.get_compare") - async def test_get_diff_original_base(self, mock_get_compare): + def test_get_diff_original_base(self, mock_get_compare): comparison = make_sample_comparison(adjusted_base=True) mock_get_compare.return_value = {"diff": "magic string"} - result = await comparison.get_diff(use_original_base=True) + result = comparison.get_diff(use_original_base=True) assert result == "magic string" assert comparison._original_base_diff == "magic string" @@ -90,12 +87,11 @@ async def test_get_diff_original_base(self, mock_get_compare): ), ] - @pytest.mark.asyncio @patch("shared.torngit.github.Github.get_compare") - async def test_get_diff_bases_match_original_base(self, mock_get_compare): + def test_get_diff_bases_match_original_base(self, mock_get_compare): comparison = make_sample_comparison(adjusted_base=False) mock_get_compare.return_value = {"diff": "magic string"} - result = await comparison.get_diff(use_original_base=True) + result = comparison.get_diff(use_original_base=True) assert result == "magic string" assert comparison._original_base_diff == "magic string" @@ -106,7 +102,7 @@ async def test_get_diff_bases_match_original_base(self, mock_get_compare): # In this test case, the adjusted and original base commits are the # same. If we get one, we should set the cache for the other. - adjusted_base_result = await comparison.get_diff(use_original_base=False) + adjusted_base_result = comparison.get_diff(use_original_base=False) assert comparison._adjusted_base_diff == "magic string" # Make sure we only called the Git provider API once @@ -118,12 +114,11 @@ async def test_get_diff_bases_match_original_base(self, mock_get_compare): ), ] - @pytest.mark.asyncio @patch("shared.torngit.github.Github.get_compare") - async def test_get_diff_bases_match_adjusted_base(self, mock_get_compare): + def test_get_diff_bases_match_adjusted_base(self, mock_get_compare): comparison = make_sample_comparison(adjusted_base=False) mock_get_compare.return_value = {"diff": "magic string"} - result = await comparison.get_diff(use_original_base=False) + result = comparison.get_diff(use_original_base=False) assert result == "magic string" assert comparison._adjusted_base_diff == "magic string" @@ -134,7 +129,7 @@ async def test_get_diff_bases_match_adjusted_base(self, mock_get_compare): # In this test case, the adjusted and original base commits are the # same. If we get one, we should set the cache for the other. - adjusted_base_result = await comparison.get_diff(use_original_base=True) + adjusted_base_result = comparison.get_diff(use_original_base=True) assert comparison._adjusted_base_diff == "magic string" # Make sure we only called the Git provider API once diff --git a/services/comparison/types.py b/services/comparison/types.py index 603dab364..0229e8bca 100644 --- a/services/comparison/types.py +++ b/services/comparison/types.py @@ -15,9 +15,9 @@ class FullCommit(object): class ReportUploadedCount(TypedDict): - flag: str = "" - base_count: int = 0 - head_count: int = 0 + flag: str + base_count: int + head_count: int @dataclass diff --git a/services/notification/notifiers/base.py b/services/notification/notifiers/base.py index 61e1ae7f2..39c4737bf 100644 --- a/services/notification/notifiers/base.py +++ b/services/notification/notifiers/base.py @@ -14,7 +14,7 @@ class NotificationResult(object): notification_attempted: bool = False notification_successful: bool = False - explanation: str = None + explanation: str | None = None data_sent: Mapping[str, Any] | None = None data_received: Mapping[str, Any] | None = None github_app_used: int | None = None @@ -53,7 +53,7 @@ def __init__( notifier_yaml_settings: Mapping[str, Any], notifier_site_settings: Mapping[str, Any], current_yaml: Mapping[str, Any], - decoration_type: Decoration = None, + decoration_type: Decoration | None = None, gh_installation_name_to_use: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, ): """ @@ -78,7 +78,7 @@ def __init__( def name(self) -> str: raise NotImplementedError() - async def notify(self, comparison: Comparison, **extra_data) -> NotificationResult: + def notify(self, comparison: Comparison, **extra_data) -> NotificationResult: raise NotImplementedError() def is_enabled(self) -> bool: diff --git a/services/notification/notifiers/checks/base.py b/services/notification/notifiers/checks/base.py index c575bf234..1279f1cf2 100644 --- a/services/notification/notifiers/checks/base.py +++ b/services/notification/notifiers/checks/base.py @@ -2,9 +2,9 @@ from contextlib import nullcontext from typing import Dict +from asgiref.sync import async_to_sync from shared.torngit.exceptions import TorngitClientError, TorngitError -from helpers.metrics import metrics from services.notification.notifiers.base import Comparison, NotificationResult from services.notification.notifiers.status.base import StatusNotifier from services.urls import ( @@ -64,14 +64,14 @@ def paginate_annotations(self, annotations): for i in range(0, len(annotations), self.ANNOTATIONS_PER_REQUEST): yield annotations[i : i + self.ANNOTATIONS_PER_REQUEST] - async def build_payload(self, comparison) -> Dict[str, str]: + def build_payload(self, comparison) -> Dict[str, str]: raise NotImplementedError() def get_status_external_name(self) -> str: status_piece = f"/{self.title}" if self.title != "default" else "" return f"codecov/{self.context}{status_piece}" - async def notify(self, comparison: Comparison): + def notify(self, comparison: Comparison): if comparison.pull is None or (): log.debug( "Falling back to commit_status: Not a pull request", @@ -152,7 +152,7 @@ async def notify(self, comparison: Comparison): ) ) if not comparison.has_head_report(): - payload = await self.build_payload(comparison) + payload = self.build_payload(comparison) elif ( flag_coverage_not_uploaded_behavior == "exclude" and not self.flag_coverage_was_uploaded(comparison) @@ -171,7 +171,7 @@ async def notify(self, comparison: Comparison): filtered_comparison = comparison.get_filtered_comparison( **self.get_notifier_filters() ) - payload = await self.build_payload(filtered_comparison) + payload = self.build_payload(filtered_comparison) payload["state"] = "success" payload["output"]["summary"] = ( payload.get("output", {}).get("summary", "") @@ -181,12 +181,12 @@ async def notify(self, comparison: Comparison): filtered_comparison = comparison.get_filtered_comparison( **self.get_notifier_filters() ) - payload = await self.build_payload(filtered_comparison) + payload = self.build_payload(filtered_comparison) if comparison.pull: payload["url"] = get_pull_url(comparison.pull) else: payload["url"] = get_commit_url(comparison.head.commit) - return await self.maybe_send_notification(comparison, payload) + return self.maybe_send_notification(comparison, payload) except TorngitClientError as e: if e.code == 403: raise e @@ -316,7 +316,6 @@ def get_lines_to_annotate(self, comparison, files_with_change): previous_line = line return line_headers - @metrics.timer("worker.services.notifications.notifiers.checks.create_annotations") def create_annotations(self, comparison, diff): files_with_change = [ {"type": _diff["type"], "path": path, "segments": _diff["segments"]} @@ -343,7 +342,7 @@ def create_annotations(self, comparison, diff): annotations.append(annotation) return annotations - async def send_notification(self, comparison: Comparison, payload): + def send_notification(self, comparison: Comparison, payload): title = self.get_status_external_name() head = comparison.head.commit repository_service = self.repository_service(head) @@ -382,12 +381,9 @@ async def send_notification(self, comparison: Comparison, payload): ) # We need to first create the check run, get that id and update the status - with metrics.timer( - "worker.services.notifications.notifiers.checks.create_check_run" - ): - check_id = await repository_service.create_check_run( - check_name=title, head_sha=head.commitid - ) + check_id = async_to_sync(repository_service.create_check_run)( + check_name=title, head_sha=head.commitid + ) if len(output.get("annotations", [])) > self.ANNOTATIONS_PER_REQUEST: annotation_pages = list( @@ -401,27 +397,21 @@ async def send_notification(self, comparison: Comparison, payload): ), ) for annotation_page in annotation_pages: - with metrics.timer( - "worker.services.notifications.notifiers.checks.update_check_run" - ): - await repository_service.update_check_run( - check_id, - state, - output={ - "title": output.get("title"), - "summary": output.get("summary"), - "annotations": annotation_page, - }, - url=payload.get("url"), - ) + async_to_sync(repository_service.update_check_run)( + check_id, + state, + output={ + "title": output.get("title"), + "summary": output.get("summary"), + "annotations": annotation_page, + }, + url=payload.get("url"), + ) else: - with metrics.timer( - "worker.services.notifications.notifiers.checks.update_check_run" - ): - await repository_service.update_check_run( - check_id, state, output=output, url=payload.get("url") - ) + async_to_sync(repository_service.update_check_run)( + check_id, state, output=output, url=payload.get("url") + ) return NotificationResult( notification_attempted=True, diff --git a/services/notification/notifiers/checks/patch.py b/services/notification/notifiers/checks/patch.py index 81947626c..feadb1a77 100644 --- a/services/notification/notifiers/checks/patch.py +++ b/services/notification/notifiers/checks/patch.py @@ -1,5 +1,4 @@ from database.enums import Notification -from helpers.metrics import metrics from services.notification.notifiers.base import Comparison from services.notification.notifiers.checks.base import ChecksNotifier from services.notification.notifiers.mixins.status import StatusPatchMixin @@ -13,7 +12,7 @@ class PatchChecksNotifier(StatusPatchMixin, ChecksNotifier): def notification_type(self) -> Notification: return Notification.checks_patch - async def build_payload(self, comparison: Comparison): + def build_payload(self, comparison: Comparison): """ This method build the paylod of the patch github checks. @@ -29,52 +28,49 @@ async def build_payload(self, comparison: Comparison): "summary": message, }, } - with metrics.timer( - "worker.services.notifications.notifiers.checks.patch.build_payload" - ): - state, message = await self.get_patch_status(comparison) - codecov_link = self.get_codecov_pr_link(comparison) - - title = message + state, message = self.get_patch_status(comparison) + codecov_link = self.get_codecov_pr_link(comparison) - should_use_upgrade = self.should_use_upgrade_decoration() - if should_use_upgrade: - message = self.get_upgrade_message(comparison) - title = "Codecov Report" + title = message - checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",)) + should_use_upgrade = self.should_use_upgrade_decoration() + if should_use_upgrade: + message = self.get_upgrade_message(comparison) + title = "Codecov Report" - should_annotate = ( - checks_yaml_field.get("annotations", False) - if checks_yaml_field is not None - else True - ) + checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",)) - flags = self.notifier_yaml_settings.get("flags") - paths = self.notifier_yaml_settings.get("paths") - if ( - flags is not None - or paths is not None - or should_use_upgrade - or should_annotate is False - ): - return { - "state": state, - "output": { - "title": f"{title}", - "summary": "\n\n".join([codecov_link, message]), - }, - } - diff = await comparison.get_diff(use_original_base=True) - # TODO: Look into why the apply diff in get_patch_status is not saving state at this point - comparison.head.report.apply_diff(diff) - annotations = self.create_annotations(comparison, diff) + should_annotate = ( + checks_yaml_field.get("annotations", False) + if checks_yaml_field is not None + else True + ) + flags = self.notifier_yaml_settings.get("flags") + paths = self.notifier_yaml_settings.get("paths") + if ( + flags is not None + or paths is not None + or should_use_upgrade + or should_annotate is False + ): return { "state": state, "output": { "title": f"{title}", "summary": "\n\n".join([codecov_link, message]), - "annotations": annotations, }, } + diff = comparison.get_diff(use_original_base=True) + # TODO: Look into why the apply diff in get_patch_status is not saving state at this point + comparison.head.report.apply_diff(diff) + annotations = self.create_annotations(comparison, diff) + + return { + "state": state, + "output": { + "title": f"{title}", + "summary": "\n\n".join([codecov_link, message]), + "annotations": annotations, + }, + } diff --git a/services/notification/notifiers/checks/project.py b/services/notification/notifiers/checks/project.py index 34f00222f..cb47b37f1 100644 --- a/services/notification/notifiers/checks/project.py +++ b/services/notification/notifiers/checks/project.py @@ -1,5 +1,4 @@ from database.enums import Notification -from helpers.metrics import metrics from services.notification.notifiers.base import Comparison from services.notification.notifiers.checks.base import ChecksNotifier from services.notification.notifiers.mixins.message import MessageMixin @@ -14,11 +13,11 @@ class ProjectChecksNotifier(MessageMixin, StatusProjectMixin, ChecksNotifier): def notification_type(self) -> Notification: return Notification.checks_project - async def get_message(self, comparison: Comparison, yaml_comment_settings): + def get_message(self, comparison: Comparison, yaml_comment_settings): pull_dict = comparison.enriched_pull.provider_pull - return await self.create_message(comparison, pull_dict, yaml_comment_settings) + return self.create_message(comparison, pull_dict, yaml_comment_settings) - async def build_payload(self, comparison: Comparison): + def build_payload(self, comparison: Comparison): """ This method build the paylod of the project github checks. @@ -34,52 +33,48 @@ async def build_payload(self, comparison: Comparison): "summary": message, }, } - with metrics.timer( - "worker.services.notifications.notifiers.checks.project.build_payload" - ): - state, summary = await self.get_project_status(comparison) - codecov_link = self.get_codecov_pr_link(comparison) - title = summary + state, summary = self.get_project_status(comparison) + codecov_link = self.get_codecov_pr_link(comparison) - should_use_upgrade = self.should_use_upgrade_decoration() - if should_use_upgrade: - summary = self.get_upgrade_message(comparison) - title = "Codecov Report" - flags = self.notifier_yaml_settings.get("flags") - paths = self.notifier_yaml_settings.get("paths") - yaml_comment_settings = ( - read_yaml_field(self.current_yaml, ("comment",)) or {} - ) - if yaml_comment_settings is True: - yaml_comment_settings = self.site_settings.get("comment", {}) - # copying to a new variable because we will be modifying that - settings_to_be_used = dict(yaml_comment_settings) - if "flag" in settings_to_be_used.get("layout", ""): - old_flags_list = settings_to_be_used.get("layout", "").split(",") - new_flags_list = [x for x in old_flags_list if "flag" not in x] - settings_to_be_used["layout"] = ",".join(new_flags_list) + title = summary - if ( - flags is not None - or paths is not None - or should_use_upgrade - or not settings_to_be_used - ): - return { - "state": state, - "output": { - "title": f"{title}", - "summary": "\n\n".join([codecov_link, summary]), - }, - } + should_use_upgrade = self.should_use_upgrade_decoration() + if should_use_upgrade: + summary = self.get_upgrade_message(comparison) + title = "Codecov Report" + flags = self.notifier_yaml_settings.get("flags") + paths = self.notifier_yaml_settings.get("paths") + yaml_comment_settings = read_yaml_field(self.current_yaml, ("comment",)) or {} + if yaml_comment_settings is True: + yaml_comment_settings = self.site_settings.get("comment", {}) + # copying to a new variable because we will be modifying that + settings_to_be_used = dict(yaml_comment_settings) + if "flag" in settings_to_be_used.get("layout", ""): + old_flags_list = settings_to_be_used.get("layout", "").split(",") + new_flags_list = [x for x in old_flags_list if "flag" not in x] + settings_to_be_used["layout"] = ",".join(new_flags_list) - message = await self.get_message(comparison, settings_to_be_used) + if ( + flags is not None + or paths is not None + or should_use_upgrade + or not settings_to_be_used + ): return { "state": state, "output": { "title": f"{title}", "summary": "\n\n".join([codecov_link, summary]), - "text": "\n".join(message), }, } + + message = self.get_message(comparison, settings_to_be_used) + return { + "state": state, + "output": { + "title": f"{title}", + "summary": "\n\n".join([codecov_link, summary]), + "text": "\n".join(message), + }, + } diff --git a/services/notification/notifiers/comment/__init__.py b/services/notification/notifiers/comment/__init__.py index 79c1a9bc6..a5874dab2 100644 --- a/services/notification/notifiers/comment/__init__.py +++ b/services/notification/notifiers/comment/__init__.py @@ -3,6 +3,7 @@ from typing import Any, List, Mapping import sentry_sdk +from asgiref.sync import async_to_sync from shared.torngit.base import TorngitBaseAdapter from shared.torngit.exceptions import ( TorngitClientError, @@ -11,7 +12,6 @@ ) from database.enums import Notification -from helpers.metrics import metrics from services.billing import BillingPlan from services.comparison import ComparisonProxy from services.comparison.types import Comparison @@ -75,12 +75,10 @@ def name(self) -> str: def notification_type(self) -> Notification: return Notification.comment - async def get_diff(self, comparison: Comparison): - return await comparison.get_diff() + def get_diff(self, comparison: Comparison): + return comparison.get_diff() - async def notify( - self, comparison: ComparisonProxy, **extra_data - ) -> NotificationResult: + def notify(self, comparison: ComparisonProxy, **extra_data) -> NotificationResult: # TODO: remove this when we don't need it anymore # this line is measuring how often we try to comment on a PR that is closed if comparison.pull is not None and comparison.pull.state != "open": @@ -94,17 +92,11 @@ async def notify( ) for condition in self.notify_conditions: - condition_result = ( - await condition.check_condition(notifier=self, comparison=comparison) - if condition.is_async_condition - else condition.check_condition(notifier=self, comparison=comparison) + condition_result = condition.check_condition( + notifier=self, comparison=comparison ) if condition_result == False: - side_effect_result = ( - condition.on_failure_side_effect(self, comparison) - if condition.is_async_condition is False - else (await condition.on_failure_side_effect(self, comparison)) - ) + side_effect_result = condition.on_failure_side_effect(self, comparison) default_result = NotificationResult( notification_attempted=False, explanation=condition.failure_explanation, @@ -114,10 +106,7 @@ async def notify( return default_result.merge(side_effect_result) pull = comparison.pull try: - with metrics.timer( - "worker.services.notifications.notifiers.comment.build_message" - ): - message = await self.build_message(comparison) + message = self.build_message(comparison) except TorngitClientError: log.warning( "Unable to fetch enough information to build message for comment", @@ -135,10 +124,7 @@ async def notify( ) data = {"message": message, "commentid": pull.commentid, "pullid": pull.pullid} try: - with metrics.timer( - "worker.services.notifications.notifiers.comment.send_notifications" - ): - return await self.send_actual_notification(data) + return self.send_actual_notification(data) except TorngitServerFailureError: log.warning( "Unable to send comments because the provider server was not reachable or errored", @@ -153,7 +139,7 @@ async def notify( data_received=None, ) - async def send_actual_notification(self, data: Mapping[str, Any]): + def send_actual_notification(self, data: Mapping[str, Any]): message = "\n".join(data["message"]) # Append tracking parameters to any codecov urls in the message @@ -166,19 +152,19 @@ async def send_actual_notification(self, data: Mapping[str, Any]): behavior = self.notifier_yaml_settings.get("behavior", "default") if behavior == "default": - res = await self.send_comment_default_behavior( + res = self.send_comment_default_behavior( data["pullid"], data["commentid"], message ) elif behavior == "once": - res = await self.send_comment_once_behavior( + res = self.send_comment_once_behavior( data["pullid"], data["commentid"], message ) elif behavior == "new": - res = await self.send_comment_new_behavior( + res = self.send_comment_new_behavior( data["pullid"], data["commentid"], message ) elif behavior == "spammy": - res = await self.send_comment_spammy_behavior( + res = self.send_comment_spammy_behavior( data["pullid"], data["commentid"], message ) return NotificationResult( @@ -189,10 +175,10 @@ async def send_actual_notification(self, data: Mapping[str, Any]): data_received=res["data_received"], ) - async def send_comment_default_behavior(self, pullid, commentid, message): + def send_comment_default_behavior(self, pullid, commentid, message): if commentid: try: - res = await self.repository_service.edit_comment( + res = async_to_sync(self.repository_service.edit_comment)( pullid, commentid, message ) return { @@ -210,7 +196,7 @@ async def send_comment_default_behavior(self, pullid, commentid, message): extra=dict(pullid=pullid, commentid=commentid), ) try: - res = await self.repository_service.post_comment(pullid, message) + res = async_to_sync(self.repository_service.post_comment)(pullid, message) return { "notification_attempted": True, "notification_successful": True, @@ -230,10 +216,10 @@ async def send_comment_default_behavior(self, pullid, commentid, message): "data_received": None, } - async def send_comment_once_behavior(self, pullid, commentid, message): + def send_comment_once_behavior(self, pullid, commentid, message): if commentid: try: - res = await self.repository_service.edit_comment( + res = async_to_sync(self.repository_service.edit_comment)( pullid, commentid, message ) return { @@ -261,7 +247,7 @@ async def send_comment_once_behavior(self, pullid, commentid, message): "explanation": "no_permissions", "data_received": None, } - res = await self.repository_service.post_comment(pullid, message) + res = async_to_sync(self.repository_service.post_comment)(pullid, message) return { "notification_attempted": True, "notification_successful": True, @@ -269,10 +255,10 @@ async def send_comment_once_behavior(self, pullid, commentid, message): "data_received": {"id": res["id"]}, } - async def send_comment_new_behavior(self, pullid, commentid, message): + def send_comment_new_behavior(self, pullid, commentid, message): if commentid: try: - await self.repository_service.delete_comment(pullid, commentid) + async_to_sync(self.repository_service.delete_comment)(pullid, commentid) except TorngitObjectNotFoundError: log.info("Comment was already deleted") except TorngitClientError: @@ -292,7 +278,7 @@ async def send_comment_new_behavior(self, pullid, commentid, message): "data_received": None, } try: - res = await self.repository_service.post_comment(pullid, message) + res = async_to_sync(self.repository_service.post_comment)(pullid, message) return { "notification_attempted": True, "notification_successful": True, @@ -314,8 +300,8 @@ async def send_comment_new_behavior(self, pullid, commentid, message): "data_received": None, } - async def send_comment_spammy_behavior(self, pullid, commentid, message): - res = await self.repository_service.post_comment(pullid, message) + def send_comment_spammy_behavior(self, pullid, commentid, message): + res = async_to_sync(self.repository_service.post_comment)(pullid, message) return { "notification_attempted": True, "notification_successful": True, @@ -328,7 +314,7 @@ def is_enabled(self) -> bool: self.notifier_yaml_settings, dict ) - async def build_message(self, comparison: Comparison) -> List[str]: + def build_message(self, comparison: Comparison) -> list[str]: if self.should_use_upgrade_decoration(): return self._create_upgrade_message(comparison) if self.is_processing_upload(): @@ -340,9 +326,7 @@ async def build_message(self, comparison: Comparison) -> List[str]: if comparison.pull.is_first_coverage_pull: return self._create_welcome_message() pull_dict = comparison.enriched_pull.provider_pull - return await self.create_message( - comparison, pull_dict, self.notifier_yaml_settings - ) + return self.create_message(comparison, pull_dict, self.notifier_yaml_settings) def should_see_project_coverage_cta(self): """ diff --git a/services/notification/notifiers/comment/conditions.py b/services/notification/notifiers/comment/conditions.py index df9855863..bf2a5dd52 100644 --- a/services/notification/notifiers/comment/conditions.py +++ b/services/notification/notifiers/comment/conditions.py @@ -110,31 +110,28 @@ def check_condition( class HasEnoughRequiredChanges(AsyncNotifyCondition): failure_explanation = "changes_required" - async def _check_unexpected_changes(comparison: ComparisonProxy) -> bool: + @classmethod + def _check_unexpected_changes(comparison: ComparisonProxy) -> bool: """Returns a bool that indicates wether there are unexpected changes""" - changes = await comparison.get_changes() - if changes: - return True - return False + return bool(comparison.get_changes()) - async def _check_coverage_change(comparison: ComparisonProxy) -> bool: + @classmethod + def _check_coverage_change(comparison: ComparisonProxy) -> bool: """Returns a bool that indicates wether there is any change in coverage""" - diff = await comparison.get_diff() + diff = comparison.get_diff() res = comparison.head.report.calculate_diff(diff) - if res is not None and res["general"].lines > 0: - return True - return False + return res is not None and res["general"].lines > 0 - async def _check_any_change(comparison: ComparisonProxy) -> bool: - unexpected_changes = await HasEnoughRequiredChanges._check_unexpected_changes( - comparison - ) - coverage_changes = await HasEnoughRequiredChanges._check_coverage_change( + @classmethod + def _check_any_change(comparison: ComparisonProxy) -> bool: + unexpected_changes = HasEnoughRequiredChanges._check_unexpected_changes( comparison ) + coverage_changes = HasEnoughRequiredChanges._check_coverage_change(comparison) return unexpected_changes or coverage_changes - async def _check_coverage_drop(comparison: ComparisonProxy) -> bool: + @classmethod + def _check_coverage_drop(comparison: ComparisonProxy) -> bool: no_head_coverage = comparison.head.report.totals.coverage is None no_base_report = comparison.project_coverage_base.report is None no_base_coverage = ( @@ -162,8 +159,9 @@ async def _check_coverage_drop(comparison: ComparisonProxy) -> bool: # Need to take the project threshold into consideration return diff < 0 and abs(diff) >= (threshold + Decimal(0.01)) - async def _check_uncovered_patch(comparison: ComparisonProxy): - diff = await comparison.get_diff(use_original_base=True) + @classmethod + def _check_uncovered_patch(comparison: ComparisonProxy) -> bool: + diff = comparison.get_diff(use_original_base=True) totals = comparison.head.report.apply_diff(diff) coverage_not_affected_by_patch = totals and totals.lines == 0 if totals is None or coverage_not_affected_by_patch: @@ -175,7 +173,8 @@ async def _check_uncovered_patch(comparison: ComparisonProxy): 0.01 ) - async def check_condition_OR_group( + @classmethod + def check_condition_OR_group( condition_group: CoverageCommentRequiredChangesORGroup, comparison: ComparisonProxy, ) -> bool: @@ -195,13 +194,12 @@ async def check_condition_OR_group( if condition_group & individual_condition.value: if cache_results[individual_condition] is None: function_to_call = functions_lookup[individual_condition] - cache_results[individual_condition] = await function_to_call( - comparison - ) + cache_results[individual_condition] = function_to_call(comparison) final_result |= cache_results[individual_condition] return final_result - async def check_condition( + @classmethod + def check_condition( notifier: AbstractBaseNotifier, comparison: ComparisonProxy ) -> bool: if comparison.pull and comparison.pull.commentid: @@ -219,10 +217,6 @@ async def check_condition( # False --> 0 (no_requirements) required_changes = [int(required_changes)] return all( - [ - await HasEnoughRequiredChanges.check_condition_OR_group( - or_group, comparison - ) - for or_group in required_changes - ] + HasEnoughRequiredChanges.check_condition_OR_group(or_group, comparison) + for or_group in required_changes ) diff --git a/services/notification/notifiers/mixins/message/__init__.py b/services/notification/notifiers/mixins/message/__init__.py index 2e5d54b07..3851c9c78 100644 --- a/services/notification/notifiers/mixins/message/__init__.py +++ b/services/notification/notifiers/mixins/message/__init__.py @@ -25,9 +25,7 @@ class MessageMixin(object): - async def create_message( - self, comparison: ComparisonProxy, pull_dict, yaml_settings - ): + def create_message(self, comparison: ComparisonProxy, pull_dict, yaml_settings): """ Assemble the various components of the PR comments message in accordance with their YAML configuration. See https://docs.codecov.io/docs/pull-request-comments for more context on the different parts of a PR comment. @@ -41,9 +39,9 @@ async def create_message( Thus, the comment block of the codecov YAML is passed as the "yaml_settings" parameter for these Notifiers. """ - changes = await comparison.get_changes() - diff = await comparison.get_diff(use_original_base=True) - behind_by = await comparison.get_behind_by() + changes = comparison.get_changes() + diff = comparison.get_diff(use_original_base=True) + behind_by = comparison.get_behind_by() base_report = comparison.project_coverage_base.report head_report = comparison.head.report pull = comparison.pull @@ -75,7 +73,7 @@ async def create_message( # note: since we're using append, calling write("") will add a newline to the message write = message.append - await self._possibly_write_install_app(comparison, write) + self._possibly_write_install_app(comparison, write) # Write Header write(f'## [Codecov]({links["pull"]}?dropdown=coverage&src=pr&el=h1) Report') @@ -112,7 +110,7 @@ async def create_message( current_yaml, ) - await self.write_section_to_msg( + self.write_section_to_msg( comparison, changes, diff, links, write, section_writer, behind_by ) @@ -145,7 +143,7 @@ async def create_message( self.repository, layout, show_complexity, settings, current_yaml ) - await self.write_section_to_msg( + self.write_section_to_msg( comparison, changes, diff, @@ -170,7 +168,7 @@ async def create_message( settings, current_yaml, ) - await self.write_section_to_msg( + self.write_section_to_msg( comparison, changes, diff, @@ -181,7 +179,7 @@ async def create_message( return [m for m in message if m is not None] - async def _possibly_write_install_app( + def _possibly_write_install_app( self, comparison: ComparisonProxy, write: Callable ) -> None: """Write a message if the user does not have any GH installations @@ -233,14 +231,14 @@ def _team_plan_notification( return message - async def write_section_to_msg( + def write_section_to_msg( self, comparison, changes, diff, links, write, section_writer, behind_by=None ): wrote_something: bool = False with metrics.timer( f"worker.services.notifications.notifiers.comment.section.{section_writer.name}" ): - for line in await section_writer.write_section( + for line in section_writer.write_section( comparison, diff, changes, links, behind_by=behind_by ): wrote_something |= line is not None diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index 718a02017..0efaeb7ab 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -4,7 +4,6 @@ from decimal import Decimal from enum import Enum, auto from itertools import starmap -from typing import List from urllib.parse import urlencode from shared.helpers.yaml import walk @@ -71,17 +70,17 @@ def __init__(self, repository, layout, show_complexity, settings, current_yaml): def name(self): return self.__class__.__name__ - async def write_section(self, *args, **kwargs): - return [i async for i in self.do_write_section(*args, **kwargs)] + def write_section(self, *args, **kwargs): + return [i for i in self.do_write_section(*args, **kwargs)] class NullSectionWriter(BaseSectionWriter): - async def write_section(*args, **kwargs): + def write_section(*args, **kwargs): return [] class NewFooterSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): hide_project_coverage = self.settings.get("hide_project_coverage", False) if hide_project_coverage: yield ("") @@ -118,12 +117,11 @@ def _possibly_include_test_result_setup_confirmation(self, comparison): yield "" yield (":white_check_mark: All tests successful. No failed tests found.") - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): yaml = self.current_yaml base_report = comparison.project_coverage_base.report head_report = comparison.head.report pull_dict = comparison.enriched_pull.provider_pull - repo_service = comparison.repository_service.service diff_totals = head_report.apply_diff(diff) if diff_totals: @@ -206,7 +204,7 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non ) | set(c.path for c in changes or []) overlay = comparison.get_overlay(OverlayType.line_execution_count) files_in_critical = set( - await overlay.search_files_for_critical_changes( + overlay.search_files_for_critical_changes( all_potentially_affected_critical_files ) ) @@ -229,7 +227,7 @@ class AnnouncementSectionWriter(BaseSectionWriter): # "Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/)" # This is disabled as of CODE-1885. But we might bring it back later. ] - async def do_write_section(self, comparison: ComparisonProxy, *args, **kwargs): + def do_write_section(self, comparison: ComparisonProxy, *args, **kwargs): if self._potential_ats_user(comparison): message_to_display = AnnouncementSectionWriter.ats_message else: @@ -271,9 +269,9 @@ def _potential_ats_user(self, comparison: ComparisonProxy) -> bool: class ImpactedEntrypointsSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): overlay = comparison.get_overlay(OverlayType.line_execution_count) - impacted_endpoints = await overlay.find_impacted_endpoints() + impacted_endpoints = overlay.find_impacted_endpoints() if impacted_endpoints: yield "| Related Entrypoints |" yield "|---|" @@ -284,7 +282,7 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non class FooterSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): pull_dict = comparison.enriched_pull.provider_pull yield ("------") yield ("") @@ -310,7 +308,7 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non class ReachSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): pull = comparison.enriched_pull.database_pull yield ( "[![Impacted file tree graph]({})]({}?src=pr&el=tree)".format( @@ -328,7 +326,7 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non class DiffSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): base_report = comparison.project_coverage_base.report head_report = comparison.head.report if base_report is None: @@ -361,7 +359,7 @@ def _get_tree_cell(typ, path, metrics, compare, is_critical): class NewFilesSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): # create list of files changed in diff base_report = comparison.project_coverage_base.report head_report = comparison.head.report @@ -399,7 +397,7 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non if self.settings.get("show_critical_paths", False): overlay = comparison.get_overlay(OverlayType.line_execution_count) files_in_critical = set( - await overlay.search_files_for_critical_changes(all_files) + overlay.search_files_for_critical_changes(all_files) ) def tree_cell(typ, path, metrics, _=None): @@ -442,7 +440,7 @@ def tree_cell(typ, path, metrics, _=None): class FileSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): # create list of files changed in diff base_report = comparison.project_coverage_base.report head_report = comparison.head.report @@ -480,7 +478,7 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non if self.settings.get("show_critical_paths", False): overlay = comparison.get_overlay(OverlayType.line_execution_count) files_in_critical = set( - await overlay.search_files_for_critical_changes(all_files) + overlay.search_files_for_critical_changes(all_files) ) def tree_cell(typ, path, metrics, _=None): @@ -536,7 +534,7 @@ def tree_cell(typ, path, metrics, _=None): class FlagSectionWriter(BaseSectionWriter): - async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + def do_write_section(self, comparison, diff, changes, links, behind_by=None): # flags base_report = comparison.project_coverage_base.report head_report = comparison.head.report @@ -666,16 +664,16 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non class ComponentsSectionWriter(BaseSectionWriter): - async def _get_table_data_for_components( + def _get_table_data_for_components( self, all_components, comparison: ComparisonProxy - ) -> List[dict]: + ) -> list[dict]: component_data = [] for component in all_components: flags = component.get_matching_flags(comparison.head.report.flags.keys()) filtered_comparison = comparison.get_filtered_comparison( flags, component.paths ) - diff = await filtered_comparison.get_diff() + diff = filtered_comparison.get_diff() component_data.append( { "name": component.get_display_name(), @@ -692,14 +690,14 @@ async def _get_table_data_for_components( ) return component_data - async def do_write_section( + def do_write_section( self, comparison: ComparisonProxy, diff, changes, links, behind_by=None ): all_components = get_components_from_yaml(self.current_yaml) if all_components == []: return # fast return if there's noting to process - component_data_to_show = await self._get_table_data_for_components( + component_data_to_show = self._get_table_data_for_components( all_components, comparison ) @@ -790,7 +788,7 @@ def _write_install_github_app_warning(self, comparison: ComparisonProxy) -> str: return ":exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality." return "" - async def do_write_section(self, comparison: ComparisonProxy, *args, **kwargs): + def do_write_section(self, comparison: ComparisonProxy, *args, **kwargs): messages_ordering = [ self.Messages.INSTALL_GITHUB_APP_WARNING, self.Messages.DIFFERENT_UPLOAD_COUNT_WARNING, diff --git a/services/notification/notifiers/mixins/status.py b/services/notification/notifiers/mixins/status.py index cb22ca06f..999c1b434 100644 --- a/services/notification/notifiers/mixins/status.py +++ b/services/notification/notifiers/mixins/status.py @@ -1,17 +1,17 @@ import logging from decimal import Decimal, InvalidOperation -from typing import Any, Callable, List, Optional, Tuple, Union from services.comparison import ComparisonProxy, FilteredComparison +from services.comparison.types import Comparison from services.yaml.reader import round_number log = logging.getLogger(__name__) class StatusPatchMixin(object): - async def get_patch_status( + def get_patch_status( self, comparison: ComparisonProxy | FilteredComparison - ) -> Tuple[str, str]: + ) -> tuple[str, str]: threshold = self.notifier_yaml_settings.get("threshold", "0.0") # check if user has erroneously added a % to this input and fix @@ -23,7 +23,8 @@ async def get_patch_status( except (InvalidOperation, TypeError): threshold = Decimal("0.0") - totals = await comparison.get_patch_totals() + target_coverage: Decimal | None + totals = comparison.get_patch_totals() if self.notifier_yaml_settings.get("target") not in ("auto", None): target_coverage = Decimal( str(self.notifier_yaml_settings.get("target")).replace("%", "") @@ -78,7 +79,7 @@ def is_a_change_worth_noting(self, change) -> bool: return (t.misses + t.partials) > 0 return False - async def get_changes_status(self, comparison) -> Tuple[str, str]: + def get_changes_status(self, comparison: Comparison) -> tuple[str, str]: pull = comparison.pull if self.notifier_yaml_settings.get("base") in ("auto", None, "pr") and pull: if not comparison.has_project_coverage_base_report(): @@ -89,7 +90,7 @@ async def get_changes_status(self, comparison) -> Tuple[str, str]: return (state, description) # filter changes - changes = await comparison.get_changes() + changes = comparison.get_changes() if changes: changes = list(filter(self.is_a_change_worth_noting, changes)) @@ -114,9 +115,9 @@ async def get_changes_status(self, comparison) -> Tuple[str, str]: class StatusProjectMixin(object): DEFAULT_REMOVED_CODE_BEHAVIOR = "adjust_base" - async def _apply_removals_only_behavior( - self, comparison: Union[ComparisonProxy, FilteredComparison] - ) -> Optional[Tuple[str, str]]: + def _apply_removals_only_behavior( + self, comparison: ComparisonProxy | FilteredComparison + ) -> tuple[str, str] | None: """ Rule for passing project status on removals_only behavior: Pass if code was _only removed_ (i.e. no addition, no unexpected changes) @@ -125,30 +126,22 @@ async def _apply_removals_only_behavior( "Applying removals_only behavior to project status", extra=dict(commit=comparison.head.commit.commitid), ) - impacted_files_dict = await comparison.get_impacted_files() - impacted_files = impacted_files_dict.get("files", []) + impacted_files = comparison.get_impacted_files().get("files", []) + no_added_no_unexpected_change = all( - map( - lambda file_dict: ( - file_dict.get("added_diff_coverage", []) == [] - and file_dict.get("unexpected_line_changes") == [] - ), - impacted_files, - ) - ) - some_removed = any( - map( - lambda file_dict: (file_dict.get("removed_diff_coverage", []) != []), - impacted_files, - ) + not file.get("added_diff_coverage") + and not file.get("unexpected_line_changes") + for file in impacted_files ) + some_removed = any(file.get("removed_diff_coverage") for file in impacted_files) + if no_added_no_unexpected_change and some_removed: return ("success", ", passed because this change only removed code") return None - async def _apply_adjust_base_behavior( - self, comparison: ComparisonProxy - ) -> Optional[Tuple[str, str]]: + def _apply_adjust_base_behavior( + self, comparison: ComparisonProxy | FilteredComparison + ) -> tuple[str, str] | None: """ Rule for passing project status on adjust_base behavior: We adjust the BASE of the comparison by removing from it lines that were removed in HEAD @@ -168,28 +161,23 @@ async def _apply_adjust_base_behavior( ) return None - impacted_files_dict = await comparison.get_impacted_files() - impacted_files = impacted_files_dict.get("files", []) - - def get_sum_from_lists( - coverage_diff_info: List[Any], comparison_fn: Callable[[Any], int] - ): - return sum(map(comparison_fn, coverage_diff_info)) + impacted_files = comparison.get_impacted_files().get("files", []) hits_removed = 0 misses_removed = 0 partials_removed = 0 + for file_dict in impacted_files: - removed_diff_coverage_list = file_dict.get("removed_diff_coverage", []) - if removed_diff_coverage_list is not None: - hits_removed += get_sum_from_lists( - removed_diff_coverage_list, lambda item: 1 if item[1] == "h" else 0 + removed_diff_coverage_list = file_dict.get("removed_diff_coverage") + if removed_diff_coverage_list: + hits_removed += sum( + 1 if item[1] == "h" else 0 for item in removed_diff_coverage_list ) - misses_removed += get_sum_from_lists( - removed_diff_coverage_list, lambda item: 1 if item[1] == "m" else 0 + misses_removed += sum( + 1 if item[1] == "m" else 0 for item in removed_diff_coverage_list ) - partials_removed += get_sum_from_lists( - removed_diff_coverage_list, lambda item: 1 if item[1] == "p" else 0 + partials_removed += sum( + 1 if item[1] == "p" else 0 for item in removed_diff_coverage_list ) base_totals = comparison.project_coverage_base.report.totals @@ -235,9 +223,9 @@ def get_sum_from_lists( ) return None - async def _apply_fully_covered_patch_behavior( - self, comparison: ComparisonProxy - ) -> Optional[Tuple[str, str]]: + def _apply_fully_covered_patch_behavior( + self, comparison: ComparisonProxy | FilteredComparison + ) -> tuple[str, str] | None: """ Rule for passing project status on fully_covered_patch behavior: Pass if patch coverage is 100% and there are no unexpected changes @@ -246,21 +234,20 @@ async def _apply_fully_covered_patch_behavior( "Applying fully_covered_patch behavior to project status", extra=dict(commit=comparison.head.commit.commitid), ) - impacted_files_dict = await comparison.get_impacted_files() - impacted_files = impacted_files_dict.get("files", []) + impacted_files = comparison.get_impacted_files().get("files", []) + no_unexpected_changes = all( - map( - lambda file_dict: file_dict.get("unexpected_line_changes") == [], - impacted_files, - ) + not file.get("unexpected_line_changes") for file in impacted_files ) + if not no_unexpected_changes: log.info( "Unexpected changes when applying patch_100 behavior", extra=dict(commit=comparison.head.commit.commitid), ) return None - diff = await comparison.get_diff(use_original_base=True) + + diff = comparison.get_diff(use_original_base=True) patch_totals = comparison.head.report.apply_diff(diff) if patch_totals is None or patch_totals.lines == 0: # Coverage was not changed by patch @@ -273,9 +260,9 @@ async def _apply_fully_covered_patch_behavior( ) return None - async def get_project_status( - self, comparison: Union[ComparisonProxy, FilteredComparison] - ) -> Tuple[str, str]: + def get_project_status( + self, comparison: ComparisonProxy | FilteredComparison + ) -> tuple[str, str]: state, message = self._get_project_status(comparison) if state == "success": return (state, message) @@ -289,13 +276,11 @@ async def get_project_status( # Apply removed_code_behavior removed_code_result = None if removed_code_behavior == "removals_only": - removed_code_result = await self._apply_removals_only_behavior( - comparison - ) + removed_code_result = self._apply_removals_only_behavior(comparison) elif removed_code_behavior == "adjust_base": - removed_code_result = await self._apply_adjust_base_behavior(comparison) + removed_code_result = self._apply_adjust_base_behavior(comparison) elif removed_code_behavior == "fully_covered_patch": - removed_code_result = await self._apply_fully_covered_patch_behavior( + removed_code_result = self._apply_fully_covered_patch_behavior( comparison ) else: @@ -313,7 +298,9 @@ async def get_project_status( return (removed_code_state, message + removed_code_message) return (state, message) - def _get_project_status(self, comparison) -> Tuple[str, str]: + def _get_project_status( + self, comparison: ComparisonProxy | FilteredComparison + ) -> tuple[str, str]: if comparison.head.report.totals.coverage is None: state = self.notifier_yaml_settings.get("if_not_found", "success") message = "No coverage information found on head" diff --git a/services/notification/notifiers/status/base.py b/services/notification/notifiers/status/base.py index 9597f42c4..c214aa225 100644 --- a/services/notification/notifiers/status/base.py +++ b/services/notification/notifiers/status/base.py @@ -10,7 +10,6 @@ from database.models.core import Commit from helpers.cache import cache from helpers.match import match -from helpers.metrics import metrics from services.comparison import ComparisonProxy from services.notification.notifiers.base import ( AbstractBaseNotifier, @@ -40,7 +39,7 @@ def store_results(self, comparison: Comparison, result: NotificationResult) -> b def name(self): return f"status-{self.context}" - async def build_payload(self, comparison) -> Dict[str, str]: + def build_payload(self, comparison) -> Dict[str, str]: raise NotImplementedError() def get_upgrade_message(self) -> str: @@ -159,7 +158,7 @@ def get_github_app_used(self) -> int | None: ) return selected_installation_id - async def notify(self, comparison: ComparisonProxy): + def notify(self, comparison: ComparisonProxy): payload = None if not self.can_we_set_this_status(comparison): return NotificationResult( @@ -254,10 +253,10 @@ async def notify(self, comparison: ComparisonProxy): data_sent=payload, ) - async def status_already_exists( + def status_already_exists( self, comparison: ComparisonProxy, title, state, description ) -> bool: - statuses = await comparison.get_existing_statuses() + statuses = comparison.get_existing_statuses() if statuses: exists = statuses.get(title) return ( @@ -271,7 +270,7 @@ def get_status_external_name(self) -> str: status_piece = f"/{self.title}" if self.title != "default" else "" return f"codecov/{self.context}{status_piece}" - async def maybe_send_notification( + def maybe_send_notification( self, comparison: ComparisonProxy, payload: dict ) -> NotificationResult: base_commit = ( @@ -298,7 +297,7 @@ async def maybe_send_notification( get_config("setup", "cache", "send_status_notification", default=600) ) # 10 min default cache.get_backend().set(cache_key, ttl, payload) - return await self.send_notification(comparison, payload) + return self.send_notification(comparison, payload) else: log.info( "Notification payload unchanged. Skipping notification.", @@ -317,7 +316,7 @@ async def maybe_send_notification( data_sent=None, ) - async def _send_notification_with_metrics( + def _send_notification_with_metrics( self, repo_service: TorngitBaseAdapter, commit_sha: str, @@ -327,19 +326,16 @@ async def _send_notification_with_metrics( description: str, url: str, ): - with metrics.timer( - "worker.services.notifications.notifiers.status.set_commit_status" - ): - return await repo_service.set_commit_status( - commit=commit_sha, - status=state, - context=title, - coverage=coverage, - description=description, - url=url, - ) + return repo_service.set_commit_status( + commit=commit_sha, + status=state, + context=title, + coverage=coverage, + description=description, + url=url, + ) - async def send_notification(self, comparison: ComparisonProxy, payload): + def send_notification(self, comparison: ComparisonProxy, payload): title = self.get_status_external_name() head_commit_sha = comparison.head.commit.commitid repository_service = self.repository_service(comparison.head.commit) @@ -347,7 +343,7 @@ async def send_notification(self, comparison: ComparisonProxy, payload): state = payload["state"] message = payload["message"] url = payload["url"] - if not await self.status_already_exists(comparison, title, state, message): + if not self.status_already_exists(comparison, title, state, message): state = ( "success" if self.notifier_yaml_settings.get("informational") else state ) diff --git a/services/notification/notifiers/status/changes.py b/services/notification/notifiers/status/changes.py index 1e7c8be03..8899d12fd 100644 --- a/services/notification/notifiers/status/changes.py +++ b/services/notification/notifiers/status/changes.py @@ -1,5 +1,4 @@ import logging -from typing import Dict from database.enums import Notification from services.notification.notifiers.mixins.status import StatusChangesMixin @@ -27,11 +26,11 @@ class ChangesStatusNotifier(StatusChangesMixin, StatusNotifier): def notification_type(self) -> Notification: return Notification.status_changes - async def build_payload(self, comparison) -> Dict[str, str]: + def build_payload(self, comparison) -> dict[str, str]: if self.is_empty_upload(): state, message = self.get_status_check_for_empty_upload() return {"state": state, "message": message} - state, message = await self.get_changes_status(comparison) + state, message = self.get_changes_status(comparison) if self.should_use_upgrade_decoration(): message = self.get_upgrade_message() diff --git a/services/notification/notifiers/status/patch.py b/services/notification/notifiers/status/patch.py index 47e50cdea..1c127fa43 100644 --- a/services/notification/notifiers/status/patch.py +++ b/services/notification/notifiers/status/patch.py @@ -22,12 +22,12 @@ class PatchStatusNotifier(StatusPatchMixin, StatusNotifier): def notification_type(self) -> Notification: return Notification.status_patch - async def build_payload(self, comparison: Comparison): + def build_payload(self, comparison: Comparison): if self.is_empty_upload(): state, message = self.get_status_check_for_empty_upload() return {"state": state, "message": message} - state, message = await self.get_patch_status(comparison) + state, message = self.get_patch_status(comparison) if self.should_use_upgrade_decoration(): message = self.get_upgrade_message() diff --git a/services/notification/notifiers/status/project.py b/services/notification/notifiers/status/project.py index 0b10e0333..9f9c54f45 100644 --- a/services/notification/notifiers/status/project.py +++ b/services/notification/notifiers/status/project.py @@ -30,12 +30,12 @@ class ProjectStatusNotifier(StatusProjectMixin, StatusNotifier): def notification_type(self) -> Notification: return Notification.status_project - async def build_payload(self, comparison: Comparison): + def build_payload(self, comparison: Comparison): if self.is_empty_upload(): state, message = self.get_status_check_for_empty_upload() return {"state": state, "message": message} - state, message = await self.get_project_status(comparison) + state, message = self.get_project_status(comparison) if self.should_use_upgrade_decoration(): message = self.get_upgrade_message() return {"state": state, "message": message} diff --git a/services/notification/notifiers/tests/unit/test_status.py b/services/notification/notifiers/tests/unit/test_status.py index 484b6787c..b2124bf3f 100644 --- a/services/notification/notifiers/tests/unit/test_status.py +++ b/services/notification/notifiers/tests/unit/test_status.py @@ -525,8 +525,7 @@ async def build_payload(self, comparison): await notifier.notify(comparison) send_notification.assert_called_once - @pytest.mark.asyncio - async def test_notify_multiple_shas( + def test_notify_multiple_shas( self, sample_comparison, mocker, @@ -539,17 +538,17 @@ async def test_notify_multiple_shas( "url": get_pull_url(comparison.pull), } - async def set_status_side_effect(commit, *args, **kwargs): + def set_status_side_effect(commit, *args, **kwargs): return {"id": f"{commit}-status-set"} class TestNotifier(StatusNotifier): - async def build_payload(self, comparison): + def build_payload(self, comparison): return payload def get_github_app_used(self) -> None: return None - async def status_already_exists( + def status_already_exists( self, comparison: ComparisonProxy, title, state, description ) -> Coroutine[Any, Any, bool]: return False @@ -570,7 +569,7 @@ async def status_already_exists( mocker.patch.object( TestNotifier, "repository_service", return_value=fake_repo_service ) - result = await notifier.notify(comparison) + result = notifier.notify(comparison) assert result == NotificationResult( notification_attempted=True, notification_successful=True, @@ -585,8 +584,7 @@ async def status_already_exists( ) assert fake_repo_service.set_commit_status.call_count == 2 - @pytest.mark.asyncio - async def test_notify_cached( + def test_notify_cached( self, sample_comparison, mocker, @@ -600,7 +598,7 @@ async def test_notify_cached( } class TestNotifier(StatusNotifier): - async def build_payload(self, comparison): + def build_payload(self, comparison): return payload notifier = TestNotifier( @@ -618,7 +616,7 @@ async def build_payload(self, comparison): ) send_notification = mocker.patch.object(TestNotifier, "send_notification") - result = await notifier.notify(comparison) + result = notifier.notify(comparison) assert result == NotificationResult( notification_attempted=False, notification_successful=None, @@ -1437,8 +1435,7 @@ async def test_notify_path_and_flags_filter_something_on_base( sample_comparison_matching_flags, expected_result ) - @pytest.mark.asyncio - async def test_notify_pass_via_removals_only_behavior( + def test_notify_pass_via_removals_only_behavior( self, mock_configuration, sample_comparison, mocker ): mock_get_impacted_files = mocker.patch.object( @@ -1479,12 +1476,11 @@ async def test_notify_pass_via_removals_only_behavior( "message": "60.00% (target 80.00%), passed because this change only removed code", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result mock_get_impacted_files.assert_called() - @pytest.mark.asyncio - async def test_notify_pass_adjust_base_behavior( + def test_notify_pass_adjust_base_behavior( slef, mock_configuration, sample_comparison_negative_change, mocker ): sample_comparison = sample_comparison_negative_change @@ -1523,12 +1519,11 @@ async def test_notify_pass_adjust_base_behavior( "message": f"50.00% (-10.00%) compared to {sample_comparison.project_coverage_base.commit.commitid[:7]}, passed because coverage increased by +0.00% when compared to adjusted base (50.00%)", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result mock_get_impacted_files.assert_called() - @pytest.mark.asyncio - async def test_notify_removed_code_behavior_fail( + def test_notify_removed_code_behavior_fail( self, mock_configuration, sample_comparison, mocker ): mock_get_impacted_files = mocker.patch.object( @@ -1569,12 +1564,11 @@ async def test_notify_removed_code_behavior_fail( "message": "60.00% (target 80.00%)", "state": "failure", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result mock_get_impacted_files.assert_called() - @pytest.mark.asyncio - async def test_notify_adjust_base_behavior_fail( + def test_notify_adjust_base_behavior_fail( slef, mock_configuration, sample_comparison_negative_change, mocker ): sample_comparison = sample_comparison_negative_change @@ -1613,12 +1607,11 @@ async def test_notify_adjust_base_behavior_fail( "message": f"50.00% (-10.00%) compared to {sample_comparison.project_coverage_base.commit.commitid[:7]}", "state": "failure", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result mock_get_impacted_files.assert_called() - @pytest.mark.asyncio - async def test_notify_adjust_base_behavior_skips_if_target_coverage_defined( + def test_notify_adjust_base_behavior_skips_if_target_coverage_defined( slef, mock_configuration, sample_comparison_negative_change, mocker ): sample_comparison = sample_comparison_negative_change @@ -1640,12 +1633,11 @@ async def test_notify_adjust_base_behavior_skips_if_target_coverage_defined( "message": "50.00% (target 80.00%)", "state": "failure", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result mock_get_impacted_files.assert_not_called() - @pytest.mark.asyncio - async def test_notify_removed_code_behavior_unknown( + def test_notify_removed_code_behavior_unknown( self, mock_configuration, sample_comparison ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -1663,11 +1655,10 @@ async def test_notify_removed_code_behavior_unknown( "message": "60.00% (target 80.00%)", "state": "failure", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result - @pytest.mark.asyncio - async def test_notify_fully_covered_patch_behavior_fail( + def test_notify_fully_covered_patch_behavior_fail( self, comparison_with_multiple_changes, mock_repo_provider, @@ -1715,12 +1706,11 @@ async def test_notify_fully_covered_patch_behavior_fail( "message": "50.00% (target 70.00%)", "state": "failure", } - result = await notifier.build_payload(comparison_with_multiple_changes) + result = notifier.build_payload(comparison_with_multiple_changes) assert result == expected_result mock_get_impacted_files.assert_called() - @pytest.mark.asyncio - async def test_notify_fully_covered_patch_behavior_success( + def test_notify_fully_covered_patch_behavior_success( self, comparison_100_percent_patch, mock_repo_provider, @@ -1768,12 +1758,11 @@ async def test_notify_fully_covered_patch_behavior_success( "message": "28.57% (target 70.00%), passed because patch was fully covered by tests, and no indirect coverage changes", "state": "success", } - result = await notifier.build_payload(comparison_100_percent_patch) + result = notifier.build_payload(comparison_100_percent_patch) assert result == expected_result mock_get_impacted_files.assert_called() - @pytest.mark.asyncio - async def test_notify_fully_covered_patch_behavior_no_coverage_change( + def test_notify_fully_covered_patch_behavior_no_coverage_change( self, mock_configuration, sample_comparison, mocker ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -1929,14 +1918,13 @@ async def test_notify_fully_covered_patch_behavior_no_coverage_change( "message": "60.00% (target 70.00%), passed because coverage was not affected by patch", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert result == expected_result mock_get_impacted_files.assert_called() class TestPatchStatusNotifier(object): - @pytest.mark.asyncio - async def test_build_payload( + def test_build_payload( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -1951,11 +1939,10 @@ async def test_build_payload( "message": "66.67% of diff hit (target 50.00%)", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_upgrade_payload( + def test_build_upgrade_payload( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -1971,11 +1958,10 @@ async def test_build_upgrade_payload( "message": "Please activate this user to display a detailed status check", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_target_coverage_failure( + def test_build_payload_target_coverage_failure( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -1990,11 +1976,10 @@ async def test_build_payload_target_coverage_failure( "message": "66.67% of diff hit (target 70.00%)", "state": "failure", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_not_auto_not_string( + def test_build_payload_not_auto_not_string( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -2009,11 +1994,10 @@ async def test_build_payload_not_auto_not_string( "message": "66.67% of diff hit (target 57.00%)", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_target_coverage_failure_within_threshold( + def test_build_payload_target_coverage_failure_within_threshold( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -2034,11 +2018,10 @@ async def test_build_payload_target_coverage_failure_within_threshold( "message": "66.67% of diff hit (within 5.00% threshold of 70.00%)", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_get_patch_status_bad_threshold( + def test_get_patch_status_bad_threshold( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -2059,11 +2042,10 @@ async def test_get_patch_status_bad_threshold( "message": "66.67% of diff hit (target 70.00%)", "state": "failure", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_get_patch_status_bad_threshold_fixed( + def test_get_patch_status_bad_threshold_fixed( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -2086,11 +2068,10 @@ async def test_get_patch_status_bad_threshold_fixed( "message": "66.67% of diff hit (within 5.00% threshold of 70.00%)", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_no_diff( + def test_build_payload_no_diff( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_repo_provider.get_compare.return_value = { @@ -2136,11 +2117,10 @@ async def test_build_payload_no_diff( "message": f"Coverage not affected when comparing {base_commit.commitid[:7]}...{head_commit.commitid[:7]}", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_no_diff_no_base_report( + def test_build_payload_no_diff_no_base_report( self, sample_comparison_without_base_with_pull, mock_repo_provider, @@ -2185,11 +2165,10 @@ async def test_build_payload_no_diff_no_base_report( current_yaml=UserYaml({}), ) expected_result = {"message": "Coverage not affected", "state": "success"} - result = await notifier.build_payload(comparison) + result = notifier.build_payload(comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_without_base_report( + def test_build_payload_without_base_report( self, sample_comparison_without_base_report, mock_repo_provider, @@ -2208,11 +2187,10 @@ async def test_build_payload_without_base_report( "message": "No report found to compare against", "state": "success", } - result = await notifier.build_payload(comparison) + result = notifier.build_payload(comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_with_multiple_changes( + def test_build_payload_with_multiple_changes( self, comparison_with_multiple_changes, mock_repo_provider, @@ -2234,13 +2212,12 @@ async def test_build_payload_with_multiple_changes( "message": "50.00% of diff hit (target 76.92%)", "state": "failure", } - result = await notifier.build_payload(comparison_with_multiple_changes) + result = notifier.build_payload(comparison_with_multiple_changes) assert expected_result == result class TestChangesStatusNotifier(object): - @pytest.mark.asyncio - async def test_build_payload( + def test_build_payload( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -2255,11 +2232,10 @@ async def test_build_payload( "message": "No indirect coverage changes found", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_upgrade_payload( + def test_build_upgrade_payload( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" @@ -2275,11 +2251,10 @@ async def test_build_upgrade_payload( "message": "Please activate this user to display a detailed status check", "state": "success", } - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_with_multiple_changes( + def test_build_payload_with_multiple_changes( self, comparison_with_multiple_changes, mock_repo_provider, @@ -2301,11 +2276,10 @@ async def test_build_payload_with_multiple_changes( "message": "3 files have indirect coverage changes not visible in diff", "state": "failure", } - result = await notifier.build_payload(comparison_with_multiple_changes) + result = notifier.build_payload(comparison_with_multiple_changes) assert expected_result == result - @pytest.mark.asyncio - async def test_build_payload_without_base_report( + def test_build_payload_without_base_report( self, sample_comparison_without_base_report, mock_repo_provider, @@ -2324,11 +2298,10 @@ async def test_build_payload_without_base_report( "message": "Unable to determine changes, no report found at pull request base", "state": "success", } - result = await notifier.build_payload(comparison) + result = notifier.build_payload(comparison) assert expected_result == result - @pytest.mark.asyncio - async def test_notify_path_filter( + def test_notify_path_filter( self, sample_comparison, mock_repo_provider, mock_configuration, mocker ): mocked_send_notification = mocker.patch.object( @@ -2348,12 +2321,11 @@ async def test_notify_path_filter( "state": "success", "url": f"test.example.br/gh/{sample_comparison.head.commit.repository.slug}/pull/{sample_comparison.pull.pullid}", } - result = await notifier.notify(sample_comparison) + result = notifier.notify(sample_comparison) assert result == mocked_send_notification.return_value mocked_send_notification.assert_called_with(sample_comparison, expected_result) - @pytest.mark.asyncio - async def test_build_passing_empty_upload_payload( + def test_build_passing_empty_upload_payload( self, sample_comparison, mock_repo_provider, mock_configuration ): mock_configuration.params["setup"]["codecov_url"] = "test.example.br" @@ -2366,5 +2338,5 @@ async def test_build_passing_empty_upload_payload( decoration_type=Decoration.passing_empty_upload, ) expected_result = {"state": "success", "message": "Non-testable files changed."} - result = await notifier.build_payload(sample_comparison) + result = notifier.build_payload(sample_comparison) assert expected_result == result diff --git a/services/notification/tests/unit/test_comparison.py b/services/notification/tests/unit/test_comparison.py index 1c5c3b915..bec5650ec 100644 --- a/services/notification/tests/unit/test_comparison.py +++ b/services/notification/tests/unit/test_comparison.py @@ -1,12 +1,10 @@ -import pytest from shared.reports.types import Change from services.comparison import ComparisonProxy, FilteredComparison class TestFilteredComparison(object): - @pytest.mark.asyncio - async def test_get_existing_statuses(self, mocker): + def test_get_existing_statuses(self, mocker): mocked_get_existing_statuses = mocker.patch.object( ComparisonProxy, "get_existing_statuses" ) @@ -14,11 +12,10 @@ async def test_get_existing_statuses(self, mocker): comparison = ComparisonProxy(mocker.MagicMock()) filtered_comparison = comparison.get_filtered_comparison(flags, path_patterns) assert isinstance(filtered_comparison, FilteredComparison) - res = await filtered_comparison.get_existing_statuses() + res = filtered_comparison.get_existing_statuses() assert res == mocked_get_existing_statuses.return_value - @pytest.mark.asyncio - async def test_get_changes_rust_vs_python(self, mocker): + def test_get_changes_rust_vs_python(self, mocker): mocker.patch.object(ComparisonProxy, "get_diff") mocker.patch( "services.comparison.get_changes", @@ -29,6 +26,6 @@ async def test_get_changes_rust_vs_python(self, mocker): return_value=[Change(path="banana"), Change(path="pear")], ) comparison = ComparisonProxy(mocker.MagicMock()) - res = await comparison.get_changes() + res = comparison.get_changes() expected_result = [Change(path="apple"), Change(path="pear")] assert expected_result == res diff --git a/tasks/compute_comparison.py b/tasks/compute_comparison.py index e8ae99f96..a249f1bd0 100644 --- a/tasks/compute_comparison.py +++ b/tasks/compute_comparison.py @@ -82,7 +82,7 @@ def run_impl( comparison.error = None try: - impacted_files = self.serialize_impacted_files(comparison_proxy) + impacted_files = comparison_proxy.get_impacted_files() except TorngitRateLimitError: log.warning( "Unable to compute comparison due to rate limit error", @@ -106,6 +106,7 @@ def run_impl( db_session.commit() self.compute_component_comparisons(db_session, comparison, comparison_proxy) db_session.commit() + return {"successful": True} def compute_flag_comparison(self, db_session, comparison, comparison_proxy): @@ -199,7 +200,7 @@ def get_flag_comparison_totals( totals = dict( head_totals=head_totals, base_totals=base_totals, patch_totals=None ) - diff = async_to_sync(comparison_proxy.get_diff)() + diff = comparison_proxy.get_diff() if diff: patch_totals = flag_head_report.apply_diff(diff) if patch_totals: @@ -277,7 +278,7 @@ def compute_component_comparison( filtered.project_coverage_base.report.totals.asdict() ) component_comparison.head_totals = filtered.head.report.totals.asdict() - diff = async_to_sync(comparison_proxy.get_diff)() + diff = comparison_proxy.get_diff() if diff: patch_totals = filtered.head.report.apply_diff(diff) if patch_totals: @@ -318,10 +319,6 @@ def get_comparison_proxy( ), ) - @sentry_sdk.trace - def serialize_impacted_files(self, comparison_proxy): - return async_to_sync(comparison_proxy.get_impacted_files)() - @sentry_sdk.trace def store_results(self, comparison, impacted_files): repository = comparison.compare_commit.repository