From 50828ff58a773c27aa3aeee5867f6dc00e047620 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Tue, 18 Feb 2025 14:46:34 +0100 Subject: [PATCH] Do not double-filter Reports in `ReportPaths` Previously, the `ReportPaths.files` accessor would manually filter through the `unfiltered_report`. Even though the `self.report` was already filtered. Now the custom filtering logic is a bit updated, but taking care of existing bugs in `FilteredReport`, as that is still yielding files even though they are not matching any `flags`. --- graphql_api/tests/test_branch.py | 49 ++++++++++++++--------------- services/path.py | 54 +++++++++++++++----------------- 2 files changed, 50 insertions(+), 53 deletions(-) diff --git a/graphql_api/tests/test_branch.py b/graphql_api/tests/test_branch.py index 65db99834c..7d3c628727 100644 --- a/graphql_api/tests/test_branch.py +++ b/graphql_api/tests/test_branch.py @@ -772,18 +772,15 @@ def test_fetch_path_contents_component_filter_missing_coverage( @patch("services.components.component_filtered_report") @patch("services.components.commit_components") @patch("shared.reports.api_report_service.build_report_from_commit") + @patch("services.path.ReportPaths.files", new_callable=PropertyMock) def test_fetch_path_contents_component_filter_has_coverage( - self, report_mock, commit_components_mock, filtered_mock + self, files_mock, report_mock, commit_components_mock, filtered_mock ): - components = ["Global"] - variables = { - "org": self.org.username, - "repo": self.repo.name, - "branch": self.branch.name, - "path": "", - "filters": {"components": components}, - } - + files_mock.return_value = [ + "folder/fileB.py", + "folder/subfolder/fileC.py", + "folder/subfolder/fileD.py", + ] report_mock.return_value = MockReport() commit_components_mock.return_value = [ Component.from_dict( @@ -810,6 +807,14 @@ def test_fetch_path_contents_component_filter_has_coverage( ] filtered_mock.return_value = MockReport() + components = ["Global"] + variables = { + "org": self.org.username, + "repo": self.repo.name, + "branch": self.branch.name, + "path": "", + "filters": {"components": components}, + } data = self.gql_request(query_files, variables=variables) assert data == { @@ -841,18 +846,15 @@ def test_fetch_path_contents_component_filter_has_coverage( @patch("services.components.component_filtered_report") @patch("services.components.commit_components") @patch("shared.reports.api_report_service.build_report_from_commit") - @patch("services.report.files_belonging_to_flags") - @patch("services.report.files_in_sessions") + @patch("services.path.ReportPaths.files", new_callable=PropertyMock) def test_fetch_path_contents_component_and_flag_filters( self, - session_files_mock, - flag_files_mock, + files_mock, report_mock, commit_components_mock, filtered_mock, ): - session_files_mock.return_value = ["fileA.py"] - flag_files_mock.return_value = ["fileA.py"] + files_mock.return_value = ["fileA.py"] report_mock.return_value = MockReport() commit_components_mock.return_value = [ Component.from_dict( @@ -945,11 +947,11 @@ def test_fetch_path_contents_component_and_flag_filters( @patch("services.components.component_filtered_report") @patch("services.components.commit_components") @patch("shared.reports.api_report_service.build_report_from_commit") - @patch("services.report.files_belonging_to_flags") + @patch("services.path.ReportPaths.files", new_callable=PropertyMock) def test_fetch_path_contents_component_and_flag_filters_unknown_flags( - self, flag_files_mock, report_mock, commit_components_mock, filtered_mock + self, files_mock, report_mock, commit_components_mock, filtered_mock ): - flag_files_mock.return_value = ["fileA.py"] + files_mock.return_value = ["fileA.py"] report_mock.return_value = MockNoFlagsReport() commit_components_mock.return_value = [ Component.from_dict( @@ -1035,18 +1037,15 @@ def test_fetch_path_contents_component_and_flag_filters_unknown_flags( @patch("services.components.component_filtered_report") @patch("services.components.commit_components") @patch("shared.reports.api_report_service.build_report_from_commit") - @patch("services.report.files_belonging_to_flags") - @patch("services.report.files_in_sessions") + @patch("services.path.ReportPaths.files", new_callable=PropertyMock) def test_fetch_path_contents_component_flags_filters( self, - session_files_mock, - flag_files_mock, + files_mock, report_mock, commit_components_mock, filtered_mock, ): - session_files_mock.return_value = ["fileA.py"] - flag_files_mock.return_value = ["fileA.py"] + files_mock.return_value = ["fileA.py"] report_mock.return_value = MockReport() commit_components_mock.return_value = [ Component.from_dict( diff --git a/services/path.py b/services/path.py index 372bd90a1f..ea9295464f 100644 --- a/services/path.py +++ b/services/path.py @@ -8,12 +8,11 @@ import sentry_sdk from asgiref.sync import async_to_sync from django.conf import settings +from shared.reports.filtered import FilteredReport, FilteredReportFile from shared.reports.resources import Report from shared.reports.types import ReportTotals from shared.torngit.exceptions import TorngitClientError -from shared.utils.match import Matcher -import services.report as report_service from codecov_auth.models import Owner from core.models import Commit from services.repo_providers import RepoProviderService @@ -150,13 +149,12 @@ def __init__( report: Report, path: str | None = None, search_term: str | None = None, - filter_flags: list[str] = [], - filter_paths: list[str] = [], + filter_flags: list[str] = None, + filter_paths: list[str] = None, ): - self.report = report - self.unfiltered_report = report - self.filter_flags = filter_flags - self.filter_paths = filter_paths + self.report: Report | FilteredReport = report + self.filter_flags = filter_flags or [] + self.filter_paths = filter_paths or [] self.prefix = path or "" # Filter report if flags or paths exist @@ -172,37 +170,37 @@ def __init__( ] if search_term: + search_term = search_term.lower() self._paths = [ path - for path in self.paths - if search_term.lower() in path.relative_path.lower() + for path in self._paths + if search_term in path.relative_path.lower() ] @cached_property def files(self) -> list[str]: - # No filtering, just return files in Report - if not self.filter_flags and not self.filter_paths: + # No flags filtering, just return (path-filtered) files in Report + if not self.filter_flags: return self.report.files + # When there is a flag filter, `FilteredReport` currently yields + # `FilteredReportFile`s without actually checking whether they match the sessions. + # Before that bug is fixed, lets do the filtering manually here. Once that bug is fixed, + # this should just forward to `self.report.files` like above. files = [] - # Do flag filtering if needed - if self.filter_flags: - files = report_service.files_belonging_to_flags( - commit_report=self.unfiltered_report, flags=self.filter_flags - ) - else: - files = [file.name for file in self.unfiltered_report] - - # Do path filtering if needed - if self.filter_paths: - matcher = Matcher(self.filter_paths) - files = [file for file in files if matcher.match(file)] - + for file in self.report: + if isinstance(file, FilteredReportFile): + found = False + for _ln, line in file.lines: + if line and any(s.id in file.session_ids for s in line.sessions): + found = True + break + if not found: + continue + + files.append(file.name) return files - def _filter_commit_report(self) -> None: - self.report = self.report.filter(flags=self.filter_flags) - @property def paths(self) -> list[PrefixedPath]: return self._paths