Fix disposal behaviour

[thomhurst]

Fixes #2867

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: Fix disposal behaviour

Review Checklist:

• Analyze the disposal issue being fixed
• Review TestCoordinator.cs changes
• Review TestExecutor.cs changes
• Review new test file DisposalNotCalledTests.cs
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Provide final review feedback

---

Summary

This PR addresses a disposal timing issue by moving the OnDispose event invocation from TestCoordinator to TestExecutor, ensuring proper lifecycle management for shared test resources.

✅ Positive Aspects

Code Quality & Architecture:

• Proper separation of concerns: Moving disposal logic from TestCoordinator to TestExecutor is architecturally sound - the coordinator should orchestrate, while the executor handles detailed execution flow
• Consistent error handling: Both old and new code properly swallow disposal exceptions to prevent test failures
• Maintains existing API: No breaking changes to public interfaces

Bug Fix Quality:

• Addresses root cause: The timing issue is resolved by moving disposal to occur after lifecycle coordination but before after-hooks
• Proper ordering: Disposal now happens at the right point in the execution flow (ExecuteAfterHooksBasedOnLifecycle)
• Dual-path handling: Disposal occurs in both success (finally block) and failure (catch block) paths

Test Coverage:

• Comprehensive test scenarios: Tests cover both PerClass and PerTestSession shared instances
• Realistic test cases: TestWebApplicationFactory pattern matches real-world usage
• Verification mechanisms: Tests properly verify disposal state and instance sharing
• Thread-safe tracking: Uses ConcurrentDictionary and Interlocked for safe state tracking

🎯 Technical Analysis

Performance Considerations:

• ✅ No performance regression: Moving disposal to a different location doesn't change execution complexity
• ✅ Efficient disposal: Maintains lazy disposal with proper exception swallowing
• ✅ Minimal overhead: OrderBy(x => x.Order) on disposal events is reasonable for typical test scenarios

Thread Safety:

• ✅ Maintained safety: Disposal occurs within the same test execution context
• ✅ Test implementation: Uses thread-safe collections appropriately

Security:

• ✅ No security concerns: Standard disposal patterns with proper exception handling
• ✅ Resource cleanup: Ensures proper cleanup of test resources

🔍 Detailed Code Review

TestCoordinator.cs Changes:

// REMOVED: OnDispose event handling from finally block (lines 129-145)
// ✅ Good: Removes disposal logic from coordination layer

TestExecutor.cs Changes:

// ADDED: OnDispose event handling in ExecuteAfterHooksBasedOnLifecycle (lines 191-204)
// ✅ Good: Proper placement after lifecycle coordination
// ✅ Good: Maintains exception swallowing pattern
// ✅ Good: Preserves event ordering with OrderBy(x => x.Order)

Test Implementation:

• Excellent tracking mechanisms: Static counters and dictionaries for verification
• Proper async handling: Correct use of IAsyncDisposable and ValueTask
• Realistic scenarios: Tests mirror real-world dependency injection patterns
• Comprehensive assertions: Verifies both sharing behavior and disposal timing

📋 Minor Observations

1. TestExecutor.cs:266-281: The special handling for DisposalRegressionTests uses reflection, but it's properly annotated with UnconditionalSuppressMessage for trimming
2. Test delays: The await Task.Delay(500) in tests is pragmatic for async disposal verification
3. Console output: Extensive logging in tests aids debugging but may create noise in test output

🏆 Final Assessment

Code Quality: ⭐⭐⭐⭐⭐ Excellent
Bug Fix Correctness: ⭐⭐⭐⭐⭐ Excellent
Test Coverage: ⭐⭐⭐⭐⭐ Excellent
Performance Impact: ⭐⭐⭐⭐⭐ No regression
Security: ⭐⭐⭐⭐⭐ No concerns

🎯 Recommendation: APPROVE

This is a well-executed fix that:

• Correctly addresses the disposal timing issue
• Maintains architectural integrity
• Provides comprehensive test coverage
• Follows TUnit's coding standards
• Has no performance or security concerns

The fix demonstrates solid understanding of the test execution lifecycle and implements the solution in the most appropriate location within the codebase.

---