Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #4256

Copilot AI review requested due to automatic review settings January 7, 2026 20:26
@thomhurst
Copy link
Owner Author

Summary

Fixes duplicated output in detailed mode by conditionally adding StandardOutputProperty/StandardErrorProperty only when NOT in detailed output mode.

Critical Issues

None found ✅

Suggestions

1. Update EstimateCount to reflect conditional output properties

Location: TUnit.Engine/Extensions/TestExtensions.cs:154

The estimate count still adds 3 for Timing + StdOut + StdErr unconditionally, but now StdOut/StdErr are only added when NOT in detailed output mode. While this doesn't cause functional issues (just over-allocates the list capacity), it would be more accurate to:

if (isFinalState)
{
    count += 1; // Timing (always)
    if (!IsDetailedOutput(testContext))
    {
        count += 2; // StdOut + StdErr (only when not detailed)
    }
}

Why it matters: Accurate capacity estimation reduces allocations and follows the performance-first principle.

2. Consider thread safety for static cache

Location: TUnit.Engine/Extensions/TestExtensions.cs:14, 207

The new _cachedIsDetailedOutput static field follows the same pattern as _cachedIsTrxEnabled. Both use nullable booleans for caching without locks. This is generally safe since:

  • The cached value won't change during a test run
  • Race conditions would just result in duplicate reads, not incorrect values

However, if you ever want to be extra defensive, consider using Lazy<bool> or adding a comment explaining why thread safety isn't a concern here.

Technical Review

Logic correctness: The fix properly addresses the root cause - in detailed mode, output is shown in real-time by the platform, so including it again in properties causes duplication.

Follows TUnit patterns:

  • Proper caching pattern matching existing IsTrxEnabled
  • Appropriate use of VerbosityService
  • No allocations in hot path (cached lookup)

AOT compatible: No reflection issues introduced

No VSTest dependencies: Uses only Microsoft.Testing.Platform

Verdict

APPROVE - No critical issues. The suggestions are minor optimizations that don't block merging.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes issue #4256 where test output was duplicated when running in "Detailed" output mode. The root cause was that in detailed mode, the Microsoft.Testing.Platform already displays output in real-time, and TUnit was also adding it to the test node properties, resulting in duplicate display.

Key Changes

  • Added a public IsDetailedOutput property to VerbosityService to expose the detailed output mode flag
  • Modified test node creation logic to conditionally skip adding StandardOutputProperty and StandardErrorProperty when in detailed mode
  • Implemented a caching mechanism for the detailed output check to avoid repeated service lookups

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
TUnit.Engine/Services/VerbosityService.cs Exposed the internal _isDetailedOutput field via a new public property IsDetailedOutput to make it accessible to other components
TUnit.Engine/Extensions/TestExtensions.cs Added logic to check if detailed output mode is enabled and conditionally skip adding output properties to test nodes to prevent duplication; implemented caching for the detailed output check

Comment on lines +210 to +225
private static bool IsDetailedOutput(TestContext testContext)
{
if (_cachedIsDetailedOutput.HasValue)
{
return _cachedIsDetailedOutput.Value;
}

if (testContext.Services.GetService<VerbosityService>() is not {} verbosityService)
{
_cachedIsDetailedOutput = false;
return false;
}

_cachedIsDetailedOutput = verbosityService.IsDetailedOutput;
return _cachedIsDetailedOutput.Value;
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static field _cachedIsDetailedOutput is accessed and modified from multiple threads without synchronization, which could lead to a race condition. While the race is benign in this case (both threads would cache the same value), it violates thread-safety best practices and could theoretically result in the check being performed multiple times.

Consider using Volatile.Read and Volatile.Write, or initializing this value once during service provider initialization rather than lazy caching, similar to how the value is stored in VerbosityService itself. The IsTrxEnabled method has the same issue.

Copilot uses AI. Check for mistakes.
@thomhurst thomhurst merged commit 29e0de1 into main Jan 7, 2026
19 checks passed
@thomhurst thomhurst deleted the bug/4256 branch January 7, 2026 21:42
This was referenced Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: using dotnet run --output detailed repeats the log

2 participants