After update to v5, Equals() does not work as expected when using FluentAssersions' .BeEquevalentTo()

[dmytro-gokun]

After updating to v5 when using FluentAssertions's .BeEquiavalentTo() on classes having UnitNet's types as members, we get false negatives, like:

Expected property actual.Mileage to be 0 m, but found 0 km

This worked properly in v4.

As far as I can understand, the reason for this is that .BeEquiavalentTo() uses .Equals() method which is implemented as:

        public bool Equals(Length other)
        {
            return new { Value, Unit }.Equals(new { other.Value, other.Unit });
        }

IMO, this is incorrect behavior. 0 km == 0m, 1km == 1000 m etc. Is not that a valid assumption when writing unit tests?

[angularsen]

This is unfortunately by design and it has been discussed at length over years.

It seems trivial on the surface, but different quantities have units of different scale where 0 Fahrenheit != 0 Celsius. You you also very quickly run into rounding errors that affect equality. We simply couldn't find a one size fits all for equality, strict equality is at least easy to reason about although not always intuitive.

I believe it is best summarized here:
#1100 (comment)

[wenndemann]

Having the same problem after updating to v5. UnitsNet become useless for me since I have to deal with different units all around my project.
I totally agree to @dmytro-gokun that 0km == 0m. Your argument that 0 Celsius != 0 Fahrenheit is true but 0 Celsius == 32 Fahrenheit should be true.
Is the a possibility to change the default behavior of Equal? If not it should be considered to add support for this.

[dmytro-gokun]

@angularsen While i agree that '0 Fahrenheit != 0 Celsius', it also seems apparent to me that for cases where it makes sense (e.g. 0km and 0m) the comparison should yield true. Think of it this way: Length.Zero produces '0 m' and we may have some parsing code which yields results in kilometers. When we write test code, we are forced to do Length.FromKilometers(0) to make the library happy, which looks kind of cumbersome and flaky.

One could argue, that we should somehow instruct FluentAssertions to use proper Equals overload with proper ComparisonType and tolerance. I'd would argue though, that having some sensible default tolerance to be used by Equals() w/o parameter would make much more sense. It will keep happy all the folks who were happy with old behavior, while leaving an escape hatch for those who want to specify tolerance/ComparisonType manually.

[angularsen]

I'm very much open to suggestions on a better equality check, but please read up on the background for the strict equality decision first:
#1100 (comment)
#1017 (comment)

0 Celsius == 32 Fahrenheit should be true

Yes, but when multiplying or converting units several times, you would be looking at 0 Celsius == 31.9999999999 Fahrenheit.

If we set a default allowed rounding error (option 2 discussed in linked comments) so that 0 Celsius and 31.999.. Fahrenheit are equal, then we no longer satisfy the GetHashCode() requirement:

Two objects that are equal return hash codes that are equal.
https://learn.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-7.0#remarks

If we make the rounding error configurable for those that need it and accept the hash code mismatch, it would probably have to be a global static config. This has its own downsides, such as affecting libraries depending on UnitsNet assuming maybe the default behavior or several pieces of code fighting to manipulate the same config.

At least with configurable equality as opt-in, you make an informed choice. It will still not fix the problem that people discover this equality problem the hard way to begin with.

Hope this helps!

[tmilnthorp]

Very difficult to do. For example, there are 3 feet in 1 yard.

var l1 = UnitsNet.Length.FromYards(1.0);
var l2 = UnitsNet.Length.FromFeet(3.0);

Console.WriteLine(l1.Feet);
Console.WriteLine(l2.Yards);

Depending on the order you convert one to the other for comparison you get different results:

3
1.0000000000000002

How could we assume what tolerance is "good enough" for any particular application?

Is BeEquevalentTo the correct choice? Since we implement IComparable<T>, the doc would seem to indicate that you should be using the BeApproximately methods.

[dmytro-gokun]

@angularsen I think i understand the rational behind the decision now. Thank you 👍

But, i still needed some sort of solution to use UnitsNet's types with FluentAssertions. I ended up with this code which works just fine:

public static class NullableMassExtensions
{
    public static NullableMassAssertions Should(this Mass? @this)
    {
        return new NullableMassAssertions(@this);
    }
}

public sealed class NullableMassAssertions : ReferenceTypeAssertions<Mass?, NullableMassAssertions>
{
    #region Constructor

    public NullableMassAssertions(Mass? value)
        : base(value)
    {
    }

    #endregion

    #region ReferenceTypeAssertions implementation

    protected override string Identifier => nameof(Mass);

    #endregion

    #region Public methods

