You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR Review: Fix overload resolution priorities for member assertions
Review Checklist:
Read and understand the changed files
Analyze code quality and best practices
Check for potential bugs or issues
Evaluate performance considerations
Review security concerns
Assess test coverage implications
Verify alignment with TUnit coding standards
Summary
This PR adjusts the [OverloadResolutionPriority] attributes for multiple Member method overloads in AssertionExtensions.cs to fix issue #3930. The changes establish a clear hierarchy for overload resolution, prioritizing AOT-compatible strongly-typed overloads over legacy reflection-based ones.
Code Quality Assessment
✅ Strengths
Well-structured overload hierarchy: The new priority system follows a logical pattern:
Medium priority (1-2): Specific type-preserving overloads
Lowest priority (-1 to 0): Legacy reflection-based overloads
AOT compatibility prioritization: The changes correctly favor AOT-compatible overloads over RequiresDynamicCode variants, which aligns with TUnit's performance-first philosophy.
Consistent documentation: All modified overloads maintain clear documentation about their purpose and AOT compatibility implications.
Proper API versioning: The snapshot files are correctly updated, following TUnit's mandatory snapshot testing practice.
⚠️Areas of Concern
Missing priority on one overload: The generic Member<TObject, TMember> overload at line 531 previously had no explicit priority but now has [OverloadResolutionPriority(0)]. While this is good for clarity, it could potentially be a breaking change if code was relying on the default resolution behavior.
Complex overload matrix: With 7+ overloads for the Member method across different type scenarios, the resolution priority system is becoming complex. Consider if further consolidation is possible in future versions.
Security & Performance
Security: ✅ No security concerns. Changes are limited to compile-time overload resolution.
The API snapshot tests in both DotNet9_0 and DotNet10_0 are properly updated, ensuring the public API changes are tracked. The changes are purely attribute-based, so runtime behavior testing may not be strictly necessary, but integration tests covering the member assertion scenarios would be valuable.
TUnit Standards Compliance
✅ Fully compliant with TUnit development standards:
Follows the snapshot testing requirements for public API changes
Maintains AOT compatibility prioritization
Uses proper modern C# attributes and patterns
No performance regressions in hot paths
Recommendation
✅ APPROVE - This is a well-executed fix that improves overload resolution clarity while maintaining backward compatibility. The changes align with TUnit's AOT-first approach and follow all established patterns.
Minor Suggestions:
Consider adding a comment block above the Member method group explaining the overload priority strategy for future maintainers
Integration tests covering edge cases of member assertion overload selection would provide additional confidence
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3930