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

Add flakes to the failsafe-summary.xml. Fix a bug in the equals metho… #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bingx1
Copy link

@bingx1 bingx1 commented Oct 9, 2024

Summary of these changes:

  • failsafe-summary.xml has been updated to include a flakes element
  • flakes was already being recorded by the RunResult class, but this was not exposed in the .xml previously
  • This should have been caught by the existing testAppendSerialization test, but was not due to a bug in the RunResult.toEquals method where it did not take the flakes field into account for equality comparison between RunResult objects.

}

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

Choose a reason for hiding this comment

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

I added this test to check for backwards compatibility. It ensures that failsafe-summary.xml files produced by older versions of the plugin (i.e without the flakes field) can still be read into memory. In these cases, we simply set flakes to 0.

Choose a reason for hiding this comment

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

Love your work.

@alex-atlassian
Copy link

Summary of these changes:

  • failsafe-summary.xml has been updated to include a flakes element

s/include a/add an optional/

Please note that both xsd and xml have been updated.

@@ -84,12 +85,14 @@ 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);

Choose a reason for hiding this comment

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

Probably not a problem:

What happens when you don't put flakes in the xml string? Does this fail well?

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/atlassian-forks/maven-surefire/pull/1/files#diff-5bf0b71acbf01d365bb28569f502903f8cb33d96ec98b0c1c53bab5a5aa7ee5fR95

Yeah; this function doesn't throw when the field isn't present. It will just return a blank string (and I'm handling that case in the linked line of code). We'll set flakes = 0 when the flakes XML element isn't present.

Choose a reason for hiding this comment

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

Looks good

: 1 ; udiff /tmp/old.xml /tmp/new.xml
--- /tmp/old.xml	2024-10-10 14:04:00
+++ /tmp/new.xml	2024-10-10 14:04:02
@@ -8,6 +8,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" nillable="true"/>
 <xsd:element name="failureMessage" type="xsd:string" nillable="true"/>
 </xsd:sequence>
 <xsd:attribute name="result" type="errorType" use="optional"/>

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

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

Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary empty lines

Copy link
Author

Choose a reason for hiding this comment

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

I only did this to be consistent with the person who wrote the other test in this file 😆

@hubertgrzeskowiak
Copy link
Member

Looks good to me!
Let's not merge this into master though, but raise a PR from this branch instead. This is how we should do it generally. My bad for not starting with a good example :coneofshame:

@alex-atlassian
Copy link

Let's do it.
My bad for being paranoid and untrusting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants