Address PR review: extract disposal helper, add error recording, fix stale comment

- Extract disposal span logic into DisposeTestInstanceWithSpanAsync() helper to
  reduce #if NET nesting in the finally block (review issue #1)
- Add TUnitActivitySource.RecordException() calls on disposal span when OnDispose
  or DisposeTestInstance throws (review issue #2)
- Fix stale comment in TestInitializer.cs referencing "data source initialization"
  instead of "test object initialization" (review issue #4)

Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/3802acb9-a7fe-4976-8c3f-5bb246c098d2

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>

Author: Copilot

TUnit.Engine/Services/TestExecution/TestCoordinator.cs

@@ -304,55 +304,69 @@ private async ValueTask ExecuteTestLifecycleAsync(AbstractExecutableTest test, C
         }
         finally
         {
+            await DisposeTestInstanceWithSpanAsync(test).ConfigureAwait(false);
+        }
+    }
+
+    /// <summary>
+    /// Disposes the test instance and fires OnDispose callbacks, wrapped in an OpenTelemetry
+    /// activity span for trace timeline visibility.
+    /// Parented under the class activity because the test case activity has already been stopped
+    /// by this point (disposal runs after TestExecutor.ExecuteAsync completes).
+    /// </summary>
+    private async ValueTask DisposeTestInstanceWithSpanAsync(AbstractExecutableTest test)
+    {
 #if NET
-            Activity? disposalActivity = null;
-            if (TUnitActivitySource.Source.HasListeners())
-            {
-                // Parented under the class activity because the test case activity has already
-                // been stopped by this point (disposal runs after TestExecutor.ExecuteAsync completes).
-                // The initialization activity is a child of the test case span since it runs within it.
-                disposalActivity = TUnitActivitySource.StartActivity(
-                    "test instance disposal",
-                    ActivityKind.Internal,
-                    test.Context.ClassContext.Activity?.Context ?? default,
-                    [new("tunit.test.id", test.Context.Id)]);
-            }
-            try
-            {
+        Activity? disposalActivity = null;
+        if (TUnitActivitySource.Source.HasListeners())
+        {
+            disposalActivity = TUnitActivitySource.StartActivity(
+                "test instance disposal",
+                ActivityKind.Internal,
+                test.Context.ClassContext.Activity?.Context ?? default,
+                [new("tunit.test.id", test.Context.Id)]);
+        }
+        try
+        {
 #endif
-            // Dispose test instance and fire OnDispose after each attempt
-            // This ensures each retry gets a fresh instance
-            var onDispose = test.Context.InternalEvents.OnDispose;
-            if (onDispose?.InvocationList != null)
+        // Dispose test instance and fire OnDispose after each attempt
+        // This ensures each retry gets a fresh instance
+        var onDispose = test.Context.InternalEvents.OnDispose;
+        if (onDispose?.InvocationList != null)
+        {
+            foreach (var invocation in onDispose.InvocationList)
             {
-                foreach (var invocation in onDispose.InvocationList)
+                try
                 {
-                    try
-                    {
-                        await invocation.InvokeAsync(test.Context, test.Context).ConfigureAwait(false);
-                    }
-                    catch (Exception disposeEx)
-                    {
-                        await _logger.LogErrorAsync($"Error during OnDispose for {test.TestId}: {disposeEx}").ConfigureAwait(false);
-                    }
+                    await invocation.InvokeAsync(test.Context, test.Context).ConfigureAwait(false);
+                }
+                catch (Exception disposeEx)
+                {
+#if NET
+                    TUnitActivitySource.RecordException(disposalActivity, disposeEx);
+#endif
+                    await _logger.LogErrorAsync($"Error during OnDispose for {test.TestId}: {disposeEx}").ConfigureAwait(false);
                 }
             }
+        }
 
-            try
-            {
-                await TestExecutor.DisposeTestInstance(test).ConfigureAwait(false);
-            }
-            catch (Exception disposeEx)
-            {
-                await _logger.LogErrorAsync($"Error disposing test instance for {test.TestId}: {disposeEx}").ConfigureAwait(false);
-            }
+        try
+        {
+            await TestExecutor.DisposeTestInstance(test).ConfigureAwait(false);
+        }
+        catch (Exception disposeEx)
+        {
 #if NET
-            }
-            finally
-            {
-                TUnitActivitySource.StopActivity(disposalActivity);
-            }
+            TUnitActivitySource.RecordException(disposalActivity, disposeEx);
 #endif
+            await _logger.LogErrorAsync($"Error disposing test instance for {test.TestId}: {disposeEx}").ConfigureAwait(false);
+        }
+#if NET
         }
+        finally
+        {
+            TUnitActivitySource.StopActivity(disposalActivity);
+        }
+#endif
     }
 }

TUnit.Engine/TestInitializer.cs

@@ -34,7 +34,7 @@ public async ValueTask InitializeTestObjectsAsync(AbstractExecutableTest test, C
     {
         // Data source initialization now runs inside the test case span, so any spans it
         // creates (container startup, auth calls, connection pools, etc.) will appear nested
-        // inside the test's trace timeline via the "data source initialization" child activity
+        // inside the test's trace timeline via the "test object initialization" child activity
         // created by TestExecutor.
         await _objectLifecycleService.InitializeTestObjectsAsync(test.Context, cancellationToken);
     }