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

Conversation

@Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Feb 18, 2025

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.

grafik
In this profile, you can see calls to (unoptimized) match, once in ReadonlyReport.filter, and then again within ReportPaths.files.

@Swatinem Swatinem requested a review from a team February 18, 2025 13:49
@Swatinem Swatinem self-assigned this Feb 18, 2025
@codecov
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.07%. Comparing base (de5e284) to head (50828ff).
Report is 11 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1159   +/-   ##
=======================================
  Coverage   96.07%   96.07%           
=======================================
  Files         836      836           
  Lines       19723    19724    +1     
=======================================
+ Hits        18948    18950    +2     
+ Misses        775      774    -1     
Flag Coverage Δ
unit 95.93% <100.00%> (+<0.01%) ⬆️
unit-latest-uploader 95.93% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-staging
Copy link

codecov-staging bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@codecov-qa
Copy link

codecov-qa bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.93%. Comparing base (de5e284) to head (50828ff).
Report is 11 commits behind head on main.

✅ All tests successful. No failed tests found.

@codecov-public-qa
Copy link

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
2746 5 2741 6
View the top 3 failed tests by shortest run time
services/tests/test_path.py::TestReportPaths::test_files
Stack Traces | 0.003s run time
self = <services.tests.test_path.TestReportPaths testMethod=test_files>

    def test_files(self):
        flags = ["flag-123"]
        report = Report()
        session_a_id, _ = report.add_session(Session(flags=["flag-123"]))
    
        file_a = ReportFile("foo/file1.py")
        file_a.append(1, ReportLine.create(coverage=1, sessions=[[session_a_id, 1]]))
        report.append(file_a)
    
>       report_paths = ReportPaths(report=report, filter_flags=flags)

services/tests/test_path.py:231: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../local/lib/python3.12.../site-packages/sentry_sdk/tracing_utils.py:673: in func_with_tracing
    return func(*args, **kwargs)
services/path.py:170: in __init__
    for full_path in self.files
.../local/lib/python3.12/functools.py:998: in __get__
    val = self.func(instance)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <services.path.ReportPaths object at 0x7ff9dbfe3260>

    @cached_property
    def files(self) -> list[str]:
        # 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 = []
        for file in self.report:
            if isinstance(file, FilteredReportFile):
                found = False
>               for _ln, line in self.report_file.lines:
E               AttributeError: 'ReportPaths' object has no attribute 'report_file'

services/path.py:196: AttributeError
api/internal/tests/views/test_coverage_viewset.py::CoverageViewSetTests::test_tree_no_data_for_flags
Stack Traces | 0.127s run time
self = <test_coverage_viewset.CoverageViewSetTests testMethod=test_tree_no_data_for_flags>
build_report_from_commit = <MagicMock name='build_report_from_commit' id='140712244092064'>

    @patch("shared.reports.api_report_service.build_report_from_commit")
    def test_tree_no_data_for_flags(self, build_report_from_commit):
        build_report_from_commit.return_value = sample_report()
>       res = self._tree(flags="Does_not_exist")

.../tests/views/test_coverage_viewset.py:346: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../tests/views/test_coverage_viewset.py:61: in _tree
    return self.client.get(url)
.../local/lib/python3.12.../django/test/client.py:927: in get
    response = super().get(path, data=data, secure=secure, headers=headers, **extra)
