Enhance DefaultLogger with extensibility features

[thomhurst]

No description provided.

[claude]

Code Review: Enhance DefaultLogger with extensibility features

Thank you for this PR! This is a well-thought-out enhancement that adds valuable extensibility points to the DefaultLogger class. Here's my detailed feedback:

---

✅ Strengths

1. Good Design: The refactoring properly follows the Template Method pattern, making the logger extensible without breaking existing functionality.

2. Comprehensive Documentation: Excellent addition of the logging documentation with clear examples showing how to use the new extension points.

3. API Snapshots Updated: Properly updated all the public API snapshot files across different target frameworks.

4. Non-Breaking Change: This is a purely additive change that maintains backward compatibility.

---

🔍 Code Quality Observations

Performance Considerations (TUnit.Core/Logging/DefaultLogger.cs)

1. StringBuilder allocation in hot path (line 85):

   • GenerateMessage is called for every log statement and creates a new StringBuilder each time
   • According to CLAUDE.md Rule 4, hot paths should minimize allocations
   • Recommendation: Consider using StringBuilder pooling via ObjectPool<StringBuilder> or at minimum reusing/returning to ArrayPool

   // Example improvement:
   private static readonly ObjectPool<StringBuilder> StringBuilderPool = 
       ObjectPool.Create<StringBuilder>();
   
   protected virtual string GenerateMessage(string message, Exception? exception, LogLevel logLevel)
   {
       var stringBuilder = StringBuilderPool.Get();
       try 
       {
           stringBuilder.Clear();
           // ... existing logic
           return stringBuilder.ToString();
       }
       finally 
       {
           StringBuilderPool.Return(stringBuilder);
       }
   }

2. LINQ in hot path (line 97):

   • _values.Any() uses LINQ which allocates
   • Recommendation: Use _values.Count > 0 or !_values.IsEmpty instead

---

Thread Safety Concerns

3. TestHeaderLogger example has race condition (docs/docs/customization-extensibility/logging.md:49-68):

   • The _hasOutputHeader field is not thread-safe
   • If multiple threads log simultaneously, the header could be output multiple times or not at all
   • Recommendation: Update the documentation example to show proper synchronization:

   private int _hasOutputHeader;
   
   protected override string GenerateMessage(string message, Exception? exception, LogLevel logLevel)
   {
       var baseMessage = base.GenerateMessage(message, exception, logLevel);
       
       // Thread-safe check
       if (Interlocked.Exchange(ref _hasOutputHeader, 1) == 0 && Context is TestContext testContext)
       {
           var testId = $"{testContext.TestDetails.ClassType.Name}.{testContext.TestDetails.TestName}";
           return $"--- {testId} ---\n{baseMessage}";
       }
       
       return baseMessage;
   }

---

Missing Async Consistency

4. MultiDestinationLogger example is sync-only (docs/docs/customization-extensibility/logging.md:90-105):

   • Only overrides WriteToOutput but not WriteToOutputAsync
   • If async logging is used, messages won't be written to the additional destination
   • Recommendation: Update the example to override both methods:

   protected override void WriteToOutput(string message, bool isError)
   {
       base.WriteToOutput(message, isError);
       _additionalOutput.WriteLine(message);
   }
   
   protected override async ValueTask WriteToOutputAsync(string message, bool isError)
   {
       await base.WriteToOutputAsync(message, isError);
       await _additionalOutput.WriteLineAsync(message);
   }

---

🧪 Testing

5. Missing Tests:
   • No tests were added to verify the new extensibility points work correctly
   • Recommendation: Add tests to verify:
     • Custom loggers can override GenerateMessage
     • Custom loggers can override WriteToOutput and WriteToOutputAsync
     • The Context property is accessible to derived classes
     • Both sync and async paths work correctly with overridden methods

---

📝 Documentation

6. Minor issue in documentation examples (docs/docs/customization-extensibility/logging.md:79):
   • The example shows creating a logger manually, but doesn't explain how to integrate it with the test context's default logger
   • Recommendation: Add a note explaining the relationship between custom loggers and TestContext.Current.GetDefaultLogger()

---

🎯 Summary

This PR adds valuable extensibility features to TUnit's logging system. The main areas for improvement are:

1. Performance: Address StringBuilder allocation in the hot path (logging is frequently called)
2. Thread Safety: Fix the race condition in the documentation example
3. Completeness: Update the MultiDestinationLogger example to handle async logging
4. Testing: Add tests to verify the new extensibility points

The changes align well with TUnit's philosophy of being modern and extensible. Once the performance and thread-safety concerns are addressed, this will be a solid enhancement!

---

🔧 Action Items

High Priority:

• Fix thread-safety issue in TestHeaderLogger documentation example
• Add async override in MultiDestinationLogger documentation example

Medium Priority:

• Consider StringBuilder pooling in GenerateMessage to reduce allocations
• Replace _values.Any() with _values.Count > 0 or !_values.IsEmpty
• Add tests for the new extensibility points

Low Priority:

• Clarify documentation about integrating custom loggers with TestContext

[Copilot]

Pull request overview

This PR enhances the DefaultLogger class with extensibility features to allow custom logger implementations. It also updates documentation to remove specific performance benchmark numbers in favor of more general language that won't become outdated.

Key changes:

• Made DefaultLogger extensible by exposing virtual methods and a protected Context property
• Refactored duplicated output writing logic into reusable virtual methods
• Updated documentation with extensibility examples and removed hard-coded benchmark numbers

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
TUnit.Core/Logging/DefaultLogger.cs	Added protected Context property and three virtual methods (GenerateMessage, WriteToOutput, WriteToOutputAsync) to enable custom logger implementations; refactored duplicate output logic
docs/docs/customization-extensibility/logging.md	Added comprehensive documentation on custom loggers with practical examples showing how to extend DefaultLogger for custom formatting and output destinations
docs/docs/migration/xunit.md	Removed specific "1.3x faster" claim in favor of general performance improvement statement
docs/docs/migration/nunit.md	Removed specific "1.2x faster" claim in favor of general performance improvement statement
docs/docs/migration/mstest.md	Removed specific "1.3x faster" claim in favor of general performance improvement statement
docs/docs/advanced/performance-best-practices.md	Removed specific "11.65x faster" claim for AOT in favor of "significant speed improvements"
TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt	Updated public API snapshot to reflect new protected members in DefaultLogger
TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt	Updated public API snapshot to reflect new protected members in DefaultLogger
TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt	Updated public API snapshot to reflect new protected members in DefaultLogger
TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt	Updated public API snapshot to reflect new protected members in DefaultLogger

[Copilot]

The WriteToOutput method references the 'context' parameter directly instead of using the protected Context property. This creates an inconsistency since a protected Context property was added specifically for derived classes to access the context. Consider using 'Context' instead of 'context' for consistency with the extensibility design.

[Copilot]

The WriteToOutputAsync method references the 'context' parameter directly instead of using the protected Context property. This creates an inconsistency since a protected Context property was added specifically for derived classes to access the context. Consider using 'Context' instead of 'context' for consistency with the extensibility design.