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 48d5ea11..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,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" 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..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 @@ -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 `` 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 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 `` 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"]