refactor: streamline exception assertion methods and improve type inference

[thomhurst]

Fixes #3521

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

I've thoroughly analyzed this PR that refactors exception assertion methods and improves type inference. Here's my comprehensive feedback:

Review Checklist:

• Read and understand the issue Assert.ThrowsAsync(Func<Task>) seems faulty #3521 context
• Analyze the new test file to understand requirements
• Review changes to exception assertion methods
• Check code quality and best practices compliance
• Evaluate potential bugs or issues
• Assess performance considerations
• Review security implications
• Verify test coverage adequacy
• Check TUnit-specific requirements (dual-mode, etc.)
• Provide comprehensive feedback

---

Summary

This PR successfully fixes issue #3521 where Assert.ThrowsAsync(Func<Task>) was incorrectly passing when no exception was thrown. The refactoring streamlines the exception assertion methods and improves type inference through better API design.

Code Quality & Best Practices ✅

Strengths:

• Excellent test coverage: Tests3521.cs provides comprehensive test coverage for all affected scenarios (method groups, lambdas, async lambdas, generic versions, etc.)
• Clean refactoring: The removal of 89 lines from Assert.cs while adding equivalent functionality shows good consolidation
• Type safety: The new fluent API design with ThrowsAssertion<TException> provides better compile-time type checking
• Consistent API: The changes maintain backward compatibility while providing a more consistent fluent interface

Code Style Compliance:

• Modern C# syntax used appropriately
• Proper async/await patterns
• Good use of generic constraints (where TException : Exception)
• Follows TUnit's coding standards from CLAUDE.md

Technical Analysis

What the refactoring does:

1. Consolidates logic: Moves exception assertion logic from static methods in Assert.cs to fluent methods like That(action).Throws<T>()
2. Improves type inference: Uses method overloads and fluent API to provide better IntelliSense and compile-time checking
3. Fixes the bug: The issue was that ThrowsAsync methods weren't properly validating that exceptions were actually thrown

Key Changes:

• Assert.cs:251-275: New ThrowsAsync methods now delegate to That(action).Throws<TException>()
• ThrowsAssertion.cs:207-225: Added WithExceptionType() method for runtime type checking
• AsyncDelegateAssertion.cs:151-182: Added instance Throws() methods to avoid type inference issues

Potential Issues & Concerns

⚠️ Performance Consideration:

The new fluent API creates more intermediate objects (ThrowsAssertion<T>, AsyncDelegateAssertion) compared to the previous direct static methods. However, this is likely negligible for test scenarios and the improved type safety is worth the trade-off.

🔍 Minor Code Quality Issues:

1. Line 219 in ThrowsAssertion.cs: Potential null reference

if (exception != null && !expectedExceptionType.IsInstanceOfType(exception))

This is fine since there's a null check, but the flow could be clearer.

2. Exception handling consistency: The error messages are consistent but could benefit from more standardized formatting.

Security & Robustness ✅

• No security concerns: All changes are in test assertion logic
• Proper exception handling: Good use of try-catch patterns
• Type safety: Proper generic constraints prevent runtime type errors
• Null safety: Appropriate null checks throughout

Test Coverage ✅

Excellent test coverage in Tests3521.cs:

• Tests method groups, lambdas, and async lambdas ✅
• Tests both generic and non-generic overloads ✅
• Tests both sync and async versions ✅
• Verifies error messages are correct ✅
• Includes positive tests to ensure functionality still works ✅

TUnit-Specific Compliance ✅

• Dual-mode compatibility: These changes are in the assertions library which works with both source-generated and reflection modes ✅
• Public API snapshots updated: All .NET version snapshots properly updated ✅
• Performance conscious: The refactoring doesn't introduce significant performance overhead ✅

Recommendations

Minor Improvements:

1. Consider adding XML documentation examples for the new fluent API patterns
2. The WithExceptionType method could benefit from caching the type validation logic
3. Consider adding analyzer rules to guide users toward the new fluent API

Future Considerations:

• The fluent API pattern introduced here could be extended to other assertion types for consistency
• Consider deprecation warnings for the old static methods in a future major version

Final Assessment

This is a high-quality refactoring that successfully:

