Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #4112

Copilot AI review requested due to automatic review settings December 22, 2025 12:43
@thomhurst thomhurst enabled auto-merge (squash) December 22, 2025 12:43
@claude
Copy link
Contributor

claude bot commented Dec 22, 2025

Pull Request Review

Summary

This PR successfully implements custom equality predicate support for equivalency assertions by introducing a new FuncEqualityComparer<T> helper class and adding .Using(Func<T?, T?, bool>) overloads to relevant assertion classes. This addresses issue #4112 and aligns the implementation with the documented API.


✅ Positive Aspects

  1. Clean Implementation: The FuncEqualityComparer<T> adapter pattern is elegant and follows the established codebase patterns. It properly wraps a function predicate into an IEqualityComparer<T>.

  2. Comprehensive Coverage: The PR correctly adds the Using overload to all relevant assertion types:

    • IsEquivalentToAssertion
    • NotEquivalentToAssertion
    • IsEquatableOrEqualToAssertion
    • DictionaryContainsKeyAssertion
  3. Good Test Coverage: The test file includes multiple scenarios:

    • Basic predicate usage
    • Property-based comparison
    • Negative case (IsNotEquivalentTo)
  4. Documentation: The FuncEqualityComparer class has excellent inline comments explaining the intentional constant hash code return (lines 19-23).

  5. API Snapshot Updates: All public API snapshots have been properly updated and committed (.verified.txt files).

  6. Null Safety: The predicate null check in the constructor is good defensive programming.


🔍 Observations & Suggestions

1. Performance Consideration - GetHashCode Implementation (TUnit.Assertions/Conditions/Helpers/FuncEqualityComparer.cs:24)

The constant GetHashCode returning 0 will cause O(n²) performance for large collections, as noted in the comments. While this is acceptable for the current use case, consider:

Suggestion: Add an XML documentation warning about performance implications:

/// <summary>
/// An IEqualityComparer implementation that uses a custom Func for equality comparison.
/// This allows users to pass lambda predicates to assertion methods like Using().
/// </summary>
/// <remarks>
/// <para>
/// ⚠️ PERFORMANCE WARNING: This comparer always returns a constant hash code,
/// which forces O(n²) comparisons in collection equivalency checks.
/// For large collections (>1000 elements), consider implementing IEqualityComparer{T} directly.
/// </para>
/// </remarks>

2. Test Quality (TUnit.Assertions.Tests/Bugs/Tests1600.cs:32)

The test at line 32 uses (x, y) => true, which always passes. While this tests the API signature, it doesn't validate correctness.

Suggestion: Consider a more meaningful test:

[Test]
public async Task Custom_Predicate_Always_True()
{
    MyModel[] array1 = [new(), new(), new()];
    MyModel[] array2 = [new(), new(), new()];
    
    // Verify that even with different IDs, the predicate makes them equivalent
    await Assert.That(array1).IsEquivalentTo(array2).Using((x, y) => true);
    
    // Add assertion to verify IDs are actually different
    await Assert.That(array1[0].Id).IsNotEqualTo(array2[0].Id);
}

3. Missing Edge Case Tests

Consider adding tests for:

  • Empty collections with predicates
  • Null handling in predicates (u1?.Name suggests nullability - test null inputs)
  • Single-element collections
  • Collections with duplicate elements
  • Predicate that throws an exception

Example:

[Test]
public async Task Custom_Predicate_With_Null_Elements()
{
    User?[] array1 = [new User("Alice", 30), null];
    User?[] array2 = [null, new User("Alice", 30)];
    
    await Assert.That(array1)
        .IsEquivalentTo(array2)
        .Using((u1, u2) => u1?.Name == u2?.Name && u1?.Age == u2?.Age);
}

4. Consistency Check (All assertion files)

All Using overloads follow the same pattern, which is excellent for consistency. The implementation correctly uses SetComparer and returns this for fluent chaining.

5. Minor: Sealed Class (FuncEqualityComparer.cs:8)

Good use of sealed on the helper class - this is a performance best practice for internal utilities.


🎯 Adherence to TUnit Guidelines (from CLAUDE.md)

Code Style: Uses modern C# syntax (collection expressions, nullable reference types, file-scoped namespaces)
Performance: Minimal allocations, but documented trade-off with constant hash code
Dual-Mode Compatibility: ⚠️ CRITICAL QUESTION: Does this need source generator changes?

⚠️ IMPORTANT QUESTION: Dual-Mode Implementation

According to CLAUDE.md Rule 1, ALL changes must work in both source-generated and reflection modes. This PR only modifies assertion classes in TUnit.Assertions.

Question for reviewers: Do these assertion API changes require corresponding updates in:

  • TUnit.Core.SourceGenerator?
  • TUnit.Engine (reflection path)?

Since assertions are runtime-evaluated (not compile-time generated), this might not require dual-mode implementation. However, this should be explicitly verified.


🔒 Security & Safety

✅ No security concerns identified
✅ Null checking is properly handled
✅ No reflection or dynamic code that would break AOT
✅ No external dependencies added


📊 Test Execution

The snapshot tests have been properly updated (all 4 .NET target framework versions). The implementation appears sound based on code review.

Recommendation: The PR author should verify that:

dotnet test TUnit.Assertions.Tests --filter "FullyQualifiedName~Tests1600"

passes successfully.


✅ Final Recommendation

APPROVE with minor suggestions

This is a well-implemented feature that addresses a real user need (issue #4112). The code is clean, follows established patterns, and includes proper tests and documentation.

Before merging, please confirm:

  1. All tests pass (especially the new Tests1600 tests)
  2. Dual-mode compatibility verified (or confirmed not needed for assertions)
  3. Consider adding the performance warning to XML docs
  4. Consider adding edge case tests for null handling

Great work! This will significantly improve the user experience by eliminating the need to implement IEqualityComparer for simple comparison scenarios. 🎉

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds custom predicate support for equivalency assertions by introducing a FuncEqualityComparer<T> helper class. This enhancement allows users to pass lambda predicates directly to Using() methods instead of having to implement IEqualityComparer<T>, significantly improving the API's usability for quick comparisons.

Key Changes

  • Introduced FuncEqualityComparer<T> that wraps Func<T?, T?, bool> predicates into IEqualityComparer<T> implementations
  • Added Using(Func<T?, T?, bool>) overloads to four assertion types: IsEquivalentToAssertion, NotEquivalentToAssertion, IsEquatableOrEqualToAssertion, and DictionaryContainsKeyAssertion
  • Added comprehensive tests demonstrating usage with lambda predicates for collection equivalency checks

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
TUnit.Assertions/Conditions/Helpers/FuncEqualityComparer.cs New internal helper class that adapts Func predicates to IEqualityComparer interface with proper GetHashCode handling for custom comparers
TUnit.Assertions/Conditions/IsEquivalentToAssertion.cs Added Using(Func) overload to support lambda predicates for collection equivalency assertions
TUnit.Assertions/Conditions/NotEquivalentToAssertion.cs Added Using(Func) overload to support lambda predicates for negative collection equivalency assertions
TUnit.Assertions/Conditions/PredicateAssertions.cs Added Using(Func) overload to IsEquatableOrEqualToAssertion for custom equality predicates
TUnit.Assertions/Conditions/DictionaryAssertions.cs Added Using(Func) overload to DictionaryContainsKeyAssertion for custom key comparison predicates
TUnit.Assertions.Tests/Bugs/Tests1600.cs Added test cases demonstrating predicate-based assertions with lambda expressions and property comparisons
TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.*.verified.txt Updated API snapshots to reflect the new public Using(Func) overloads across all target frameworks

Comment on lines +25 to +56
[Test]
public async Task Custom_Predicate()
{
MyModel[] array1 = [new(), new(), new()];
MyModel[] array2 = [new(), new(), new()];

// Using a lambda predicate instead of implementing IEqualityComparer
await Assert.That(array1).IsEquivalentTo(array2).Using((x, y) => true);
}

[Test]
public async Task Custom_Predicate_With_Property_Comparison()
{
var users1 = new[] { new User("Alice", 30), new User("Bob", 25) };
var users2 = new[] { new User("Bob", 25), new User("Alice", 30) };

// Elements have different order but are equivalent by name and age
await Assert.That(users1)
.IsEquivalentTo(users2)
.Using((u1, u2) => u1?.Name == u2?.Name && u1?.Age == u2?.Age);
}

[Test]
public async Task Custom_Predicate_Not_Equivalent()
{
var users1 = new[] { new User("Alice", 30), new User("Bob", 25) };
var users2 = new[] { new User("Charlie", 35), new User("Diana", 28) };

await Assert.That(users1)
.IsNotEquivalentTo(users2)
.Using((u1, u2) => u1?.Name == u2?.Name && u1?.Age == u2?.Age);
}
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test coverage for the new predicate overloads is incomplete. While IsEquivalentTo and NotEquivalentTo assertions are well tested, there are no tests for the other two assertion types that received the same Using(Func) overload:

  1. IsEquatableOrEqualToAssertion (in PredicateAssertions.cs)
  2. DictionaryContainsKeyAssertion (in DictionaryAssertions.cs)

Consider adding test cases similar to these existing ones to verify that the predicate overload works correctly for IsEquatableOrEqualTo and ContainsKey assertions. This would ensure complete coverage of the new API surface.

Copilot uses AI. Check for mistakes.
@thomhurst thomhurst disabled auto-merge December 22, 2025 13:04
@thomhurst thomhurst merged commit cde7364 into main Dec 22, 2025
18 of 19 checks passed
@thomhurst thomhurst deleted the feature/4112 branch December 22, 2025 13:05
This was referenced Dec 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Missing custom equality predicate

2 participants