Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Comparison order used by ShouldEqual() may be incorrect #200

Closed
NameOfTheDragon opened this Issue · 4 comments

2 participants

@NameOfTheDragon

I came across this issue while working through Kent Beck's "TDD by Example" but with the added twist of using MSpec instead of nUnit. Please refer to http://stackoverflow.com/questions/21104942/why-doesnt-my-equals-method-get-called.

My classes implement a custom definition of equality which doesn't rely on the type of the object. Specifically, the base class and a subclass can be considered equal as long as the fields have appropriate values, the Equals method in the base class implements this definition of equality. There is a class diagram in the StackOverflow question.

When I use an assertion along the lines of baseClass.ShouldEqual(subClass) then that assertion always fails on the basis that the types aren't the same, even though my definition of equality allows that and Liskov is not violated.

ShouldEqual() uses ShouldExtensionMethods.SafeEquals<T>(actual, expected)) which uses AssertComparer<T>().Compare(left, right).

AssertComparer<T>.Compare() uses a list of comparer strategies that it gets from ComparerFactory.GetComparers<T>().

The list of comparer strategies is as follows:

        (IComparerStrategy<T>) new EnumerableComparer<T>(),
        (IComparerStrategy<T>) new GenericTypeComparer<T>(),
        (IComparerStrategy<T>) new TypeComparer<T>(),
        (IComparerStrategy<T>) new ComparableComparer<T>(),
        (IComparerStrategy<T>) new EquatableComparer<T>()

This results in type equality being tested before the class' own definition of equality. It is possible for any class to make its own definition of equality, including equality with a class of a different type (possibly a subclass base class, as in my example on Stack Overflow). I believe that MSpec should allow for this situation.

Shouldn't the comparison order use the class' custom definition of equality before the type-based comparison? In other words, should the order of the comparison strategies be as follows:

        (IComparerStrategy<T>) new EnumerableComparer<T>(),
        (IComparerStrategy<T>) new GenericTypeComparer<T>(),
        (IComparerStrategy<T>) new ComparableComparer<T>(),
        (IComparerStrategy<T>) new EquatableComparer<T>(),
        (IComparerStrategy<T>) new TypeComparer<T>()
@danielmarbach

Haven't put much thought into it because I'm tired and need to go to bed. But looking through your reasoning and the stackoverflow issue indicates that mspec should actually call your equals implementation but doesn't which is a bug. Under the assumption that the original author of that strategy selection did the order of the comparers on purpose changing the order would be a breaking change. But I think it would be for the good. I will give more thought to that tomorrow. Thanks for raising the issue

@danielmarbach

I looked again in your SO post. The tests should be:

  [Subject(typeof(ShouldExtensionMethods))]
  public class when_comparing_different_classes_with_equals_for_equality
  {
    Because of = () => FiveFrancs = new Franc(5, "CHF");
    It should_equal_money_with_currency_set_to_francs = () => FiveFrancs.Equals(new Money(5, "CHF")).ShouldBeTrue();
    static Franc FiveFrancs;
  }

  [Subject(typeof(ShouldExtensionMethods))]
  public class when_comparing_different_classes_with_should_equal_for_equality
  {
    Because of = () => FiveFrancs = new Franc(5, "CHF");
    It should_be_ten_francs_when_multiplied_by_2 = () => FiveFrancs.Times(2).ShouldEqual(Money.Franc(10));
    It should_be_fifteen_francs_when_multiplied_by_3 = () => FiveFrancs.Times(3).ShouldEqual(Money.Franc(15));
    static Franc FiveFrancs;
  }

  public class Money : IEquatable<Money>
  {
    readonly int _amount;
    readonly string _currency;

    public Money(int amount, string currency)
    {
      _amount = amount;
      _currency = currency;
    }

    public Money Times(int multiplier)
    {
      return new Money(multiplier * _amount, _currency);
    }

    public static Money Franc(int amount)
    {
      return new Franc(amount, "CHF");
    }

    public bool Equals(Money other)
    {
      return _amount == other._amount && _currency == other._currency;
    }

    public override bool Equals(object obj)
    {
      if (ReferenceEquals(null, obj))
        return false;
      if (ReferenceEquals(this, obj))
        return true;
      return Equals(obj as Money);
    }

    public override int GetHashCode()
    {
      unchecked
      {
        return (_amount * 997) ^ _currency.GetHashCode();
      }
    }
  }

  public class Franc : Money 
  {
    public Franc(int amount, string currency) : base(amount, currency)
    {
    }
  }

With those I could repro the bug.

@danielmarbach

I have this fixed on my machine. As soon as I push this issue will be closed. I'll keep you posted

@danielmarbach

@NameOfTheDragon can you try latest unstable release? The fix will be in 0.7.0. I'll close this issue

@danielmarbach danielmarbach referenced this issue in machine/machine.specifications.should
Closed

Comparison order used by ShouldEqual() may be incorrect #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.