-
-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Excluding and Including options should fail when applied on types with value semantics #2571
Comments
Looking at the example a bit closer, I notice that you also override |
First, thanks for the detailed and kind description ❤️ I see how It seems to me the question is whether The internal I tried as a quick hack to change 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 |
Thanks a lot both @dennisdoomen and @jnyrup for looking into this. Interesting point that it’s about 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 |
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 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. |
I don't think that is what we should be doing. Instead, we can fail the assertion when you try to use |
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 |
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 implementedIEquatable<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 explicitT
(because it cannot be inferred by the compiler, which adds to the friction) forBeEquivalentTo()
call that usesExcluding()
.Reproduction Steps
Expected behavior
Explicit exclusions should never be ignored, and this assertion should pass:
Actual behavior
Exclusions are ignored so this assertion fails:
Regression?
This is a regression but only from very old FluentAssertions versions like 3.5.0.
Known Workarounds
ComparingByMembers<T>()
wheneverExcluding()
is also used. This is particularly painful becauseT
has to be specified explicitly: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.
The text was updated successfully, but these errors were encountered: