Use original JUnit test class name instead of suite class in test XML report (#2625)

* Use original JUnit test class name instead of suite class in test XML report

* Correct expected/actual assertion order

Author: swarren12

src/com/facebook/buck/jvm/java/JavaTest.java

@@ -83,6 +83,7 @@
 import java.util.SortedSet;
 import java.util.concurrent.Callable;
 import java.util.logging.Level;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
@@ -393,10 +394,11 @@ public Path getPathToTestOutputDirectory() {
   /** @return a test case result, named "main", signifying a failure of the entire test class. */
   private TestCaseSummary getTestClassFailedSummary(String testClass, String message, long time) {
     return new TestCaseSummary(
-        testClass,
-        ImmutableList.of(
-            new TestResultSummary(
-                testClass, "main", ResultType.FAILURE, time, message, "", "", "")));
+      testClass,
+      false,
+      ImmutableList.of(
+        new TestResultSummary(
+          testClass, "main", ResultType.FAILURE, time, message, "", "", "")));
   }
 
   @Override
@@ -444,7 +446,12 @@ public Callable<TestResults> interpretTestResults(
           // definitively say whether or not a class should be run.  It's not possible, for example,
           // to filter testClassNames here at the buck end.
         } else if (Files.isRegularFile(testResultFile)) {
-          summaries.add(XmlTestResultParser.parse(testResultFile));
+          TestCaseSummary summary = XmlTestResultParser.parse(testResultFile);
+          if (summary.isTestSuite()) {
+            summaries.addAll(unrollTestSuiteSummary(summary));
+          } else {
+            summaries.add(summary);
+          }
         }
       }
 
@@ -615,6 +622,15 @@ protected ImmutableSet<Path> getRuntimeClasspath(BuildContext buildContext) {
         .build();
   }
 
+  private List<TestCaseSummary> unrollTestSuiteSummary(TestCaseSummary summary) {
+    Map<String, List<TestResultSummary>> summariesByActualTestCase = summary.getTestResults()
+      .stream().collect(Collectors.groupingBy(TestResultSummary::getTestCaseName));
+
+    return summariesByActualTestCase.entrySet().stream()
+      .map(entry -> new TestCaseSummary(entry.getKey(), false, entry.getValue()))
+      .collect(Collectors.toList());
+  }
+
   public interface AdditionalClasspathEntriesProvider {
     ImmutableList<Path> getAdditionalClasspathEntries(SourcePathResolverAdapter resolver);
   }

src/com/facebook/buck/test/TestCaseSummary.java

@@ -29,6 +29,7 @@ public class TestCaseSummary implements TestCaseSummaryExternalInterface<TestRes
   private static final int MAX_STATUS_WIDTH = 7;
 
   private final String testCaseName;
+  private final boolean testSuite;
   private final ImmutableList<TestResultSummary> testResults;
   private final boolean isDryRun;
   private final boolean hasAssumptionViolations;
@@ -38,7 +39,15 @@ public class TestCaseSummary implements TestCaseSummaryExternalInterface<TestRes
   private final long totalTime;
 
   public TestCaseSummary(String testCaseName, List<TestResultSummary> testResults) {
+    this(testCaseName, false, testResults);
+  }
+
+  public TestCaseSummary(
+      String testCaseName,
+      boolean testSuite,
+      List<TestResultSummary> testResults) {
     this.testCaseName = testCaseName;
+    this.testSuite = testSuite;
     this.testResults = ImmutableList.copyOf(testResults);
 
     boolean isDryRun = false;
@@ -110,6 +119,11 @@ public long getTotalTime() {
     return totalTime;
   }
 
+  /** @return whether this summary represents a suite of tests or a single class */
+  public boolean isTestSuite() {
+    return testSuite;
+  }
+
   /** @return a one-line, printable summary */
   public String getOneLineSummary(Locale locale, boolean hasPassingDependencies, Ansi ansi) {
     String statusText;

src/com/facebook/buck/test/XmlTestResultParser.java

@@ -139,6 +139,7 @@ private static TestCaseSummary doParse(String xml) throws IOException, SAXExcept
     Element root = doc.getDocumentElement();
     Preconditions.checkState("testcase".equals(root.getTagName()));
     String testCaseName = root.getAttribute("name");
+    boolean testSuite = false;
 
     NodeList testElements = doc.getElementsByTagName("test");
     List<TestResultSummary> testResults = Lists.newArrayListWithCapacity(testElements.getLength());
@@ -176,13 +177,20 @@ private static TestCaseSummary doParse(String xml) throws IOException, SAXExcept
         stdErr = null;
       }
 
+      String actualTestCaseName = node.getAttribute("suite");
+      if (actualTestCaseName != null && !actualTestCaseName.isEmpty()) {
+        testSuite |= !actualTestCaseName.equals(testCaseName);
+      } else {
+        actualTestCaseName = testCaseName;
+      }
+
       TestResultSummary testResult =
           new TestResultSummary(
-              testCaseName, testName, type, time, message, stacktrace, stdOut, stdErr);
+              actualTestCaseName, testName, type, time, message, stacktrace, stdOut, stdErr);
       testResults.add(testResult);
     }
 
