Make CoberturaParser more robost on broken input files.

Add a new optional property to the parser to ignore all errors. Additionally, use a default value of 2/2 when branch coverage is missing the details. See jenkinsci/code-coverage-api-plugin#785
jenkinsci · Oct 11, 2023 · 4d859c7 · 4d859c7
1 parent 95b35ab
commit 4d859c7
Show file tree

Hide file tree

Showing 6 changed files with 235 additions and 11 deletions.
diff --git a/src/main/java/edu/hm/hafner/coverage/Node.java b/src/main/java/edu/hm/hafner/coverage/Node.java
@@ -177,6 +177,17 @@ protected void removeChild(final Node child) {
         child.parent = null;
     }
 
+    /**
+     * Returns whether this node has a child with the specified name.
+     *
+     * @param childName
+     *         the name of the child to look for
+     * @return {@code true} if this node has a child with the specified name, {@code false} otherwise
+     */
+    public boolean hasChild(final String childName) {
+        return children.stream().map(Node::getName).anyMatch(childName::equals);
+    }
+
     /**
      * Adds alls given nodes as children to the current node.
      *
@@ -187,7 +198,6 @@ public void addAllChildren(final Collection<? extends Node> nodes) {
         nodes.forEach(this::addChild);
     }
 
-
     /**
      * Returns the parent node.
      *
@@ -779,4 +789,5 @@ private Optional<Node> filterTreeByMapping(final Function<Node, Optional<Node>>
         copy.addAllChildren(prunedChildren);
         return Optional.of(copy);
     }
+
 }
diff --git a/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java b/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java
@@ -40,6 +40,7 @@ public class CoberturaParser extends CoverageParser {
     private static final Pattern BRANCH_PATTERN = Pattern.compile(".*\\((?<covered>\\d+)/(?<total>\\d+)\\)");
     private static final PathUtil PATH_UTIL = new PathUtil();
 
+    private static final Coverage DEFAULT_BRANCH_COVERAGE = new CoverageBuilder(Metric.BRANCH).setCovered(2).setMissed(0).build();
     private static final Coverage LINE_COVERED = new CoverageBuilder(Metric.LINE).setCovered(1).setMissed(0).build();
     private static final Coverage LINE_MISSED = new CoverageBuilder(Metric.LINE).setCovered(0).setMissed(1).build();
 
@@ -62,6 +63,27 @@ public class CoberturaParser extends CoverageParser {
     private static final QName BRANCH = new QName("branch");
     private static final QName CONDITION_COVERAGE = new QName("condition-coverage");
 
+    private final boolean ignoreErrors; // since 0.26.0
+
+    /**
+     * Creates a new instance of {@link CoberturaParser}.
+     */
+    public CoberturaParser() {
+        this(false);
+    }
+
+    /**
+     * Creates a new instance of {@link CoberturaParser}.
+     *
+     * @param ignoreErrors
+     *         determines whether to ignore errors
+     */
+    public CoberturaParser(final boolean ignoreErrors) {
+        super();
+
+        this.ignoreErrors = ignoreErrors;
+    }
+
     @Override
     protected ModuleNode parseReport(final Reader reader, final FilteredLog log) {
         try {
@@ -80,7 +102,7 @@ protected ModuleNode parseReport(final Reader reader, final FilteredLog log) {
                         readSource(eventReader, root);
                     }
                     else if (PACKAGE.equals(tagName)) {
-                        readPackage(eventReader, root, startElement);
+                        readPackage(eventReader, root, startElement, log);
                         isEmpty = false;
                     }
                 }
@@ -96,7 +118,7 @@ else if (PACKAGE.equals(tagName)) {
     }
 
     private void readPackage(final XMLEventReader reader, final ModuleNode root,
-            final StartElement currentStartElement) throws XMLStreamException {
+            final StartElement currentStartElement, final FilteredLog log) throws XMLStreamException {
         var packageNode = root.findOrCreatePackageNode(getValueOf(currentStartElement, NAME));
 
         while (reader.hasNext()) {
@@ -109,7 +131,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);
+                    readClassOrMethod(reader, fileNode, nextElement, log);
                 }
             }
             else if (event.isEndElement()) {
@@ -128,7 +150,7 @@ private String getFileName(final String relativePath) {
 
     @SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.CognitiveComplexity"})
     private Node readClassOrMethod(final XMLEventReader reader, final FileNode fileNode,
-            final StartElement parentElement) throws XMLStreamException {
+            final StartElement parentElement, final FilteredLog log) throws XMLStreamException {
         var lineCoverage = Coverage.nullObject(Metric.LINE);
         var branchCoverage = Coverage.nullObject(Metric.BRANCH);
 
@@ -159,8 +181,13 @@ private Node readClassOrMethod(final XMLEventReader reader, final FileNode fileN
                     }
                 }
                 else if (METHOD.equals(nextElement.getName())) {
-                    Node methodNode = readClassOrMethod(reader, fileNode, nextElement);
-                    node.addChild(methodNode);
+                    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);
+                    }
                 }
             }
             else if (event.isEndElement()) {
@@ -222,11 +249,13 @@ else if (event.isEndElement()) {
     }
 
     private Coverage readBranchCoverage(final StartElement line) {
-        String conditionCoverageAttribute = getValueOf(line, CONDITION_COVERAGE);
+        return getOptionalValueOf(line, CONDITION_COVERAGE).map(this::fromConditionCoverage).orElse(DEFAULT_BRANCH_COVERAGE);
+    }
+
+    private Coverage fromConditionCoverage(final String conditionCoverageAttribute) {
         var matcher = BRANCH_PATTERN.matcher(conditionCoverageAttribute);
         if (matcher.matches()) {
-            var builder = new CoverageBuilder();
-            return builder.setMetric(Metric.BRANCH)
+            return new CoverageBuilder().setMetric(Metric.BRANCH)
                     .setCovered(matcher.group("covered"))
                     .setTotal(matcher.group("total"))
                     .build();

diff --git a/src/test/java/edu/hm/hafner/coverage/parser/AbstractParserTest.java b/src/test/java/edu/hm/hafner/coverage/parser/AbstractParserTest.java
@@ -26,9 +26,15 @@ abstract class AbstractParserTest {
     private final FilteredLog log = new FilteredLog("Errors");
 
     ModuleNode readReport(final String fileName) {
+        var parser = createParser();
+
+        return readReport(fileName, parser);
+    }
+
+    ModuleNode readReport(final String fileName, final CoverageParser parser) {
         try (InputStream stream = AbstractParserTest.class.getResourceAsStream(fileName);
                 Reader reader = new InputStreamReader(Objects.requireNonNull(stream), StandardCharsets.UTF_8)) {
-            return createParser().parse(reader, log);
+            return parser.parse(reader, log);
         }
         catch (IOException e) {
             throw new AssertionError(e);

diff --git 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,42 @@ CoberturaParser createParser() {
         return new CoberturaParser();
     }
 
+    @Test
+    void shouldIgnoreMissingConditionAttribute() {
+        Node duplicateMethods = readReport("cobertura-missing-condition-coverage.xml");
+
+        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(getLog().hasErrors()).isFalse();
+
+        var file = duplicateMethods.getAllFileNodes().get(0);
+        assertThat(file.getCoveredOfLine(61)).isEqualTo(2);
+        assertThat(file.getMissedOfLine(61)).isEqualTo(0);
+    }
+
+    @Test
+    void shouldIgnoreDuplicateMethods() {
+        Node duplicateMethods = readReport("cobertura-duplicate-methods.xml", new CoberturaParser(true));
+
+        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(getLog().hasErrors()).isTrue();
+        assertThat(getLog().getErrorMessages())
+                .contains("Skipping duplicate method 'VisualOn.Data.DataSourceProvider' for class 'Enumerate()'");
+
+        var file = duplicateMethods.getAllFileNodes().get(0);
+        assertThat(file.getCoveredOfLine(61)).isEqualTo(2);
+        assertThat(file.getMissedOfLine(61)).isEqualTo(0);
+    }
+
     @Test
     void shouldMergeCorrectly729() {
         var builder = new CoverageBuilder();

diff --git a/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-methods.xml b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-methods.xml
@@ -0,0 +1,75 @@
+<?xml version="1.0" encoding="utf-8"?>
+<coverage line-rate="0.8301000000000001" branch-rate="0.75" version="1.9" timestamp="1663310122" lines-covered="44"
+          lines-valid="53" branches-covered="3" branches-valid="4">
+  <sources>
+    <source>/CoverageTest.Service/</source>
+  </sources>
+  <packages>
+    <package name="CoverageTest.Service" line-rate="0.8301000000000001" branch-rate="0.75" complexity="8">
+      <classes>
+        <class name="VisualOn.Data.DataSourceProvider" filename="src/VisualOn.Core/Data/DataSourceProvider.cs"
+               line-rate="0.925" branch-rate="0.7692307692307693" complexity="18">
+          <methods>
+            <method name="Enumerate" signature="()" line-rate="1" branch-rate="1" complexity="2">
+              <lines>
+                <line number="61" hits="2" branch="true" condition-coverage="100% (2/2)"/>
+                <line number="63" hits="2" branch="false"/>
+                <line number="65" hits="1" branch="false"/>
+              </lines>
+            </method>
+            <method name="Enumerate" signature="()" line-rate="1" branch-rate="1" complexity="2">
+              <lines>
+                <line number="69" hits="1" branch="false"/>
+                <line number="71" hits="1" branch="true" condition-coverage="100% (2/2)"/>
+                <line number="73" hits="1" branch="false"/>
+                <line number="75" hits="1" branch="false"/>
+              </lines>
+            </method>
+          </methods>
+          <lines>
+            <line number="13" hits="2" branch="false"/>
+            <line number="15" hits="2" branch="false"/>
+            <line number="16" hits="2" branch="false"/>
+            <line number="17" hits="2" branch="false"/>
+            <line number="20" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="24" hits="2" branch="false"/>
+            <line number="25" hits="2" branch="true" condition-coverage="100% (4/4)"/>
+            <line number="26" hits="2" branch="false"/>
+            <line number="28" hits="2" branch="false"/>
+            <line number="30" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="33" hits="2" branch="false"/>
+            <line number="37" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="38" hits="1" branch="false"/>
+            <line number="40" hits="2" branch="false"/>
+            <line number="41" hits="2" branch="false"/>
+            <line number="46" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="47" hits="0" branch="false"/>
+            <line number="48" hits="2" branch="false"/>
+            <line number="53" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="54" hits="0" branch="false"/>
+            <line number="56" hits="2" branch="false"/>
+            <line number="61" hits="2" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="63" hits="2" branch="false"/>
+            <line number="65" hits="1" branch="false"/>
+            <line number="69" hits="1" branch="false"/>
+            <line number="71" hits="1" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="73" hits="1" branch="false"/>
+            <line number="75" hits="1" branch="false"/>
+            <line number="77" hits="1" branch="false"/>
+            <line number="81" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="82" hits="0" branch="false"/>
+            <line number="84" hits="2" branch="false"/>
+            <line number="85" hits="2" branch="false"/>
+            <line number="87" hits="2" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="88" hits="2" branch="false"/>
+            <line number="90" hits="2" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="91" hits="2" branch="false"/>
+            <line number="93" hits="2" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="94" hits="2" branch="false"/>
+            <line number="96" hits="2" branch="false"/>
+          </lines>
+        </class>
+      </classes>
+    </package>
+  </packages>
+</coverage>
diff --git a/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-missing-condition-coverage.xml b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-missing-condition-coverage.xml
@@ -0,0 +1,67 @@
+<?xml version="1.0" encoding="utf-8"?>
+<coverage line-rate="0.8301000000000001" branch-rate="0.75" version="1.9" timestamp="1663310122" lines-covered="44"
+          lines-valid="53" branches-covered="3" branches-valid="4">
+  <sources>
+    <source>/CoverageTest.Service/</source>
+  </sources>
+  <packages>
+    <package name="CoverageTest.Service" line-rate="0.8301000000000001" branch-rate="0.75" complexity="8">
+      <classes>
+        <class name="VisualOn.Data.DataSourceProvider" filename="src/VisualOn.Core/Data/DataSourceProvider.cs"
+               line-rate="0.925" branch-rate="0.7692307692307693" complexity="18">
+          <methods>
+            <method name="Enumerate" signature="()" line-rate="1" branch-rate="1" complexity="2">
+              <lines>
+                <line number="61" hits="2" branch="true"/>
+                <line number="63" hits="2" branch="false"/>
+                <line number="65" hits="1" branch="false"/>
+              </lines>
+            </method>
+          </methods>
+          <lines>
+            <line number="13" hits="2" branch="false"/>
+            <line number="15" hits="2" branch="false"/>
+            <line number="16" hits="2" branch="false"/>
+            <line number="17" hits="2" branch="false"/>
+            <line number="20" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="24" hits="2" branch="false"/>
+            <line number="25" hits="2" branch="true" condition-coverage="100% (4/4)"/>
+            <line number="26" hits="2" branch="false"/>
+            <line number="28" hits="2" branch="false"/>
+            <line number="30" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="33" hits="2" branch="false"/>
+            <line number="37" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="38" hits="1" branch="false"/>
+            <line number="40" hits="2" branch="false"/>
+            <line number="41" hits="2" branch="false"/>
+            <line number="46" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="47" hits="0" branch="false"/>
+            <line number="48" hits="2" branch="false"/>
+            <line number="53" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="54" hits="0" branch="false"/>
+            <line number="56" hits="2" branch="false"/>
+            <line number="61" hits="2" branch="true"/>
+            <line number="63" hits="2" branch="false"/>
+            <line number="65" hits="1" branch="false"/>
+            <line number="69" hits="1" branch="false"/>
+            <line number="71" hits="1" branch="true"/>
+            <line number="73" hits="1" branch="false"/>
+            <line number="75" hits="1" branch="false"/>
+            <line number="77" hits="1" branch="false"/>
+            <line number="81" hits="2" branch="true" condition-coverage="50% (1/2)"/>
+            <line number="82" hits="0" branch="false"/>
+            <line number="84" hits="2" branch="false"/>
+            <line number="85" hits="2" branch="false"/>
+            <line number="87" hits="2" branch="true"/>
+            <line number="88" hits="2" branch="false"/>
+            <line number="90" hits="2" branch="true"/>
+            <line number="91" hits="2" branch="false"/>
+            <line number="93" hits="2" branch="true"/>
+            <line number="94" hits="2" branch="false"/>
+            <line number="96" hits="2" branch="false"/>
+          </lines>
+        </class>
+      </classes>
+    </package>
+  </packages>
+</coverage>