mocked.Equals(mocked) returns false by default if mocked class overrides Equals

[stakx]

This is a follow-up to #791 (comment).

In the absence of any setup for Equals, Moq currently (attempts to) provide a default implementation that performs reference equality (as is the default in .NET for reference types). @BrunoJuchli discovered that when a mocked type overrides object.Equals, but does not provide a setup for it, then that default implementation will not execute:

Reproduction code:

public class X { }

public class Y
{
    public override bool Equals(object obj) => base.Equals(obj);
}

var xMock = new Mock<X>();
Assert.True(xMock.Object.Equals(xMock.Object));

var yMock = new Mock<Y>();
Assert.True(yMock.Object.Equals(yMock.Object));

Actual behavior:

The second assertion (i.e. the one on yMock) fails.

Expected behavior:

Both assertions should pass.

Possible solution:

In the following line:

https://github.com/moq/moq4/blob/bf19ad64540b9e3e71b8a869b2b1ab204ff1dbcb/src/Moq/Interception/InterceptionAspects.cs#L101

change method.DeclaringType to method.GetBaseDefinition().DeclaringType.

There's possible one or two other code locations where the same change would have to be applied. We should also try to keep the added runtime cost for this change minimal (if possible).

[BrunoJuchli]

@stakx
Thanks a lot!
Please note that for us this is not a big issue. We only override Equals when we also implement IEquatable<T> - which we setup in this case anyway. So we wrote an extension method .SetupEqualsSelf() which - for now - setups both object.Equals(object) and T.Equals(T).

Thus, "fixing" the behavior means we'd get to remove a single line from that extension method.

So absolutely no pressure from my side to fix this, but I also think it's good that you created the issue so if someone else has a problem with equals behavior he/she might find it ;-)

[stakx]

Glad to hear that this isn't blocking you. But I'll still look into a bugfix.

_{(Off-topic rumination: I had been thinking about a potential new feature for Moq that would give users the ability to register default implementations for whole interfaces. I was originally considering this in the context of people mocking Entity Framework DbContext and DbSet, and often getting it wrong due to the many involved interfaces. Such a feature just might prove useful to your use case as well, though. At this point, however, that feature is mostly a semi-crazy thought experiment.)}

[stakx]

There are three unit tests for object.ToString() which are very relevant here, and make me suspect that it might be best to keep the status quo unchanged.

(Click to expand and see the unit tests.)

https://github.com/moq/moq4/blob/bf19ad64540b9e3e71b8a869b2b1ab204ff1dbcb/tests/Moq.Tests/MockFixture.cs#L73-L78

https://github.com/moq/moq4/blob/bf19ad64540b9e3e71b8a869b2b1ab204ff1dbcb/tests/Moq.Tests/MockFixture.cs#L89-L94

https://github.com/moq/moq4/blob/bf19ad64540b9e3e71b8a869b2b1ab204ff1dbcb/tests/Moq.Tests/MockFixture.cs#L97-L102

The last two tests hint at the distinction Moq makes between "partial mocks" and "full mocks":

• "Partial mocks" are what you get when you set CallBase = true, which allows the base class' implementation to surface in the gaps between setups.

• "Full mocks" mean that you need to provide all implementations, and the base implementation stays stowed away. The "gaps between setups" are filled with default values (loose mocks) or exceptions (strict mocks).

Moq's provisioning of default object member implementations does not fit neatly into that scheme: They are modelled after .NET's default object member implementations, and as such mean that mocks are always "partial mocks" with respect to object members. However, as soon as the mocked type overrides these members itself, things start falling apart:

1. According to what I just wrote, you'd think that Moq's default implementation would have to perform a CallBase to stay true to the "partial mocks" behavior... even when CallBase == false.

2. However, that is a breaking change for object.ToString because it would break one of the tests mentioned above.

3. We could apply (1) to just object.Equals and leave object.ToString as it is today; but then we have even less consistency.

So any change we might make appears to make matters worse (less consistent) in some way. If we leave everything as is, we have at least some consistency, even though it 's not always sensible in cases such as the one that led to this issue being raised.

• If there's no overrides in the base class, Moq will provide a default implementation very close to what .NET default object behavior would do. This only violates the "partial" / "full" mock when CallBase == false.

• If there are overrides in the base class, or if CallBase == true, the "partial" / "full" distinction comes into play and there are clear rules defining the mode of operation.

[BrunoJuchli]

@stakx
Thanks for looking into this and explaining it to us in that level of detail.

Generally I completely agree with you - with a twist.

Nitpickers Corner

If there's no overrides in the base class, Moq will provide a default implementation very close to what .NET default object behavior would do. This only violates the "partial" / "full" mock when CallBase == false.

IMO it's not "a default implementation very close to" but rather it's 100% identical. It might seem like nitpicking but I think it's relevant.

The Twist

What's the reason to have loose mocks? We don't want to have to setup stuff which we don't care about = which are not relevant for the test.
Example: system under test (SUT) is logging a parameter which uses .ToString().
In our test case, we're not interested in how and whether logging is performed, because either of:

• this is an aspect we never test (this is the case for most projects I worked on)
• we separate tests by aspects and each test is covering one single aspect. Each test is responsible for setting up all relevant parts of the aspect. As logging is a cross-cutting concern, it's something which happens in each test case but we'd probably consider it noise and thus would like to ignore it.

If, however, we fail to setup something which is relevant for the test we've made a mistake. The test will fail, but observed symptom (failure message) might not directly point us to the missing setup.
I guess this is why strict mocks have been invented ;-)

Now let me get back to the loose mock.
My thesis is that if the SUT calls .Equals() it's always relevant for the test. Equals(). Different from .ToString(), which just provides data which usually is logic irrelevant - .Equals() is used to make distinctions - which is logic relevant.
If calls to Equals() are always relevant for the SUT, a wrongly setup .Equals() method is always a problem, too.

Luckily moq has introduced a slight inconsistency and, as long as i don't override .Equals(), the mock has reference-equality semantics exactly identical to the real thing.
So for most cases - as long as there's no override of .Equals() - things just work out of the box.

Imagine moq not having done so. How many times would you have had to setup .Equals()? How would you have known to do so? How arduous would that have been? After all, we usually take reference equality as a "default for granted". It's not something you think about a lot when you don't alter the Equals implementation (by overriding Equals).

Now let me get back to overridden Equals implementations. If, as I expect, calls to .Equals() are always relevant and I forget to setup an overridden Equals implementation, I'll always run into troubles. A test might pass because I expect a filter to return an empty list - but just because equals defaults to false. Another test will fail because I expected a result but I don't get any - because equals defaults to false. Now I might fix this one, and it will be correct. But I might forget about the "return empty" test - and leave it broken. Why? It passes...
So here, with Equals, strict semantics would really help.

And that's why I come back to my proposal of throwing an exception.

• mocked doesn't override equals => provide reference equality out of the box
• mocked overrides equals => provide strict behavior (throw an exception) by default

which, to me, seems an inconsistent but very much sensible design choice.
...of course, only in case I'm right with an incorrectly set-up Equals method always being a problem...

---

I had been thinking about a potential new feature for Moq that would give users the ability to register default implementations for whole interfaces. I was originally considering this in the context of people mocking Entity Framework DbContext and DbSet, and often getting it wrong due to the many involved interfaces. Such a feature just might prove useful to your use case as well, though. At this point, however, that feature is mostly a semi-crazy thought experiment.)

That could be very useful for implementing equality behavior. As I mentioned in another post, for "our" use case it would be very neat if we could register a custom default-setup for IEquatable<T>.Equals<T> to implement reference equality semantics.

[stakx]

[...] my proposal of throwing an exception.

• mocked doesn't override equals => provide reference equality out of the box
• mocked overrides equals => provide strict behavior (throw an exception) by default

I've let this issue stew in the back of my mind for a while, yet I remain unsure how to resolve this best.

I'll admit that ToString is perhaps in a different category than Equals and GetHashCode, and it might be OK to have some inconsistency between those categories.

However, I am fairly certain that introducing by-default strict mock behavior for Equals (and GetHashCode) would be a too severe breaking change. I suspect it might make more sense to simply defer to the base implementation for these methods by default.

[BrunoJuchli]

However, I am fairly certain that introducing by-default strict mock behavior for Equals (and GetHashCode) would be a too severe breaking change. I suspect it might make more sense to simply defer to the base implementation for these methods by default.

Hi stakx
Given a mock, I usually expect most properties not to be setup.
If none are setup and the implementation of the mocked-class is used, every mock will be equal to every other mock. That seems a bit dangerous (and unexpected) to me.

Also, could there be cases where calling the base-implementation of Equals will break verify's like foo.Verify(x => x.SomeMethod(myMock)) because myMock.Equals(myMock) returns false?

[stakx]

If none are setup and the implementation of the mocked-class is used, every mock will be equal to every other mock.

Why? That would only be true if the mocked class had an implementation of Equals that returns true unconditionally.

My thinking was that if someone makes an effort to provide custom equality for a type, why shouldn't that equality behavior apply to a mock object like it does to any other instance?

could there be cases where calling the base-implementation of Equals will break verify's like foo.Verify(x => x.SomeMethod(myMock)) because myMock.Equals(myMock) returns false?

Yes, but according to your earlier argument (that tests should explicitly set up Equals if they rely on it), wouldn't such a breakage be a good thing? Also, myMock.Equals(myMock) would no longer return false by default, but either true (if there is no Equals override, you'd get standard .NET reference equality semantics) or whatever the base class' Equals implementation deems correct in that case.

[BrunoJuchli]

That "none" in

If none are setup

was meant to relate to properties.

Example:

public class Foo
{
    public int Value { get; set; }

    public override bool Equals(object obj)
    {
          if(other is Foo other)
          {
               return this.Value == other.Value;
          }
    }
}

Here, Mock.Of<Foo>() == Mock.Of<Foo>() will return true, right?
What if the property were a string, an object or something entirely arbitrary like Bar (probably the same as object? What's the default property value behavior of Moq?
Where are the values coming from that a custom Equals method uses to compare? What would happen in case I create a new Mock with DefaultValue = DefaultValue.Mock?

All of our classes have a constructor like protected Foo() which is not initializing any properties/fields, it's sole use is for Moq (it's added to the IL via EmptyConstructor.Fody). So for us a class Foo mock usually behaves exactly like an interface IFoo mock. This is not the case for the Equals method.

My thinking was that if someone makes an effort to provide custom equality for a type, why shouldn't that equality behavior apply to a mock object like it does to any other instance?

For the same reason as we're using a mock - and not the real thing - in the first place: I want to isolate my test from the actual behavior of the mocked class.
For all the non Equals and GetHashCode methods someone has implemented them as well for a reason - and we're still mocking them away ;-)

Yes, but according to your earlier argument (that tests should explicitly set up Equals if they rely on it), wouldn't such a breakage be a good thing? Also, myMock.Equals(myMock) would no longer return false by default, but either true (if there is no Equals override, you'd get standard .NET reference equality semantics) or whatever the base class' Equals implementation deems correct in that case.

You may have tests where the equality of Foo is not relevant for the system under test, but it becomes relevant for the test because of a Mock.Verify(x => x.SomeMethod(foo).

Foo.Equals(..) may still be overriden - and matter in another system under test, where it should be setup.
Note that, that setting equivalency up properly is a pain:

var a1 = new Mock<Foo>();
var a2 = new Mock<Foo>();
a1.Setup(x => x.Equals(It.IAny<object>()).Returns(false); // need to override default behavior
a1.Setup(x => x.Equals(a2)).Returns(true);
a2.Setup(x => x.Equals(It.IAny<object>()).Returns(false); // need to override default behavior
a2.Setup(x => x.Equals(a1)).Returns(true);

Of course, in the test where equality is only needed for method call verifciation it can be simplified by:

Mock.Verify(x => x.SomeMethod(It.Is<Foo>(f => ReferenceEquals(f, foo))

---

A simpler explanation why I don't like it:

adding an Equals override to Foo via refactoring means I have to search for all tests where Foo is a parameter in a method-call verification and adapt all these. Otherwise I risk my tests to be faulty.

I'll quote Telerik:

The purpose of mocking is to isolate and focus on the code being tested and not on the behavior or state of external dependencies

[stakx]

It's been a while, and although your argument definitely has some merit, I don't feel comfortable making this change. Since you mentioned that this is not actually a roadblock for you, I'll close this issue. However, I'll mark it as unresolved in case someone else in the future wants to take another look at this.