fix(testresult): add skipped message to <skipped> not <failure> (#189)

Currently skipped tests with messages return
`<skipped></skipped><failure message="...">`, but all the other examples
of JUnit I can find return `<skipped message="...">`.

The inclusion of both a `<skipped>` and `<failure>` confuses parsers,
making them think that the test failed when it was actually skipped.

Skipping tests was added in
#37.

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

@@ -28,7 +28,9 @@ public TestResult(TestPlan testPlan, TestIdentifier id, boolean isDynamic) {
 
   public boolean isError() {
     TestExecutionResult result = getResult();
-    if (result == null || result.getStatus() == TestExecutionResult.Status.SUCCESSFUL) {
+    if (result == null
+        || result.getStatus() == TestExecutionResult.Status.SUCCESSFUL
+        || isSkipped()) {
       return false;
     }
 
@@ -37,7 +39,9 @@ public boolean isError() {
 
   public boolean isFailure() {
     TestExecutionResult result = getResult();
-    if (result == null || result.getStatus() == TestExecutionResult.Status.SUCCESSFUL) {
+    if (result == null
+        || result.getStatus() == TestExecutionResult.Status.SUCCESSFUL
+        || isSkipped()) {
       return false;
     }
 
@@ -81,28 +85,12 @@ public void toXml(XMLStreamWriter xml) {
 
           if (isDisabled() || isSkipped()) {
             xml.writeStartElement("skipped");
+            writeThrowableMessage(xml);
             xml.writeEndElement();
           }
           if (isFailure() || isError()) {
-            Throwable throwable = getResult().getThrowable().orElse(null);
-
             xml.writeStartElement(isFailure() ? "failure" : "error");
-            if (throwable == null) {
-              // Stub out the values
-              xml.writeAttribute("message", "unknown cause");
-              xml.writeAttribute("type", RuntimeException.class.getName());
-              xml.writeEndElement();
-              return;
-            }
-
-            xml.writeAttribute(
-                "message", escapeIllegalCharacters(String.valueOf(throwable.getMessage())));
-            xml.writeAttribute("type", throwable.getClass().getName());
-
-            StringWriter stringWriter = new StringWriter();
-            throwable.printStackTrace(new PrintWriter(stringWriter));
-
-            xml.writeCData(escapeIllegalCharacters(stringWriter.toString()));
+            writeThrowableMessage(xml);
             xml.writeEndElement();
           }
 
@@ -124,6 +112,31 @@ public void toXml(XMLStreamWriter xml) {
         });
   }
 
+  private void writeThrowableMessage(XMLStreamWriter xml) {
+    write(
+        () -> {
+          Throwable throwable = null;
+          if (getResult() != null) {
+            throwable = getResult().getThrowable().orElse(null);
+          }
+          if (throwable == null) {
+            // Stub out the values
+            xml.writeAttribute("message", "unknown cause");
+            xml.writeAttribute("type", RuntimeException.class.getName());
+            return;
+          }
+
+          xml.writeAttribute(
+              "message", escapeIllegalCharacters(String.valueOf(throwable.getMessage())));
+          xml.writeAttribute("type", throwable.getClass().getName());
+
+          StringWriter stringWriter = new StringWriter();
+          throwable.printStackTrace(new PrintWriter(stringWriter));
+
+          xml.writeCData(escapeIllegalCharacters(stringWriter.toString()));
+        });
+  }
+
   @Override
   protected Optional<BaseResult> get(TestIdentifier id) {
     return getTestId().equals(id) ? Optional.of(this) : Optional.empty();

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

@@ -84,6 +84,42 @@ public void skippedTestsAreNotDisabled() {
     result.markFinished(testExecutionResult);
 
     assertFalse(result.isDisabled());
+    assertTrue(result.isSkipped());
+  }
+
+  @Test
+  public void skippedTestsAreNotFailures() {
+    TestPlan testPlan = Mockito.mock(TestPlan.class);
+
+    TestResult result = new TestResult(testPlan, identifier, false);
+    TestExecutionResult testExecutionResult =
+        TestExecutionResult.aborted(new TestAbortedException("skipping is fun"));
+    result.markFinished(testExecutionResult);
+
+    assertTrue(result.isSkipped());
+    assertFalse(result.isFailure());
+    assertFalse(result.isError());
+    assertFalse(result.isDisabled());
+  }
+
+  @Test
+  public void skippedTestsContainMessages() {
+    var test = new TestResult(Mockito.mock(TestPlan.class), identifier, false);
+    test.markFinished(TestExecutionResult.aborted(new TestAbortedException("skipping is fun")));
+
+    var root = generateTestXml(test).getDocumentElement();
+    assertNotNull(root);
+    assertEquals("testcase", root.getTagName());
+
+    var skipped = root.getElementsByTagName("skipped");
+    assertEquals(1, skipped.getLength());
+
+    var failures = root.getElementsByTagName("failure");
+    assertEquals(0, failures.getLength());
+
+    var message = skipped.item(0).getAttributes().getNamedItem("message");
+    assertNotNull(message);
+    assertEquals("skipping is fun", message.getTextContent());
   }
 
   @Test
@@ -113,9 +149,9 @@ public void disabledTestsAreMarkedAsSkipped() {
     assertEquals(1, allTestCases.getLength());
     Node testCase = allTestCases.item(0);
 
-    boolean skippedSeen = containsChild("skipped", testCase);
+    Node skipped = getChild("skipped", testCase);
 
-    assertTrue(skippedSeen);
+    assertNotNull(skipped);
   }
 
   @Test
@@ -212,18 +248,18 @@ private Document generateTestXml(BaseResult result) {
     }
   }
 
-  private boolean containsChild(String childTagName, Node withinNode) {
+  private Node getChild(String childTagName, Node withinNode) {
     NodeList childNodes = withinNode.getChildNodes();
     for (int i = 0; i < childNodes.getLength(); i++) {
       Node node = childNodes.item(i);
       if (node.getNodeType() != Node.ELEMENT_NODE) {
         continue;
       }
       if (childTagName.equals(node.getNodeName())) {
-        return true;
+        return node;
       }
     }
-    return false;
+    return null;
   }
 
   private UniqueId createId(String testName) {