Match enumerable arguments by comparing contents rather than via `Equals`

[blairconrad]

Match enumerable arguments by comparing contents rather than via `Equals`

[afakebot]

This change has been released as part of FakeItEasy 8.0.0.

[DaveCousineau]

Just hit this... empty arrays are now considered equal so I cannot distinguish a call with one empty array versus another. I feel like I disagree with this change. We are now no longer asserting the reference but the contents? This should have been enabled via some other mechanism. I do not see the value of this change. This blurs certain kinds of calls together with no way to distinguish between them.

This may not have mattered, except that if you setup calls with collections that are now considered equal by element, then when the calls actually happen, you get the return results in the opposite order.

var array1 = new object[0];
var array2 = new object[0];
var array3 = new object[0];

A.CallTo(() => mock.GetSomething(array1)).Returns(1).Once();
A.CallTo(() => mock.GetSomething(array2)).Returns(2).Once();
A.CallTo(() => mock.GetSomething(array3)).Returns(3).Once();

var result1 = mock.GetSomething(array1); // 3
var result2 = mock.GetSomething(array2); // 2
var result3 = mock.GetSomething(array3); // 1

[thomaslevesque]

Hi @DaveCousineau,

Sorry you're having trouble with the new release. I realize the new behavior isn't great in your use case, but as far as we can tell, it's pretty uncommon to rely on reference equality for collections. In most cases, people want arrays or lists that contain the same items to be considered equal, and were often surprised that it wasn't the case. We saw a lot of questions about this on StackOverflow and in the Gitter chat, which is what led to this change. (The new behavior is also what Moq does, FWIW).

If it turns out our assumption was wrong and many people come complaining about this, we'll reevaluate, of course, but I think the new behavior is what people want most of the time.

If it's important for your scenario that arrays are compared by reference, you can use the IsSameAs argument constraint:

A.CallTo(() => mock.GetSomething(A<object[]>.That.IsSameAs(array1))).Returns(1).Once();
A.CallTo(() => mock.GetSomething(A<object[]>.That.IsSameAs(array2))).Returns(2).Once();
A.CallTo(() => mock.GetSomething(A<object[]>.That.IsSameAs(array3))).Returns(3).Once();

If that feels too verbose, you can always create an extension method like this:

public static T ByRef(this T value) where T : class
{
    return A<T>.That.IsSameAs(value);
}

A.CallTo(() => mock.GetSomething(array1.ByRef())).Returns(1).Once();
A.CallTo(() => mock.GetSomething(array2.ByRef())).Returns(2).Once();
A.CallTo(() => mock.GetSomething(array3.ByRef())).Returns(3).Once();

At this point, we don't intend to include this extension method in FakeItEasy, because extension methods that apply to all (or most) types tend to pollute Intellisense, but if you use it often it might be worth it.

[DaveCousineau]

Thanks, at least there is a way to specify reference equality so it's not a dead-end problem.

I feel like you should have enabled this with something like A<object[]>.That.IsEquivalentTo(array1) instead but at least it's not an impossible situation.

(I guess it basically adds a third type of equality to reason about. I now can't reason just about reference and value equality, I now need to consider "collection equality" when using FIE. Something like that should be made explicit, not be implicit.)

[thomaslevesque]

I feel like you should have enabled this with something like A<object[]>.That.IsEquivalentTo(array1)

We actually had this already (A<object[]>.That.IsSameSequenceAs(array1)). We just made it the default for collections, because that's what people need most of the time.

[blairconrad]

@DaveCousineau, as @thomaslevesque mentioned, we were surprised and dismayed that this change negatively affected you. I'm interested in learning more about your use case in the hopes that it will help us avoid or at least anticipate similar problems in the future.

Are you using 3 different empty arrays in your test because your production code is relying on an equality with reference semantics or was it a convenient way to ensure that you get different results returned from GetSomething on subsequent calls? I guess I'm wondering if something like Specifying different behaviors for successive calls or ReturnsNextFromSequence would apply to your tests instead, perhaps better revealing the intention (and likely being more readable in the bargain).

[DaveCousineau]

I have code that consumes a 2d jagged array. Because this part of the code does not process the contents of the inner arrays, which is done somewhere else, the contents of the inner arrays are irrelevant, so I was just passing around empty arrays for testing purposes.

The data is processed in a loop and depending on the results it may stop early.

I am using strict mocks and I try to match my setup to the usage exactly, so I had a similar loop to register the calls to mirror how they will be called.

With the change, the data from the calls was now returned in reverse, as in my example. This was very hard to diagnose because the return values were themselves mocks which I could not uniquely identify and so there was no way to really see that they were reversed. I started to realize it was as if all of the arrays were somehow equal which is already why I didn't use Array.Empty. I kept double checking to make sure I didn't. Eventually I accidentally passed an unrelated empty array and still got back my mock result and realized it was FIE treating the arrays as the same array.

was it a convenient way to ensure that you get different results returned from GetSomething on subsequent calls?

I guess you could say it that way, but really, no, I was setting the calls up in the same way that they are called. (You say "subsequent calls" but these are not necessarily "subsequent calls" if the arguments are not the same.)

I guess I'm wondering if something like Specifying different behaviors for successive calls or ReturnsNextFromSequence would apply to your tests instead, perhaps better revealing the intention (and likely being more readable in the bargain).

yes, definitely possible, but the code is fixed for now using .That.IsSameAs. and now that I know about this I will hopefully not trip over it again.

[blairconrad]

Thanks for the explanation, @DaveCousineau. Happy FakeItEasying.