review: address #5694 round 2 — flush guard revert, null-getter seman…

…tics

- Revert the `HasCapturedOutput` guard around the Console flush in
  `TestCoordinator`. The guard was a point-in-time check and could silently
  drop output from background writes landing after the check; the flush
  itself is a cheap no-op when nothing is buffered. Also drop the
  `ConsoleLineBuffer` ref-tracking that was only there to detect captured
  output for the (now-removed) guard.
- Change `PropertyInjectionMetadata.GetOrCreateGetter()` to return `null`
  when the property is not readable instead of memoizing a `_ => null`
  stub. Updates `GetPropertyValues` and `TraverseSourceGeneratedProperties`
  to skip such properties, preserving the pre-PR "non-readable means no
  yield" enumeration semantic.
- Add `Debug.Assert(!string.IsNullOrEmpty(TestSessionId))` in the two
  primary `TestSessionId` consumer paths (`ExecutableTest<T>.CreateInstanceAsync`
  and `GenericTestMetadata` runtime-resolution path) so the
  `string.Empty` default surface misuse in debug builds without adding
  release-mode cost.

TUnit.Core/Context.cs

@@ -112,14 +112,8 @@ public void AddAsyncLocalValues()
 
     // Fast path for callers that need to know whether anything was ever captured —
     // lets the result-building code skip the reader/writer lock acquisition entirely
