[SUREFIRE-2224] StatelessXmlReporter#getTestProblems() does not properly reflect report schema structure

[michael-o]

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue filed
  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.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the
  commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean install to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.
• You have run the integration tests successfully (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.

[michael-o]

@NissMoony have a look. A preluding simplification to your PR.

[michael-o]

I believe that this one is incomplete and requires a full ticket with a fix.

[michael-o]

@kriegaex Have a look at this one as well. I am not happy that stackTrace is under this variable as well, but maybe just because it is internal (not public) it does not matter much.

[kriegaex]

@michael-o, actually, I am a bit out of context here and not 100% sure what that code does and currently am busy otherwise, i.e. I am just looking at the code here on the website, which IMO is not a good way to conduct a proper review. You have to check out the code and see what it does, inspect/run tests covering the code etc.

It looks as if in case of reported test errors or failures, it adds something like an "error" or "failure" tag to the XML and then, if any stack trace exists, also a "stackTrace" element and "system-out"/"system-err".

Am I seeing correctly, that you just refactored the code with no functional changes, only reflecting the nesting structure of the XML output in the logical structure (if block) of the source code? If so, it looks good to me.

[michael-o]

@michael-o, actually, I am a bit out of context here and not 100% sure what that code does and currently am busy otherwise, i.e. I am just looking at the code here on the website, which IMO is not a good way to conduct a proper review. You have to check out the code and see what it does, inspect/run tests covering the code etc.

It looks as if in case of reported test errors or failures, it adds something like an "error" or "failure" tag to the XML and then, if any stack trace exists, also a "stackTrace" element and "system-out"/"system-err".

Am I seeing correctly, that you just refactored the code with no functional changes, only reflecting the nesting structure of the XML output in the logical structure (if block) of the source code? If so, it looks good to me.

Just refactoring and abiding to the XML schemas: https://maven.apache.org/surefire/maven-surefire-plugin/xsd/surefire-test-report.xsd and https://maven.apache.org/surefire/maven-surefire-plugin/xsd/surefire-test-report.xsd

[marcphilipp]

This still needs to be called if stackTrace != null and !enableNestedOutErrElements. Otherwise, error and failure elements won't contain the stack trace as their content.

[michael-o]

I think I understand what you mean. Let me rephrase and I guess you are right here, the schema is poorly designed. According to https://maven.apache.org/surefire/maven-surefire-plugin/xsd/surefire-test-report.xsd both failure and error aren't complex elements, thus do not contain stackTrace. Their element body contains the strack trace? Is that the case, if so I will cancel the vote and work on a fix and let you know.

[marcphilipp]

Yes, that's right.

Before (with 3.2.3 and earlier)

<testcase name="addsTwoNumbers" classname="com.example.project.CalculatorTests" time="0.014">
  <failure message="1 + 1 should equal 2 ==&gt; expected: &lt;3&gt; but was: &lt;2&gt;" type="org.opentest4j.AssertionFailedError"><![CDATA[org.opentest4j.AssertionFailedError: 1 + 1 should equal 2 ==> expected: <3> but was: <2>
  at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
  at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
  at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
  at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
  at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:563)
  at com.example.project.CalculatorTests.addsTwoNumbers(CalculatorTests.java:26)
  at java.base/java.lang.reflect.Method.invoke(Method.java:578)
  at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
  at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
]]></failure>
</testcase>

After this change

<testcase name="addsTwoNumbers" classname="com.example.project.CalculatorTests" time="0.014">
  <failure message="1 + 1 should equal 2 ==&gt; expected: &lt;3&gt; but was: &lt;2&gt;" type="org.opentest4j.AssertionFailedError"/>
</testcase>

[michael-o]

Thanks for the confirmation. I will cancel the vote today, create an improvement request to change the schema in the next major to a complex element with stackTrace to align with the rest and another one to fix this regression. I will ping you as soon as I have something to test.

[michael-o]

Vote canceled and repo dropped.

[michael-o]

Fix: #709