Refactor and optimize querying `pathContents` #1141

Swatinem · 2025-02-07T09:26:23Z

First of all, this fixes and adds missing type annotations.

It then deduplicates the code between pathContents and the deprecated variant, which were 99% identical.

Resolving the path contents was also optimized a little bit by not filtering the report for "files matching any session in the report", aka "all files". Other optimizations include avoiding some intermediate allocations by sorting lists in-place, and simplifying some functions.

First of all, this fixes and adds missing type annotations. It then deduplicates the code between `pathContents` and the `deprecated` variant, which were 99% identical. Resolving the path contents was also optimized a little bit by not filtering the report for "files matching *any* session in the report", aka "all files". Other optimizations include avoiding some intermediate allocations by sorting lists in-place, and simplifying some functions.

codecov · 2025-02-07T10:51:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.05%. Comparing base (7bd85ae) to head (3929f05).
Report is 15 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1141      +/-   ##
==========================================
- Coverage   96.07%   96.05%   -0.02%     
==========================================
  Files         837      838       +1     
  Lines       19855    20323     +468     
==========================================
+ Hits        19075    19521     +446     
- Misses        780      802      +22

Flag	Coverage Δ
unit	`95.96% <100.00%> (-0.02%)`	⬇️
unit-latest-uploader	`95.96% <100.00%> (-0.02%)`	⬇️

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-notifications · 2025-02-07T10:51:19Z

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 · 2025-02-07T10:51:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.96%. Comparing base (7bd85ae) to head (3929f05).
Report is 15 commits behind head on main.

✅ All tests successful. No failed tests found.

github-actions · 2025-02-07T10:51:30Z

✅ All tests successful. No failed tests were found.

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

codecov-public-qa · 2025-02-07T10:51:30Z

❌ 5 Tests Failed:

Tests completed	Failed	Passed	Skipped
2737	5	2732	6

View the top 3 failed tests by shortest run time

services/tests/test_path.py::TestReportPathsNested::test_single_directory

Stack Traces | 0.007s run time

self = <services.tests.test_path.TestReportPathsNested testMethod=test_single_directory>

    def test_single_directory(self):
        report_paths = ReportPaths(self.report, path="dir")
>       assert report_paths.single_directory() == [
            File(full_path="dir/file1.py", totals=totals1),
            Dir(
                full_path="dir/subdir",
                children=[
                    File(full_path="dir/subdir/file2.py", totals=totals2),
                    Dir(
                        full_path="dir/subdir/dir1",
                        children=[
                            File(full_path=".../subdir/dir1/file3.py", totals=totals3),
                        ],
                    ),
                    Dir(
                        full_path="dir/subdir/dir2",
                        children=[
                            File(full_path=".../subdir/dir2/file3.py", totals=totals3),
                        ],
                    ),
                ],
            ),
        ]
