From e5470422742ecb7686a525b7399d946088ee7e9f Mon Sep 17 00:00:00 2001 From: Eljees <3.14hell@gmail.com> Date: Sat, 15 Aug 2026 18:18:28 +0300 Subject: [PATCH 1/2] perf: index the Clover file lookup instead of rescanning per source path `get_src_path_line_nodes_clover` walked every `.//file` in the report on every call, and it is called once per source path, so N source paths against a report of M files did N*M element visits and N*M `relative_path` calls. Build the lookup once per report: a map from repository-relative path, plus a map from the last path segment for the suffix match that absolute-path reports rely on. Both store document position, so matches are still returned in document order. Follow-up to the review on #617. --- .../violations_reporter.py | 70 +++++++++++++++---- tests/test_clover_violations_reporter.py | 5 +- tests/test_violations_reporter.py | 38 ++++++++++ 3 files changed, 95 insertions(+), 18 deletions(-) diff --git a/diff_cover/violationsreporters/violations_reporter.py b/diff_cover/violationsreporters/violations_reporter.py index 48d5ea11..b0620b26 100644 --- a/diff_cover/violationsreporters/violations_reporter.py +++ b/diff_cover/violationsreporters/violations_reporter.py @@ -51,6 +51,11 @@ def __init__( # once per source file in `_cache_file`. self._report_formats = [self._detect_report_format(root) for root in xml_roots] + # Lookup tables for the `` elements of each Clover report, built + # on first use. Without them every source path walks every `.//file` + # again, which is quadratic in a report with many files. + self._clover_file_index = [None] * len(xml_roots) + self._src_roots = src_roots or [""] self._expand_coverage_report = expand_coverage_report self._branch_coverage = branch_coverage @@ -125,8 +130,41 @@ def get_src_path_line_nodes_cobertura(self, index, xml_document, src_path): lines = [clazz.findall("./lines/line") for clazz in classes] return list(itertools.chain(*lines)) - @staticmethod - def get_src_path_line_nodes_clover(xml_document, src_path): + def _clover_file_lookup(self, index, xml_document): + """ + Return the `` lookup tables for the report at `index`, building + them on first use. + + A file is matched either by its path relative to the repository root or, + when the report carries absolute paths, by a suffix test. The suffix can + only match when the last path segment does, so candidates for it are + grouped by that segment and the test is applied to the few that share it. + + Both tables store the document position of each element so that callers + can return matches in document order, as a direct walk would. + """ + lookup = self._clover_file_index[index] + if lookup is None: + by_relative_path = defaultdict(list) + by_last_segment = defaultdict(list) + for position, file_tree in enumerate(xml_document.findall(".//file")): + file_path = file_tree.get("path") or file_tree.get("name") + if not file_path: + continue + + normalized_file_path = util.to_unix_path(file_path) + relative_file_path = util.to_unix_path( + GitPathTool.relative_path(file_path) + ) + by_relative_path[relative_file_path].append((position, file_tree)) + by_last_segment[normalized_file_path.rsplit("/", 1)[-1]].append( + (position, normalized_file_path, file_tree) + ) + lookup = (by_relative_path, by_last_segment) + self._clover_file_index[index] = lookup + return lookup + + def get_src_path_line_nodes_clover(self, index, xml_document, src_path): """ Return a list of nodes containing line information for `src_path` in `xml_document`. @@ -134,20 +172,22 @@ def get_src_path_line_nodes_clover(xml_document, src_path): If file is not present in `xml_document`, return None """ - files = [] normalized_src_path = util.to_unix_path(src_path) - for file_tree in xml_document.findall(".//file"): - file_path = file_tree.get("path") or file_tree.get("name") - if not file_path: - continue + by_relative_path, by_last_segment = self._clover_file_lookup( + index, xml_document + ) - normalized_file_path = util.to_unix_path(file_path) - relative_file_path = util.to_unix_path(GitPathTool.relative_path(file_path)) - if ( - relative_file_path == normalized_src_path - or normalized_file_path.endswith(f"/{normalized_src_path}") - ): - files.append(file_tree) + matches = {} + for position, file_tree in by_relative_path.get(normalized_src_path, ()): + matches[position] = file_tree + suffix = f"/{normalized_src_path}" + for position, normalized_file_path, file_tree in by_last_segment.get( + normalized_src_path.rsplit("/", 1)[-1], () + ): + if normalized_file_path.endswith(suffix): + matches[position] = file_tree + + files = [matches[position] for position in sorted(matches)] if not files: return None lines = [] @@ -246,7 +286,7 @@ def _cache_file(self, src_path): report_format = self._report_formats[i] if report_format == "clover": line_nodes = self.get_src_path_line_nodes_clover( - xml_document, src_path + i, xml_document, src_path ) _number = "num" _hits = "count" diff --git a/tests/test_clover_violations_reporter.py b/tests/test_clover_violations_reporter.py index e79df37a..0c6425c7 100644 --- a/tests/test_clover_violations_reporter.py +++ b/tests/test_clover_violations_reporter.py @@ -13,7 +13,6 @@ def test_get_src_path_clover(datadir): GitPathTool._cwd = "/" GitPathTool._root = "/" clover_report = etree.parse(str(datadir / "test.xml")) - result = XmlCoverageReporter.get_src_path_line_nodes_clover( - clover_report, "isLucky.js" - ) + reporter = XmlCoverageReporter([clover_report]) + result = reporter.get_src_path_line_nodes_clover(0, clover_report, "isLucky.js") assert sorted([int(line.attrib["num"]) for line in result]) == [2, 3, 5, 6, 8, 12] diff --git a/tests/test_violations_reporter.py b/tests/test_violations_reporter.py index b67c099a..a265705d 100644 --- a/tests/test_violations_reporter.py +++ b/tests/test_violations_reporter.py @@ -551,6 +551,44 @@ def test_report_format_is_detected_once(self): coverage.violations("file2.java") assert coverage._report_formats == ["clover"] + def test_file_lookup_is_built_once(self, mocker): + # Locating a source file used to walk every `.//file` again, so a report + # with N files cost N walks for N source paths. Each `` is now + # classified once, which shows up as one `relative_path` call per file + # however many source paths are looked up. + file_paths = ["file1.java", "subdir/file2.java", "subdir/file3.java"] + xml = self._coverage_xml(file_paths, self.FEW_VIOLATIONS, self.FEW_MEASURED) + + relative_path = mocker.patch( + "diff_cover.violationsreporters.violations_reporter." + "GitPathTool.relative_path", + side_effect=lambda path: path, + ) + coverage = XmlCoverageReporter([xml]) + for src_path in file_paths: + assert coverage.violations(src_path) == self.FEW_VIOLATIONS + + assert relative_path.call_count == len(file_paths), ( + "expected each to be classified once, got " + f"{relative_path.call_count} calls for {len(file_paths)} files" + ) + + def test_absolute_paths_still_match_by_suffix(self): + # Reports that carry absolute paths are matched by suffix, not equality. + # The lookup groups those candidates by their last path segment, so a + # file whose segment collides with another must still resolve correctly. + xml = self._coverage_xml( + ["/build/one/shared.java", "/build/two/shared.java"], + self.FEW_VIOLATIONS, + self.FEW_MEASURED, + ) + + coverage = XmlCoverageReporter([xml]) + + assert coverage.violations("two/shared.java") == self.FEW_VIOLATIONS + assert coverage.measured_lines("two/shared.java") == self.FEW_MEASURED + assert coverage.violations("nowhere/shared.java") == set() + def test_two_inputs_first_violate(self): # Construct the XML report file_paths = ["file1.java"] From 90823f073dd027ba53ce494f9984f1b15dea4462 Mon Sep 17 00:00:00 2001 From: Eljees <3.14hell@gmail.com> Date: Sat, 15 Aug 2026 23:42:30 +0300 Subject: [PATCH 2/2] refactor: move the Clover file index into its own module Adding the index in this branch took violations_reporter.py to 1019 lines, past pylint's too-many-lines limit of 1000. The lookup is self-contained, so it moves to violationsreporters/clover.py as CloverFileIndex, and the module is back to 958 lines. XmlCoverageReporter keeps the per-report cache and builds the index on first use, so the dispatch in _cache_file and the signature of get_src_path_line_nodes_clover are unchanged. The moved code resolves GitPathTool in its own module, so the Clover tests patch it there. Skipping a element that carries neither path nor name had no test of its own; it has one now. Requested in review on #618. --- diff_cover/violationsreporters/clover.py | 84 +++++++++++++++++++ .../violations_reporter.py | 71 ++-------------- tests/test_violations_reporter.py | 30 +++++-- 3 files changed, 111 insertions(+), 74 deletions(-) create mode 100644 diff_cover/violationsreporters/clover.py diff --git a/diff_cover/violationsreporters/clover.py b/diff_cover/violationsreporters/clover.py new file mode 100644 index 00000000..a12bbd8c --- /dev/null +++ b/diff_cover/violationsreporters/clover.py @@ -0,0 +1,84 @@ +""" +Reading the ```` elements of a Clover XML coverage report. +""" + +import itertools +from collections import defaultdict + +from diff_cover import util +from diff_cover.git_path import GitPathTool + + +class CloverFileIndex: + """ + The ```` elements of one Clover report, indexed for lookup by path. + + Building the index costs one walk of the document. Looking a source path up + by walking the document instead costs that walk once per source path, which + is quadratic in a report with many files. + """ + + def __init__(self, xml_document): + """ + Index every ```` element of `xml_document`. + + A file is matched either by its path relative to the repository root + or, when the report carries absolute paths, by a suffix test. The + suffix can only match when the last path segment does, so candidates + for it are grouped by that segment and the test is applied to the few + that share it. + + Both tables store the document position of each element so that + lookups can return matches in document order, as a direct walk would. + """ + self._by_relative_path = defaultdict(list) + self._by_last_segment = defaultdict(list) + for position, file_tree in enumerate(xml_document.findall(".//file")): + file_path = file_tree.get("path") or file_tree.get("name") + if not file_path: + continue + + normalized_file_path = util.to_unix_path(file_path) + relative_file_path = util.to_unix_path(GitPathTool.relative_path(file_path)) + self._by_relative_path[relative_file_path].append((position, file_tree)) + self._by_last_segment[normalized_file_path.rsplit("/", 1)[-1]].append( + (position, normalized_file_path, file_tree) + ) + + def _files(self, src_path): + """ + Return the ```` elements for `src_path`, in document order. + """ + normalized_src_path = util.to_unix_path(src_path) + + matches = {} + for position, file_tree in self._by_relative_path.get(normalized_src_path, ()): + matches[position] = file_tree + suffix = f"/{normalized_src_path}" + for position, normalized_file_path, file_tree in self._by_last_segment.get( + normalized_src_path.rsplit("/", 1)[-1], () + ): + if normalized_file_path.endswith(suffix): + matches[position] = file_tree + + return [matches[position] for position in sorted(matches)] + + def line_nodes(self, src_path): + """ + Return a list of nodes containing line information for `src_path`. + + If the file is not present in the report, return None + """ + files = self._files(src_path) + if not files: + return None + lines = [] + for file_tree in files: + # Clover marks an executable line as one of these three types. PHPUnit's + # writer emits `method` for the declaration line of every function it + # measured; leaving it out reported those lines as unmeasured. + # https://github.com/sebastianbergmann/php-code-coverage/blob/main/src/Report/Clover.php + lines.append(file_tree.findall('./line[@type="method"]')) + lines.append(file_tree.findall('./line[@type="stmt"]')) + lines.append(file_tree.findall('./line[@type="cond"]')) + return list(itertools.chain(*lines)) diff --git a/diff_cover/violationsreporters/violations_reporter.py b/diff_cover/violationsreporters/violations_reporter.py index b0620b26..f3a53681 100644 --- a/diff_cover/violationsreporters/violations_reporter.py +++ b/diff_cover/violationsreporters/violations_reporter.py @@ -17,6 +17,7 @@ RegexBasedDriver, Violation, ) +from diff_cover.violationsreporters.clover import CloverFileIndex class XmlCoverageReporter(BaseViolationReporter): @@ -51,9 +52,7 @@ def __init__( # once per source file in `_cache_file`. self._report_formats = [self._detect_report_format(root) for root in xml_roots] - # Lookup tables for the `` elements of each Clover report, built - # on first use. Without them every source path walks every `.//file` - # again, which is quadratic in a report with many files. + # A `CloverFileIndex` for each Clover report, built on first use. self._clover_file_index = [None] * len(xml_roots) self._src_roots = src_roots or [""] @@ -130,40 +129,6 @@ def get_src_path_line_nodes_cobertura(self, index, xml_document, src_path): lines = [clazz.findall("./lines/line") for clazz in classes] return list(itertools.chain(*lines)) - def _clover_file_lookup(self, index, xml_document): - """ - Return the `` lookup tables for the report at `index`, building - them on first use. - - A file is matched either by its path relative to the repository root or, - when the report carries absolute paths, by a suffix test. The suffix can - only match when the last path segment does, so candidates for it are - grouped by that segment and the test is applied to the few that share it. - - Both tables store the document position of each element so that callers - can return matches in document order, as a direct walk would. - """ - lookup = self._clover_file_index[index] - if lookup is None: - by_relative_path = defaultdict(list) - by_last_segment = defaultdict(list) - for position, file_tree in enumerate(xml_document.findall(".//file")): - file_path = file_tree.get("path") or file_tree.get("name") - if not file_path: - continue - - normalized_file_path = util.to_unix_path(file_path) - relative_file_path = util.to_unix_path( - GitPathTool.relative_path(file_path) - ) - by_relative_path[relative_file_path].append((position, file_tree)) - by_last_segment[normalized_file_path.rsplit("/", 1)[-1]].append( - (position, normalized_file_path, file_tree) - ) - lookup = (by_relative_path, by_last_segment) - self._clover_file_index[index] = lookup - return lookup - def get_src_path_line_nodes_clover(self, index, xml_document, src_path): """ Return a list of nodes containing line information for `src_path` @@ -171,35 +136,9 @@ def get_src_path_line_nodes_clover(self, index, xml_document, src_path): If file is not present in `xml_document`, return None """ - - normalized_src_path = util.to_unix_path(src_path) - by_relative_path, by_last_segment = self._clover_file_lookup( - index, xml_document - ) - - matches = {} - for position, file_tree in by_relative_path.get(normalized_src_path, ()): - matches[position] = file_tree - suffix = f"/{normalized_src_path}" - for position, normalized_file_path, file_tree in by_last_segment.get( - normalized_src_path.rsplit("/", 1)[-1], () - ): - if normalized_file_path.endswith(suffix): - matches[position] = file_tree - - files = [matches[position] for position in sorted(matches)] - if not files: - return None - lines = [] - for file_tree in files: - # Clover marks an executable line as one of these three types. PHPUnit's - # writer emits `method` for the declaration line of every function it - # measured; leaving it out reported those lines as unmeasured. - # https://github.com/sebastianbergmann/php-code-coverage/blob/main/src/Report/Clover.php - lines.append(file_tree.findall('./line[@type="method"]')) - lines.append(file_tree.findall('./line[@type="stmt"]')) - lines.append(file_tree.findall('./line[@type="cond"]')) - return list(itertools.chain(*lines)) + if self._clover_file_index[index] is None: + self._clover_file_index[index] = CloverFileIndex(xml_document) + return self._clover_file_index[index].line_nodes(src_path) @staticmethod def _detect_report_format(xml_document): diff --git a/tests/test_violations_reporter.py b/tests/test_violations_reporter.py index a265705d..feb2f8e2 100644 --- a/tests/test_violations_reporter.py +++ b/tests/test_violations_reporter.py @@ -464,12 +464,15 @@ class TestCloverXmlCoverageReporterTest: @pytest.fixture(autouse=True) def patch_git_patch(self, mocker): - # Paths generated by git_path are always the given argument - _git_path_mock = mocker.patch( - "diff_cover.violationsreporters.violations_reporter.GitPathTool" - ) - _git_path_mock.relative_path = lambda path: path - _git_path_mock.absolute_path = lambda path: path + # Paths generated by git_path are always the given argument. The Clover + # lookup resolves them in its own module, so patch both. + for module in ( + "diff_cover.violationsreporters.violations_reporter", + "diff_cover.violationsreporters.clover", + ): + _git_path_mock = mocker.patch(f"{module}.GitPathTool") + _git_path_mock.relative_path = lambda path: path + _git_path_mock.absolute_path = lambda path: path def test_violations(self): # Construct the XML report @@ -560,8 +563,7 @@ def test_file_lookup_is_built_once(self, mocker): xml = self._coverage_xml(file_paths, self.FEW_VIOLATIONS, self.FEW_MEASURED) relative_path = mocker.patch( - "diff_cover.violationsreporters.violations_reporter." - "GitPathTool.relative_path", + "diff_cover.violationsreporters.clover.GitPathTool.relative_path", side_effect=lambda path: path, ) coverage = XmlCoverageReporter([xml]) @@ -589,6 +591,18 @@ def test_absolute_paths_still_match_by_suffix(self): assert coverage.measured_lines("two/shared.java") == self.FEW_MEASURED assert coverage.violations("nowhere/shared.java") == set() + def test_file_without_a_path_is_skipped(self): + # A `` carries its path in `path`, or in `name` from PHPUnit's + # writer. One with neither cannot be matched against a source path, and + # it must not keep the rest of the report from being indexed. + xml = self._coverage_xml(["file1.java"], self.FEW_VIOLATIONS, self.FEW_MEASURED) + xml.find(".//package").insert(0, etree.Element("file")) + + coverage = XmlCoverageReporter([xml]) + + assert coverage.violations("file1.java") == self.FEW_VIOLATIONS + assert coverage.measured_lines("file1.java") == self.FEW_MEASURED + def test_two_inputs_first_violate(self): # Construct the XML report file_paths = ["file1.java"]