    public AndConstraint<NullableMassAssertions> Be(
        Mass? expected,
        string because = "",
        params object[] becauseArgs)
    {
        return BeApproximately(expected, 0, ComparisonType.Absolute, because, becauseArgs);
    }

    public AndConstraint<NullableMassAssertions> BeApproximately(
        Mass? expected,
        double tolerance,
        ComparisonType comparisonType,
        string because = "",
        params object[] becauseArgs)
    {
        Execute.Assertion
            .BecauseOf(because, becauseArgs)
            .ForCondition(Compare(Subject, expected, tolerance, comparisonType))
            .WithDefaultIdentifier(Identifier)
            .FailWith("Expected {context} to be {0}{reason}, but found {1}.", expected, Subject);

        return new AndConstraint<NullableMassAssertions>(this);
    }

    #endregion

    #region Private methods

    private static bool Compare(Mass? actual, Mass? expected, double tolerance, ComparisonType comparisonType)
    {
        return (actual, expected) switch
        {
            (null, null) => true,
            (not null, not null) => actual.Value.Equals(expected.Value, tolerance, comparisonType),
            _ => false,
        };
    }

    #endregion
}

The only problem here is that i have to duplicate this code for Length, Speed etc. I cannot make this generic because i cannot call Equals(expected.Value, tolerance, comparisonType) on an arbitrary type. If that method were on some interface (perhaps IQuantity<TUnitType>?) i would be able to do that. Do you think it would make sense to do such a change to the library? Are you willing to? :)

[angularsen]

@dmytro-gokun Proposal in #1215, please take a look if this will work for you.

[dmytro-gokun]

@angularsen Yes, that should work. If understand it correctly, with this change, the following code should compile and work as expected:

public static class NullableIQuantityExtensions
{
    public static NullableIQuantityAssertions<TSelf, TUnitType, TValueType> Should<TSelf, TUnitType, TValueType>(this TSelf? @this)
        where TSelf : IQuantity<TSelf, TUnitType, TValueType>
        where TUnitType : Enum
        where TValueType : struct
    {
        return new NullableIQuantityAssertions<TSelf, TUnitType, TValueType>(@this);
    }
}

public sealed class NullableIQuantityAssertions<TSelf, TUnitType, TValueType>
    : ReferenceTypeAssertions<TSelf?, NullableIQuantityAssertions<TSelf, TUnitType, TValueType>>
    where TSelf : IQuantity<TSelf, TUnitType, TValueType>
    where TUnitType : Enum
    where TValueType : struct
{
    #region Constructor

    public NullableIQuantityAssertions(TSelf? value)
        : base(value)
    {
    }

    #endregion

    #region ReferenceTypeAssertions implementation

    protected override string Identifier => nameof(TSelf);

    #endregion

    #region Public methods

    public AndConstraint<NullableIQuantityAssertions<TSelf, TUnitType, TValueType>> Be(
        TSelf? expected,
        string because = "",
        params object[] becauseArgs)
    {
        return BeApproximately(expected, default, ComparisonType.Absolute, because, becauseArgs);
    }

    public AndConstraint<NullableIQuantityAssertions<TSelf, TUnitType, TValueType>> BeApproximately(
        TSelf? expected,
        TValueType tolerance,
        ComparisonType comparisonType,
        string because = "",
        params object[] becauseArgs)
    {
        Execute.Assertion
            .BecauseOf(because, becauseArgs)
            .ForCondition(Compare(Subject, expected, tolerance, comparisonType))
            .WithDefaultIdentifier(Identifier)
            .FailWith("Expected {context} to be {0}{reason}, but found {1}.", expected, Subject);

        return new AndConstraint<NullableIQuantityAssertions<TSelf, TUnitType, TValueType>>(this);
    }

    #endregion

    #region Private methods

    private static bool Compare(TSelf? actual, TSelf? expected, TValueType tolerance, ComparisonType comparisonType)
    {
        return (actual, expected) switch
        {
            (null, null) => true,
            (not null, not null) => actual.Value.Equals(expected.Value, tolerance, comparisonType),
            _ => false,
        };
    }

    #endregion
}

[tmilnthorp]

I think it would still be easier for you to use BeInRange.

var length = Length.FromMeters(10.01);
var expected = Length.FromMeters(10.0);

// Check within 5% of expected
length.Should().BeInRange(expected - (expected * 0.05), expected + (expected * 0.05));

// Or check with 1mm tolerance
length.Should().BeInRange(expected - Length.FromMilliMeters(1.0), expected + Length.FromMilliMeters(1.0));

[angularsen]

That, and you always have the option of comparing the value for a known unit.

length.Meters.Should().BeApproximately(3.14, 0.01);