NumericAssertions does not handle NaN correctly

[jnyrup]

Description

NumericAssertions uses CompareTo to decide if a subject is e.g. smaller than the expected.
That is technically mixing different concepts, as CompareTo only decides the relative ordering between two items, i.e. how they should be sorted.
What NumericAssertions should be doing when "comparing" is arithmetic comparisons that corresponds to using operators like <, == and so on.

Complete minimal example reproducing the issue

double.NaN.Should().BeLessThan(0);

Expected behavior:

Should fail, as comparing NaN to anything using <, <=, ==, >= or > returns false.

Actual behavior:

The test passes.

Versions

• Which version of Fluent Assertions are you using?
  • 6.1.0 (master)

[dlemstra]

A possible solution for this could be:

public static class AssertionExtensions
{
    public static NumericAssertions<double> Should(this double actualValue)
    {
        return new DoubleAssertions(actualValue);
    }
}

// Could also be public but it doesn't add any other functionality so I thought it could be internal.
internal sealed class DoubleAssertions : NumericAssertions<double>
{
    public DoubleAssertions(double value)
       : base(value)
    {
    }

    private protected override bool IsNan(double value)
        => double.IsNaN(value);
}

[DebuggerNonUserCode]
public class NumericAssertions<T, TAssertions>
    where T : struct, IComparable<T>
    where TAssertions : NumericAssertions<T, TAssertions>
{
    private protected virtual bool IsNan(T value)
        => false;

    public AndConstraint<TAssertions> BeLessThan(T expected, string because = "", params object[] becauseArgs)
    {
        Execute.Assertion
            // Checking both Subject and expected, but maybe this should be a separate assertion?
            .ForCondition(Subject.HasValue && !IsNan(Subject.Value) && !IsNan(expected) && Subject.Value.CompareTo(expected) < 0)
            .BecauseOf(because, becauseArgs)
            .FailWith("Expected {context:value} to be less than {0}{reason}, but found {1}.", expected, Subject);

        return new AndConstraint<TAssertions>((TAssertions)this);
    }
}

I don't mind creating a PR for this if the team would consider this a good option. And there should probably also be some extra checks for float.PositiveInfinity and double.NegativeInfinity but that feels like something for a separate PR.

[dennisdoomen]

It's debatable, see for instance this discussion. In .NET 4.0, sorting seems to treat them as smaller than everything else.

[jnyrup]

I don't follow what part is debatable here.
The link is about how floating points are ordered.
Putting NaN before everything else (to me) just seems like a necessary arbitrary choice, exactly because NaN is false compared to anything according to IEEE standards.

Similarly for ComparableTypeAssertions we took the choice to use Equals for Be and CompareTo() == 0 for BeRankedEquallyTo.

[dennisdoomen]

My point is that since .NET 4.0 orders NaN in a certain way, BeLessThan should follow the same semantics.

[jnyrup]

Thanks for clarifying, now I'm sure that I disagree.
I think BeLessThan should give the same outcome as using <.

[dennisdoomen]

That's exactly what I'm trying to say ;-)

No wait, now I'm confused. I was testing this in my IDE. But now I'm not sure anymore if I was using > and < or IComparable<T>.

[jnyrup]

With static abstract methods in interfaces this would have been easy.

public class NumericAssertions<T>
    where T : INumber<T>
{
    private readonly T subject;

    public NumericAssertions(T subject) => this.subject = subject;
    
    public void BeLessThan(T expected)
    {
        var succes = subject < expected; // https://source.dot.net/#System.Private.CoreLib/IComparisonOperators.cs,26
    }
}

sharplab

We could use the inheritance approach as suggested above, but instead of IsNaN expose the concrete comparison operators.

internal sealed class DoubleAssertions : NumericAssertions<double>
{
    public DoubleAssertions(double value)
       : base(value)
    {
    }

    private protected override bool AreEqual(double a, double b) => a == b;

    private protected override bool AreNotEqual(double a, double b) => a != b;
    
    private protected override bool IsLessThan(double a, double b) => a < b;
    
    private protected override bool IsLessThanOrEqualTo(double a, double b) => a <= b;
    
    private protected override bool IsGreaterThan(double a, double b) => a > b;
    
    private protected override bool IsGreaterThanOrEqualTo(double a, double b) => a >= b;
}

[dennisdoomen]

That would be great, if it wasn't a preview feature only available in .NET 6....