From 394ea86b51cc420aeaccf5652d1e13fea3c04820 Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Fri, 14 Jul 2023 08:58:53 +0200 Subject: [PATCH 1/2] Merge all values of the hierarchy using `max`. --- .../edu/hm/hafner/coverage/ModuleNode.java | 1 + .../java/edu/hm/hafner/coverage/Node.java | 16 +++-- .../edu/hm/hafner/coverage/PackageNode.java | 5 -- .../hm/hafner/coverage/ModuleNodeTest.java | 2 +- .../coverage/parser/CoberturaParserTest.java | 60 +++++++++++++++++-- .../coverage/parser/cobertura-merge-a.xml | 56 +++++++++++++++++ .../coverage/parser/cobertura-merge-b.xml | 56 +++++++++++++++++ 7 files changed, 181 insertions(+), 15 deletions(-) create mode 100644 src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-a.xml create mode 100644 src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-b.xml diff --git a/src/main/java/edu/hm/hafner/coverage/ModuleNode.java b/src/main/java/edu/hm/hafner/coverage/ModuleNode.java index e4210a1a..0901f9b4 100644 --- a/src/main/java/edu/hm/hafner/coverage/ModuleNode.java +++ b/src/main/java/edu/hm/hafner/coverage/ModuleNode.java @@ -100,6 +100,7 @@ private void mergeSinglePackage(final Node packageNode) { if (isEqual(packageNode, existing)) { // replace existing with merged two nodes removeChild(existing); + // FIXME: at the root the values need to be added rather than merged Node merged = existing.merge(packageNode); addChild(merged); diff --git a/src/main/java/edu/hm/hafner/coverage/Node.java b/src/main/java/edu/hm/hafner/coverage/Node.java index 0aaf9242..708da85f 100644 --- a/src/main/java/edu/hm/hafner/coverage/Node.java +++ b/src/main/java/edu/hm/hafner/coverage/Node.java @@ -647,11 +647,14 @@ private void mergeChildren(final Node other) { private void mergeValues(final Value otherValue) { if (getMetricsOfValues().anyMatch(v -> v.equals(otherValue.getMetric()))) { var old = getValueOf(otherValue.getMetric()); - if (hasChildren()) { - replaceValue(old.add(otherValue)); + try { + var max = old.max(otherValue); + + replaceValue(max); } - else { - replaceValue(old.max(otherValue)); + catch (AssertionError exception) { + throw new IllegalStateException(String.format("Cannot merge coverage of %s because of: %s", + this, exception.getMessage()), exception); } } } @@ -676,7 +679,10 @@ public int hashCode() { @Override public String toString() { - return String.format("[%s] %s <%d>", getMetric(), getName(), children.size()); + return getValue(Metric.LINE) + .map(lineCoverage -> String.format("[%s] %s <%d, %s>", + getMetric(), getName(), getChildren().size(), lineCoverage)) + .orElse(String.format("[%s] %s <%d>", getMetric(), getName(), children.size())); } public boolean isEmpty() { diff --git a/src/main/java/edu/hm/hafner/coverage/PackageNode.java b/src/main/java/edu/hm/hafner/coverage/PackageNode.java index d3ec4a49..16bacb17 100644 --- a/src/main/java/edu/hm/hafner/coverage/PackageNode.java +++ b/src/main/java/edu/hm/hafner/coverage/PackageNode.java @@ -110,9 +110,4 @@ public ClassNode createClassNode(final String className) { addChild(classNode); return classNode; } - - @Override - public String toString() { - return String.format("[%s] %s <%d>", getMetric(), getName(), getChildren().size()); - } } diff --git a/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java b/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java index f3cb4928..76a6499a 100644 --- a/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java +++ b/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java @@ -85,7 +85,7 @@ void shouldDetectExistingPackagesOnSplit() { var subPackage = new PackageNode("edu.hm.hafner"); root.addChild(subPackage); - subPackage.addValue(builder.setMissed(10).build()); + subPackage.addValue(builder.setCovered(10).setMissed(10).build()); subPackage.addChild(new FileNode("OtherFile.c", "edu.hm.hafner/OtherFile.c")); assertThat(root.getValue(LINE)).contains(builder.setCovered(20).setMissed(10).build()); diff --git a/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java b/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java index b4777c31..86028634 100644 --- a/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java +++ b/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java @@ -28,6 +28,55 @@ CoberturaParser createParser() { return new CoberturaParser(); } + @Test + void shouldMergeCorrectly729() { + var builder = new CoverageBuilder(); + + Node a = readReport("cobertura-merge-a.xml"); + assertThat(a.aggregateValues()).containsExactly( + builder.setMetric(MODULE).setCovered(1).setMissed(0).build(), + builder.setMetric(PACKAGE).setCovered(1).setMissed(0).build(), + builder.setMetric(FILE).setCovered(1).setMissed(0).build(), + builder.setMetric(CLASS).setCovered(1).setMissed(0).build(), + builder.setMetric(METHOD).setCovered(3).setMissed(0).build(), + builder.setMetric(LINE).setCovered(20).setMissed(0).build(), + builder.setMetric(BRANCH).setCovered(2).setMissed(1).build(), + new LinesOfCode(20)); + + Node b = readReport("cobertura-merge-b.xml"); + assertThat(b.aggregateValues()).containsExactly( + builder.setMetric(MODULE).setCovered(1).setMissed(0).build(), + builder.setMetric(PACKAGE).setCovered(1).setMissed(0).build(), + builder.setMetric(FILE).setCovered(1).setMissed(0).build(), + builder.setMetric(CLASS).setCovered(1).setMissed(0).build(), + builder.setMetric(METHOD).setCovered(1).setMissed(2).build(), + builder.setMetric(LINE).setCovered(16).setMissed(4).build(), + builder.setMetric(BRANCH).setCovered(0).setMissed(3).build(), + new LinesOfCode(20)); + + var left = a.merge(b); + var right = b.merge(a); + + assertThat(left.aggregateValues()).containsExactly( + builder.setMetric(MODULE).setCovered(1).setMissed(0).build(), + builder.setMetric(PACKAGE).setCovered(1).setMissed(0).build(), + builder.setMetric(FILE).setCovered(1).setMissed(0).build(), + builder.setMetric(CLASS).setCovered(1).setMissed(0).build(), + builder.setMetric(METHOD).setCovered(3).setMissed(0).build(), + builder.setMetric(LINE).setCovered(20).setMissed(0).build(), + builder.setMetric(BRANCH).setCovered(2).setMissed(1).build(), + new LinesOfCode(20)); + assertThat(right.aggregateValues()).containsExactly( + builder.setMetric(MODULE).setCovered(1).setMissed(0).build(), + builder.setMetric(PACKAGE).setCovered(1).setMissed(0).build(), + builder.setMetric(FILE).setCovered(1).setMissed(0).build(), + builder.setMetric(CLASS).setCovered(1).setMissed(0).build(), + builder.setMetric(METHOD).setCovered(3).setMissed(0).build(), + builder.setMetric(LINE).setCovered(20).setMissed(0).build(), + builder.setMetric(BRANCH).setCovered(2).setMissed(1).build(), + new LinesOfCode(20)); + } + @Test void shouldCountCorrectly625() { Node tree = readReport("cobertura-counter-aggregation.xml"); @@ -91,12 +140,15 @@ void shouldReadCoberturaIssue599() { var builder = new CoverageBuilder(); assertThat(tree).hasOnlyMetrics(MODULE, PACKAGE, FILE, CLASS, METHOD, LINE, BRANCH, LOC); - assertThat(tree.aggregateValues()).containsAnyOf( + assertThat(tree.aggregateValues()).containsExactly( builder.setMetric(MODULE).setCovered(1).setMissed(0).build(), builder.setMetric(PACKAGE).setCovered(4).setMissed(3).build(), - builder.setMetric(FILE).setCovered(6).setMissed(12).build(), - builder.setMetric(CLASS).setCovered(6).setMissed(12).build(), - builder.setMetric(METHOD).setCovered(4).setMissed(1).build()); + builder.setMetric(FILE).setCovered(6).setMissed(6).build(), + builder.setMetric(CLASS).setCovered(6).setMissed(6).build(), + builder.setMetric(METHOD).setCovered(14).setMissed(24).build(), + builder.setMetric(LINE).setCovered(52).setMissed(85).build(), + builder.setMetric(BRANCH).setCovered(21).setMissed(11).build(), + new LinesOfCode(137)); assertThat(tree.findPackage("libs.env.src")).isNotEmpty().get().satisfies( p -> { diff --git a/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-a.xml b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-a.xml new file mode 100644 index 00000000..37cd24cc --- /dev/null +++ b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-a.xml @@ -0,0 +1,56 @@ + + + + + ${MY_DIRECTORY_PLACEHOLDER} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-b.xml b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-b.xml new file mode 100644 index 00000000..b0479dbb --- /dev/null +++ b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-merge-b.xml @@ -0,0 +1,56 @@ + + + + + ${MY_DIRECTORY_PLACEHOLDER} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 78b600101d2ea7f9785f9852db8d4bd9ecb818af Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Tue, 19 Sep 2023 21:54:02 +0200 Subject: [PATCH 2/2] Add test for splitting the packages with real data. --- .../edu/hm/hafner/coverage/ModuleNode.java | 1 - .../hm/hafner/coverage/ModuleNodeTest.java | 35 - .../java/edu/hm/hafner/coverage/NodeTest.java | 2 +- .../coverage/parser/JacocoParserTest.java | 70 + .../coverage/parser/file-subpackage.xml | 1756 +++++++++++++++++ 5 files changed, 1827 insertions(+), 37 deletions(-) create mode 100644 src/test/resources/edu/hm/hafner/coverage/parser/file-subpackage.xml diff --git a/src/main/java/edu/hm/hafner/coverage/ModuleNode.java b/src/main/java/edu/hm/hafner/coverage/ModuleNode.java index 0901f9b4..e4210a1a 100644 --- a/src/main/java/edu/hm/hafner/coverage/ModuleNode.java +++ b/src/main/java/edu/hm/hafner/coverage/ModuleNode.java @@ -100,7 +100,6 @@ private void mergeSinglePackage(final Node packageNode) { if (isEqual(packageNode, existing)) { // replace existing with merged two nodes removeChild(existing); - // FIXME: at the root the values need to be added rather than merged Node merged = existing.merge(packageNode); addChild(merged); diff --git a/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java b/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java index 76a6499a..08576cc0 100644 --- a/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java +++ b/src/test/java/edu/hm/hafner/coverage/ModuleNodeTest.java @@ -2,8 +2,6 @@ import org.junit.jupiter.api.Test; -import edu.hm.hafner.coverage.Coverage.CoverageBuilder; - import static edu.hm.hafner.coverage.Metric.FILE; import static edu.hm.hafner.coverage.Metric.*; import static edu.hm.hafner.coverage.assertions.Assertions.*; @@ -66,39 +64,6 @@ void shouldSplitPackagesIntoHierarchy() { ); } - @Test - void shouldDetectExistingPackagesOnSplit() { - var root = new ModuleNode("Root"); - Node eduPackage = new PackageNode("edu"); - Node differentPackage = new PackageNode("org"); - - root.addChild(differentPackage); - root.addChild(eduPackage); - - eduPackage.addChild(new FileNode("File.c", "edu/File.c")); - - var builder = new CoverageBuilder().setMetric(LINE); - eduPackage.addValue(builder.setCovered(10).setMissed(0).build()); - - assertThat(root.getAll(PACKAGE)).hasSize(2); - assertThat(root.getValue(LINE)).contains(builder.build()); - - var subPackage = new PackageNode("edu.hm.hafner"); - root.addChild(subPackage); - subPackage.addValue(builder.setCovered(10).setMissed(10).build()); - subPackage.addChild(new FileNode("OtherFile.c", "edu.hm.hafner/OtherFile.c")); - assertThat(root.getValue(LINE)).contains(builder.setCovered(20).setMissed(10).build()); - - root.splitPackages(); - assertThat(root.getAll(PACKAGE)).hasSize(4); - - assertThat(root.getChildren()).hasSize(2).satisfiesExactlyInAnyOrder( - org -> assertThat(org.getName()).isEqualTo("org"), - edu -> assertThat(edu.getName()).isEqualTo("edu")); - - assertThat(root.getValue(LINE)).contains(builder.setCovered(20).setMissed(10).build()); - } - @Test void shouldKeepNodesAfterSplitting() { var root = new ModuleNode("Root"); diff --git a/src/test/java/edu/hm/hafner/coverage/NodeTest.java b/src/test/java/edu/hm/hafner/coverage/NodeTest.java index 5b0bf1e7..f83921f7 100644 --- a/src/test/java/edu/hm/hafner/coverage/NodeTest.java +++ b/src/test/java/edu/hm/hafner/coverage/NodeTest.java @@ -420,7 +420,7 @@ void shouldThrowErrorIfCoveredPlusMissedLinesDifferInReports() { method.addValue(new CoverageBuilder().setMetric(LINE).setCovered(5).setMissed(5).build()); methodOtherCov.addValue(new CoverageBuilder().setMetric(LINE).setCovered(2).setMissed(7).build()); - assertThatExceptionOfType(AssertionError.class) + assertThatExceptionOfType(IllegalStateException.class) .isThrownBy(() -> module.merge(sameProject)) .withMessageContaining("Cannot compute maximum of coverages", "(5/10)", "(2/9)"); } diff --git a/src/test/java/edu/hm/hafner/coverage/parser/JacocoParserTest.java b/src/test/java/edu/hm/hafner/coverage/parser/JacocoParserTest.java index de171625..999eae65 100644 --- a/src/test/java/edu/hm/hafner/coverage/parser/JacocoParserTest.java +++ b/src/test/java/edu/hm/hafner/coverage/parser/JacocoParserTest.java @@ -3,6 +3,7 @@ import java.util.List; import java.util.NoSuchElementException; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -37,6 +38,75 @@ private static Coverage getCoverage(final Node node, final Metric metric) { return (Coverage) node.getValue(metric).get(); } + @ParameterizedTest(name = "[{index}] Split packages after read: {0}") + @ValueSource(booleans = {true, false}) + @DisplayName("Read and merge two coverage reports with different packages") + void shouldMergeProjects(final boolean splitPackages) { + ModuleNode model = readReport("jacoco-analysis-model.xml"); + + assertThat(model.getAll(PACKAGE)).extracting(Node::getName).containsExactly( + "edu.hm.hafner.analysis.parser.dry.simian", + "edu.hm.hafner.analysis.parser.gendarme", + "edu.hm.hafner.analysis.parser.dry", + "edu.hm.hafner.analysis.parser.checkstyle", + "edu.hm.hafner.analysis.registry", + "edu.hm.hafner.analysis.parser.findbugs", + "edu.hm.hafner.analysis.parser.pmd", + "edu.hm.hafner.analysis", + "edu.hm.hafner.analysis.parser.fxcop", + "edu.hm.hafner.analysis.parser.dry.dupfinder", + "edu.hm.hafner.analysis.parser.jcreport", + "edu.hm.hafner.analysis.parser.pylint", + "edu.hm.hafner.analysis.parser.pvsstudio", + "edu.hm.hafner.analysis.parser.dry.cpd", + "edu.hm.hafner.util", + "edu.hm.hafner.analysis.parser", + "edu.hm.hafner.analysis.parser.ccm", + "edu.hm.hafner.analysis.parser.violations"); + + ModuleNode style = readReport("jacoco-codingstyle.xml"); + + assertThat(style.getAll(PACKAGE)).extracting(Node::getName).containsExactly( + "edu.hm.hafner.util"); + + var builder = new CoverageBuilder().setMetric(LINE); + + var left = new ModuleNode("root"); + model.getAll(PACKAGE).forEach(p -> left.addChild(p.copyTree())); + + assertThat(left.find(PACKAGE, "edu.hm.hafner.util")).isPresent() + .get().satisfies(p -> assertThat(p.getValue(LINE)).contains( + builder.setCovered(60).setTotal(62).build())); + + var right = new ModuleNode("root"); + style.getAll(PACKAGE).forEach(p -> right.addChild(p.copyTree())); + + assertThat(right.find(PACKAGE, "edu.hm.hafner.util")).isPresent() + .get().satisfies(p -> assertThat(p.getValue(LINE)).contains( + builder.setCovered(294).setTotal(323).build())); + + if (splitPackages) { + left.splitPackages(); + right.splitPackages(); + } + + var merged = left.merge(right); + + var packageName = splitPackages ? "util" : "edu.hm.hafner.util"; + assertThat(merged.find(PACKAGE, packageName)).isPresent() + .get().satisfies(p -> assertThat(p.getValue(LINE)).contains( + builder.setCovered(294 + 60).setTotal(323 + 62).build())); + } + + @Test + void shouldReadAndSplitSubpackage() { + ModuleNode model = readReport("file-subpackage.xml"); + + model.splitPackages(); + assertThat(model.getAll(PACKAGE)).extracting(Node::getName).containsExactly( + "util", "hafner", "hm", "edu"); + } + @Test void shouldReadAggregationWithGroups() { ModuleNode jenkinsRoot = readReport("jacoco-jenkins.xml"); diff --git a/src/test/resources/edu/hm/hafner/coverage/parser/file-subpackage.xml b/src/test/resources/edu/hm/hafner/coverage/parser/file-subpackage.xml new file mode 100644 index 00000000..c6157076 --- /dev/null +++ b/src/test/resources/edu/hm/hafner/coverage/parser/file-subpackage.xml @@ -0,0 +1,1756 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +