From c2128544780367ca4a6e5d008864b5275726d726 Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Thu, 2 Nov 2023 15:37:02 +0100 Subject: [PATCH] Ignore duplicate classes in Cobertura reports. Duplicate classes and methods now get a new name to make them unique. --- .../edu/hm/hafner/coverage/ClassNode.java | 15 +++ .../edu/hm/hafner/coverage/Percentage.java | 9 ++ .../coverage/parser/CoberturaParser.java | 51 ++++---- .../hm/hafner/coverage/PercentageTest.java | 1 + .../coverage/parser/CoberturaParserTest.java | 49 +++++-- .../parser/cobertura-duplicate-classes.xml | 121 ++++++++++++++++++ 6 files changed, 212 insertions(+), 34 deletions(-) create mode 100644 src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-classes.xml diff --git a/src/main/java/edu/hm/hafner/coverage/ClassNode.java b/src/main/java/edu/hm/hafner/coverage/ClassNode.java index 961cb7d6..b97e069a 100644 --- a/src/main/java/edu/hm/hafner/coverage/ClassNode.java +++ b/src/main/java/edu/hm/hafner/coverage/ClassNode.java @@ -36,4 +36,19 @@ public MethodNode createMethodNode(final String methodName, final String signatu addChild(fileNode); return fileNode; } + + /** + * Returns whether this node contains a method with the specified name and signature. + * + * @param methodName + * the name of the method + * @param signature + * the signature of the method + * + * @return {@code true} if this node has a child with the specified name and signature, {@code false} otherwise + */ + public boolean hasMethod(final String methodName, final String signature) { + return getAllMethodNodes().stream() + .anyMatch(m -> m.getMethodName().equals(methodName) && m.getSignature().equals(signature)); + } } diff --git a/src/main/java/edu/hm/hafner/coverage/Percentage.java b/src/main/java/edu/hm/hafner/coverage/Percentage.java index 059f8982..95a3aaa9 100644 --- a/src/main/java/edu/hm/hafner/coverage/Percentage.java +++ b/src/main/java/edu/hm/hafner/coverage/Percentage.java @@ -117,6 +117,15 @@ public double toDouble() { return (double) items * 100.0 / total; } + /** + * Returns this percentage as an int value in the interval [0, 100]. + * + * @return the coverage percentage + */ + public int toInt() { + return Math.round(items * 100.0f / total); + } + /** * Formats a percentage to plain text and rounds the value to two decimals. * diff --git a/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java b/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java index 8f3b88c1..631a5cca 100644 --- a/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java +++ b/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java @@ -13,12 +13,12 @@ import org.apache.commons.lang3.StringUtils; +import edu.hm.hafner.coverage.ClassNode; import edu.hm.hafner.coverage.Coverage; import edu.hm.hafner.coverage.Coverage.CoverageBuilder; import edu.hm.hafner.coverage.CoverageParser; import edu.hm.hafner.coverage.CyclomaticComplexity; import edu.hm.hafner.coverage.FileNode; -import edu.hm.hafner.coverage.MethodNode; import edu.hm.hafner.coverage.Metric; import edu.hm.hafner.coverage.ModuleNode; import edu.hm.hafner.coverage.Node; @@ -127,7 +127,7 @@ private void readPackage(final XMLEventReader reader, final ModuleNode root, var relativePath = PATH_UTIL.getRelativePath(fileName); var fileNode = packageNode.findOrCreateFileNode(getFileName(fileName), getTreeStringBuilder().intern(relativePath)); - readClassOrMethod(reader, fileNode, nextElement, log); + readClassOrMethod(reader, fileNode, fileNode, nextElement, log); } } else if (event.isEndElement()) { @@ -145,13 +145,14 @@ private String getFileName(final String relativePath) { } @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.CognitiveComplexity"}) - private Node readClassOrMethod(final XMLEventReader reader, final FileNode fileNode, - final StartElement parentElement, final FilteredLog log) throws XMLStreamException { + private void readClassOrMethod(final XMLEventReader reader, + final FileNode fileNode, final Node parentNode, + final StartElement element, final FilteredLog log) throws XMLStreamException { var lineCoverage = Coverage.nullObject(Metric.LINE); var branchCoverage = Coverage.nullObject(Metric.BRANCH); - Node node = createNode(fileNode, parentElement); - getOptionalValueOf(parentElement, COMPLEXITY) + Node node = createNode(parentNode, element, log); + getOptionalValueOf(element, COMPLEXITY) .ifPresent(c -> node.addValue(new CyclomaticComplexity(readComplexity(c)))); while (reader.hasNext()) { @@ -175,19 +176,13 @@ private Node readClassOrMethod(final XMLEventReader reader, final FileNode fileN } lineCoverage = lineCoverage.add(currentLineCoverage); - if (CLASS.equals(parentElement.getName())) { // Counters are stored at file level + if (CLASS.equals(element.getName())) { // Counters are stored at file level int lineNumber = getIntegerValueOf(nextElement, NUMBER); fileNode.addCounters(lineNumber, coverage.getCovered(), coverage.getMissed()); } } else if (METHOD.equals(nextElement.getName())) { - Node methodNode = readClassOrMethod(reader, fileNode, nextElement, log); - if (node.hasChild(methodNode.getName()) && ignoreErrors()) { - log.logError("Skipping duplicate method '%s' for class '%s'", node.getName(), methodNode.getName()); - } - else { - node.addChild(methodNode); - } + readClassOrMethod(reader, fileNode, node, nextElement, log); // recursive call } } else if (event.isEndElement()) { @@ -197,7 +192,7 @@ else if (event.isEndElement()) { if (branchCoverage.isSet()) { node.addValue(branchCoverage); } - return node; + return; } } } @@ -208,17 +203,29 @@ private Coverage computeLineCoverage(final int coverage) { return coverage > 0 ? LINE_COVERED : LINE_MISSED; } - private Node createNode(final FileNode file, final StartElement parentElement) { - var name = getValueOf(parentElement, NAME); + private Node createNode(final Node parentNode, final StartElement element, final FilteredLog log) { + var name = getValueOf(element, NAME); if (StringUtils.isBlank(name)) { // each node must have a unique name - name = UUID.randomUUID().toString(); + name = createId(); } - if (CLASS.equals(parentElement.getName())) { - return file.createClassNode(name); // connect the class with the file + if (CLASS.equals(element.getName())) { + if (parentNode.hasChild(name) && ignoreErrors()) { + log.logError("Found a duplicate class '%s' in '%s'", name, parentNode.getName()); + name = name + "-" + createId(); + } + return ((FileNode)parentNode).createClassNode(name); } - else { - return new MethodNode(name, getValueOf(parentElement, SIGNATURE)); + var signature = getValueOf(element, SIGNATURE); + var classNode = (ClassNode) parentNode; + if (classNode.hasMethod(name, signature) && ignoreErrors()) { + log.logError("Found a duplicate method '%s' with signature '%s' in '%s'", name, signature, parentNode.getName()); + name = name + "-" + createId(); } + return classNode.createMethodNode(name, signature); + } + + private String createId() { + return UUID.randomUUID().toString(); } private int readComplexity(final String c) { diff --git a/src/test/java/edu/hm/hafner/coverage/PercentageTest.java b/src/test/java/edu/hm/hafner/coverage/PercentageTest.java index ec4dc755..e349bb3a 100644 --- a/src/test/java/edu/hm/hafner/coverage/PercentageTest.java +++ b/src/test/java/edu/hm/hafner/coverage/PercentageTest.java @@ -20,6 +20,7 @@ void shouldHandleOverflow() { Fraction fraction = Fraction.getFraction(Integer.MAX_VALUE - 1, Integer.MAX_VALUE - 1); Percentage percentage = Percentage.valueOf(fraction); assertThat(percentage.toDouble()).isEqualTo(100); + assertThat(percentage.toInt()).isEqualTo(100); } @Test 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 60c7d325..44d50d8d 100644 --- a/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java +++ b/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java @@ -38,12 +38,18 @@ CoberturaParser createParser() { @Test void shouldIgnoreMissingConditionAttribute() { - Node duplicateMethods = readReport("cobertura-missing-condition-coverage.xml"); + Node missingCondition = readReport("cobertura-missing-condition-coverage.xml"); + + assertThat(missingCondition.getAll(FILE)).extracting(Node::getName) + .containsExactly("DataSourceProvider.cs"); + assertThat(missingCondition.getAll(CLASS)).extracting(Node::getName) + .containsExactly("VisualOn.Data.DataSourceProvider"); + assertThat(missingCondition.getAll(METHOD)).extracting(Node::getName) + .containsExactly("Enumerate()"); - verifySmallTree(duplicateMethods); assertThat(getLog().hasErrors()).isFalse(); - verifyBranchCoverageOfLine61(duplicateMethods); + verifyBranchCoverageOfLine61(missingCondition); } private void verifyBranchCoverageOfLine61(final Node duplicateMethods) { @@ -53,28 +59,47 @@ private void verifyBranchCoverageOfLine61(final Node duplicateMethods) { } @Test - void shouldIgnoreDuplicateMethods() { - Node duplicateMethods = readReport("cobertura-duplicate-methods.xml", + void shouldIgnoreDuplicateClasses() { + Node duplicateClasses = readReport("cobertura-duplicate-classes.xml", new CoberturaParser(ProcessingMode.IGNORE_ERRORS)); - verifySmallTree(duplicateMethods); + assertThat(duplicateClasses.getAll(FILE)).extracting(Node::getName) + .containsExactly("DataSourceProvider.cs"); + assertThat(duplicateClasses.getAll(CLASS)).extracting(Node::getName).hasSize(2) + .contains("VisualOn.Data.DataSourceProvider") + .element(1).asString().startsWith("VisualOn.Data.DataSourceProvider-"); + assertThat(getLog().hasErrors()).isTrue(); assertThat(getLog().getErrorMessages()) - .contains("Skipping duplicate method 'VisualOn.Data.DataSourceProvider' for class 'Enumerate()'"); + .contains("Found a duplicate class 'VisualOn.Data.DataSourceProvider' in 'DataSourceProvider.cs'"); - verifyBranchCoverageOfLine61(duplicateMethods); + verifyBranchCoverageOfLine61(duplicateClasses); assertThatIllegalArgumentException().isThrownBy( - () -> readReport("cobertura-duplicate-methods.xml", new CoberturaParser())); + () -> readReport("cobertura-duplicate-classes.xml", new CoberturaParser())); } - private void verifySmallTree(final Node duplicateMethods) { + @Test + void shouldIgnoreDuplicateMethods() { + Node duplicateMethods = readReport("cobertura-duplicate-methods.xml", + new CoberturaParser(ProcessingMode.IGNORE_ERRORS)); + assertThat(duplicateMethods.getAll(FILE)).extracting(Node::getName) .containsExactly("DataSourceProvider.cs"); assertThat(duplicateMethods.getAll(CLASS)).extracting(Node::getName) .containsExactly("VisualOn.Data.DataSourceProvider"); - assertThat(duplicateMethods.getAll(METHOD)).extracting(Node::getName) - .containsExactly("Enumerate()"); + assertThat(duplicateMethods.getAll(METHOD)).extracting(Node::getName).hasSize(2) + .contains("Enumerate()") + .element(1).asString().startsWith("Enumerate-"); + + assertThat(getLog().hasErrors()).isTrue(); + assertThat(getLog().getErrorMessages()) + .contains("Found a duplicate method 'Enumerate' with signature '()' in 'VisualOn.Data.DataSourceProvider'"); + + verifyBranchCoverageOfLine61(duplicateMethods); + + assertThatIllegalArgumentException().isThrownBy( + () -> readReport("cobertura-duplicate-methods.xml", new CoberturaParser())); } @Test @Issue("jenkinsci/code-coverage-api-plugin#729") diff --git a/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-classes.xml b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-classes.xml new file mode 100644 index 00000000..d9c8d1f3 --- /dev/null +++ b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-classes.xml @@ -0,0 +1,121 @@ + + + + /CoverageTest.Service/ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +