[SUREFIRE-2277] Fix bug in RunResult serialisation/deserialisation to (from) failsafe-summary.xml #790
Conversation
…the equals method for RunResult where it did not account for the flakes field.
|
|
||
| return new RunResult( | ||
| parseInt(completed), | ||
| parseInt(errors), | ||
| parseInt(failures), | ||
| parseInt(skipped), | ||
| isBlank(flakes) ? 0 : parseInt(flakes), |
There was a problem hiding this comment.
nit: do you reckon it's worth adding a comment why we are treating this differently, i.e. because of backward compatibility?
| @@ -64,6 +69,49 @@ public void testSkipped() throws Exception { | |||
| writeReadCheck(new RunResult(3, 2, 1, 0, null, true)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testFlakes() throws Exception { | |||
There was a problem hiding this comment.
can you be more specific about the exception thrown?
| } | ||
|
|
||
| @Test | ||
| public void testLegacyDeserialization() throws Exception { |
There was a problem hiding this comment.
can you be more specific about the exception thrown?
There was a problem hiding this comment.
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.
| RunResult expected = new RunResult(3, 2, 1, 0, 0, null, false); | ||
| RunResult actual = FailsafeSummaryXmlUtils.toRunResult(legacySummary); | ||
|
|
||
| assertThat(actual.getCompletedCount()).isEqualTo(expected.getCompletedCount()); |
There was a problem hiding this comment.
assertEquals is simpler and more obvious
There was a problem hiding this comment.
Again, I wrote this assertion in such a way to be consistent with the other test methods in the same file. I can change it, but do I only change it in this test or in the existing code too? I also feel this assertion isn't particularly unclear
This PR fixes the bug described in https://issues.apache.org/jira/browse/SUREFIRE-2277
Problem
There is a bug in
RunResult.testAppendSerialization.This test creates a
RunResultobject in-memory, serialises it, writes it to disk and then again deserialises the same file into aRunResultin-memory. I have run the test with the debugger and found that the final in-memoryRunResultobject is not the same as the initial one.It shouldn't be passing on master, but is due to a bug in the
RunResult.equalsmethod which is used in an assertion in the test. The source of the bug is that the value ofRunResult.flakesisn't preserved during serialisation to and fromfailsafe-summary.xml. The bug is slipping through and the test passes because theRunResult.equalsmethod doesn't consider theRunResult.flakesfield.Fix
I have modified the
failsafe-summary.xsdto include an optional<flakes>element, which will allowRunResult.flakesto be persisted in thefailsafe-summary.xmlduring serialisation. I have also changed the serialisation and deserialisation methods forRunResultto account forflakes.I have also added a test,
RunResultTest.testLegacyDeserializationfor backwards compatibility. It tests that deserialising a legacyfailsafe-summary.xmlstill works. The behaviour is that when theflakesXML element is not present in thefailsafe-summary.xml, the in-memoryRunResultwill haveRunResult.flakesset to 0 after deserialisation.Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,where you replace
SUREFIRE-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean installto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean install).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.
Our company, Atlassian, has a corporate CLA signed, and we added our team to it.
Contributors: Alex Courtis, Bing Xu and Hubert Grzeskowiak