Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.

Do not double-filter Reports in ReportPaths #1159

Merged
merged 1 commit into from
Feb 25, 2025
Merged

Do not double-filter Reports in ReportPaths #1159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 24 additions & 25 deletions graphql_api/tests/test_branch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 == {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
54 changes: 26 additions & 28 deletions services/path.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -150,13 +149,12 @@
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
Expand All @@ -172,37 +170,37 @@
]

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
Copy link
Contributor

@adrian-codecov adrian-codecov Feb 20, 2025

Choose a reason for hiding this comment

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

Wait so are we changing the behavior of this filtering?

Copy link
Contributor Author

@Swatinem Swatinem Feb 21, 2025

Choose a reason for hiding this comment

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

We have to paper over a bug in the FilteredReport, in order to keep the filtering here in api the same.
As I am working towards getting a clearer picture of what all the various Report classes do, I might just fix these bugs "upstream", so we do not need another workaround here anymore.

Copy link
Contributor

@adrian-codecov adrian-codecov Feb 21, 2025

Choose a reason for hiding this comment

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

Gotcha, so is this PR dependant on that change to maintain this functionality?

Copy link
Contributor Author

@Swatinem Swatinem Feb 24, 2025

Choose a reason for hiding this comment

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

No, thats the reason I have that check here. Otherwise I could just wait for this stuff to be fixed in shared.

# `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

Check warning on line 199 in services/path.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/path.py#L191-L199 

Added lines #L191 - L199 were not covered by tests

files.append(file.name)

Check warning on line 201 in services/path.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/path.py#L201 

Added line #L201 was not covered by tests
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
Expand Down
Loading