fix(reporters): unwrap TestFailedException for failure grouping

[thomhurst]

Summary

• GitHub / HTML / JUnit reporters grouped every failure as TestFailedException because SimplifyStacktrace wraps the real exception in a TestFailedException to strip TUnit-internal frames from the console stack trace. Quick diagnosis and Failures by Cause both showed the wrapper type instead of e.g. InvalidOperationException.
• Added WrappedException property + static Unwrap helper on TUnitFailedException. Unwrap is null-tolerant ([NotNullIfNotNull]) and loops to handle wrapper-of-wrapper.
• GitHubReporter (GetExceptionTypeName, GetError), HtmlReporter.MapException, and JUnitXmlWriter (WriteFailure, WriteError) now unwrap before reading type / message / stack. MTP's built-in console reporter still receives the wrapper, so filtered stack traces are preserved.

Test plan

• New test AfterRunAsync_Unwraps_TestFailedException_For_Grouping verifies grouping and Quick diagnosis use the inner exception type.
• All 14 existing GitHubReporterTests pass.
• All 15 StackTraceFilterTests pass (no regression in wrapper behaviour).
• TUnit.Engine builds clean across netstandard2.0;net8.0;net9.0;net10.0.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

This is a clean, well-scoped fix for a real UX problem. The root cause (reporters reading type/message from the TUnitFailedException wrapper rather than the original exception) is clearly diagnosed, and the solution is appropriately targeted.

What's done well

• The while loop in Unwrap() correctly handles potential chains of wrappers (future-proof).
• [NotNullIfNotNull] is applied correctly — the static signature accurately communicates the null-preservation guarantee.
• The test AfterRunAsync_Unwraps_TestFailedException_For_Grouping directly validates the original bug (grouping key and Quick Diagnosis both show the real type), and the existing 14 reporter tests confirm no regression.
• The split between MTP's console reporter (keeps the wrapper for filtered stack traces) and the file reporters (unwrap to the real exception) is a sound design decision that's explained in the PR summary.

---

Issues worth discussing

1. Stack trace behavior change in JUnit/HTML (medium)

In JUnitXmlWriter.WriteFailure and WriteError, the writer calls exception.ToString() after unwrapping:

var exception = TUnitFailedException.Unwrap(failed.Exception);
// ...
writer.WriteString(SanitizeForXml(exception.ToString()));

exception.ToString() on the original exception includes its full, unfiltered stack trace — including TUnit-internal frames that TUnitFailedException.FilterStackTrace was specifically stripping. Previously these reporters saw only the filtered trace; now they see the raw one.

For CI tooling (GitLab/Jenkins consuming JUnit XML) and HTML reports, having the full stack trace is arguably better — developers drilling into a report want all the frames. But this is a silent behavior change worth explicitly acknowledging in the PR description or a comment near the unwrap call, since the original intent of FilterStackTrace was to reduce noise.

2. WrappedException naming could be clearer

WrappedException is grammatically ambiguous — it could mean "the exception that is wrapped" or "the exception that wraps something". The held value is the original exception that TUnitFailedException encapsulates, so OriginalException or InnerException (modulo the existing InnerException semantics) would remove any guesswork. Minor, but API names on a public abstract class matter.

3. Test coverage is incomplete for HTML and JUnit reporters

The new test validates GitHubReporter grouping, but HtmlReporter.MapException and JUnitXmlWriter.WriteFailure/WriteError have no dedicated tests for the unwrap behavior. If someone later refactors those code paths and drops the Unwrap call, there's no safety net. A couple of focused unit tests for those writers (similar in style to the new GitHubReporterTests test) would close this gap.

4. Hardcoded fallback type in WriteFailure/WriteError is now misleading (pre-existing, surfaced by this PR)

// When exception == null after Unwrap:
writer.WriteAttributeString("type", "TestFailedException");

Before this PR, a null exception was rare (only when the property had no exception). That path now carries an inaccurate type literal — if failed.Exception is null, attributing it to "TestFailedException" is wrong. The fallback would read better as "UnknownException" or just "Unknown". This predates the PR but is now more visible.

---

Summary

The core fix is correct and the approach is sound. The main things worth addressing before merge:

1. Decide whether the unfiltered stack trace in JUnit/HTML output is intentional, and document it if so.
2. Add at least minimal tests for HtmlReporter and JUnitXmlWriter unwrap paths.
3. Consider renaming WrappedException → OriginalException for clarity (optional, given it's on a public type).