Use `IEquatable` instead of `Equals` where possible

[niklasda]

I run in to an expected test failure when I was substituting my Dictionary<> based cache. The key I use for the dictionary is a composite of 2 values, and we know that the key object has to implement IEquatable<> in order to find the value in the dictionary again. This works:

ICompositeKeyCache cache = new CompositeKeyCache();
cache.Add(new CompositeKey(Constants.KeyPart1, Constants.KeyPart2), Constants.ReturnValue);
string retVal = cache.Get(new CompositeKey(Constants.KeyPart1, Constants.KeyPart2));

Unfortunately the substitute I used for testing this failed in this case:

var cacheSub = Substitute.For<ICompositeKeyCache>();
cacheSub.Get(new CompositeKey(Constants.KeyPart1, Constants.KeyPart)).Returns(Constants.ReturnValue);
string retVal = cacheSub.Get(new CompositeKey(Constants.KeyPart1, Constants.KeyPart2));

After some investigation I learned that in order for the substitute to work the key objects also has to override the Equals() method. Overriding the Equals() method also works in the actual dictionary.

Somewhat annoying when the test fails and actual code works. I think implementing IEquatable<> should also work with NSubstitute.

see complete code here: https://github.com/niklasda/NSubstituteTestCaseProject

Keep up the good work

[dtchepak]

Thanks for the comprehensive repro case!

I haven't had a chance to dig into the NSubstitute side of this yet, but in the meantime I thought I'd offer a couple mitigations. First, I think the duplicate code in the object.Equals override can be simplified to:

        public override bool Equals(object obj)
        {
            var other = obj as CompositeKeyFail;
            return (other != null) && Equals(other);
        }

Secondly, if you have R# (or similar) you can generate equality members. In my VS it is ALT+Insert, and that will generate the IEquatable.Equals with an object.Equals that delegates to it.

This doesn't fix the root of the issue you are having but hopefully will make things easier for you while using the current NSub release.

[dtchepak]

Implementation note:

I think the EqualsArgumentMatcher is the relevant class.

This has given us problems before (with related tests) so need to make sure we don't break anything.

(Backup of original report repo: https://github.com/dtchepak/NSubstituteTestCaseProject)

[dtchepak]

Simple repro:

namespace NSubWorkshop
{
    using System;
    using NSubstitute;
    using Xunit;

    public class IntWrap : IEquatable<IntWrap>
    {
        public IntWrap(int value) {
            Value = value;
        }

        public int Value { get; set; }

        public int CompareTo(IntWrap other) {
            return Value.CompareTo(other.Value);
        }

        // Uncommenting this causes test to pass:
        //public override bool Equals(object obj) {
        //    return Equals(obj as IntWrap);
        //}

        public bool Equals(IntWrap other) {
            return other != null &&
                   Value == other.Value;
        }

        public override int GetHashCode() {
            return -1937169414 + Value.GetHashCode();
        }
    }

    public interface ISample
    {
        void Use(IntWrap value);
    }

    public class SampleFixture
    {

        [Fact]
        public void Sample() {
            var value1 = new IntWrap(42);
            var value2 = new IntWrap(42);
            var sub = Substitute.For<ISample>();

            sub.Use(value1);

            sub.Received().Use(value2);

        }
    }
}

[zvirja]

@dtchepak @alexandrnikitin Just want to clarify whether we still want to support this issue. As David stated above, if struct would override the Equals() method it would successfully work. To my knowledge, the IEquatable<> interface is an auxiliary util to avoid boxing and when it's implemented it's mandatory to override the Equals() method as well.

So if people are following best practices, they are fully compatible with NSubstitute. Does it make sense to specifically handle scenario when type itself is inconsistent (reports different equality status depending on the method is used)?

Would be also happy to see that I'm overlooking something.

[dtchepak]

@zvirja When you put it like that I'm happier to just use Equals. It is much easier to understand "all comparisons will be done with Equals" than "comparisons will be done with IEquatable if it exists otherwise Equals and if they disagree IEquatable will be preferred".