.../local/lib/python3.12.../django/test/client.py:457: in get
    return self.generic(
.../local/lib/python3.12.../django/test/client.py:609: in generic
    return self.request(**r)
.../local/lib/python3.12.../django/test/client.py:891: in request
    self.check_exception(response)
.../local/lib/python3.12.../django/test/client.py:738: in check_exception
    raise exc_value
.../local/lib/python3.12.../core/handlers/exception.py:55: in inner
    response = get_response(request)
.../local/lib/python3.12.../core/handlers/base.py:197: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
.../local/lib/python3.12.../integrations/django/views.py:89: in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
.../local/lib/python3.12.../views/decorators/csrf.py:56: in wrapper_view
    return view_func(*args, **kwargs)
.../local/lib/python3.12.../site-packages/rest_framework/viewsets.py:124: in view
    return self.dispatch(request, *args, **kwargs)
.../local/lib/python3.12............/site-packages/rest_framework/views.py:509: in dispatch
    response = self.handle_exception(exc)
.../local/lib/python3.12............/site-packages/rest_framework/views.py:469: in handle_exception
    self.raise_uncaught_exception(exc)
.../local/lib/python3.12............/site-packages/rest_framework/views.py:480: in raise_uncaught_exception
    raise exc
.../local/lib/python3.12............/site-packages/rest_framework/views.py:506: in dispatch
    response = handler(request, *args, **kwargs)
.../internal/coverage/views.py:67: in tree
    paths = self.get_object()
.../internal/coverage/views.py:59: in get_object
    paths = ReportPaths(
.../local/lib/python3.12.../site-packages/sentry_sdk/tracing_utils.py:679: in func_with_tracing
    return func(*args, **kwargs)
services/path.py:170: in __init__
    for full_path in self.files
.../local/lib/python3.12/functools.py:998: in __get__
    val = self.func(instance)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <services.path.ReportPaths object at 0x7ffa1f83fb30>

    @cached_property
    def files(self) -> list[str]:
        # 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 = []
        for file in self.report:
            if isinstance(file, FilteredReportFile):
                found = False
>               for _ln, line in self.report_file.lines:
E               AttributeError: 'ReportPaths' object has no attribute 'report_file'

services/path.py:196: AttributeError
graphql_api/tests/test_branch.py::TestBranch::test_fetch_path_contents_component_and_flag_filters
Stack Traces | 0.451s run time
self = <graphql_api.tests.test_branch.TestBranch testMethod=test_fetch_path_contents_component_and_flag_filters>
session_files_mock = <MagicMock name='files_in_sessions' id='140708283399440'>
flag_files_mock = <MagicMock name='files_belonging_to_flags' id='140708283397232'>
report_mock = <MagicMock name='build_report_from_commit' id='140708283368320'>
commit_components_mock = <MagicMock name='commit_components' id='140708283382480'>
filtered_mock = <MagicMock name='component_filtered_report' id='140708263856608'>

    @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")
    def test_fetch_path_contents_component_and_flag_filters(
        self,
        session_files_mock,
        flag_files_mock,
        report_mock,
        commit_components_mock,
        filtered_mock,
    ):
        session_files_mock.return_value = ["fileA.py"]
        flag_files_mock.return_value = ["fileA.py"]
        report_mock.return_value = MockReport()
        commit_components_mock.return_value = [
            Component.from_dict(
                {
                    "component_id": "unit",
                    "name": "unit",
                    "paths": ["fileA.py"],
                    "flag_regexes": "flag-a",
                }
            ),
            Component.from_dict(
                {
                    "component_id": "integration",
                    "name": "integration",
                    "paths": ["fileB.py"],
                }
            ),
            Component.from_dict(
                {
                    "component_id": "global",
                    "name": "Global",
                    "paths": ["(?s:.*/[^\\/]*\\.py.*)\\Z"],
                    "flag_regexes": "flag-a",
                }
            ),
        ]
        filtered_mock.return_value = MockReport()
    
        query_files = """
            query FetchFiles($org: String!, $repo: String!, $branch: String!, $path: String!, $filters: PathContentsFilters!) {
                owner(username: $org) {
                    repository(name: $repo) {
                        ... on Repository {
                            branch(name: $branch) {
                                head {
                                    pathContents (path: $path, filters: $filters) {
                                        __typename
                                        ... on PathContents {
                                            results {
                                                __typename
                                                name
                                                path
                                            }
                                        }
                                        ... on MissingCoverage {
                                            message
                                        }
                                        ... on UnknownFlags {
                                            message
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        """
        components, flags = ["unit"], ["flag-a"]
        variables = {
            "org": self.org.username,
            "repo": self.repo.name,
            "branch": self.branch.name,
            "path": "",
            "filters": {"components": components, "flags": flags},
        }
        data = self.gql_request(query_files, variables=variables)
    
>       assert data == {
            "owner": {
                "repository": {
                    "branch": {
                        "head": {
                            "pathContents": {
                                "__typename": "PathContents",
                                "results": [
                                    {
                                        "__typename": "PathContentFile",
                                        "name": "fileA.py",
                                        "path": "fileA.py",
                                    }
                                ],
                            }
                        }
                    }
                }
            }
        }
E       AssertionError: assert {'owner': {'r...': [...]}}}}}} == {'owner': {'r...': [...]}}}}}}
E         
E         Differing items:
E         {'owner': {'repository': {'branch': {'head': {'pathContents': {'__typename': 'PathContents', 'results': [...]}}}}}} != {'owner': {'repository': {'branch': {'head': {'pathContents': {'__typename': 'PathContents', 'results': [...]}}}}}}
E         Use -v to get more diff

