[SUREFIRE-2167] Simplify deserialization of elapsed time in TestSuiteXmlParser

[jira-importer]

Michael Osipov opened SUREFIRE-2167 and commented

The current code uses a convoluted solution based on NumberFormat which does not even correspond to the serialization code in StatelessXmlReporter. These days, we can use (Float#parseFloat() to deserialize the string representation of a float value since:

• Easier to understand and this is not human output (separation of concerns)
• It is much stricter than NumberFormat which ignores multiple decimal separators (This issue caused SUREFIRE-2165)

---

Affects: 3.1.0

Issue Links:

• SUREFIRE-2175 Turn time attribute to xs:float in Surefire test report schema
  ("causes")

• MINVOKER-341 Make elapsed time field type consistent with Maven Surefire

• SUREFIRE-2164 Simplify serialization of elapsed time in StatelessXmlReporter

• SUREFIRE-2165 TestSuiteXmlParser does not capture text per element causing concatenated text produced
  ("requires")

• SUREFIRE-1533 surefire-test-report.xsd has too narrow a definition for testcase time attribute
  ("is required by")

• SUREFIRE-2176 Invalid formatting of time attribute
  ("is depended upon by")

Backported to: 3.1.1

[jira-importer]

Michael Osipov commented

Fixed with 9e971d20c82f4c222b0bd8cb34f646433e00252b.

[thomasmey]

Hi, @michael-o sadly this change did break the reporting of reports generated with LegacyXmlReportGeneratingListener in junit. The junit code seems create the time attributlike the maven-surefire reporting plugin did parse before, i.e. via NumberFormat.parse().
See also https://github.com/junit-team/junit-framework/blob/main/junit-platform-reporting/src/main/java/org/junit/platform/reporting/legacy/xml/XmlReportWriter.java#L167

For testsuites the Float.parse seems to work when the time number is smaller then 1000, the time attribute will look like this then: time="394.879", but when the testsuite runs longer the created time attribute will look like time="1,323.333", which make Float parse fail.

can you please revert this change and make things compatible again?

[michael-o]

Le me get this straight, the value is solely parsed by JUnit, not us, correct? If so, the fix should be on their end since nobody has complained so far.

Can you assemble an MWE for me? I'd like to take a look.

[thomasmey]

Hi, it's the other way around: I create an XML report file with junit legacy XML reporter and try to use maven surefire reporter to convert it to html

Sadly this change did break the parsing of testsuite XML element with time >1000 since NumberFormat is used to parse and print the time string, as it was done in the surefire reporter plugin before this change.

Not sure if surefire can consume XML reports from itself when runtime is bigger then 16,667 minutes.

[michael-o]

Hi, it's the other way around: I create an XML report file with junit legacy XML reporter and try to use maven surefire reporter to convert it to html

Sadly this change did break the parsing of testsuite XML element with time >1000 since NumberFormat is used to parse and print the time string, as it was done in the surefire reporter plugin before this change.

Not sure if surefire can consume XML reports from itself when runtime is bigger then 16,667 minutes.

A sample would still need nice. I have a simple idea.

[thomasmey]

I'll work on a sample over the next days

[thomasmey]

here is a little reproducer:

pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>de.m3y3r.surefire</groupId>
  <artifactId>reporter-bug</artifactId>
  <version>0.1.0-SNAPSHOT</version>
  <packaging>jar</packaging>

  <dependencies>
    <dependency>
        <groupId>org.junit.jupiter</groupId>
        <artifactId>junit-jupiter-engine</artifactId>
        <version>5.9.1</version>
        <scope>test</scope>
    </dependency>
  </dependencies>

  <build>
    <plugins>
        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <version>3.1.0</version>
        </plugin>
    </plugins>
  </build>
</project>

LongTest.java

package de.m3y3r.surefire.bug;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Assertions;
import java.util.concurrent.TimeUnit;

public class LongTest {

        @Test
        public void longRunningTest() throws InterruptedException {
                Thread.currentThread().sleep(TimeUnit.SECONDS.toMillis(1000));
                Assertions.assertTrue(true);
        }
}

Run with:
mvn org.apache.maven.plugins:maven-surefire-report-plugin:3.5.4:report

and it will fail with similar error message:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-report-plugin:3.5.4:report (default-cli) on project reporter-bug: Execution default-cli of goal org.apache.maven.plugins:maven-surefire-report-plugin:3.5.4:report failed: Error parsing JUnit XML report surefire-example/target/surefire-reports/TEST-de.m3y3r.surefire.bug.LongTest.xml: Failed to parse time value: For input string: "3,622.023" -> [Help 1]

[michael-o]

here is a little reproducer:

pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>de.m3y3r.surefire</groupId>
  <artifactId>reporter-bug</artifactId>
  <version>0.1.0-SNAPSHOT</version>
  <packaging>jar</packaging>

  <dependencies>
    <dependency>
        <groupId>org.junit.jupiter</groupId>
        <artifactId>junit-jupiter-engine</artifactId>
        <version>5.9.1</version>
        <scope>test</scope>
    </dependency>
  </dependencies>

  <build>
    <plugins>
        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <version>3.1.0</version>
        </plugin>
    </plugins>
  </build>
</project>

LongTest.java

package de.m3y3r.surefire.bug;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Assertions;
import java.util.concurrent.TimeUnit;

public class LongTest {

        @Test
        public void longRunningTest() throws InterruptedException {
                Thread.currentThread().sleep(TimeUnit.SECONDS.toMillis(1000));
                Assertions.assertTrue(true);
        }
}

Run with: mvn org.apache.maven.plugins:maven-surefire-report-plugin:3.5.4:report

and it will fail with similar error message:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-report-plugin:3.5.4:report (default-cli) on project reporter-bug: Execution default-cli of goal org.apache.maven.plugins:maven-surefire-report-plugin:3.5.4:report failed: Error parsing JUnit XML report surefire-example/target/surefire-reports/TEST-de.m3y3r.surefire.bug.LongTest.xml: Failed to parse time value: For input string: "3,622.023" -> [Help 1]

Looks reasonable, can you wrap in a PR? I will pick this up next week.

[michael-o]

@thomasmey Please try:

index c0643fd92..1b0693cc7 100644
--- a/surefire-report-parser/src/main/java/org/apache/maven/plugins/surefire/report/TestSuiteXmlParser.java
+++ b/surefire-report-parser/src/main/java/org/apache/maven/plugins/surefire/report/TestSuiteXmlParser.java
@@ -114,7 +114,7 @@ public void startElement(String uri, String localName, String qName, Attributes
                         currentSuite = defaultSuite;
                         String timeStr = attributes.getValue("time");
                         if (timeStr != null) {
-                            defaultSuite.setTimeElapsed(Float.parseFloat(timeStr));
+                            defaultSuite.setTimeElapsed(Float.parseFloat(timeStr.replaceAll(",", "")));
                         } else {
                             consoleLogger.warning("No time attribute found on testsuite element");
                         }
@@ -157,7 +157,7 @@ public void startElement(String uri, String localName, String qName, Attributes
                         testCase.setFullClassName(currentSuite.getFullClassName())
                                 .setClassName(currentSuite.getName())
                                 .setFullName(currentSuite.getFullClassName() + "." + testCase.getName())
-                                .setTime(timeStr != null ? Float.parseFloat(timeStr) : 0.0f);
+                                .setTime(timeStr != null ? Float.parseFloat(timeStr.replaceAll(",", "")) : 0.0f);

                         if (currentSuite != defaultSuite) {
                             currentSuite.setTimeElapsed(testCase.getTime() + currentSuite.getTimeElapsed());
@@ -228,7 +228,7 @@ public void endElement(String uri, String localName, String qName) throws SAXExc
                 break;
             case "time":
                 try {
-                    defaultSuite.setTimeElapsed(Float.parseFloat(currentElement.toString()));
+                    defaultSuite.setTimeElapsed(Float.parseFloat(currentElement.toString().replaceAll(",", "")));
                 } catch (NumberFormatException e) {
                     throw new SAXException("Failed to parse time value", e);
                 }

No fuzz, simple and stupid patch!