ComparingByMembers<object> lets structural equality assertions always succeed

[ursenzler]

Description

I want to be able to configure FluentAssertions' structural equality so that it uses ComparingByMembers for all types.
The background for this is that I have a mix of F# and C# code and all F# records implement Equals by default. In case of an object-graph with a mix starts using Equals when hitting the first F# record, as well for the nested C# classes.

I tried ComparingByMembers<object>, but that results in the tests always being succeeded. And I guess that should be fixed.

Complete minimal example reproducing the issue

using FluentAssertions;
using Xunit;

namespace CSharp.FluentAssertionWrapper
{
    public class Facts
    {
        public struct C
        {
            public D D { get; set; }
        }
        public class D
        {
            public string S { get; set; }
            public int I { get; set; }
        }

        [Fact]
        public void Foo()
        {
            var a = new C { D = new D { S = "a", I = 17 } };
            var b = new C { D = new D { S = "a", I = 18 } };

            a.Should().BeEquivalentTo(   // this should fail, but succeeds
                b,
                config => config.ComparingByMembers<object>());
        }
    }
}

Expected behavior:

All types are structurally compared correctly.

Actual behavior:

Assertion always succeeds.

Versions

• Which version of Fluent Assertions are you using? 5.10.3
• Which .NET runtime and version are you targeting? netcoreapp3.1

Additional Information

None.

[ursenzler]

After some debugging through FluentAssertions, I found out that the above test fails when the string values for S differ, but not when the integer values for I differ.

I think the problem is that when ComparingByMembers<object> is used then FluentAssertion tries to compare basic types by their properties. They have none, so they are treated es structurally equal.

Adding a ComparingByValue<int> does not work because either FluentAssertion says that the type is already set up to be compared by members (when put below) or because it is ignored (when put ahead).

This leads me to the conclusion that I would need something like: compare all reference types by members.
But as it looks to me, that would be very hard to be implemented in the current code base :-(

[dennisdoomen]

Is there a way to detect that a type is F#-specific?

[ursenzler]

Yes. From F# code you can use e.g. FSharpType.IsRecord

[ursenzler]

Another solution for my problem would also be when I could register a callback with the following signature:

Func<Type, ComparisonMode> with ComparisonMode something like enum ComparisonMode { ByMember, ByValue }

Then I could do the checking for F# types on my own.

[dennisdoomen]

That's what I was thinking,e.g. ComparingByMembers(Func<Type, bool> predicate)

[ursenzler]

I think that would work.

Should I try to contribute that?
I probably need some help though. I don't really understand the code around SelfReferenceEquivalencyAssertionOptions, which is somehow involved in the affected code.

[ursenzler]

We probably should change the Signature to:

ComparingByMembers(Func<Type, bool?> predicate)

So I can pass null when I want to use the default behaviour. Otherwise, I have to handle every type on my own.

I was able to build a prototype and will check some cases with it.

[dennisdoomen]

It could be just an overload of ComparingByMembers. The internal referenceTypes and valueTypes collection would need to change to a collection class that contains Func<Type, bool>.

[ursenzler]

You can find my prototype at https://github.com/ursenzler/fluentassertions/tree/comparingByMembersCallback.
I went a bit a different approach. I don't see the solution with a collection of Funcs.

My use case works correctly this way.

I'm not sure how you'd like the tests to look like. I didn't find a way to align them with the existing tests because the new method is an additional way to accomplish what is already there.
If you let me know, I can add them.

[dennisdoomen]

Thanks for taking a stab at it. The problem with your implementation is that multiple calls to your overload will only remember the last predicate, which is different from the other methods. That's why I was suggesting to have a collection of predicates.

[ursenzler]

Ah, now I understand:

Collecting a collection of Func<Type, bool>
When checking for equality then look if there is an entry for the type in the collection:
| found none -> continue as before
| found 1 -> apply it and use the mode returned if any
| found > 1 -> should probably be prohibited by checking that there is not yet an entry for the same type when defining with `ComparingMembers(Func<...>)

I'll change that...

[ursenzler]

Okay, after misunderstanding everything :-D, here a new - hopefully better - version: https://github.com/ursenzler/fluentassertions/blob/f3598478a92f20172a5d8fcb69d0a65aa93ad1a2/Src/FluentAssertions/Equivalency/SelfReferenceEquivalencyAssertionOptions.cs#L165

This time with some Facts, as well.

[dennisdoomen]

Yep, this looks much closer to what I had in mind.

[jnyrup]

Sorry for being late at the party.

The bug here seems to stem from

EqualityStrategy IEquivalencyAssertionOptions.GetEqualityStrategy(Type type)
{
    EqualityStrategy strategy;

    if (referenceTypes.Any(type.IsSameOrInherits))
    {
        strategy = EqualityStrategy.ForceMembers;
    }

where int satisfies referenceTypes.Any(type.IsSameOrInherits) due to referenceTypes containing typeof(object) but it does not make sense (to me) to compare a primitive type by its members.
In fact the Foo example fails in net47 (but not in netcoreapp) with a the "The maximum recursion depth was reached".

A solution that seems to work is to change the conditional to

if (!type.IsPrimitive && referenceTypes.Any(type.IsSameOrInherits))

With that change the Foo example now fails with the expected

Expected member E.I to be 18, but found 17

[ursenzler]

That could be a simpler solution than #1383

I'll check it against our codebase.

[jnyrup]

@ursenzler When you get around testing against your codebase the fix has been merged to develop.

[ursenzler]

@jnyrup First, thanks for the info!
Finally finding some time to give this a try. I ran into a problem using the current development branch.
We use some EquivalencySteps and in them, we use EquivalencyAssertionOptionsExtensions, which was changed to internal.
How can I access the expected type of an assertion in V6?
All internal code in V6 that I found, still uses EquivalencyAssertionOptionsExtensions.

[ursenzler]

But in general, I think it works - I got some simpler scenarios, which don't need equivalency steps, to work.
So, should I close my PR (#1383)?

[jnyrup]

@ursenzler Glad to hear it worked out.
You can close #1383

Regarding EquivalencyAssertionOptionsExtensions:

Strictly speaking this a breaking change as the class is public.
I don't think anyone is using this, but I would keep it as it is.
In a future release, the typo should be fixed and marking the class as internal.

#1088 (comment)_

I'm open to keep it public in v6.

Note that the class is currently undergoing some changes in https://github.com/fluentassertions/fluentassertions/pull/1379/files#diff-5aca5b14c46d3fb96ba470da10e4efe4

[dennisdoomen]

Since it's extending our own code, I think I need to move it either to the IEquivalencyAssertionOptions or IEquivalencyContext interfaces.