• ✅ Fixes the reported bug (Assert.ThrowsAsync(Func<Task>) seems faulty #3521)
• ✅ Improves API design and type inference
• ✅ Maintains backward compatibility
• ✅ Provides comprehensive test coverage
• ✅ Follows TUnit's development standards

The code is production-ready and the changes are well-thought-out. The extensive test coverage gives confidence that the refactoring doesn't introduce regressions.

Recommendation: Approve and merge 🚀

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

I've thoroughly analyzed this PR that refactors exception assertion methods and improves type inference. Here's my comprehensive feedback:

Review Checklist:

• Read and understand the issue Assert.ThrowsAsync(Func<Task>) seems faulty #3521 context
• Analyze the new test file to understand requirements
• Review changes to exception assertion methods
• Check code quality and best practices compliance
• Evaluate potential bugs or issues
• Assess performance considerations
• Review security implications
• Verify test coverage adequacy
• Check TUnit-specific requirements (dual-mode, etc.)
• Provide comprehensive feedback

---

Summary

This PR successfully fixes issue #3521 where Assert.ThrowsAsync(Func<Task>) was incorrectly passing when no exception was thrown. The refactoring streamlines the exception assertion methods and improves type inference through better API design.

Code Quality & Best Practices ✅

Strengths:

• Excellent test coverage: Tests3521.cs provides comprehensive test coverage for all affected scenarios (method groups, lambdas, async lambdas, generic versions, etc.)
• Clean refactoring: The removal of 89 lines from Assert.cs while adding equivalent functionality shows good consolidation
• Type safety: The new fluent API design with ThrowsAssertion<TException> provides better compile-time type checking
• Consistent API: The changes maintain backward compatibility while providing a more consistent fluent interface

Code Style Compliance:

• Modern C# syntax used appropriately
• Proper async/await patterns
• Good use of generic constraints (where TException : Exception)
• Follows TUnit's coding standards from CLAUDE.md

Technical Analysis

What the refactoring does:

1. Consolidates logic: Moves exception assertion logic from static methods in Assert.cs to fluent methods like That(action).Throws<T>()
2. Improves type inference: Uses method overloads and fluent API to provide better IntelliSense and compile-time checking
3. Fixes the bug: The issue was that ThrowsAsync methods weren't properly validating that exceptions were actually thrown

Key Changes:

• Assert.cs:251-275: New ThrowsAsync methods now delegate to That(action).Throws<TException>()
• ThrowsAssertion.cs:207-225: Added WithExceptionType() method for runtime type checking
• AsyncDelegateAssertion.cs:151-182: Added instance Throws() methods to avoid type inference issues

Potential Issues & Concerns

⚠️ Performance Consideration:

The new fluent API creates more intermediate objects (ThrowsAssertion<T>, AsyncDelegateAssertion) compared to the previous direct static methods. However, this is likely negligible for test scenarios and the improved type safety is worth the trade-off.

🔍 Minor Code Quality Issues:

1. Line 219 in ThrowsAssertion.cs: Potential null reference

if (exception != null && !expectedExceptionType.IsInstanceOfType(exception))

This is fine since there's a null check, but the flow could be clearer.

2. Exception handling consistency: The error messages are consistent but could benefit from more standardized formatting.

Security & Robustness ✅

• No security concerns: All changes are in test assertion logic
• Proper exception handling: Good use of try-catch patterns
• Type safety: Proper generic constraints prevent runtime type errors
• Null safety: Appropriate null checks throughout

Test Coverage ✅

Excellent test coverage in Tests3521.cs:

• Tests method groups, lambdas, and async lambdas ✅
• Tests both generic and non-generic overloads ✅
• Tests both sync and async versions ✅
• Verifies error messages are correct ✅
• Includes positive tests to ensure functionality still works ✅

TUnit-Specific Compliance ✅

• Dual-mode compatibility: These changes are in the assertions library which works with both source-generated and reflection modes ✅
• Public API snapshots updated: All .NET version snapshots properly updated ✅
• Performance conscious: The refactoring doesn't introduce significant performance overhead ✅

Recommendations

Minor Improvements:

1. Consider adding XML documentation examples for the new fluent API patterns
2. The WithExceptionType method could benefit from caching the type validation logic
3. Consider adding analyzer rules to guide users toward the new fluent API

Future Considerations:

• The fluent API pattern introduced here could be extended to other assertion types for consistency
• Consider deprecation warnings for the old static methods in a future major version

Final Assessment

This is a high-quality refactoring that successfully:

• ✅ Fixes the reported bug (Assert.ThrowsAsync(Func<Task>) seems faulty #3521)
• ✅ Improves API design and type inference
• ✅ Maintains backward compatibility
• ✅ Provides comprehensive test coverage
• ✅ Follows TUnit's development standards

The code is production-ready and the changes are well-thought-out. The extensive test coverage gives confidence that the refactoring doesn't introduce regressions.

Recommendation: Approve and merge 🚀

---