From 4d859c7fd76cd4ee691656a61c8847689d45878c Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Wed, 11 Oct 2023 09:53:21 +0200 Subject: [PATCH 1/3] 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 https://github.com/jenkinsci/code-coverage-api-plugin/issues/785 --- .../java/edu/hm/hafner/coverage/Node.java | 13 +++- .../coverage/parser/CoberturaParser.java | 47 +++++++++--- .../coverage/parser/AbstractParserTest.java | 8 +- .../coverage/parser/CoberturaParserTest.java | 36 +++++++++ .../parser/cobertura-duplicate-methods.xml | 75 +++++++++++++++++++ .../cobertura-missing-condition-coverage.xml | 67 +++++++++++++++++ 6 files changed, 235 insertions(+), 11 deletions(-) create mode 100644 src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-methods.xml create mode 100644 src/test/resources/edu/hm/hafner/coverage/parser/cobertura-missing-condition-coverage.xml diff --git a/src/main/java/edu/hm/hafner/coverage/Node.java b/src/main/java/edu/hm/hafner/coverage/Node.java index 733bdc65..83342bbc 100644 --- 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 nodes) { nodes.forEach(this::addChild); } - /** * Returns the parent node. * @@ -779,4 +789,5 @@ private Optional filterTreeByMapping(final Function> 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 index 301e87af..efb6752f 100644 --- 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(".*\\((?\\d+)/(?\\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 index c1a939b7..c0cc9057 100644 --- 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 index 86028634..90a4b767 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,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 new file mode 100644 index 00000000..0bb1324d --- /dev/null +++ b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-duplicate-methods.xml @@ -0,0 +1,75 @@ + + + + /CoverageTest.Service/ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 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 new file mode 100644 index 00000000..ccd9d7b4 --- /dev/null +++ b/src/test/resources/edu/hm/hafner/coverage/parser/cobertura-missing-condition-coverage.xml @@ -0,0 +1,67 @@ + + + + /CoverageTest.Service/ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From c25b72eb49f0203bb8d926c75848cdc01e46368a Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Wed, 11 Oct 2023 10:32:12 +0200 Subject: [PATCH 2/3] Improve test case to show that `ignoreErrors` works indeed. --- .../coverage/parser/CoberturaParserTest.java | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) 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 90a4b767..70b31056 100644 --- a/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java +++ b/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java @@ -32,14 +32,13 @@ CoberturaParser createParser() { 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()"); + verifySmallTree(duplicateMethods); assertThat(getLog().hasErrors()).isFalse(); + verifyBranchCoverageOfLine61(duplicateMethods); + } + + private void verifyBranchCoverageOfLine61(final Node duplicateMethods) { var file = duplicateMethods.getAllFileNodes().get(0); assertThat(file.getCoveredOfLine(61)).isEqualTo(2); assertThat(file.getMissedOfLine(61)).isEqualTo(0); @@ -49,19 +48,24 @@ void shouldIgnoreMissingConditionAttribute() { void shouldIgnoreDuplicateMethods() { Node duplicateMethods = readReport("cobertura-duplicate-methods.xml", new CoberturaParser(true)); + verifySmallTree(duplicateMethods); + assertThat(getLog().hasErrors()).isTrue(); + assertThat(getLog().getErrorMessages()) + .contains("Skipping duplicate method 'VisualOn.Data.DataSourceProvider' for class 'Enumerate()'"); + + verifyBranchCoverageOfLine61(duplicateMethods); + + assertThatIllegalArgumentException().isThrownBy( + () -> readReport("cobertura-duplicate-methods.xml", new CoberturaParser())); + } + + private void verifySmallTree(final Node duplicateMethods) { 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 From a654daadee4e3a716e85cee53b3db89d87297123 Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Wed, 11 Oct 2023 10:51:03 +0200 Subject: [PATCH 3/3] Use an enum in the parent class to define the resilience. --- .../hm/hafner/coverage/CoverageParser.java | 38 +++++++++++++++++++ .../coverage/parser/CoberturaParser.java | 14 +++---- .../coverage/parser/CoberturaParserTest.java | 4 +- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/main/java/edu/hm/hafner/coverage/CoverageParser.java b/src/main/java/edu/hm/hafner/coverage/CoverageParser.java index d39ffc9b..db2122a1 100644 --- a/src/main/java/edu/hm/hafner/coverage/CoverageParser.java +++ b/src/main/java/edu/hm/hafner/coverage/CoverageParser.java @@ -18,9 +18,47 @@ * @author Ullrich Hafner */ public abstract class CoverageParser implements Serializable { + /** + * Defines how to handle fatal errors during parsing. + */ + public enum ProcessingMode { + /** All fatal errors will be ignored and logged. */ + IGNORE_ERRORS, + /** An exception will be thrown if a fatal error is detected. */ + FAIL_FAST + } + private static final long serialVersionUID = 3941742254762282096L; private transient TreeStringBuilder treeStringBuilder = new TreeStringBuilder(); + private final ProcessingMode processingMode; // since 0.26.0 + + /** + * Creates a new instance of {@link CoverageParser}. + * + * @param processingMode + * determines whether to ignore errors + */ + protected CoverageParser(final ProcessingMode processingMode) { + this.processingMode = processingMode; + } + + /** + * Creates a new instance of {@link CoverageParser} that will fail on all errors. + */ + protected CoverageParser() { + this(ProcessingMode.FAIL_FAST); + } + + /** + * Returns whether to ignore errors or to fail fast. + * + * @return true if errors should be ignored, false if an exception should be thrown on errors + */ + protected boolean ignoreErrors() { + return processingMode == ProcessingMode.IGNORE_ERRORS; + } + /** * Parses a report provided by the given reader. * 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 efb6752f..d2452d81 100644 --- a/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java +++ b/src/main/java/edu/hm/hafner/coverage/parser/CoberturaParser.java @@ -63,25 +63,21 @@ 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); + this(ProcessingMode.FAIL_FAST); } /** * Creates a new instance of {@link CoberturaParser}. * - * @param ignoreErrors + * @param processingMode * determines whether to ignore errors */ - public CoberturaParser(final boolean ignoreErrors) { - super(); - - this.ignoreErrors = ignoreErrors; + public CoberturaParser(final ProcessingMode processingMode) { + super(processingMode); } @Override @@ -182,7 +178,7 @@ private Node readClassOrMethod(final XMLEventReader reader, final FileNode fileN } else if (METHOD.equals(nextElement.getName())) { Node methodNode = readClassOrMethod(reader, fileNode, nextElement, log); - if (node.hasChild(methodNode.getName()) && ignoreErrors) { + if (node.hasChild(methodNode.getName()) && ignoreErrors()) { log.logError("Skipping duplicate method '%s' for class '%s'", node.getName(), methodNode.getName()); } else { 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 70b31056..26ac1a08 100644 --- a/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java +++ b/src/test/java/edu/hm/hafner/coverage/parser/CoberturaParserTest.java @@ -7,6 +7,7 @@ import edu.hm.hafner.coverage.Coverage; import edu.hm.hafner.coverage.Coverage.CoverageBuilder; +import edu.hm.hafner.coverage.CoverageParser.ProcessingMode; import edu.hm.hafner.coverage.CyclomaticComplexity; import edu.hm.hafner.coverage.FileNode; import edu.hm.hafner.coverage.FractionValue; @@ -46,7 +47,8 @@ private void verifyBranchCoverageOfLine61(final Node duplicateMethods) { @Test void shouldIgnoreDuplicateMethods() { - Node duplicateMethods = readReport("cobertura-duplicate-methods.xml", new CoberturaParser(true)); + Node duplicateMethods = readReport("cobertura-duplicate-methods.xml", + new CoberturaParser(ProcessingMode.IGNORE_ERRORS)); verifySmallTree(duplicateMethods); assertThat(getLog().hasErrors()).isTrue();