fix: BazelJUnitOutputListener logging xml output on suite-level failures (#328)

fixes: #327 

1. Updated code to propagate the test suite failure to each test case.
2. The test suite failure will be propagated to each child node, even if
the test is not executed.
3. Additionally, this change fixes the bug introduced in #300, which
incorrectly logged the test_suite as a testcase and logged incorrect
values for the "tests" attribute in the <testsuite> node.

Author: amishra-u

java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListener.java

@@ -5,6 +5,7 @@
 import java.io.BufferedWriter;
 import java.io.Closeable;
 import java.io.IOException;
+import java.io.Writer;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -50,8 +51,12 @@ public class BazelJUnitOutputListener implements TestExecutionListener, Closeabl
   public BazelJUnitOutputListener(Path xmlOut) {
     try {
       Files.createDirectories(xmlOut.getParent());
-      BufferedWriter writer = Files.newBufferedWriter(xmlOut, UTF_8);
-      xml = XMLOutputFactory.newFactory().createXMLStreamWriter(writer);
+      // Use LazyFileWriter to defer file creation until the first write operation.
+      // This prevents premature file creation in cases where the JVM terminates abruptly
+      // before any content is written. If no output is generated, Bazel has custom logic
+      // to create the XML file from test.log, but this logic only activates if the
+      // output file does not already exist.
+      xml = XMLOutputFactory.newFactory().createXMLStreamWriter(new LazyFileWriter(xmlOut));
       xml.writeStartDocument("UTF-8", "1.0");
     } catch (IOException | XMLStreamException e) {
       throw new IllegalStateException("Unable to create output file", e);
@@ -140,8 +145,9 @@ private Map<TestData, List<TestData>> matchTestCasesToSuites_locked(
         throw new IllegalStateException(
             "Unexpected test organization for test Case: " + testCase.getId());
       }
-      if (includeIncompleteTests || testCase.getDuration() != null) {
-        knownSuites.computeIfAbsent(parent, id -> new ArrayList<>()).add(testCase);
+      knownSuites.computeIfAbsent(parent, id -> new ArrayList<>());
+      if (testCase.getId().isTest() && (includeIncompleteTests || testCase.getDuration() != null)) {
+        knownSuites.get(parent).add(testCase);
       }
     }
 
@@ -153,18 +159,19 @@ private Map<TestData, List<TestData>> matchTestCasesToSuites_locked(
   // This is really just documentation until someone actually turns on a static analyser.
   // If they do, we can decide whether we want to pick up the dependency.
   // @GuardedBy("resultsLock")
-  private List<TestData> findTestCases_locked() {
+  private List<TestData> findTestCases_locked(String engineId) {
     return results.values().stream()
         // Ignore test plan roots. These are always the engine being used.
         .filter(result -> !testPlan.getRoots().contains(result.getId()))
         .filter(
-            result -> {
-              // Find the test results we will convert to `testcase` entries. These
-              // are identified by the fact that they have no child test cases in the
-              // test plan, or they are marked as tests.
-              TestIdentifier id = result.getId();
-              return id.getSource() != null || id.isTest() || testPlan.getChildren(id).isEmpty();
-            })
+            result ->
+                engineId == null
+                    || result
+                        .getId()
+                        .getUniqueIdObject()
+                        .getEngineId()
+                        .map(engineId::equals)
+                        .orElse(false))
         .collect(Collectors.toList());
   }
 
@@ -196,20 +203,38 @@ private void outputIfTestRootIsComplete(TestIdentifier testIdentifier) {
       return;
     }
 
-    output(false);
+    output(false, testIdentifier.getUniqueIdObject().getEngineId().orElse(null));
   }
 
-  private void output(boolean includeIncompleteTests) {
+  private void output(boolean includeIncompleteTests, /*@Nullable*/ String engineId) {
     synchronized (this.resultsLock) {
-      List<TestData> testCases = findTestCases_locked();
+      List<TestData> testCases = findTestCases_locked(engineId);
       Map<TestData, List<TestData>> testSuites =
           matchTestCasesToSuites_locked(testCases, includeIncompleteTests);
 
       // Write the results
       try {
         for (Map.Entry<TestData, List<TestData>> suiteAndTests : testSuites.entrySet()) {
-          new TestSuiteXmlRenderer(testPlan)
-              .toXml(xml, suiteAndTests.getKey(), suiteAndTests.getValue());
+          TestData suite = suiteAndTests.getKey();
+          List<TestData> tests = suiteAndTests.getValue();
+          if (suite.getResult() != null
+              && suite.getResult().getStatus() != TestExecutionResult.Status.SUCCESSFUL) {
+            // If a test suite fails or is skipped, all its tests must be included in the XML output
+            // with the same result as the suite, since the XML format does not support marking a
+            // suite as failed or skipped. This aligns with Bazel's XmlWriter for JUnitRunner.
+            getTestsFromSuite(suite.getId())
+                .forEach(
+                    testIdentifier -> {
+                      TestData test = results.get(testIdentifier.getUniqueIdObject());
+                      if (test == null) {
+                        // add test to results.
+                        test = getResult(testIdentifier);
+                        tests.add(test);
+                      }
+                      test.mark(suite.getResult()).skipReason(suite.getSkipReason());
+                    });
+          }
+          new TestSuiteXmlRenderer(testPlan).toXml(xml, suite, tests);
         }
       } catch (XMLStreamException e) {
         throw new RuntimeException(e);
@@ -227,6 +252,18 @@ private void output(boolean includeIncompleteTests) {
     }
   }
 
+  private List<TestIdentifier> getTestsFromSuite(TestIdentifier suiteIdentifier) {
+    return testPlan.getChildren(suiteIdentifier).stream()
+        .flatMap(
+            testIdentifier -> {
+              if (testIdentifier.isContainer()) {
+                return getTestsFromSuite(testIdentifier).stream();
+              }
+              return Stream.of(testIdentifier);
+            })
+        .collect(Collectors.toList());
+  }
+
   @Override
   public void reportingEntryPublished(TestIdentifier testIdentifier, ReportEntry entry) {
     getResult(testIdentifier).addReportEntry(entry);
@@ -248,7 +285,7 @@ public void close() {
       return;
     }
     if (wasInterrupted.get()) {
-      output(true);
+      output(true, null);
     }
     try {
       xml.writeEndDocument();
@@ -257,4 +294,41 @@ public void close() {
       LOG.log(Level.SEVERE, "Unable to close xml output", e);
     }
   }
+
+  static class LazyFileWriter extends Writer {
+    private final Path path;
+    private BufferedWriter delegate;
+    private boolean isCreated = false;
+
+    public LazyFileWriter(Path path) {
+      this.path = path;
+    }
+
+    private void createWriterIfNeeded() throws IOException {
+      if (!isCreated) {
+        delegate = Files.newBufferedWriter(path, UTF_8);
+        isCreated = true;
+      }
+    }
+
+    @Override
+    public void write(char[] cbuf, int off, int len) throws IOException {
+      createWriterIfNeeded();
+      delegate.write(cbuf, off, len);
+    }
+
+    @Override
+    public void flush() throws IOException {
+      if (isCreated) {
+        delegate.flush();
+      }
+    }
+
+    @Override
+    public void close() throws IOException {
+      if (isCreated) {
+        delegate.close();
+      }
+    }
+  }
 }

java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/StubbedTestDescriptor.java

@@ -11,9 +11,15 @@
 public class StubbedTestDescriptor implements TestDescriptor {
 
   private final UniqueId uniqueId;
+  private final Type type;
 
   public StubbedTestDescriptor(UniqueId uniqueId) {
+    this(uniqueId, Type.TEST);
+  }
+
+  public StubbedTestDescriptor(UniqueId uniqueId, Type type) {
     this.uniqueId = uniqueId;
+    this.type = type;
   }
 
   @Override
@@ -68,7 +74,7 @@ public void removeFromHierarchy() {
 
   @Override
   public Type getType() {
-    return Type.TEST;
+    return type;
   }
 
   @Override

java/test/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListenerTest.java

@@ -8,6 +8,7 @@
 import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
 import static org.junit.platform.launcher.LauncherConstants.STDERR_REPORT_ENTRY_KEY;
 import static org.junit.platform.launcher.LauncherConstants.STDOUT_REPORT_ENTRY_KEY;
+import static org.mockito.Mockito.when;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.File;
@@ -17,8 +18,10 @@
 import java.io.StringWriter;
 import java.io.Writer;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.Collection;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
@@ -28,6 +31,7 @@
 import javax.xml.stream.XMLStreamWriter;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.TestExecutionResult;
 import org.junit.platform.engine.UniqueId;
@@ -258,6 +262,57 @@ public void interruptedTestsAreMarkedAsFailed() {
     assertEquals("Test timed out and was interrupted", failure.getTextContent());
   }
 
+  @Test
+  void testSuiteError(@TempDir Path tempDir)
+      throws ParserConfigurationException, IOException, SAXException {
+    TestIdentifier root =
+        TestIdentifier.from(new StubbedTestDescriptor(UniqueId.parse("[engine:mocked]")));
+    TestIdentifier suiteIdentifier =
+        TestIdentifier.from(
+            new StubbedTestDescriptor(
+                UniqueId.parse("[engine:mocked]/[class:ExampleTest]"),
+                TestDescriptor.Type.CONTAINER));
+    TestData suite =
+        new TestData(suiteIdentifier)
+            .started()
+            .mark(TestExecutionResult.failed(new RuntimeException("test suite error")));
+    TestPlan testPlan = Mockito.mock(TestPlan.class);
+
+    TestIdentifier child1 = TestIdentifier.from(new StubbedTestDescriptor(createId("child1")));
+    TestIdentifier child2 = TestIdentifier.from(new StubbedTestDescriptor(createId("child2")));
+
+    when(testPlan.getChildren(suite.getId())).thenReturn(Set.of(child1, child2));
+    when(testPlan.getRoots()).thenReturn(Set.of(root));
+
+    Path xmlPath = tempDir.resolve("test.xml");
+    try (BazelJUnitOutputListener listener = new BazelJUnitOutputListener(xmlPath)) {
+      listener.testPlanExecutionStarted(testPlan);
+      listener.executionStarted(root);
+      listener.executionStarted(suiteIdentifier);
+      listener.executionFinished(
+          suiteIdentifier, TestExecutionResult.failed(new RuntimeException("test suite error")));
+      listener.executionFinished(root, TestExecutionResult.successful());
+    }
+
+    Document document =
+        DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(xmlPath.toFile());
+    document.getDocumentElement().normalize();
+
+    NodeList testsuiteList = document.getDocumentElement().getElementsByTagName("testsuite");
+    assertEquals(1, testsuiteList.getLength());
+    Element testsuite = (Element) testsuiteList.item(0);
+    assertEquals("2", testsuite.getAttribute("tests"));
+    assertEquals("2", testsuite.getAttribute("failures"));
+
+    NodeList testcaseList = testsuite.getElementsByTagName("testcase");
+    assertEquals(2, testcaseList.getLength());
+
+    for (int i = 0; i < testcaseList.getLength(); i++) {
+      Element testcase = (Element) testcaseList.item(i);
+      assertNotNull(getChild("failure", testcase));
+    }
+  }
+
   @Test
   void throwablesWithNullMessageAreSerialized() {
     var test = new TestData(identifier).mark(TestExecutionResult.failed(new Throwable()));