Claude finished @thomhurst's task —— View job

Pull Request Review: Avoid swallowing hook exceptions

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze HookOrchestratingTestExecutorAdapter.cs changes
• Analyze HookOrchestrator.cs changes
• Analyze SingleTestExecutor.cs changes
• Review new test file AfterTestExceptionTests.cs
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage
• Provide comprehensive feedback

Summary

This PR significantly improves exception handling in TUnit's hook execution system by ensuring that exceptions from "after" hooks are properly propagated rather than being swallowed. The changes affect three core engine classes and add comprehensive test coverage.

Code Quality & Best Practices ✅

Strengths:

• Consistent Exception Handling: All "after" hook methods now follow the same pattern: collect exceptions, log them, and throw wrapped exceptions
• Proper Exception Aggregation: Multiple hook failures are correctly aggregated using AggregateException
• Separation of Concerns: Before hooks continue to fail fast (appropriate), while after hooks collect all failures before throwing
• Good Use of Custom Exceptions: HookFailedException provides a clear distinction between hook failures and test failures

Minor Considerations:

• The exception handling logic is duplicated across multiple methods. Consider extracting common patterns into helper methods for better maintainability

Potential Bugs & Issues ⚠️

Logic Improvements:

1. HookOrchestratingTestExecutorAdapter.cs:146-167: The test state update logic correctly handles the case where a passing test should be marked as failed due to after hook failures
2. Exception Chaining: The AggregateException handling in lines 170-187 properly combines test and hook failures

Potential Edge Cases:

• Consider cancellation scenarios: What happens if hooks are cancelled? The current implementation should handle this correctly since CancellationException will bubble up naturally

Performance Considerations ✅

• Minimal Impact: The addition of exception collection (List<Exception>) and conditional throwing adds negligible overhead
• Exception Creation: AggregateException creation only occurs on failure paths, so no performance impact on happy paths
• Execution Flow: After hooks continue to execute even if earlier ones fail, which is the correct behavior for cleanup scenarios

Security Concerns ✅

• Exception Information: The changes properly preserve original exception information without exposing sensitive data
• Resource Cleanup: The exception handling doesn't interfere with proper resource disposal in finally blocks

Test Coverage ✅

The new test file AfterTestExceptionTests.cs provides excellent coverage:

Covered Scenarios:

• ✅ Passing test with failing after hook → Should fail test
• ✅ Multiple after hooks failing → Should aggregate exceptions
• ✅ Both test and after hook failing → Should aggregate both failures
• ✅ Verification that hooks actually execute → Prevents false positives

Test Quality:

• Good use of static flags to verify execution order
• Proper async/await patterns
• Clear, descriptive test names
• Covers both sync and async hook scenarios

Behavioral Changes

Before this PR:

• After hook exceptions were logged but swallowed
• Tests could pass even if cleanup hooks failed
• Multiple hook failures were not properly reported

After this PR:

• After hook exceptions cause test failure (correct behavior)
• Multiple failures are aggregated and reported together
• Better visibility into cleanup failures

Recommendations

1. Code Organization (Optional):

// Consider extracting to reduce duplication:
private static Exception CreateHookException(List<Exception> exceptions, string contextMessage)
{
    return exceptions.Count == 1 
        ? new HookFailedException(exceptions[0]) 
        : new HookFailedException($"Multiple {contextMessage} hooks failed", new AggregateException(exceptions));
}

2. Documentation:
Consider adding XML comments to clarify the new exception behavior, especially for the "after hooks now fail tests" behavioral change.

3. Logging Enhancement (Optional):
The current logging is good, but consider adding the number of failed hooks to make debugging easier:

await _logger.LogErrorAsync($"AfterEveryTest hook failed ({exceptions.Count} total failures): {ex.Message}");

Conclusion

This is an excellent improvement that addresses a significant gap in TUnit's exception handling. The implementation is thorough, well-tested, and follows established patterns. The change from swallowing after-hook exceptions to properly failing tests is the correct behavior for a testing framework.

Recommendation: ✅ Approve

The code quality is high, test coverage is comprehensive, and the behavioral change aligns with user expectations for test framework reliability.