Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SUREFIRE-2277] Fix bug in RunResult serialisation/deserialisation to (from) failsafe-summary.xml #790

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public final class FailsafeSummaryXmlUtils {
+ " <errors>%d</errors>\n"
+ " <failures>%d</failures>\n"
+ " <skipped>%d</skipped>\n"
+ " <flakes>%d</flakes>\n"
+ " %s\n"
+ "</failsafe-summary>";

Expand All @@ -84,12 +85,15 @@ public static RunResult toRunResult(File failsafeSummaryXml) throws Exception {
String skipped = xpath.evaluate("/failsafe-summary/skipped", root);
String failureMessage = xpath.evaluate("/failsafe-summary/failureMessage", root);
String timeout = xpath.evaluate("/failsafe-summary/@timeout", root);
String flakes = xpath.evaluate("/failsafe-summary/flakes", root);

return new RunResult(
parseInt(completed),
parseInt(errors),
parseInt(failures),
parseInt(skipped),
// Backwards compatability - to be replaced with parseInt in a future release
isBlank(flakes) ? 0 : parseInt(flakes),
michael-o marked this conversation as resolved.
Show resolved Hide resolved
isBlank(failureMessage) ? null : unescapeXml(failureMessage),
parseBoolean(timeout));
}
Expand All @@ -107,6 +111,7 @@ public static void fromRunResultToFile(RunResult fromRunResult, File toFailsafeS
fromRunResult.getErrors(),
fromRunResult.getFailures(),
fromRunResult.getSkipped(),
fromRunResult.getFlakes(),
msg);

Files.write(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@
package org.apache.maven.plugin.failsafe;

import java.io.File;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.StandardOpenOption;
import java.util.Locale;

import org.apache.maven.plugin.failsafe.util.FailsafeSummaryXmlUtils;
import org.apache.maven.surefire.api.suite.RunResult;
import org.apache.maven.surefire.api.util.SureFireFileManager;
import org.junit.Test;

import static java.lang.String.format;
import static org.assertj.core.api.Assertions.assertThat;

/**
Expand Down Expand Up @@ -64,6 +69,49 @@ public void testSkipped() throws Exception {
writeReadCheck(new RunResult(3, 2, 1, 0, null, true));
}

@Test
public void testFlakes() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you be more specific about the exception thrown?

writeReadCheck(new RunResult(3, 2, 1, 0, 2, null, true));
}

@Test
public void testLegacyDeserialization() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you be more specific about the exception thrown?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, yeah I could but it's a test method, I'm not convinced it's necessary to be specific here. I was following the existing code style in this particular Test class. I think in this case it may be better to be consistent with the original style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I 100% disagree. Consistency with imperfect code is not a virtue. New code should follow best practices, even when the old code does not.

File legacySummary = SureFireFileManager.createTempFile("failsafe", "test");
String legacyFailsafeSummaryXmlTemplate = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ "<failsafe-summary xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\""
+ " xsi:noNamespaceSchemaLocation=\"https://maven.apache.org/surefire/maven-surefire-plugin/xsd/failsafe-summary.xsd\""
+ " result=\"%s\" timeout=\"%s\">\n"
+ " <completed>%d</completed>\n"
+ " <errors>%d</errors>\n"
+ " <failures>%d</failures>\n"
+ " <skipped>%d</skipped>\n"
+ " %s\n"
+ "</failsafe-summary>";
String xml = format(Locale.ROOT, legacyFailsafeSummaryXmlTemplate, 0, false, 3, 2, 1, 0, "msg");
Files.write(
legacySummary.toPath(),
xml.getBytes(StandardCharsets.UTF_8),
StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING,
StandardOpenOption.WRITE);

// When the failsafe-summary.xml does not contain the <flakes> element, it should be considered as 0.
RunResult expected = new RunResult(3, 2, 1, 0, 0, null, false);
RunResult actual = FailsafeSummaryXmlUtils.toRunResult(legacySummary);

assertThat(actual.getCompletedCount()).isEqualTo(expected.getCompletedCount());
michael-o marked this conversation as resolved.
Show resolved Hide resolved

assertThat(actual.getErrors()).isEqualTo(expected.getErrors());

assertThat(actual.getFailures()).isEqualTo(expected.getFailures());

assertThat(actual.getSkipped()).isEqualTo(expected.getSkipped());

assertThat(actual.getFlakes()).isEqualTo(expected.getFlakes());

assertThat(actual).isEqualTo(expected);
}

@Test
public void testAppendSerialization() throws Exception {
RunResult simpleAggregate = getSimpleAggregate();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<xsd:element name="errors" type="xsd:int"/>
<xsd:element name="failures" type="xsd:int"/>
<xsd:element name="skipped" type="xsd:int"/>
<xsd:element name="flakes" type="xsd:int" minOccurs="0"/>
<xsd:element name="failureMessage" type="xsd:string" nillable="true"/>
</xsd:sequence>
<xsd:attribute name="result" type="errorType" use="optional"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,9 @@ public boolean equals(Object o) {
if (skipped != runResult.skipped) {
return false;
}
if (flakes != runResult.flakes) {
return false;
}
if (timeout != runResult.timeout) {
return false;
}
bingx1 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -218,6 +221,7 @@ public int hashCode() {
result = 31 * result + errors;
result = 31 * result + failures;
result = 31 * result + skipped;
result = 31 * result + flakes;
result = 31 * result + (failure != null ? failure.hashCode() : 0);
result = 31 * result + (timeout ? 1 : 0);
return result;
Expand Down