fix: ensure After(Test) hooks execute before test instance disposal

[thomhurst]

Fixes #3156

[claude]

Claude finished @thomhurst's task —— View job

---

Pull Request Review ✅

Reviewing PR #3159: "fix: ensure After(Test) hooks execute before test instance disposal"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze TestExecutor.cs changes
• Review AfterTestDisposalOrderTest.cs test implementation
• Check code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Provide comprehensive feedback

Key Changes Analysis:

TestExecutor.cs (TUnit.Engine/TestExecutor.cs:119-164):

• Refactored exception handling to move After(Test) hooks and disposal logic into the catch block
• Added explicit ordering: After(Test) hooks → Event receivers → Disposal → After(Class/Assembly/Session) hooks
• Improved error swallowing in catch block to prevent masking original test failures
• Maintained same logic in finally block for success path

AfterTestDisposalOrderTest.cs (NEW):

• Comprehensive regression test suite for the fixed behavior
• Two test classes covering sync and async disposal scenarios
• Proper validation that resources are accessible in After(Test) hooks before disposal

✅ Code Quality & Best Practices

Excellent overall quality:

• Clear execution order: The fix properly sequences After(Test) hooks before disposal, ensuring hooks can access instance resources
• Error handling: Smart use of exception swallowing in catch block prevents hook failures from masking the original test failure
• Duplication management: Successfully eliminates code duplication by extracting common disposal logic
• Modern C# patterns: Proper use of ConfigureAwait(false), async/await, and nullable reference types
• Repository compliance: Follows TUnit's coding standards and architectural patterns

Minor suggestion: Consider extracting the disposal sequence (lines 119-129 and 153-163) into a private method to reduce duplication further.

✅ Potential Bugs & Issues

No significant bugs found:

• Thread safety: Changes maintain existing thread safety guarantees
• Resource cleanup: Proper disposal ordering prevents resource access after disposal
• Exception handling: Correctly preserves original test exceptions while allowing result overrides
• State consistency: Test state management remains consistent across success and failure paths

✅ Performance Considerations

No performance regression:

• Same execution paths: No additional allocations or unnecessary async operations
• Cached hooks: Maintains existing Before hook caching strategy
• Error path optimization: Exception handling doesn't add overhead to success path
• Aligns with TUnit goals: Maintains the framework's performance-first philosophy

✅ Security Concerns

No security issues:

• Resource isolation: Proper disposal prevents resource leaks
• Exception safety: No sensitive information leaked through exceptions
• AOT compatibility: Maintains existing AOT-friendly patterns with proper suppression attributes

✅ Test Coverage

Comprehensive test coverage:

• Regression protection: AfterTestDisposalOrderTest directly tests the fixed behavior
• Multiple scenarios: Covers both sync (IDisposable) and async (IAsyncDisposable) disposal patterns
• Resource validation: Tests verify resources are accessible in After(Test) hooks before disposal
• Integration verification: After(Class) hook validates the entire execution flow

Test quality highlights:

• Clear test names and documentation linking to issue Test class is being disposed before [After(Test)] hook runs, since 0.58.3 #3156
• Proper use of [EngineTest(ExpectedResult.Pass)] attribute
• Good assertions with descriptive failure messages using .Because()
• Context validation using ObjectBag to verify hook execution

🎯 Summary

This is a well-executed fix that addresses the core issue while maintaining code quality and test coverage. The implementation properly ensures After(Test) hooks execute before test instance disposal, which is critical for hooks that need to access instance resources for cleanup or logging.

Key strengths:

• Solves the reported issue without breaking existing functionality
• Excellent test coverage with realistic scenarios
• Maintains TUnit's performance and reliability standards
• Clear, maintainable code following repository conventions

Recommendation: ✅ Approve - This PR successfully fixes the disposal ordering issue with comprehensive tests and no identified risks.

---