-    // for the (very common) case of a passing test with no output. Includes the
-    // console interceptor line buffers so partial writes (Console.Write without a
-    // newline) still force a flush at end-of-test.
-    internal virtual bool HasCapturedOutput =>
-        _outputWriter != null
-        || _errorOutputWriter != null
-        || _consoleStdOutLineBuffer != null
-        || _consoleStdErrLineBuffer != null;
+    // for the (very common) case of a passing test with no output.
+    internal virtual bool HasCapturedOutput => _outputWriter != null || _errorOutputWriter != null;
 
     public DefaultLogger GetDefaultLogger()
     {

TUnit.Core/Discovery/ObjectGraphDiscoverer.cs

@@ -343,7 +343,13 @@ private static void TraverseSourceGeneratedProperties(
             // Use the compile-time-generated getter when the source generator emitted one,
             // otherwise fall back to a cached reflection-based getter. Eliminates the
             // per-test Type.GetProperty reflection lookup on the hot path.
+            // null getter => property is not readable; skip.
             var getter = metadata.GetOrCreateGetter();
+            if (getter is null)
+            {
+                continue;
+            }
+
             var value = getter(obj);
             if (value != null && tryAdd(value, currentDepth))
             {

TUnit.Core/ExecutableTest`1.cs

@@ -1,3 +1,4 @@
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using TUnit.Core.Helpers;
 
@@ -40,6 +41,11 @@ public override async Task<object> CreateInstanceAsync()
         }
 
         var attributes = _metadata.GetOrCreateAttributes();
+        // TestSessionId defaults to string.Empty for allocation reasons — callers in the
+        // engine must overwrite it before reaching test-variant generation. Assert here
+        // rather than guarding every write site.
+        Debug.Assert(!string.IsNullOrEmpty(_metadata.TestSessionId),
+            $"TestSessionId must be assigned before creating a test instance for {_metadata.TestName}");
         var instance = await ClassConstructorHelper.TryCreateInstanceWithClassConstructor(
             attributes,
             _metadata.TestClassType,

TUnit.Core/GenericTestMetadata.cs

@@ -1,3 +1,4 @@
+using System.Diagnostics;
 using TUnit.Core.Helpers;
 
 namespace TUnit.Core;
@@ -67,6 +68,10 @@ public override Func<ExecutableTestCreationContext, TestMetadata, AbstractExecut
                 {
                     // Try to create instance with ClassConstructor attribute
                     var attributes = metadata.GetOrCreateAttributes();
+                    // TestSessionId defaults to string.Empty for allocation reasons — the
+                    // engine must assign a real session id before reaching this point.
+                    Debug.Assert(!string.IsNullOrEmpty(metadata.TestSessionId),
+                        $"TestSessionId must be assigned before creating a generic test instance for {metadata.TestName}");
                     var classInstance = await ClassConstructorHelper.TryCreateInstanceWithClassConstructor(
                         attributes,
                         TestClassType,

TUnit.Core/Interfaces/SourceGenerator/PropertyInjectionMetadata.cs

@@ -8,6 +8,10 @@ namespace TUnit.Core.Interfaces.SourceGenerator;
 /// </summary>
 public sealed class PropertyInjectionMetadata
 {
+    // Sentinel indicating the property is not readable — distinct from "not yet computed"
+    // (null) so the memoized negative result does not keep re-running reflection.
+    private static readonly Func<object, object?> s_notReadableSentinel = static _ => null;
+
     // Source-gen-supplied delegate (or null for hand-authored/reflection-only metadata).
     // Kept separate from the lazy-init reflection fallback so the public GetProperty
     // contract ("what the source generator emitted") is not polluted by internal caching.
@@ -63,13 +67,14 @@ public sealed class PropertyInjectionMetadata
     }
 
     /// <summary>
-    /// Returns a delegate that reads the property value from a given instance. Uses the
+    /// Returns a delegate that reads the property value from a given instance, or <c>null</c>
+    /// when the property is not readable (callers should skip such properties). Uses the
     /// source-generated <see cref="GetProperty"/> delegate when available, otherwise compiles
     /// a reflection-based getter once and caches it on the metadata instance.
     /// </summary>
     [UnconditionalSuppressMessage("Trimming", "IL2075",
         Justification = "ContainingType is annotated to preserve properties; reflection fallback is only used when source-gen did not emit a delegate.")]
-    internal Func<object, object?> GetOrCreateGetter()
+    internal Func<object, object?>? GetOrCreateGetter()
     {
         // Fast path: source-gen supplied a delegate.
         if (_sourceGenGetter != null)
@@ -81,19 +86,18 @@ public sealed class PropertyInjectionMetadata
         var cached = _cachedGetter;
         if (cached != null)
         {
-            return cached;
+            return ReferenceEquals(cached, s_notReadableSentinel) ? null : cached;
         }
 
         var property = ContainingType.GetProperty(PropertyName);
-        Func<object, object?> computed = property is null || !property.CanRead
-            // Memoize a stub that mirrors the behavior the previous reflection-based
-            // call sites expected (null when the property can't be read).
-            ? static _ => null
+        var computed = property is null || !property.CanRead
+            ? s_notReadableSentinel
             : property.GetValue;
 
         // Benign race: both racers compute equivalent delegates; CompareExchange makes the
         // winner visible to any subsequent reader without a lock.
         Interlocked.CompareExchange(ref _cachedGetter, computed, null);
-        return _cachedGetter!;
+        var winner = _cachedGetter!;
+        return ReferenceEquals(winner, s_notReadableSentinel) ? null : winner;
     }
 }

TUnit.Core/PropertyInjection/PropertyInjectionPlanBuilder.cs

@@ -234,7 +234,13 @@ public Task ForEachPropertyParallelAsync(
         {
             foreach (var metadata in SourceGeneratedProperties)
             {
-                yield return metadata.GetOrCreateGetter()(instance);
+                // Skip non-readable properties so the enumeration count matches the legacy
+                // reflection behavior (old code did not yield for properties without a getter).
+                var getter = metadata.GetOrCreateGetter();
+                if (getter is not null)
+                {
+                    yield return getter(instance);
+                }
             }
         }
         else if (ReflectionProperties.Length > 0)

TUnit.Engine/Services/TestExecution/TestCoordinator.cs

@@ -134,20 +134,19 @@ await RetryHelper.ExecuteWithRetry(test.Context,
         finally
         {
             // Flush console interceptors to ensure all buffered output is captured.
-            // This is critical for output from Console.Write() without newline, but Console.Out
-            // is a SynchronizedTextWriter and FlushAsync takes a lock — skip the flush entirely
-            // when the test wrote nothing, which is the hot path for passing tests with no output.
-            if (test.Context.HasCapturedOutput)
+            // This is critical for output from Console.Write() without newline. The flush
+            // is unconditional because `HasCapturedOutput` is a point-in-time check: a test
+            // that fires `Task.Run(() => Console.WriteLine(...))` without awaiting can land
+            // writes after the check, which would silently lose output. The interceptor's
+            // own FlushIfNonEmpty path is a cheap no-op when there is nothing buffered.
+            try
             {
-                try
-                {
-                    await Console.Out.FlushAsync().ConfigureAwait(false);
-                    await Console.Error.FlushAsync().ConfigureAwait(false);
-                }
-                catch (Exception flushEx)
-                {
-                    await _logger.LogErrorAsync($"Error flushing console output for {test.TestId}: {flushEx}").ConfigureAwait(false);
-                }
+                await Console.Out.FlushAsync().ConfigureAwait(false);
+                await Console.Error.FlushAsync().ConfigureAwait(false);
+            }
+            catch (Exception flushEx)
+            {
+                await _logger.LogErrorAsync($"Error flushing console output for {test.TestId}: {flushEx}").ConfigureAwait(false);
             }
 
             // Stay null on the success path — materializing the list only when something actually fails