JENKINS-37598: Inaccurate aggregation of multiple xmls containing cas…

…e information about the same testsuite

src/main/java/hudson/tasks/junit/ClassResult.java

@@ -33,7 +33,9 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * Cumulative test result of a test class.
@@ -176,7 +178,7 @@ public void add(CaseResult r) {
     @Override
     public void tally() {
         passCount=failCount=skipCount=0;
-        duration=0;
+        Set<SuiteResult>  suites=new HashSet<SuiteResult>();
         for (CaseResult r : cases) {
             r.setClass(this);
             if (r.isSkipped()) {
@@ -188,10 +190,13 @@ else if(r.isPassed()) {
             else {
                 failCount++;
             }
-            //retrieve the class duration from these cases' suite time
-            if(duration == 0){
-                duration = r.getSuiteResult().getDuration();
-            }
+            suites.add( r.getSuiteResult() );
+        }
+
+        // retrieve the class duration from these cases' suite time
+        duration = 0;
+        for (SuiteResult s : suites) {
+            duration += s.getDuration();
         }
     }
 

src/main/java/hudson/tasks/junit/SuiteResult.java

@@ -72,8 +72,8 @@ public final class SuiteResult implements Serializable {
     /** Optional ID attribute of a test suite. E.g., Eclipse plug-ins tests always have the name 'tests' but a different id. **/
     private String id;
 
-    /** Optional time attribute of a test suite. E.g., Suites can use their own time attribute or the sum of their cases' times as before.**/
-    private String time;
+    /** Optional true if time attribute is present. E.g., Suites can use their own time attribute or the sum of their cases' times as before.**/
+    private boolean hasTimeAttr;
 
     /**
      * All test cases.
@@ -165,10 +165,11 @@ private SuiteResult(File xmlReport, Element suite, boolean keepLongStdio) throws
         this.timestamp = suite.attributeValue("timestamp");
         this.id = suite.attributeValue("id");
         // check for test suite time attribute
-        if( ( this.time = suite.attributeValue("time") ) != null ){
-            duration = new TimeToFloat(this.time).parse();
+        this.hasTimeAttr = suite.attributeValue("time")!=null;
+        if( hasTimeAttr ) {
+            duration = new TimeToFloat(suite.attributeValue("time")).parse();
         }
-        
+
         Element ex = suite.element("error");
         if(ex!=null) {
             // according to junit-noframes.xsl l.229, this happens when the test class failed to load
@@ -227,7 +228,7 @@ private SuiteResult(File xmlReport, Element suite, boolean keepLongStdio) throws
         casesByName().put(cr.getName(), cr);
 
         //if suite time was not specified use sum of the cases' times
-        if(this.time == null){
+        if( !hasTimeAttr ){
             duration += cr.getDuration();
         }
     }
@@ -348,4 +349,22 @@ void setParent(hudson.tasks.junit.TestResult parent) {
     private static final long serialVersionUID = 1L;
 
     private static final Pattern SUREFIRE_FILENAME = Pattern.compile("TEST-(.+)\\.xml");
+
+    /**
+     * Merges another SuiteResult into this one.
+     * 
+     * @param sr
+     */
+    public void merge(SuiteResult sr) {
+        if (sr.hasTimeAttr ^ hasTimeAttr){
+            throw new IllegalStateException("Merging of suiteresults with incompatible time attribute usage is not supported.( "+getFile()+", "+sr.getFile()+")");
+        }
+        if (hasTimeAttr) {
+            duration += sr.getDuration();
+        }
+        for (CaseResult cr : sr.getCases()) {
+            addCase(cr);
+            cr.replaceParent(this);
+        }
+    }
 }

src/main/java/hudson/tasks/junit/TestResult.java

@@ -256,12 +256,9 @@ private void add(SuiteResult sr) {
                 if(strictEq(s.getTimestamp(),sr.getTimestamp())) {
                     return;
                 }
-
-                for (CaseResult cr: sr.getCases()) {
-                    s.addCase(cr);
-                    cr.replaceParent(s);
-                }
+
                 duration += sr.getDuration();
+                s.merge(sr);
                 return;
             }
         }

src/test/java/hudson/tasks/junit/TestResultTest.java

@@ -171,6 +171,40 @@ public void testMerge() throws IOException, URISyntaxException {
         assertEquals("Fail count should now be 1", 1, first.getFailCount());
     }
 
+    @Issue("JENKINS-37598")
+    @Test
+    public void testMergeWithTime() throws Exception {
+        TestResult testResult = new TestResult();
+        testResult.parse(getDataFile("junit-report-time-aggregation.xml"));
+        testResult.tally();
+
+        assertEquals(1, testResult.getSuites().size());
+        SuiteResult suite = testResult.getSuite("test.fs.FileSystemTests");
+        assertEquals(3, suite.getCases().size());
+        assertEquals(100, suite.getDuration(), 2);
+    }
+
+    @Issue("JENKINS-37598")
+    @Test
+    public void testMergeWithoutTime() throws Exception {
+        TestResult testResult = new TestResult();
+        testResult.parse(getDataFile("junit-report-time-aggregation2.xml"));
+        testResult.tally();
+
+        assertEquals(1, testResult.getSuites().size());
+        SuiteResult suite = testResult.getSuite("test.fs.FileSystemTests");
+        assertEquals(3, suite.getCases().size());
+        assertEquals(30, suite.getDuration(), 2);
+    }
+
+    @Issue("JENKINS-37598")
+    @Test(expected=Exception.class)
+    public void testMergeWithInconsistency() throws Exception {
+        TestResult testResult = new TestResult();
+        testResult.parse(getDataFile("junit-report-time-aggregation-invalid.xml"));
+        testResult.tally();
+    }
+
     private static final XStream XSTREAM = new XStream2();
 
     static {
@@ -179,4 +213,4 @@ public void testMerge() throws IOException, URISyntaxException {
         XSTREAM.alias("case",CaseResult.class);
         XSTREAM.registerConverter(new HeapSpaceStringConverter(),100);
     }
-}
+}

src/test/resources/hudson/tasks/junit/junit-report-time-aggregation-invalid.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<testsuites name="Automation Tests" tests="2" errors="0" failures="0" ignored="0">
+    <testsuite name="test.fs.FileSystemTests" time="99">
+      <testcase name="testPrefix1" classname="test.fs.FileSystemTest1" time="10"/>
+    </testsuite>
+    <testsuite name="test.fs.FileSystemTests">
+      <testcase name="testPrefix2" classname="test.fs.FileSystemTest1" time="10"/>
+    </testsuite>
+</testsuites>

src/test/resources/hudson/tasks/junit/junit-report-time-aggregation.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<testsuites name="Automation Tests" tests="2" errors="0" failures="0" ignored="0">
+    <testsuite name="test.fs.FileSystemTests" time="50">
+      <testcase name="testPrefix1" classname="test.fs.FileSystemTest1" time="10"/>
+      <testcase name="testPrefix1A" classname="test.fs.FileSystemTest1" time="10"/>
+    </testsuite>
+    <testsuite name="test.fs.FileSystemTests" time="50">
+      <testcase name="testPrefix2" classname="test.fs.FileSystemTest1" time="10"/>
+    </testsuite>
+</testsuites>

src/test/resources/hudson/tasks/junit/junit-report-time-aggregation2.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<testsuites name="Automation Tests" tests="2" errors="0" failures="0" ignored="0">
+    <testsuite name="test.fs.FileSystemTests">
+      <testcase name="testPrefix1" classname="test.fs.FileSystemTest1" time="10"/>
+      <testcase name="testPrefix1A" classname="test.fs.FileSystemTest1" time="10"/>
+    </testsuite>
+    <testsuite name="test.fs.FileSystemTests">
+      <testcase name="testPrefix2" classname="test.fs.FileSystemTest1" time="10"/>
+    </testsuite>
+</testsuites>