E       AssertionError: assert [File(full_pa...0, diff=0))])] == [File(full_pa... diff=0))])])]
E         
E         At index 1 diff: Dir(full_path='dir/subdir', children=[File(full_path='dir/subdir/file2.py', totals=ReportTotals(files=0, lines=10, hits=8, misses=2, partials=0, coverage='80.00000', branches=0, methods=0, messages=0, sessions=0, complexity=0, complexity_total=0, diff=0))]) != Dir(full_path='dir/subdir', children=[File(full_path='dir/subdir/file2.py', totals=ReportTotals(files=0, lines=10, hits=8, misses=2, partials=0, coverage='80.00000', branches=0, methods=0, messages=0, sessions=0, complexity=0, complexity_total=0, diff=0)), Dir(full_path='dir/subdir/dir1', children...
E         
E         ...Full output truncated (3 lines hidden), use '-vv' to show

services/tests/test_path.py:249: AssertionError

graphql_api/tests/test_branch.py::TestBranch::test_fetch_path_contents_with_files

Stack Traces | 0.455s run time

self = <graphql_api.tests.test_branch.TestBranch testMethod=test_fetch_path_contents_with_files>
report_mock = <MagicMock name='build_report_from_commit' id='139773809214720'>
critical_files = <PropertyMock name='critical_files' id='139773790127792'>

    @patch(
        "services.profiling.ProfilingSummary.critical_files", new_callable=PropertyMock
    )
    @patch("shared.reports.api_report_service.build_report_from_commit")
    def test_fetch_path_contents_with_files(self, report_mock, critical_files):
        variables = {
            "org": self.org.username,
            "repo": self.repo.name,
            "branch": self.branch.name,
            "path": "",
            "filters": {
                "ordering": {
                    "direction": "DESC",
                    "parameter": "NAME",
                }
            },
        }
        report_mock.return_value = MockReport()
        critical_files.return_value = [CriticalFile("fileA.py")]
    
        data = self.gql_request(query_files, variables=variables)
>       assert data == {
            "owner": {
                "repository": {
                    "branch": {
                        "head": {
                            "pathContents": {
                                "__typename": "PathContents",
                                "results": [
                                    {
                                        "__typename": "PathContentDir",
                                        "name": "folder",
                                        "path": "folder",
                                        "hits": 24,
                                        "misses": 0,
                                        "partials": 0,
                                        "lines": 30,
                                        "percentCovered": 80.0,
                                    },
                                    {
                                        "__typename": "PathContentFile",
                                        "name": "fileB.py",
                                        "path": "fileB.py",
                                        "hits": 8,
                                        "misses": 0,
                                        "partials": 0,
                                        "lines": 10,
                                        "percentCovered": 80.0,
                                        "isCriticalFile": False,
                                    },
                                    {
                                        "__typename": "PathContentFile",
                                        "name": "fileA.py",
                                        "path": "fileA.py",
                                        "hits": 8,
                                        "misses": 0,
                                        "partials": 0,
                                        "lines": 10,
                                        "percentCovered": 80.0,
                                        "isCriticalFile": True,
                                    },
                                ],
                            }
                        }
                    }
                }
            }
        }
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:362: AssertionError

graphql_api/tests/test_branch.py::TestBranch::test_fetch_path_contents_deprecated

Stack Traces | 0.458s run time

self = <graphql_api.tests.test_branch.TestBranch testMethod=test_fetch_path_contents_deprecated>
report_mock = <MagicMock name='build_report_from_commit' id='139773921932736'>
critical_files_mock = <PropertyMock name='critical_files' id='139773922150592'>

    @patch(
        "services.profiling.ProfilingSummary.critical_files", new_callable=PropertyMock
    )
    @patch("shared.reports.api_report_service.build_report_from_commit")
    def test_fetch_path_contents_deprecated(self, report_mock, critical_files_mock):
        report_mock.return_value = MockReport()
        critical_files_mock.return_value = []
    
        variables = {
            "org": self.org.username,
            "repo": self.repo.name,
            "branch": self.branch.name,
            "path": "",
            "filters": {},
        }
    
        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",
                                        "hits": 8,
                                        "misses": 0,
                                        "partials": 0,
                                        "lines": 10,
                                        "percentCovered": 80.0,
                                        "isCriticalFile": False,
                                    },
                                    {
                                        "__typename": "PathContentFile",
                                        "name": "fileB.py",
                                        "path": "fileB.py",
                                        "hits": 8,
                                        "misses": 0,
                                        "partials": 0,
                                        "lines": 10,
                                        "percentCovered": 80.0,
                                        "isCriticalFile": False,
                                    },
                                    {
                                        "__typename": "PathContentDir",
                                        "name": "folder",
                                        "path": "folder",
                                        "hits": 24,
                                        "misses": 0,
                                        "partials": 0,
                                        "lines": 30,
                                        "percentCovered": 80.0,
                                    },
                                ],
                            }
                        }
                    }
                }
            }
        }
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:1160: AssertionError

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

joseph-sentry

qq about a functionality change in get_sorted_paths, but lgtm to merge

graphql_api/actions/path_contents.py

graphql_api/types/commit/commit.py

Swatinem self-assigned this Feb 7, 2025

Swatinem requested a review from a team as a code owner February 7, 2025 09:26

Swatinem force-pushed the swatinem/opt-paths branch from 6af00f3 to 5f2cb34 Compare February 7, 2025 09:30

avoid string building in PrefixedPath.basename

efdc87b

Swatinem mentioned this pull request Feb 7, 2025

Optimize usage of match #1142

Merged

fix type annotation

6dc1b8c

fix test mocks, and revert basename changes

ca99ee9

joseph-sentry approved these changes Feb 12, 2025

View reviewed changes

graphql_api/actions/path_contents.py Show resolved Hide resolved

joseph-sentry reviewed Feb 12, 2025

View reviewed changes

graphql_api/types/commit/commit.py Outdated Show resolved Hide resolved

review

3929f05

Swatinem added this pull request to the merge queue Feb 13, 2025

Merged via the queue into main with commit 115912a Feb 13, 2025
16 of 19 checks passed

Swatinem deleted the swatinem/opt-paths branch February 13, 2025 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and optimize querying `pathContents` #1141

Refactor and optimize querying `pathContents` #1141

Swatinem commented Feb 7, 2025

codecov bot commented Feb 7, 2025 •

edited

Loading

codecov-notifications bot commented Feb 7, 2025 •

edited

Loading

codecov-qa bot commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading

codecov-public-qa bot commented Feb 7, 2025

joseph-sentry left a comment

Refactor and optimize querying pathContents #1141

Refactor and optimize querying pathContents #1141

Conversation

Swatinem commented Feb 7, 2025

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

codecov-notifications bot commented Feb 7, 2025 • edited Loading

Codecov Report

codecov-qa bot commented Feb 7, 2025 • edited Loading

Codecov Report

github-actions bot commented Feb 7, 2025 • edited Loading

codecov-public-qa bot commented Feb 7, 2025

❌ 5 Tests Failed:

joseph-sentry left a comment

Choose a reason for hiding this comment

Refactor and optimize querying `pathContents` #1141

Refactor and optimize querying `pathContents` #1141

codecov bot commented Feb 7, 2025 •

edited

Loading

codecov-notifications bot commented Feb 7, 2025 •

edited

Loading

codecov-qa bot commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading