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

Null reference exception when using custom comparer in equivalency options #2595

Open
SvetlanaBurlakova opened this issue Feb 28, 2024 · 6 comments

Comments

@SvetlanaBurlakova
Copy link

SvetlanaBurlakova commented Feb 28, 2024

Description

Look at the code below. The second test fails with unexpected NRE.
I'm using the latest alpha version: 7.0.0-alpha.3
.Net 6

using NUnit.Framework;

namespace UnitTests;

public class FluentAssertionsEquivalencyFailureTest
{
    [Test]
    public void Success()
    {
        var expected = new TestClass();
        var actual = new TestClass() { NullableField = DateTime.UtcNow };

        FluentActions.Invoking(() => actual.Should().BeEquivalentTo(expected))
            .Should()
            .Throw<AssertionException>()
            .WithMessage("Expected property actual.NullableField to be <null>, but found <*>.*");
    }

    [Test]
    public void NullReferenceFailure()
    {
        var expected = new TestClass();
        var actual = new TestClass() { NullableField = DateTime.UtcNow };

        FluentActions.Invoking(() => actual.Should().BeEquivalentTo(expected, o => o.Using(EqualityComparer<DateTime>.Default)))
            .Should()
            .Throw<AssertionException>()
            .WithMessage("Expected property actual.NullableField to be <null>, but found <*>.*");
        
        /*
Expected a <NUnit.Framework.AssertionException> to be thrown, but found <System.NullReferenceException>: "
"System.NullReferenceException with message "Object reference not set to an instance of an object."
     at FluentAssertions.Equivalency.Steps.EqualityComparerEquivalencyStep`1.<>c__DisplayClass2_0.<Handle>b__0() in /_/Src/FluentAssertions/Equivalency/Steps/EqualityComparerEquivalencyStep.cs:line 28
     at FluentAssertions.Execution.GivenSelector`1..ctor(Func`1 selector, AssertionScope predecessor, Boolean continueAsserting) in /_/Src/FluentAssertions/Execution/GivenSelector.cs:line 23
     at FluentAssertions.Execution.AssertionScope.Given[T](Func`1 selector) in /_/Src/FluentAssertions/Execution/AssertionScope.cs:line 214
     at FluentAssertions.Execution.ContinuedAssertionScope.Given[T](Func`1 selector) in /_/Src/FluentAssertions/Execution/ContinuedAssertionScope.cs:line 28
     at FluentAssertions.Equivalency.Steps.EqualityComparerEquivalencyStep`1.Handle(Comparands comparands, IEquivalencyValidationContext context, IEquivalencyValidator nestedValidator) in /_/Src/FluentAssertions/Equivalency/Steps/EqualityComparerEquivalencyStep.cs:line 23
     at FluentAssertions.Equivalency.Steps.RunAllUserStepsEquivalencyStep.Handle(Comparands comparands, IEquivalencyValidationContext context, IEquivalencyValidator nestedValidator) in /_/Src/FluentAssertions/Equivalency/Steps/RunAllUserStepsEquivalencyStep.cs:line 11
     at FluentAssertions.Equivalency.EquivalencyValidator.RunStepsUntilEquivalencyIsProven(Comparands comparands, IEquivalencyValidationContext context) in /_/Src/FluentAssertions/Equivalency/EquivalencyValidator.cs:line 69
     at FluentAssertions.Equivalency.EquivalencyValidator.RecursivelyAssertEquality(Comparands comparands, IEquivalencyValidationContext context) in /_/Src/FluentAssertions/Equivalency/EquivalencyValidator.cs:line 39
     at FluentAssertions.Equivalency.Steps.StructuralEqualityEquivalencyStep.AssertMemberEquality(Comparands comparands, IEquivalencyValidationContext context, IEquivalencyValidator parent, IMember selectedMember, IEquivalencyAssertionOptions options) in /_/Src/FluentAssertions/Equivalency/Steps/StructuralEqualityEquivalencyStep.cs:line 72
     at FluentAssertions.Equivalency.Steps.StructuralEqualityEquivalencyStep.Handle(Comparands comparands, IEquivalencyValidationContext context, IEquivalencyValidator nestedValidator) in /_/Src/FluentAssertions/Equivalency/Steps/StructuralEqualityEquivalencyStep.cs:line 45
     at FluentAssertions.Equivalency.EquivalencyValidator.RunStepsUntilEquivalencyIsProven(Comparands comparands, IEquivalencyValidationContext context) in /_/Src/FluentAssertions/Equivalency/EquivalencyValidator.cs:line 69
     at FluentAssertions.Equivalency.EquivalencyValidator.RecursivelyAssertEquality(Comparands comparands, IEquivalencyValidationContext context) in /_/Src/FluentAssertions/Equivalency/EquivalencyValidator.cs:line 39
     at FluentAssertions.Equivalency.EquivalencyValidator.AssertEquality(Comparands comparands, EquivalencyValidationContext context) in /_/Src/FluentAssertions/Equivalency/EquivalencyValidator.cs:line 21
     at FluentAssertions.Primitives.ObjectAssertions`2.BeEquivalentTo[TExpectation](TExpectation expectation, Func`2 config, String because, Object[] becauseArgs) in /_/Src/FluentAssertions/Primitives/ObjectAssertions.cs:line 145
     at UnitTests.FluentAssertionsEquivalencyFailureTest.<>c__DisplayClass1_0.<NullReferenceFailure>b__0() in C:\Telgorithm\src\phone\tmp\My.UnitTests\FluentAssertionsFailureTest.cs:line 25
     at FluentAssertions.Specialized.FunctionAssertions`1.InvokeSubject() in /_/Src/FluentAssertions/Specialized/FunctionAssertions.cs:line 26
     at FluentAssertions.Specialized.DelegateAssertions`2.InvokeSubjectWithInterception() in /_/Src/FluentAssertions/Specialized/DelegateAssertions.cs:line 221
.
   at FluentAssertions.Execution.LateBoundTestFramework.Throw(String message) in /_/Src/FluentAssertions/Execution/LateBoundTestFramework.cs:line 18
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message) in /_/Src/FluentAssertions/Execution/TestFrameworkProvider.cs:line 34
   at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message) in /_/Src/FluentAssertions/Execution/DefaultAssertionStrategy.cs:line 25
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc) in /_/Src/FluentAssertions/Execution/AssertionScope.cs:line 274
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc) in /_/Src/FluentAssertions/Execution/AssertionScope.cs:line 246
   at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args) in /_/Src/FluentAssertions/Execution/AssertionScope.cs:line 296
   at FluentAssertions.Specialized.DelegateAssertionsBase`2.ThrowInternal[TException](Exception exception, String because, Object[] becauseArgs) in /_/Src/FluentAssertions/Specialized/DelegateAssertionsBase.cs:line 35
   at FluentAssertions.Specialized.DelegateAssertions`2.Throw[TException](String because, Object[] becauseArgs) in /_/Src/FluentAssertions/Specialized/DelegateAssertions.cs:line 48
   at UnitTests.FluentAssertionsEquivalencyFailureTest.NullReferenceFailure() in C:\Telgorithm\src\phone\tmp\My.UnitTests\FluentAssertionsFailureTest.cs:line 25
        */
    }
}

public class TestClass
{
    public DateTime? NullableField { get; set; }
}

Reproduction Steps

For reproduction please look at the description - there is a code sample which reproduces the error.

Expected behavior

Expected that the second test behaves exactly like the first.

Actual behavior

Unexpected NRE is thrown.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Are you willing to help with a pull-request?

No

@ITaluone
Copy link
Contributor

What happens if you change actual.Should().BeEquivalentTo(expected, o => o.Using(EqualityComparer<DateTime>.Default)) to actual.Should().BeEquivalentTo(expected, o => o.Using(EqualityComparer<DateTime?>.Default)) ?

@SvetlanaBurlakova
Copy link
Author

SvetlanaBurlakova commented Mar 4, 2024

@ITaluone If I change, it does not use the comparer at all.

If you change in my example, it will work, because it will compare two nullble DateTimes with the default way, but in the real project I have custom comparer, and it is not used in case it is of DateTime?. Let me prepare more detailed example code for you:

using NUnit.Framework;

namespace UnitTests;

public class FluentAssertionsEquivalencyFailureTest
{
    [Test]
    public void Success()
    {
        var expected = new TestClass();
        var actual = new TestClass() { NullableField = DateTime.UtcNow };

        FluentActions.Invoking(() => actual.Should().BeEquivalentTo(expected))
            .Should()
            .Throw<AssertionException>()
            .WithMessage("Expected property actual.NullableField to be <null>, but found <*>.*");
    }

    [Test]
    public void NullReferenceFailure()
    {
        var expected = new TestClass();
        var actual = new TestClass() { NullableField = DateTime.UtcNow };

        FluentActions.Invoking(() => actual.Should().BeEquivalentTo(expected, o => o.Using(EqualityComparer<DateTime>.Default)))
            .Should()
            .Throw<AssertionException>()
            .WithMessage("Expected property actual.NullableField to be <null>, but found <*>.*");
    }

    [Test]
    public void TestToShowThatNullableDateTimeComparerIsNotUsed()
    {
        var dateTimeValue1 = DateTime.UtcNow;
        var dateTimeValue2 = new DateTime(dateTimeValue1.Ticks + 1, dateTimeValue1.Kind);

        var expected = new TestClass() { NullableField = dateTimeValue1 };
        var actual = new TestClass() { NullableField = dateTimeValue2 };

        FluentActions.Invoking(() => actual.Should().BeEquivalentTo(expected, o => o.Using(CustomNullableDateTimeComparer.Instance)))
            .Should()
            .NotThrow();

        // Did not expect any exception, but found NUnit.Framework.AssertionException with message
        // "Expected property actual.NullableField to be <2024-03-04 06:48:49.3389094>, but found <2024-03-04 06:48:49.3389095>.
    }

    [Test]
    public void CustomNullableDateTimeComparer_Works()
    {
        var dateTimeValue1 = DateTime.UtcNow;
        var dateTimeValue2 = new DateTime(dateTimeValue1.Ticks + 1, dateTimeValue1.Kind);

        CustomNullableDateTimeComparer.Instance.Equals(dateTimeValue1, dateTimeValue2).Should().BeTrue(); //ok

        var hashCode1 = CustomNullableDateTimeComparer.Instance.GetHashCode(dateTimeValue1);
        var hashCode2 = CustomNullableDateTimeComparer.Instance.GetHashCode(dateTimeValue2);
        hashCode1.Should().Be(hashCode2); //ok
    }
}

public class CustomNullableDateTimeComparer : IEqualityComparer<DateTime?>
{
    public static readonly CustomNullableDateTimeComparer Instance = new();

    private const long TicksEpsilon = 1000;

    private static DateTime? Round(DateTime? x) =>
        x is null ? null : new DateTime(x.Value.Ticks / TicksEpsilon * TicksEpsilon);

    public bool Equals(DateTime? x, DateTime? y) => Round(x).Equals(Round(y));
    public int GetHashCode(DateTime? obj) => Round(obj).GetHashCode();
}

public class TestClass
{
    public DateTime? NullableField { get; set; }
}

To put it in a nutshell,

  1. IEqualityComparer<SomeValueType> is always used to compare values of nullable SomeValueType?
  2. When comparing two nullable SomeValueType?s it always results in a NRE when the expectation is null but the actual value is not null because in the source code the nullable value is unsafely cast to (T) without checking it for nullability

@arocheleau
Copy link
Contributor

arocheleau commented Mar 22, 2024

The test named NullReferenceFailure in the description seems to work properly since the fix for #2489.
The test named TestToShowThatNullableDateTimeComparerIsNotUsed still need to be analyzed.

@arocheleau
Copy link
Contributor

arocheleau commented Apr 10, 2024

The reason the test named TestToShowThatNullableDateTimeComparerIsNotUsed failed seems to have to do with the fact that the EqualityComparerEquivalencyStep<T> step is not handling the comparison.

The first thing it does is making sure the types match.

if (comparands.GetExpectedType(context.Options) != typeof(T))
{
    return EquivalencyResult.ContinueWithNext;
}

In the current scenario, the types being compared are DateTime vs DateTime?. Since they don't match, the current step is not handling the comparison. It will eventually be handled by the ValueTypeEquivalencyStep step which do not invoke the user comparer. Also, the left part of the if statement strips the nullable part of a type and returns its underlying type while the right part keep it as it is.

From my point of view, the fix would be to modify the right part of the if so it returns the underlying type as well, comparing apples to apples.

if (comparands.GetExpectedType(context.Options) != typeof(T).NullableOrActualType())
{
    return EquivalencyResult.ContinueWithNext;
}

@dennisdoomen
Copy link
Member

There are a couple of things that I think might be wrong here:

  • The NullReferenceFailure test should not even use the EqualityComparer<DateTime>, since T is a DateTime and the property on the expectation is a DateTime?. However, you could argue that we should support using an IEqualityComparer<T> where a T? property is detected.
  • If you add an IEqualityComparer<T?> like TestToShowThatNullableDateTimeComparerIsNotUsed does, then it should most definitely match a property of T?. However, this fails because the comparands.GetExpectedType call from EqualityComparerEquivalencyStep strips of the nullable part. The fix that @arocheleau suggest should fix that.
  • But this does not fix the original exception at the top of this issue. This is caused by the lambda passed to the Given call in EqualityComparerEquivalencyStep

So this bug need two individual fixes.

@dennisdoomen
Copy link
Member

Sorry for the confusion, but with the current state of develop there's only one issue left, which is that if a property is nullable, an IEqualityComparer<Nullable> should match and be used. However, the opposite is not true. If the property is non-nullable, then only a non-nullable IEqualityComparer should match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants