Skip to content
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

Investigate why Option doesn't play well with FluentAssertions .BeEquivalentTo() #96

Open
JohannesMoersch opened this issue Apr 20, 2020 · 7 comments
Milestone

Comments

@JohannesMoersch
Copy link
Owner

No description provided.

@RyanMarcotte
Copy link
Collaborator

This concerned me for a bit because of implications for Functional.Primitives.FluentAssertions, but I double-checked and the library uses Option<T>.Equals for its Be assertion.

That being said, I'll keep tabs on this in case a BeEquivalentTo method makes sense in the other library.

@JohannesMoersch
Copy link
Owner Author

I've had some issues in the past, but I never actually determined the source of the problem. It's possible this is a non-issue, but I wanted to do some testing just to make sure.

@RyanMarcotte
Copy link
Collaborator

RyanMarcotte commented Mar 1, 2023

I believe I finally encountered this issue. It popped up when using BeEquivalentTo to compare two different classes with similar schemas that have an identically-named property of different type Option<T>.

[Fact]
// this test currently fails
public void BeEquivalentTo()
{
    var parentA = new ParentA(new ChildA("TEST"));
    var parentB = new ParentB(new ChildB("TEST"));

    parentA.Should().BeEquivalentTo(parentB);
}

private class ParentA
{
    public ParentA(Option<ChildA> child)
    {
        Child = child;
    }

    public Option<ChildA> Child { get; }
}

private class ParentB
{
    public ParentB(Option<ChildB> child)
    {
        Child = child;
    }

    public Option<ChildB> Child { get; }
}

private abstract class BaseChild
{
    protected BaseChild(string value)
    {
        Value = value;
    }

    public string Value { get; }
}

private class ChildA : BaseChild
{
    public ChildA(string value)
        : base(value)
    {
    }
}

private class ChildB : BaseChild
{
    public ChildB(string value)
        : base(value)
    {
    }
}
Xunit.Sdk.XunitException
Expected property parentA.Child to be Some:Sample.Tests+ChildB, but found Some:Sample.Tests+ChildA.

With configuration:
- Use declared types and members
- Compare enums by value
- Compare tuples by their properties
- Compare anonymous types by their properties
- Compare records by their members
- Include non-browsable members
- Match member by name (or throw)
- Be strict about the order of items in byte arrays
- Without automatic conversion.

@RyanMarcotte
Copy link
Collaborator

Fixed it by defining the following equivalency step:

private class OptionEquivalencyStep<T> : IEquivalencyStep
{
    public EquivalencyResult Handle(Comparands comparands, IEquivalencyValidationContext context, IEquivalencyValidator nestedValidator)
    {
        if (!(comparands.Subject is Option<T> subjectOption))
            return EquivalencyResult.ContinueWithNext;
        if (!(comparands.Expectation is Option<T> expectationOption))
            return EquivalencyResult.ContinueWithNext;

        subjectOption.HasValue().Should().Be(expectationOption.HasValue());

        if (!subjectOption.HasValue() || !expectationOption.HasValue())
            return EquivalencyResult.AssertionCompleted;
        
        var subject = subjectOption.ThrowOnNone(() => new InvalidOperationException("Expected value in subject!"));
        var expectation = expectationOption.ThrowOnNone(() => new InvalidOperationException("Expected value in expectation!"));

        subject.Should().BeEquivalentTo(expectation);

        return EquivalencyResult.AssertionCompleted;

    }
}
parentA.Should().BeEquivalentTo(parentB, options => options.Using(new OptionEquivalencyStep<BaseChild>());

@JohannesMoersch
Copy link
Owner Author

Any idea why it doesn't just compare the internal fields as I expected? I'm hoping there is an alteration we can make to the primitives to make it "just work".

@JohannesMoersch JohannesMoersch added this to the Functional 3 milestone Apr 3, 2023
@RyanMarcotte
Copy link
Collaborator

That I don't know

@RyanMarcotte
Copy link
Collaborator

RyanMarcotte commented Jul 19, 2023

I also encountered the issue when comparing collections of Option<T> using BeEquivalentTo, but solved it using the same fix I posted above

private class SimpleClass
{
    public SimpleClass(Int32 value)
    {
        Value = value;
    }

    public Int32 Value { get; }
}
        
[Fact]
public void SampleTest()
{
    var optionList1 = new[] { Option.Some(new SimpleClass(1)), Option.None(), Option.Some(new SimpleClass(2)) };
    var optionList2 = new[] { Option.Some(new SimpleClass(1)), Option.None(), Option.Some(new SimpleClass(2)) };

    // this fails
    optionList1.Should().BeEquivalentTo(optionList2);
}
Xunit.Sdk.XunitException
Expected optionList1[0] to be Some:Tests+SimpleClass, but found Some:Tests+SimpleClass.
Expected optionList1[2] to be Some:Tests+SimpleClass, but found Some:Tests+SimpleClass.

With configuration:
- Use declared types and members
- Compare enums by value
- Compare tuples by their properties
- Compare anonymous types by their properties
- Compare records by their members
- Include non-browsable members
- Include all non-private properties
- Include all non-private fields
- Match member by name (or throw)
- Be strict about the order of items in byte arrays
- Without automatic conversion.
// this works
optionList1.Should().BeEquivalentTo(optionList2, options => options.Using(new OptionEquivalencyStep<SimpleClass>()));

Modifying SimpleClass to implement IEquatable<T> also fixes the problem.

I'm guessing that the problem is how this line and EqualityComparer<T>.Default works:

=> _hasValue == other._hasValue && (!_hasValue || EqualityComparer<TValue>.Default.Equals(_value, other._value));

The type T in Option<T> must implement IEquatable<T> or override both Object.Equals and Object.GetHashCode, per Microsoft documentation.

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

No branches or pull requests

2 participants