graphql_api/tests/test_branch.py:935: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Contributor

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
2752 5 2741 6
View the top 3 failed tests by shortest run time
test_files
Stack Traces | 0.003s run time
self = &lt;services.tests.test_path.TestReportPaths testMethod=test_files&gt;

    def test_files(self):
        flags = ["flag-123"]
        report = Report()
        session_a_id, _ = report.add_session(Session(flags=["flag-123"]))
    
        file_a = ReportFile("foo/file1.py")
        file_a.append(1, ReportLine.create(coverage=1, sessions=[[session_a_id, 1]]))
        report.append(file_a)
    
&gt;       report_paths = ReportPaths(report=report, filter_flags=flags)

services/tests/test_path.py:231: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/local/lib/python3.12/site-packages/sentry_sdk/tracing_utils.py:673: in func_with_tracing
    return func(*args, **kwargs)
services/path.py:170: in __init__
    for full_path in self.files
/usr/local/lib/python3.12/functools.py:998: in __get__
    val = self.func(instance)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = &lt;services.path.ReportPaths object at 0x7ff9dbfe3260&gt;

    @cached_property
    def files(self) -&gt; list[str]:
        # 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 = []
        for file in self.report:
            if isinstance(file, FilteredReportFile):
                found = False
&gt;               for _ln, line in self.report_file.lines:
E               AttributeError: 'ReportPaths' object has no attribute 'report_file'

services/path.py:196: AttributeError
test_tree_no_data_for_flags
Stack Traces | 0.127s run time
self = &lt;test_coverage_viewset.CoverageViewSetTests testMethod=test_tree_no_data_for_flags&gt;
build_report_from_commit = &lt;MagicMock name='build_report_from_commit' id='140712244092064'&gt;

    @patch("shared.reports.api_report_service.build_report_from_commit")
    def test_tree_no_data_for_flags(self, build_report_from_commit):
        build_report_from_commit.return_value = sample_report()
&gt;       res = self._tree(flags="Does_not_exist")

api/internal/tests/views/test_coverage_viewset.py:346: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
api/internal/tests/views/test_coverage_viewset.py:61: in _tree
    return self.client.get(url)
/usr/local/lib/python3.12/site-packages/django/test/client.py:927: in get
    response = super().get(path, data=data, secure=secure, headers=headers, **extra)
/usr/local/lib/python3.12/site-packages/django/test/client.py:457: in get
    return self.generic(
/usr/local/lib/python3.12/site-packages/django/test/client.py:609: in generic
    return self.request(**r)
/usr/local/lib/python3.12/site-packages/django/test/client.py:891: in request
    self.check_exception(response)
/usr/local/lib/python3.12/site-packages/django/test/client.py:738: in check_exception
    raise exc_value
/usr/local/lib/python3.12/site-packages/django/core/handlers/exception.py:55: in inner
    response = get_response(request)
/usr/local/lib/python3.12/site-packages/django/core/handlers/base.py:197: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
/usr/local/lib/python3.12/site-packages/sentry_sdk/integrations/django/views.py:89: in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
/usr/local/lib/python3.12/site-packages/django/views/decorators/csrf.py:56: in wrapper_view
    return view_func(*args, **kwargs)
/usr/local/lib/python3.12/site-packages/rest_framework/viewsets.py:124: in view
    return self.dispatch(request, *args, **kwargs)
/usr/local/lib/python3.12/site-packages/rest_framework/views.py:509: in dispatch
    response = self.handle_exception(exc)
/usr/local/lib/python3.12/site-packages/rest_framework/views.py:469: in handle_exception
    self.raise_uncaught_exception(exc)
/usr/local/lib/python3.12/site-packages/rest_framework/views.py:480: in raise_uncaught_exception
    raise exc
/usr/local/lib/python3.12/site-packages/rest_framework/views.py:506: in dispatch
    response = handler(request, *args, **kwargs)
api/internal/coverage/views.py:67: in tree
    paths = self.get_object()
api/internal/coverage/views.py:59: in get_object
    paths = ReportPaths(
/usr/local/lib/python3.12/site-packages/sentry_sdk/tracing_utils.py:679: in func_with_tracing
    return func(*args, **kwargs)
services/path.py:170: in __init__
    for full_path in self.files
/usr/local/lib/python3.12/functools.py:998: in __get__
    val = self.func(instance)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = &lt;services.path.ReportPaths object at 0x7ffa1f83fb30&gt;

    @cached_property
    def files(self) -&gt; list[str]:
        # 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 = []
        for file in self.report:
            if isinstance(file, FilteredReportFile):
                found = False
&gt;               for _ln, line in self.report_file.lines:
E               AttributeError: 'ReportPaths' object has no attribute 'report_file'

services/path.py:196: AttributeError
test_fetch_path_contents_component_and_flag_filters
Stack Traces | 0.451s run time
self = &lt;graphql_api.tests.test_branch.TestBranch testMethod=test_fetch_path_contents_component_and_flag_filters&gt;
session_files_mock = &lt;MagicMock name='files_in_sessions' id='140708283399440'&gt;
flag_files_mock = &lt;MagicMock name='files_belonging_to_flags' id='140708283397232'&gt;
report_mock = &lt;MagicMock name='build_report_from_commit' id='140708283368320'&gt;
commit_components_mock = &lt;MagicMock name='commit_components' id='140708283382480'&gt;
filtered_mock = &lt;MagicMock name='component_filtered_report' id='140708263856608'&gt;

    @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")
    def test_fetch_path_contents_component_and_flag_filters(
        self,
        session_files_mock,
        flag_files_mock,
        report_mock,
        commit_components_mock,
        filtered_mock,
    ):
        session_files_mock.return_value = ["fileA.py"]
        flag_files_mock.return_value = ["fileA.py"]
        report_mock.return_value = MockReport()
        commit_components_mock.return_value = [
            Component.from_dict(
                {
                    "component_id": "unit",
                    "name": "unit",
                    "paths": ["fileA.py"],
                    "flag_regexes": "flag-a",
                }
            ),
            Component.from_dict(
                {
                    "component_id": "integration",
                    "name": "integration",
                    "paths": ["fileB.py"],
                }
            ),
            Component.from_dict(
                {
                    "component_id": "global",
                    "name": "Global",
                    "paths": ["(?s:.*/[^\\/]*\\.py.*)\\Z"],
                    "flag_regexes": "flag-a",
                }
            ),
        ]
        filtered_mock.return_value = MockReport()
    
        query_files = """
            query FetchFiles($org: String!, $repo: String!, $branch: String!, $path: String!, $filters: PathContentsFilters!) {
                owner(username: $org) {
                    repository(name: $repo) {
                        ... on Repository {
                            branch(name: $branch) {
                                head {
                                    pathContents (path: $path, filters: $filters) {
                                        __typename
                                        ... on PathContents {
                                            results {
                                                __typename
                                                name
                                                path
                                            }
                                        }
                                        ... on MissingCoverage {
                                            message
                                        }
                                        ... on UnknownFlags {
                                            message
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        """
        components, flags = ["unit"], ["flag-a"]
        variables = {
            "org": self.org.username,
            "repo": self.repo.name,
            "branch": self.branch.name,
            "path": "",
            "filters": {"components": components, "flags": flags},
        }
        data = self.gql_request(query_files, variables=variables)
    
&gt;       assert data == {
            "owner": {
                "repository": {
                    "branch": {
                        "head": {
                            "pathContents": {
                                "__typename": "PathContents",
                                "results": [
                                    {
                                        "__typename": "PathContentFile",
                                        "name": "fileA.py",
                                        "path": "fileA.py",
                                    }
                                ],
                            }
                        }
                    }
                }
            }
        }
E       AssertionError: assert {'owner': {'r...': [...]}}}}}} == {'owner': {'r...': [...]}}}}}}
E         
E         Differing items:
E         {'owner': {'repository': {'branch': {'head': {'pathContents': {'__typename': 'PathContents', 'results': [...]}}}}}} != {'owner': {'repository': {'branch': {'head': {'pathContents': {'__typename': 'PathContents', 'results': [...]}}}}}}
E         Use -v to get more diff

graphql_api/tests/test_branch.py:935: AssertionError

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

@Swatinem Swatinem force-pushed the swatinem/filter-reportpaths branch from a71b222 to 0f4aea1 Compare February 19, 2025 12:52
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`.
@Swatinem Swatinem force-pushed the swatinem/filter-reportpaths branch from 0f4aea1 to 50828ff Compare February 19, 2025 13:23
@Swatinem Swatinem marked this pull request as ready for review February 19, 2025 13:34
if not self.filter_flags:
return self.report.files

# When there is a flag filter, `FilteredReport` currently yields
Copy link
Contributor

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

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

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

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.

@Swatinem Swatinem added this pull request to the merge queue Feb 25, 2025
Merged via the queue into main with commit 8ed03b4 Feb 25, 2025
22 of 23 checks passed
@Swatinem Swatinem deleted the swatinem/filter-reportpaths branch February 25, 2025 07:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants