Include exception message in JUnit failure/error bodies

docs/RFCs/016-JUnit-Report.md

@@ -63,11 +63,11 @@ We target the **Jenkins/Surefire** JUnit XML flavor (the schema published at `je
         <property name="uid" value="..."/>
         <property name="trait.Category" value="..."/>
       </properties>
-      <skipped message="..."/>             <!-- 0..1 -->
-      <error message="..." type="..."/>    <!-- 0..n -->
-      <failure message="..." type="..."/>  <!-- 0..n -->
-      <system-out>...</system-out>         <!-- 0..n -->
-      <system-err>...</system-err>         <!-- 0..n -->
+      <skipped message="..."/>                  <!-- 0..1 -->
+      <error message="..." type="...">...</error>      <!-- 0..n -->
+      <failure message="..." type="...">...</failure>  <!-- 0..n -->
+      <system-out>...</system-out>              <!-- 0..n -->
+      <system-err>...</system-err>              <!-- 0..n -->
     </testcase>
     <system-out>...</system-out>
     <system-err>...</system-err>
@@ -130,17 +130,40 @@ The root `<testsuites>` `name` attribute is the module file name without extensi
 | MTP `TestNodeStateProperty`                    | JUnit element                                  |
 | ---------------------------------------------- | ---------------------------------------------- |
 | `PassedTestNodeStateProperty`                  | *(no child element)*                            |
-| `SkippedTestNodeStateProperty`                 | `<skipped message="..."/>` + reason as text     |
-| `FailedTestNodeStateProperty`                  | `<failure message="..." type="..."/>` + stack   |
-| `TimeoutTestNodeStateProperty`                 | `<error message="..." type="..."/>` + stack     |
-| `ErrorTestNodeStateProperty`                   | `<error message="..." type="..."/>` + stack     |
-| `CancelledTestNodeStateProperty` *(obsolete)*  | `<error message="..." type="..."/>` + stack     |
-| Other `WellKnownTestNodeTestRunOutcomeFailedProperties` | `<failure message="..." type="..."/>`  |
+| `SkippedTestNodeStateProperty`                 | `<skipped message="..."/>` *(no body)*          |
+| `FailedTestNodeStateProperty`                  | `<failure message="..." type="...">body</failure>` |
+| `TimeoutTestNodeStateProperty`                 | `<error message="..." type="...">body</error>`  |
+| `ErrorTestNodeStateProperty`                   | `<error message="..." type="...">body</error>`  |
+| `CancelledTestNodeStateProperty` *(obsolete)*  | `<error message="..." type="...">body</error>`  |
+| Other `WellKnownTestNodeTestRunOutcomeFailedProperties` | `<failure message="..." type="...">body</failure>` |
 | `DiscoveredTestNodeStateProperty`              | *(filtered out — not emitted)*                  |
 | `InProgressTestNodeStateProperty`              | *(filtered out — not emitted)*                  |
 
+`body` is the composed failure text described in [Failure and error body format](#failure-and-error-body-format); the element is written with explicit start/end tags whenever that text is non-empty.
+
 `Cancelled` becomes `<error>` rather than `<failure>` because cancellation indicates an interruption, not an assertion failure — `<error>` is the schema-correct bucket for "the test could not be evaluated".
 
+### Failure and error body format
+
+The body of `<failure>`/`<error>` mirrors the shape Java's `Throwable.printStackTrace()` produces, which is what genuine Surefire reports contain:
+
+```text
+System.InvalidOperationException: The expected condition was not met.
+   at MyProject.MyTests.MyTest()
+```
+
+That is, the exception type and message form a header line, followed by the stack trace. Neither the Ant/windyroad `JUnit.xsd` nor Surefire's `surefire-test-report.xsd` constrains this body — both declare it as free-form text — so the header line is schema-valid in every flavor.
+
+Writing the stack trace alone would **not** be equivalent to Java's output: .NET's `Exception.StackTrace` omits the leading `type: message` header that `Throwable.printStackTrace()` (and `Exception.ToString()`) include. Consumers that render the body rather than the `message` attribute — GitLab CI and CircleCI most notably — would then show a stack trace with no indication of *why* the test failed. This is especially damaging for fluent assertion libraries, whose stack traces are largely framework frames while the assertion message carries the actual diagnosis.
+
+The `message` and `type` attributes are still written, so consumers that read them directly are unaffected. The resulting duplication between the `message` attribute and the body is exactly what every Maven/Surefire report already exhibits, so consumers that render both have long handled it.
+
+Each part degrades gracefully: a missing exception type drops the header prefix, a missing message drops the `: ` separator, and a missing stack trace yields a header-only body.
+
+### The `type` attribute
+
+`type` is **always** emitted on `<failure>`/`<error>`. The Ant/windyroad `JUnit.xsd` marks it `use="required"` (Surefire relaxes it to optional), but MTP only supplies an exception type when the state property carried an actual `Exception` — frameworks that report a failure through `Explanation` alone leave it null. In that case the element name (`failure` or `error`) is used as the value, keeping the document valid under the stricter schema without inventing a bogus exception type name.
+
 ## Per-testcase metadata
 
 The `<properties>` block (emitted **first** inside `<testcase>`) carries:
@@ -278,3 +301,4 @@ A fresh GUID is generated for the `<TestingPlatformBuilderHook Include>` attribu
   - Parent-chain resolution with missing intermediate parents.
   - Counters at the suite and root level.
   - Outcome mapping (skipped/failed/error/timeout/cancelled).
+  - Failure/error body composition across every combination of present/absent exception type, message and stack trace, plus the `type` attribute fallback to the element name.

src/Platform/Microsoft.Testing.Extensions.JUnitReport/InternalAPI/InternalAPI.Unshipped.txt

@@ -11,3 +11,4 @@ Microsoft.Testing.Extensions.MergeOutputFileHelper
 static Microsoft.Testing.Extensions.MergeOutputFileHelper.BuildCaseFoldedProbePath(string! directory, string! probeFileName) -> string!
 static Microsoft.Testing.Extensions.MergeOutputFileHelper.EnsureOutputDoesNotAliasInput(System.Collections.Generic.IReadOnlyList<string!>! inputPaths, string! outputPath) -> void
 static Microsoft.Testing.Extensions.MergeOutputFileHelper.WriteViaTemporarySiblingAsync(string! outputPath, System.Func<string!, System.Threading.Tasks.Task!>! writeToTempAsync) -> System.Threading.Tasks.Task!
+static Microsoft.Testing.Extensions.JUnitReport.JUnitXmlWriter.BuildFailureBody(Microsoft.Testing.Extensions.JUnitReport.CapturedTestResult! result) -> string?

src/Platform/Microsoft.Testing.Extensions.JUnitReport/JUnitXmlWriter.cs

@@ -221,17 +221,66 @@ private static async Task WriteFailureOrErrorAsync(XmlWriter writer, string elem
             WriteAttribute(writer, "message", result.ErrorMessage!);
         }
 
-        if (!RoslynString.IsNullOrEmpty(result.ExceptionType))
+        // The Ant/windyroad JUnit.xsd marks `type` as use="required" (Surefire relaxes it to
+        // optional). MTP only gives us an exception type when the state property carried an
+        // actual Exception; frameworks that report a failure through `Explanation` alone leave
+        // it null. Fall back to the element name so the document stays valid under the stricter
+        // schema without inventing a bogus exception type name.
+        WriteAttribute(writer, "type", RoslynString.IsNullOrEmpty(result.ExceptionType) ? elementName : result.ExceptionType!);
+
+        string? body = BuildFailureBody(result);
+        if (!RoslynString.IsNullOrEmpty(body))
         {
-            WriteAttribute(writer, "type", result.ExceptionType!);
+            await writer.WriteStringAsync(XmlSafeText(body)).ConfigureAwait(false);
         }
 
-        if (!RoslynString.IsNullOrEmpty(result.StackTrace))
+        await writer.WriteEndElementAsync().ConfigureAwait(false);
+    }
+
+    // Mirrors the shape Java's Throwable.printStackTrace() produces, which is what genuine
+    // Surefire reports put in the <failure>/<error> body:
+    //
+    //     com.example.AssertionError: expected:<1> but was:<2>
+    //        at ...
+    //
+    // .NET's Exception.StackTrace omits that leading "type: message" header (only ToString()
+    // includes it), so writing StackTrace alone would drop the single most useful piece of
+    // diagnostic information. Consumers that render the body rather than the `message`
+    // attribute — GitLab and CircleCI most notably — would otherwise show only a stack trace.
+    // The `message` and `type` attributes are still written, so consumers reading those
+    // directly are unaffected by the duplication.
+    internal static string? BuildFailureBody(CapturedTestResult result)
+    {
+        bool hasExceptionType = !RoslynString.IsNullOrEmpty(result.ExceptionType);
+        bool hasErrorMessage = !RoslynString.IsNullOrEmpty(result.ErrorMessage);
+        bool hasStackTrace = !RoslynString.IsNullOrEmpty(result.StackTrace);
+
+        if (!hasExceptionType && !hasErrorMessage)
         {
-            await writer.WriteStringAsync(XmlSafeText(result.StackTrace)).ConfigureAwait(false);
+            return hasStackTrace ? result.StackTrace : null;
         }
 
-        await writer.WriteEndElementAsync().ConfigureAwait(false);
+        var builder = new StringBuilder();
+        if (hasExceptionType)
+        {
+            builder.Append(result.ExceptionType);
+            if (hasErrorMessage)
+            {
+                builder.Append(": ");
+            }
+        }
+
+        if (hasErrorMessage)
+        {
+            builder.Append(result.ErrorMessage);
+        }
+
+        if (hasStackTrace)
+        {
+            builder.Append('\n').Append(result.StackTrace);
+        }
+
+        return builder.ToString();
     }
 
     private static async Task WritePropertiesAsync(XmlWriter writer, TestCase tc)

test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/JUnitReportTests.cs

@@ -207,7 +207,7 @@ private static void AssertMixedOutcomesJUnitReportShape(string filePath)
         // fallback for the root passing test, then Container1 for its children).
         // Exit code is 2 (AtLeastOneTestFailed). The failure/error exceptions are
         // constructed but never thrown, so they have no stack trace and the
-        // <failure>/<error> elements are self-closing with just message/type.
+        // <failure>/<error> bodies contain only the "type: message" header line.
         const string expected = """
 <?xml version="1.0" encoding="utf-8"?>
 <testsuites name="JUnitReportTest" tests="4" failures="1" errors="1" skipped="1" time="<TIME>" timestamp="<TIMESTAMP>">
@@ -237,7 +237,7 @@ private static void AssertMixedOutcomesJUnitReportShape(string filePath)
         <property name="uid" value="test-fail" />
         <property name="testpath" value="Container1/FailingChild" />
       </properties>
-      <failure message="boom" type="System.InvalidOperationException" />
+      <failure message="boom" type="System.InvalidOperationException">System.InvalidOperationException: boom</failure>
     </testcase>
     <testcase name="SkippedChild" classname="Container1" time="<TIME>">
       <properties>
@@ -251,7 +251,7 @@ private static void AssertMixedOutcomesJUnitReportShape(string filePath)
         <property name="uid" value="test-error" />
         <property name="testpath" value="Container1/ErroredChild" />
       </properties>
-      <error message="kaboom" type="System.InvalidProgramException" />
+      <error message="kaboom" type="System.InvalidProgramException">System.InvalidProgramException: kaboom</error>
     </testcase>
   </testsuite>
 </testsuites>