Excluding and Including options should fail when applied on types with value semantics

[divega]

Description

I am updating the FluentAssertions package on a project from a very old version that would always do member-wise comparisons for Should().BeEquivalentTo(), even if the objects implemented IEquatable<T>. In general, I am ok with the new behavior, but here is a consequence I think is unexpected and undesirable: Explicit member exclusions are being completely ignored.

IMO, FluentAssertions should consider the fact that the implementation of IEquatable<T>.Equals is opaque and hence cannot be assumed to work in any particular way, so it should automatically fall back to perform member-wise comparisons when the assertion is configured with any type of explicit member exclusion.

In practice with the current behavior (which repros on 6.12 and 7.0.0 preview), I am forced to add ComparingByMembers<T>() specifying an explicit T (because it cannot be inferred by the compiler, which adds to the friction) for BeEquivalentTo() call that uses Excluding().

Reproduction Steps

// Paste this in a console app:

using FluentAssertions;
using FluentAssertions.Equivalency;

var actual = new SomeEquatableClass { Id = 1, Etag = "x" };
var expected = new SomeEquatableClass { Id = 1, Etag = "y" };

// This assertion fails, even if only Etag property has different values and it's being explicitly excluded:
actual.Should().BeEquivalentTo(expected, opt => opt.Excluding(o => o.Etag));

public class SomeEquatableClass : IEquatable<SomeEquatableClass>
{
    public int Id { get; set; }
    public string? Etag { get; set; }

    public override bool Equals(object? obj)
    {
        return Equals(obj as SomeEquatableClass);
    }
    public bool Equals(SomeEquatableClass? other)
    {
        if (other == null)
            return false;

        return Id == other.Id && StringComparer.OrdinalIgnoreCase.Equals(Etag, other.Etag);
    }

    public override int GetHashCode()
    {
        return HashCode.Combine(Id, Etag);
    }
}

Expected behavior

Explicit exclusions should never be ignored, and this assertion should pass:

actual.Should().BeEquivalentTo(expected, opt => opt.Excluding(o => o.Etag));

Actual behavior

Exclusions are ignored so this assertion fails:

actual.Should().BeEquivalentTo(expected, opt => opt.Excluding(o => o.Etag));

Regression?

This is a regression but only from very old FluentAssertions versions like 3.5.0.

Known Workarounds

• Add a call to ComparingByMembers<T>() whenever Excluding() is also used. This is particularly painful because T has to be specified explicitly:

   actual.Should().BeEquivalentTo(expected, 
       opt => opt.ComparingByMembers<SomeEquatableClass>().Excluding(o => o.Etag));

• Come up with your own exclusion extension that automates this and leverages type inference, like:

   public static EquivalencyAssertionOptions<TExpectation> ExcludingByMembers<TExpectation>(
       this EquivalencyAssertionOptions<TExpectation> options, 
       params string[] exclusions)
    {
        return options
            .ComparingByMembers<TExpectation>()
            .Excluding(memberInfo => 
                exclusions.Any(e => memberInfo.Path.Equals(e, StringComparison.OrdinalIgnoreCase)));
    }

Configuration

Repros on FA 6.12 and 7.0.0 preview.
Used to work fine with FA 3.5.0.
Tested on both .NET Framework 4.x and .NET 8.

Other information

Apart from this, the way FluentAssertions now leverages IEquatable<T>.Equals forced us to debug a bunch of incorrect equality evaluation code in our domain classes. While this is ultimately a good thing and I am thankful to FA team for this 😄, it feels like a different concern from what the tests were initially designed to cover. I know that the behavior can be customized, so this is just a comment on the choice of default behavior.

Note: I am a newbie using FluentAssertions so I will be the first to admit that my mental model might be lacking. I apologize if I am being naive or if I missed any discussion in which it was decided that this was by design (I did a search on the repo, but I did not find anything that seemed relevant).

Are you willing to help with a pull-request?

Maybe 😄

I am not sure how long it can take me to understand the FluentAssertions codebase well enough.

[dennisdoomen]

Looking at the example a bit closer, I notice that you also override Equals. That is the reason why FA assumes you want it to treat the type as having value semantics. It doesn't do anything specifically with IEquatable

[jnyrup]

First, thanks for the detailed and kind description ❤️

I see how Excluding() being ignored when FA choose to compare T using value semantics is not intuitive.

It seems to me the question is whether Excluding<T>(e => e.ETag) should also imply ComparingByMembers<T>()?
Or at least make some noise instead of silently being ignored.

The internal EqualityStrategyProvider.GetEqualityStrategy can be used to check how FA is currently going to compare Ts.
I emphasize currently because Excluding<T>(e => e.ETag).ComparingByMembers<T>() is also valid syntax.

I tried as a quick hack to change EquivalencyOptions<TExpectation>.Excluding to

public EquivalencyOptions<TExpectation> Excluding(Expression<Func<TExpectation, object>> expression)
{
    var strategy = EqualityStrategyProvider.GetEqualityStrategy(typeof(TExpectation));
    if (strategy is EqualityStrategy.Equals)
    {
        ComparingByMembers<TExpectation>();
    }

    AddSelectionRule(new ExcludeMemberByPathSelectionRule(expression.GetMemberPath()));
    return this;
}

and the provided test passed along will all existing tests 🤷

A problem with this hack is that it doesn't work for the non-generic SelfReferenceEquivalencyOptions.Excluding(Expression<Func<IMemberInfo, bool>> predicate) which leads to inconsistent behavior depending on which overload of Excluding() is used.
This could probably be workarounded by moving/copying the non-generic variant to EquivalencyOptions<TExpectation>, but that's a later discussion.

[divega]

Thanks a lot both @dennisdoomen and @jnyrup for looking into this. Interesting point that it’s about Equals(object) and not IEquatable<T>. I didn’t notice because all my classes under test have both, but also I didn’t think FA could differentiate between object.Equals and an override.

BTW, I remember trying using record instead of class while putting together the repro, and I was surprised that the behavior was different. IIRC, it did member-wise comparison for records. My understanding is that records also override Equals and implement IEquatable<T>, albeit automatically, so I am curious why and how FA treats them differently. I hope I am remembering what I saw correctly. Away from my computer at the moment.

[divega]

Today I found something that I believe aggravates the issue:

I have in my test code some assertion helper methods that do "soft" property matching like this:

opt = opt.Excluding(o => o.Path.Contains(ignoredField));

This code is supposed to exclude any top-level properties or nested properties that match the condition.

I can easily add a ComparingByMembers<T>() call for the top-level type because the specific T is known and available to the compiler from the generic arguments of the helper method, but for any of the nested properties, this doesn't work.

Currently investigating if there is a workaround I can apply for this that doesn't require me to list all the nested types my top-level types can contain.

[dennisdoomen]

It seems to me the question is whether Excluding<T>(e => e.ETag) should also imply ComparingByMembers<T>()?
Or at least make some noise instead of silently being ignored.

I don't think that is what we should be doing. Instead, we can fail the assertion when you try to use Including and Excluding on a type that is being treated as having value semantics. The only caveat is that we can only do that if the exclusion/inclusion can be directly associated with a particular path. A generic exclusion such as the one mention by @divega, cannot do that, since it may apply on any level.

[divega]

Instead, we can fail the assertion when you try to use Including and Excluding on a type that is being treated as having value semantics.

I actually agree that this is the most logic consequence given that you have decided that those types have value semantics. Any attempt to customize how comparison work for types that have value semantics should fail.

I expect that a small number of Fluent Assertions users could have exclusion calls that they didn't need and will experience the new behavior as a breaking change. But for the majority of users who did need the exclusions to work, there is an opportunity to throw an informative exception.

And I actually think that having to call ComparingByMembers<object>() or similar is a decent workaround if you need to do generic exclusions. It just isn't working great for me as reflected in #1757 (comment) 😞