perf: index the Clover file lookup instead of rescanning per source path

diff_cover/violationsreporters/clover.py

@@ -0,0 +1,84 @@
+"""
+Reading the ``<file>`` 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 ``<file>`` 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 ``<file>`` 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 ``<file>`` 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_cover/violationsreporters/violations_reporter.py

@@ -17,6 +17,7 @@
     RegexBasedDriver,
     Violation,
 )
+from diff_cover.violationsreporters.clover import CloverFileIndex
 
 
 class XmlCoverageReporter(BaseViolationReporter):
@@ -51,6 +52,9 @@ def __init__(
         # once per source file in `_cache_file`.
         self._report_formats = [self._detect_report_format(root) for root in xml_roots]
 
+        # A `CloverFileIndex` for each Clover report, built on first use.
+        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,41 +129,16 @@ 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 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`.
 
         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
-
-            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)
-        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):
@@ -246,7 +225,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"

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]

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
@@ -551,6 +554,55 @@ 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 `<file>` 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.clover.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 <file> 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_file_without_a_path_is_skipped(self):
+        # A `<file>` 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"]