-    return new TestCaseSummary(testCaseName, testResults);
+    return new TestCaseSummary(testCaseName, testSuite, testResults);
   }
 
   private static String createDetailedExceptionMessage(Path xmlFile, String xmlFileContents) {

test/com/facebook/buck/cli/TestRunningTest.java

@@ -312,44 +312,45 @@ public void testXmlGeneration() throws Exception {
 
     // Check for exactly one <tests> tag.
     NodeList testsList = doc.getElementsByTagName("tests");
-    assertEquals(testsList.getLength(), 1);
+    assertEquals(1, testsList.getLength());
 
     // Check for exactly one <test> tag.
     Element testsEl = (Element) testsList.item(0);
     NodeList testList = testsEl.getElementsByTagName("test");
-    assertEquals(testList.getLength(), 1);
+    assertEquals(1, testList.getLength());
 
     // Check the target has been set
     Element testEl = (Element) testList.item(0);
-    assertEquals(testEl.getAttribute("target"), "//foo/bar:baz");
+    assertEquals("//foo/bar:baz", testEl.getAttribute("target"));
+
+    assertEquals("TestCase", testEl.getAttribute("name"));
 
     // Check for exactly three <testresult> tags.
     // There should be two failures and one success.
     NodeList resultsList = testEl.getElementsByTagName("testresult");
-    assertEquals(resultsList.getLength(), 3);
+    assertEquals(3, resultsList.getLength());
 
     // Verify the text elements of the first <testresult> tag.
     Element passResultEl = (Element) resultsList.item(0);
-    assertEquals(passResultEl.getAttribute("name"), "passTest");
-    assertEquals(passResultEl.getAttribute("time"), "5000");
-    assertEquals(passResultEl.getAttribute("status"), "PASS");
+    assertEquals("passTest", passResultEl.getAttribute("name"));
+    assertEquals("5000", passResultEl.getAttribute("time"));
+    assertEquals("PASS", passResultEl.getAttribute("status"));
     checkXmlTextContents(passResultEl, "message", "");
     checkXmlTextContents(passResultEl, "stacktrace", "");
 
     // Verify the text elements of the second <testresult> tag.
-    assertEquals(testEl.getAttribute("name"), "TestCase");
     Element failResultEl1 = (Element) resultsList.item(1);
-    assertEquals(failResultEl1.getAttribute("name"), "failWithMsg");
-    assertEquals(failResultEl1.getAttribute("time"), "7000");
-    assertEquals(failResultEl1.getAttribute("status"), "FAIL");
+    assertEquals("failWithMsg", failResultEl1.getAttribute("name"));
+    assertEquals("7000", failResultEl1.getAttribute("time"));
+    assertEquals("FAIL", failResultEl1.getAttribute("status"));
     checkXmlTextContents(failResultEl1, "message", "Index out of bounds!");
     checkXmlTextContents(failResultEl1, "stacktrace", "Stacktrace");
 
     // Verify the text elements of the third <testresult> tag.
     Element failResultEl2 = (Element) resultsList.item(2);
-    assertEquals(failResultEl2.getAttribute("name"), "failNoMsg");
-    assertEquals(failResultEl2.getAttribute("time"), "4000");
-    assertEquals(failResultEl2.getAttribute("status"), "PASS");
+    assertEquals("failNoMsg", failResultEl2.getAttribute("name"));
+    assertEquals("4000", failResultEl2.getAttribute("time"));
+    assertEquals("PASS", failResultEl2.getAttribute("status"));
     checkXmlTextContents(failResultEl2, "message", "");
     checkXmlTextContents(failResultEl2, "stacktrace", "");
   }

test/com/facebook/buck/event/EventSerializationTest.java

@@ -237,8 +237,8 @@ public void testTestRunEventFinished() throws IOException {
     String message = ObjectMappers.WRITER.writeValueAsString(event);
     assertJsonEquals(
         "{%s,"
-            + "\"results\":[{\"testCases\":[{\"testCaseName\":\"Test1\",\"testResults\":[{"
-            + "\"testCaseName\":\"Test1\",\"type\":\"FAILURE\",\"time\":0}],"
+            + "\"results\":[{\"testCases\":[{\"testCaseName\":\"Test1\",\"testSuite\":false,"
+            + "\"testResults\":[{\"testCaseName\":\"Test1\",\"type\":\"FAILURE\",\"time\":0}],"
             + "\"failureCount\":1,\"skippedCount\":0,\"totalTime\":0,"
             + "\"success\":false}],"
             + "\"failureCount\":1,\"contacts\":[],\"labels\":[],"
@@ -324,8 +324,8 @@ public void testIndividualTestEventFinished() throws IOException {
     String message = ObjectMappers.WRITER.writeValueAsString(event);
     assertJsonEquals(
         "{%s,\"eventKey\":{\"value\":-594614477},"
-            + "\"results\":{\"testCases\":[{\"testCaseName\":\"Test1\",\"testResults\":[{"
-            + "\"testCaseName\":\"Test1\",\"type\":\"FAILURE\",\"time\":0}],"
+            + "\"results\":{\"testCases\":[{\"testCaseName\":\"Test1\",\"testSuite\":false,"
+            + "\"testResults\":[{\"testCaseName\":\"Test1\",\"type\":\"FAILURE\",\"time\":0}],"
             + "\"failureCount\":1,\"skippedCount\":0,\"totalTime\":0,"
             + "\"success\":false}],"
             + "\"failureCount\":1,\"contacts\":[],\"labels\":[],"

test/com/facebook/buck/testrunner/RunWithAnnotationIntegrationTest.java

@@ -51,7 +51,12 @@ public void testSimpleSuiteRun2TestCases() throws IOException {
 
     ProcessResult suiteTestResult = workspace.runBuckCommand("test", "//:SimpleSuiteTest");
     suiteTestResult.assertSuccess("Test should pass");
-    assertThat(suiteTestResult.getStderr(), containsString("2 Passed"));
+    assertThat(
+      suiteTestResult.getStderr(),
+      containsString("1 Passed   0 Skipped   0 Failed   com.example.Subtest1"));
+    assertThat(
+      suiteTestResult.getStderr(),
+      containsString("1 Passed   0 Skipped   0 Failed   com.example.Subtest2"));
   }
 
   @Test
@@ -62,8 +67,15 @@ public void testFailingSuiteRun3TestCasesWith1Failure() throws IOException {
 
     ProcessResult suiteTestResult = workspace.runBuckCommand("test", "//:FailingSuiteTest");
     suiteTestResult.assertTestFailure("Test should fail because of one of subtests failure");
-    assertThat(suiteTestResult.getStderr(), containsString("2 Passed"));
-    assertThat(suiteTestResult.getStderr(), containsString("1 Failed"));
+    assertThat(
+      suiteTestResult.getStderr(),
+      containsString("1 Passed   0 Skipped   0 Failed   com.example.Subtest1"));
+    assertThat(
+      suiteTestResult.getStderr(),
+      containsString("1 Passed   0 Skipped   0 Failed   com.example.Subtest2"));
+    assertThat(
+      suiteTestResult.getStderr(),
+      containsString("0 Passed   0 Skipped   1 Failed   com.example.FailingSubtest"));
   }
 
   @Test