fix: smarter stack trace filtering that preserves TUnit-internal traces

[thomhurst]

Summary

• Preserves full stack trace when all frames are TUnit-internal — if TUnit itself has a bug, the trace is no longer replaced with an empty string
• Keeps user frames that appear after TUnit frames — e.g. when assertion internals are at the top, the user's test method below them is no longer lost
• Uses at TUnit. (with dot) to avoid false positives on user namespaces that happen to start with "TUnit"
• Appends a hint about --detailed-stacktrace when frames are omitted, so users know how to get the full trace

Before

The filter walked top-to-bottom and breaked at the first at TUnit line, discarding it and everything after. This meant:

• TUnit bugs → empty stack trace
• Assertion failures → user's test method frame lost (assertion internals at top triggered the break)

After

The filter removes TUnit frames surgically (skip + continue), preserving all user frames regardless of position. When every frame is TUnit-internal, the full trace is returned unmodified.

Test plan

• New StackTraceFilterTests covering all scenarios (9 tests)
• Existing TestExecutionExceptionTests still pass
• CI passes

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 24 complexity

Metric	Results
Complexity	24

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

This is a solid fix that addresses three distinct bugs in the original stack trace filtering logic. The test coverage is thorough and the reasoning in the comments is clear. I have one actionable concern and a couple of minor observations.

---

Hardcoded CLI flag name in hint text (actionable)

File: TUnit.Engine/Exceptions/TUnitFailedException.cs:72

vsb.Append("   --- TUnit internals omitted (run with --detailed-stacktrace for full trace) ---");

The string "detailed-stacktrace" is hardcoded here, but the constant already exists one namespace away:

// TUnit.Engine/CommandLineProviders/DetailedStacktraceCommandProvider.cs
public const string DetailedStackTrace = "detailed-stacktrace";

If the option name is ever changed (or the legacy flag is deprecated in favour of the verbosity system — which TUnitMessageBus already partially supports via ShowDetailedStackTrace), this hint will silently diverge and mislead users. Prefer referencing the constant:

vsb.Append($"   --- TUnit internals omitted (run with --{DetailedStacktraceCommandProvider.DetailedStackTrace} for full trace) ---");

Also worth noting: TUnitMessageBus.SimplifyStacktrace bypasses the filter when verbosityService?.ShowDetailedStackTrace == true (i.e. debug logging), but the hint only mentions the --detailed-stacktrace flag. Not a correctness bug, but a minor UX gap for users already using verbose logging.

---

Non-frame lines set hasUserFrame = true (minor, possibly intentional)

In the first pass, lines that are neither TUnit frames nor actual stack frames — such as --- End of stack trace from previous location --- — evaluate !IsTUnitInternalFrame(trimmed) as true and set hasUserFrame = true. This means a trace consisting purely of TUnit frames separated by async infrastructure lines will run through the filter (stripping TUnit frames, keeping the separator, adding the hint) instead of being returned wholesale.

The AsyncInfrastructureLines_ArePreserved test confirms the separator is kept correctly, so the observable behaviour is fine. But it's worth being aware that the "all TUnit → return full trace" path is never triggered when async infrastructure lines are present between TUnit frames. If this is intentional, a comment would help future readers.

---

Two-pass traversal is the right call here

The two-pass approach avoids buffering all frames into an intermediate list, which would undermine the ValueStringBuilder/stackalloc allocation strategy. Good choice.

---

Test coverage

The 9 new tests cover the important scenarios well, including the tricky case of assertion internals appearing above the user's test method, and the false-positive guard on TUnitExtensions. No gaps that would let a regression slip through unnoticed.

---

Summary: The fix is correct and the approach is clean. The only actionable item is referencing DetailedStacktraceCommandProvider.DetailedStackTrace instead of the inlined string literal.

[claude]

Code Review: fix: smarter stack trace filtering that preserves TUnit-internal traces

Overall assessment: Good, targeted bug fix with solid test coverage. Minor suggestions below.

The core logic is correct and the PR successfully addresses the described bugs.

---

Correctness

The logic is correct. Tracing through all 9 test scenarios manually, the hasUserFrame/omittedAny guard pair correctly handles:

• User frames before/after TUnit frames
• All-TUnit-frame traces (returns unfiltered)
• No-TUnit-frame traces (returns unfiltered, no hint)
• Async separator lines (preserved, don't count as "user frames" for the !hasUserFrame early-return check)

One edge case not tested: When async separator lines appear between a leading block of TUnit frames and user frames:

at TUnit.Assertions.Check()                        <- TUnit, skipped
--- End of stack trace from previous location ---  <- separator, kept
at MyApp.Tests.MyTest()                            <- user frame, kept
at TUnit.Core.RunHelpers.RunAsync()                <- TUnit, skipped

Output would be:

--- End of stack trace from previous location ---
   at MyApp.Tests.MyTest()
   --- TUnit internals omitted (run with --detailed-stacktrace for full trace) ---

The separator is orphaned at the top without any frames above it. This isn't a regression, but the output reads strangely. Adding a test for this case would document the known behavior.

---

Performance: Hint string allocation

In TUnit.Engine/Exceptions/TUnitFailedException.cs:

vsb.Append($"   --- TUnit internals omitted (run with --{DetailedStacktraceCommandProvider.DetailedStackTrace} for full trace) ---");

This string interpolation allocates a new heap string on every call where frames are omitted. Since DetailedStacktraceCommandProvider.DetailedStackTrace is a const string, this can be computed at compile time:

private const string OmittedHint =
    $"   --- TUnit internals omitted (run with --{DetailedStacktraceCommandProvider.DetailedStackTrace} for full trace) ---";

With LangVersion=preview (C# 11+), const interpolated strings with only const operands are fully supported. This eliminates the runtime allocation on every filtered failure and embeds the hint as a literal in the assembly.

---

private → internal visibility change

FilterStackTrace was changed to internal static to allow StackTraceFilterTests to call it directly. Since TUnit.Engine.Tests is already in InternalsVisibleTo in TUnit.Engine.csproj, this is safe and consistent with the existing pattern in the codebase. No issue here.

---

Missing hint for --log-level Debug/Trace

In TUnitMessageBus.SimplifyStacktrace, filtering is bypassed when either --detailed-stacktrace or verbosityService?.ShowDetailedStackTrace == true (triggered by --log-level Debug/Trace). The hint appended by FilterStackTrace only mentions --detailed-stacktrace. This is technically accurate for the context in which it appears, but a code comment in FilterStackTrace explaining that it runs only from the console path (not during debug logging) would help future maintainers.

---

Suggested test coverage addition

Consider adding a test for the exact hint formatting to guard against regressions:

[Test]
public void FilteredTrace_HintHasCorrectFormat()
{
    var stackTrace = string.Join(Environment.NewLine,
        "   at MyApp.Tests.MyTest() in C:\\src\\Tests.cs:line 15",
        "   at TUnit.Core.RunHelpers.RunAsync()");

    var result = TUnitFailedException.FilterStackTrace(stackTrace);

    // Exactly: "<user frame>\n   --- TUnit internals omitted ... ---"
    // Not a double newline before the hint
    var lines = result.Split(Environment.NewLine);
    lines.Length.ShouldBe(2);
    lines[0].ShouldContain("MyApp.Tests.MyTest");
    lines[1].ShouldContain("--detailed-stacktrace");
}

---

Summary

Area	Finding	Severity
Core logic correctness	All traced scenarios produce correct output	No issue
Orphaned separator edge case	Separator appears before user frames when TUnit frames precede it in output	Minor cosmetic, not a regression
Hint string allocation	String interpolation allocates on every filtered failure — use const interpolated string instead	Minor perf
--log-level Debug not mentioned in hint	Hint only mentions --detailed-stacktrace; accurate for context but worth a comment	Documentation gap
Test coverage	Missing test for async-separator-at-top edge case and exact hint formatting	Minor

The changes are sound and represent a clear improvement over the previous break-on-first-TUnit-frame approach. The bug fix for lost user frames after assertion internals is the most impactful change and is correctly implemented.

[claude]

Code Review

This is a well-considered fix to a genuinely broken stack trace filter. The before/after behavior is accurately described in the PR summary, and the implementation handles the edge cases thoughtfully.

What Works Well

Algorithm correctness: The surgical skip-and-continue approach is the right model here. The original break-on-first-TUnit-frame was fundamentally broken for assertion-first traces.

"at TUnit." with dot: Smart precision. Avoids false positives on user types like TUnitExtensions.MyHelper that happened to share a prefix with TUnit internals.

vsb.Dispose() on early return (line 82): Correct and important — the ValueStringBuilder may have rented from ArrayPool<char> if it grew beyond the 1024-char stack-allocated buffer. ToString() calls Dispose() internally (confirmed in ValueStringBuilder.cs:85), so the happy path is fine too.

Test coverage: 9 focused unit tests covering the main scenarios — all TUnit frames, all user frames, interleaved, async separators, and the orphaned-separator edge case. Making FilterStackTrace internal to enable direct unit testing is a pragmatic call.

---

Issues and Suggestions

1. Trailing async separator before the hint (edge case, worth considering)

When the trace ends like this — a user frame, then an async separator, then a TUnit frame — the separator slips through because added is already true:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   at TUnit.Core.RunHelpers.RunAsync()

Output:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   --- TUnit internals omitted (run with --detailed-stacktrace for full trace) ---

Two "separator-like" lines back-to-back is a bit visually confusing. One option: only emit separator lines if they're followed by at least one user frame. That requires a lookahead (or a second pass), which may not be worth the complexity. At minimum, this should have a dedicated test case so any future change doesn't silently regress it.

2. Async separator between filtered TUnit frames leaks context

   at MyApp.First()
   at TUnit.Core.Something()          ← stripped
--- End of stack trace from previous location ---
   at MyApp.Second()
   at TUnit.Engine.Executor()          ← stripped

Output:

   at MyApp.First()
--- End of stack trace from previous location ---
   at MyApp.Second()
   --- TUnit internals omitted ...

The separator now implies First() and Second() have an async boundary between them — but the boundary was really between TUnit.Core.Something() and MyApp.Second(). This is slightly misleading. Again, fixing it properly requires lookahead, so whether it warrants addressing is a judgment call. But a test documenting the current behavior would prevent silent regressions.

3. Stack buffer size bump (minor)

Increased from 256 → 1024 chars. For deep traces (very common) that exceed 1024 chars, a heap allocation happens anyway. That said, 1 KB extra on the stack per test failure is totally fine in practice. No action needed, just worth noting.

---

Missing Test Cases

Scenario	Coverage
Separator before hint (see issue 1)	Missing
Separator between filtered TUnit frames (see issue 2)	Missing

---

Summary

The core fix is correct and a clear improvement over the previous break-on-first-TUnit behavior. The FilterStackTrace algorithm handles all the documented scenarios properly, and the ValueStringBuilder lifecycle is managed correctly. The two edge cases above are both low-severity and involve async separator placement — they don't affect correctness for the primary use case. Adding test cases documenting those behaviors would make this bulletproof.

[claude]

Code Review (follow-up)

This is a follow-up review against the previous two rounds of feedback. All previously flagged issues have been addressed — this is in excellent shape.

---

Previous feedback: all resolved ✅

Issue	Status
Hardcoded CLI flag name in hint — should reference	✅ Fixed — is now a private const string compiled at build time, zero allocation per filtered failure
Async infrastructure lines incorrectly counting as user frames	✅ Fixed — the only count actual stack frames commit ensures only "at "-prefixed lines affect the guard
Orphaned separator at top of output when leading TUnit frames are stripped	✅ Fixed — the !added && !isFrame guard skips non-frame lines until at least one frame has been emitted
Missing test for orphaned separator	✅ Added: OrphanedSeparator_AfterStrippedTUnitFrame_IsRemoved
Missing test for exact hint format	✅ Added: FilteredTrace_HintHasCorrectFormat
hasUserFrame variable incorrectly set by separator lines	✅ Dropped — added is now the single source of truth, which is correct given the !added && !isFrame gate

---

One remaining edge case (minor, cosmetic)

The "trailing separator before hint" scenario from the previous review round is still untested. When the trace ends with a user frame → async separator → TUnit frame:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   at TUnit.Core.RunHelpers.RunAsync()

The current output is:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   --- TUnit internals omitted (run with --detailed-stacktrace for full trace) ---

The separator before the hint reads a little oddly (implying there were frames after it that were elided). This is distinct from the leading separator case (which is now correctly handled). Stripping trailing separators would require lookahead, which isn't worth the added complexity — but a test documenting the known behavior would lock in the current output and prevent silent future regressions.

The existing AsyncInfrastructureLines_ArePreserved test places the separator between two user frames, not at the end, so it doesn't cover this case.

---

Fast path (new addition — looks good)

if (span.IndexOf("at TUnit.".AsSpan()) < 0)
    return stackTrace;

This is a clean constant-time optimization for the common case where there are no TUnit frames at all. The "at TUnit." substring check can false-positive on exception messages containing that text, but the downstream !omittedAny guard in the main loop handles that correctly — it returns the original stackTrace unmodified. No issue.

---

Summary

The implementation is correct, well-tested, and all previous actionable feedback has been addressed. The only remaining suggestion is a test for the trailing-separator edge case — worth adding but not a blocker. The core fix (surgical skip-and-continue vs. the original break-on-first-TUnit-frame) is solid and the ValueStringBuilder lifecycle is managed correctly throughout.

[claude]

Code Review (follow-up)

This is a follow-up review against the previous two rounds of feedback. All previously flagged issues have been addressed — this is in excellent shape.

---

Previous feedback: all resolved

Issue	Status
Hardcoded CLI flag name in hint — should reference DetailedStacktraceCommandProvider.DetailedStackTrace	Fixed — OmittedHint is now a private const string compiled at build time, zero allocation per filtered failure
Async infrastructure lines incorrectly counting as user frames	Fixed — the "only count actual stack frames" commit ensures only "at "-prefixed lines affect the guard
Orphaned separator at top of output when leading TUnit frames are stripped	Fixed — the !added && !isFrame guard skips non-frame lines until at least one frame has been emitted
Missing test for orphaned separator	Added: OrphanedSeparator_AfterStrippedTUnitFrame_IsRemoved
Missing test for exact hint format	Added: FilteredTrace_HintHasCorrectFormat
hasUserFrame variable incorrectly set by separator lines	Dropped — added is now the single source of truth, correct given the !added && !isFrame gate

---

One remaining edge case (minor, cosmetic)

The "trailing separator before hint" scenario from the previous review round is still untested. When the trace ends with a user frame → async separator → TUnit frame:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   at TUnit.Core.RunHelpers.RunAsync()

The current output is:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   --- TUnit internals omitted (run with --detailed-stacktrace for full trace) ---

The separator before the hint reads a little oddly — it implies there were frames after it that were elided. This is distinct from the leading separator case (which is now correctly handled). Stripping trailing separators would require lookahead, which isn't worth the added complexity — but a test documenting the known behavior would lock in the current output and prevent silent future regressions.

The existing AsyncInfrastructureLines_ArePreserved test places the separator between two user frames, not at the end, so it doesn't cover this case.

---

Fast path (new addition)

if (span.IndexOf("at TUnit.".AsSpan()) < 0)
    return stackTrace;

Clean constant-time optimization for the common case where there are no TUnit frames at all. The "at TUnit." substring check can false-positive on exception messages containing that text, but the downstream !omittedAny guard in the main loop handles that correctly — it returns the original stackTrace unmodified. No issue.

---

Summary

The implementation is correct, well-tested, and all previous actionable feedback has been addressed. The only remaining suggestion is a test for the trailing-separator edge case — worth adding but not a blocker. The core fix (surgical skip-and-continue vs. the original break-on-first-TUnit-frame) is solid and the ValueStringBuilder lifecycle is managed correctly throughout.

[claude]

Code Review (round 4 — final follow-up)

All previous feedback has been addressed. Checking the one remaining open item from round 3:

---

Previous feedback: all resolved ✅

Issue	Status
Hardcoded CLI flag name in hint	✅ Fixed (rounds 1–2)
Async infrastructure lines counting as user frames	✅ Fixed (rounds 1–2)
Orphaned separator at top when leading TUnit frames stripped	✅ Fixed (rounds 2–3)
Trailing separator before hint not tested	✅ Fixed in this round — see below

---

Trailing separator: now fixed (not just tested)

The round-3 feedback noted this output was odd when a separator preceded a stripped TUnit frame at the end of the trace:

   at MyApp.Tests.MyTest() in C:\src\Tests.cs:line 15
--- End of stack trace from previous location ---
   --- TUnit internals omitted (run with --detailed-stacktrace for full trace) ---

The current implementation addresses this correctly via the pendingSeparator/hasPendingSeparator deferred-emit strategy. Separators are only flushed when a surviving user frame follows — so a separator that precedes only stripped TUnit frames is silently dropped. Tracing through the new algorithm for the case above:

1. at MyApp.Tests.MyTest() → emitted, added = true, lastWasStripped = false
2. --- End of stack trace --- → buffered as pendingSeparator
3. at TUnit.Core.RunHelpers.RunAsync() → stripped; clears hasPendingSeparator, lastWasStripped = true
4. End of loop: added && omittedAny → append hint only

Output: at MyApp.Tests.MyTest() ...\n --- hint --- ✓

The three new tests cover every separator scenario precisely:

• TrailingSeparator_BeforeOnlyStrippedFrames_IsRemoved — exactly the edge case flagged in round 3
• Separator_BetweenStrippedTUnitAndUserFrame_IsRemoved — separator whose "before" anchor is gone
• Separator_BetweenTwoUserFrames_IsPreserved — separator with both anchors surviving, must be kept

---

No new concerns

The pendingSeparator mechanism is clean and handles all edge cases without lookahead. The lastWasStripped and hasPendingSeparator state interact correctly — separators are not silently promoted to "user frame territory" after a stripping event. The ValueStringBuilder lifecycle (including the vsb.Dispose() early-return path) is correct throughout.

---

This PR is ready to merge. All three root bugs from the original filter are fixed, all previously raised concerns are addressed, and the test suite comprehensively locks in the correct behavior.

[claude]

Follow-up Review

This is a follow-up to my previous review. All items I raised have been addressed.

---

Previous issues — resolved

Item	Status
Hint string allocation (use const interpolated string)	✅ Fixed — OmittedHint is now a private const string
Missing comment about --log-level Debug/Trace bypassing the hint	✅ Fixed — comment now explains the TUnitMessageBus.SimplifyStacktrace callsite
Orphaned separator edge case missing test	✅ Fixed — OrphanedSeparator_AfterStrippedTUnitFrame_IsRemoved covers exactly this case
Missing hint format regression test	✅ Fixed — FilteredTrace_HintHasCorrectFormat verifies exact line count and content

---

New separator tests are well-designed

The four separator edge-case tests added in this revision (OrphanedSeparator_..., TrailingSeparator_..., Separator_BetweenStripped..., Separator_BetweenTwoUserFrames_...) cover the full decision matrix for separator placement. I traced through the algorithm for each and the implementation is correct in all cases.

One case not explicitly tested (separator that comes before a stripped TUnit frame, i.e. User → separator → TUnit → User):

   at MyApp.First()
--- End of stack trace from previous location ---
   at TUnit.Core.SomeHelper.DoSomething()
   at MyApp.Second()

The code handles it correctly — when the TUnit frame is processed it clears hasPendingSeparator, discarding the buffered separator. This is a nice-to-have test but not a gap that needs addressing.

---

Implementation quality

The ValueStringBuilder lifecycle is handled correctly — vsb.Dispose() is called on the early-exit path, and vsb.ToString() on the normal path. The fast-path span scan (IndexOf("at TUnit."))) remains a worthwhile optimization. The IsTUnitInternalFrame helper keeps the filter criterion in one place with a clear comment.

---

This PR is in excellent shape. All previous feedback has been addressed, the separator edge cases are now thoroughly tested, and the implementation is correct and performant. Ready to merge.