Skip to content

Commit

Permalink
Optimize repeated path-fixing and calls to supports_labels (#875)
Browse files Browse the repository at this point in the history
  • Loading branch information
Swatinem authored Nov 12, 2024
1 parent 48e8163 commit 0d27b33
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 11 deletions.
20 changes: 17 additions & 3 deletions services/path_fixer/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging
import os.path
from pathlib import PurePath
from typing import Sequence

import sentry_sdk
from shared.yaml import UserYaml
Expand Down Expand Up @@ -119,14 +120,15 @@ def get_relative_path_aware_pathfixer(self, base_path) -> "BasePathAwarePathFixe

class BasePathAwarePathFixer(PathFixer):
def __init__(self, original_path_fixer, base_path) -> None:
self._resolved_paths: dict[tuple[str, Sequence[str]], str | None] = {}
self.original_path_fixer = original_path_fixer

# base_path argument is the file path after the "# path=" in the report containing report location, if provided.
# to get the base path we use, strip the coverage report from the path to get the base path
# e.g.: "path/to/coverage.xml" --> "path/to/"
self.base_path = [PurePath(base_path).parent] if base_path is not None else []

def __call__(self, path: str, bases_to_try: list[str] | None = None) -> str | None:
def _try_fix_path(self, path: str, bases_to_try: Sequence[str]) -> str | None:
original_path_fixer_result = self.original_path_fixer(path)
if (
original_path_fixer_result is not None
Expand All @@ -136,13 +138,25 @@ def __call__(self, path: str, bases_to_try: list[str] | None = None) -> str | No
return original_path_fixer_result

if not os.path.isabs(path):
all_base_paths_to_try = self.base_path + (
bases_to_try if bases_to_try is not None else []
all_base_paths_to_try = (
self.base_path + list(bases_to_try) if bases_to_try else self.base_path
)

for base_path in all_base_paths_to_try:
adjusted_path = os.path.join(base_path, path)
base_path_aware_result = self.original_path_fixer(adjusted_path)
if base_path_aware_result is not None:
return base_path_aware_result

return original_path_fixer_result

def __call__(
self, path: str, bases_to_try: Sequence[str] | None = None
) -> str | None:
bases_to_try = bases_to_try or tuple()
key = (path, bases_to_try)

if key not in self._resolved_paths:
self._resolved_paths[key] = self._try_fix_path(path, bases_to_try)

return self._resolved_paths[key]
4 changes: 2 additions & 2 deletions services/path_fixer/tests/unit/test_path_fixer.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def test_basepath_uses_own_result_if_main_is_none_multuple_base_paths(self):
assert pf("__init__.py") is None
assert base_aware_pf("__init__.py") is None
assert (
base_aware_pf("__init__.py", bases_to_try=["/home/travis/build/project"])
base_aware_pf("__init__.py", bases_to_try=("/home/travis/build/project",))
== "project/__init__.py"
)

Expand All @@ -136,7 +136,7 @@ def test_ambiguous_paths():
]
base_path = "/home/runner/work/owner/repo/foobar/build/coverage/coverage.xml"
# ~~~~~~
bases_to_try = ["/app"]
bases_to_try = ("/app",)
# The problem here is that the given `file_name` is ambiguous, and neither the
# `base_path` nor the `bases_to_try` is helping us narrow this down.
# The `base_path` does include one of the relevant parent directories,
Expand Down
8 changes: 4 additions & 4 deletions services/report/languages/cobertura.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import logging
import re
from typing import List
from typing import Sequence
from xml.etree.ElementTree import Element

import sentry_sdk
Expand Down Expand Up @@ -31,9 +31,9 @@ def Int(value):
return int(float(value))


def get_sources_to_attempt(xml) -> List[str]:
sources = [source.text for source in xml.iter("source")]
return [s for s in sources if isinstance(s, str) and s.startswith("/")]
def get_sources_to_attempt(xml) -> Sequence[str]:
sources = (source.text for source in xml.iter("source"))
return tuple(s for s in sources if isinstance(s, str) and s.startswith("/"))


def from_xml(xml: Element, report_builder_session: ReportBuilderSession) -> None:
Expand Down
2 changes: 1 addition & 1 deletion services/report/languages/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
def remove_non_ascii(string: str) -> str:
# ASCII control characters <=31, 127
# Extended ASCII characters: >=128
return "".join([c if 31 < ord(c) < 127 else "" for c in string])
return "".join(c if 31 < ord(c) < 127 else "" for c in string)


def child_text(parent: Element, element: str) -> str:
Expand Down
3 changes: 2 additions & 1 deletion services/report/report_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def create_coverage_line(
for label_ids in (labels_list_of_lists or [])
if label_ids
]
if self._report_builder.supports_labels()
if self._report_builder._supports_labels
else None
)
return ReportLine.create(
Expand Down Expand Up @@ -224,6 +224,7 @@ def __init__(
self.sessionid = sessionid
self.ignored_lines = ignored_lines
self.path_fixer = path_fixer
self._supports_labels = self.supports_labels()

def create_report_builder_session(self, filepath) -> ReportBuilderSession:
return ReportBuilderSession(self, filepath)
Expand Down

0 comments on commit 0d27b33

Please sign in to comment.