Excluding and Including options should fail when applied on types with value semantics about fluentassertions HOT 6 OPEN

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

from fluentassertions.

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.

from fluentassertions.

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) 😞

from fluentassertions.

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.

from fluentassertions.

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.

from fluentassertions.

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.

from fluentassertions.