Opt in parameters matching logic

[dvabuzyarov]

moq: 4.16.0

Faced an issue where a reference type (generic List) is compared as a value type with ConstantMatcher. MatchesEnumerable. Please consider this case:

    [TestFixture]
    public class IssueUnitTests
    {
        public class Model
        {
            public List<string> Children { get; set; }
        }


        public interface IChildrenProcessor
        {
            List<int> Handle(List<string> children);
        }

        public class Processor
        {
            private readonly IChildrenProcessor _childrenProcessor;

            public Processor(IChildrenProcessor childrenProcessor)
            {
                _childrenProcessor = childrenProcessor;
            }

            public List<int> Handle(Model model)
            {
                // the wrong implementation, the unit test should drive the right implementation
                return _childrenProcessor.Handle(new List<string>());
            }
        }

        [Test]
        public void EnumerableIssue()
        {
            var children = new List<string>();
            var expected = new List<int>();
            var model = new Model { Children = children };

            var childrenProcessorMock = new Mock<IChildrenProcessor>();
            childrenProcessorMock
                 // should react on particular object instance, not on any empty Enumerable<string>
                .Setup(instance => instance.Handle(children))
                .Returns(expected);

            var processor = new Processor(childrenProcessorMock.Object);
            var actual = processor.Handle(model);

// will pass, since setup reacts on any empty Enumerable<string>
            Assert.AreSame(expected, actual);
        }
    }

I can see here some inconsistencies where some reference types are treated as value types and some not. The behaviour is not obvious and I haven't found any documentation on it.

It would be nice if developers could opt in using approach per mock instance.

[stakx]

Agreed that Moq has some funky rules regarding what's evaluated by value vs. by reference, and eagerly vs. lazily. And you're right that not all of that is documented... perhaps the thinking was that having the implementation working that way would make things just work ™ in most usual scenarios.

We've been able to rectify a few such irregularities in the past, and I suppose we could rectify handling of sequences, too... but that would be almost certain to break user code.

The most promising approach I can think of to handle this cleanly is with argument matchers that would express whether to compare by value or by reference; think of something along the lines of It.IsSameAs(objectReference) and It.IsEquivalentTo(value).

I'm not sure Moq's special handling of IEnumerable<> becomes a problem frequently enough to warrant the addition of extra matchers to the core library. But when it does, it's already possible to circumvent using It.Is and a custom predicate.

It might be a good idea however to mention the by-value matching of IEnumerable<>, as it diverges from .NET's usual equality behavior. Feel free to add a hint e.g. to the quickstart.

[stakx]

We may be able to fix this scenario. Say we have:

public interface IX
{
    void Method(object[] args);
}

var mock = new Mock<IX>();

And say we created the following setup:

mock.Setup(x => x.Method(new object[] { ... });

If we didn't have the structural comparison for enumerable types in place, there would be no way to ever trigger this setup, because you won't ever be able to pass the same object[] instance when invoking mock.Object.Method.

However, if we did this instead:

var args = new object[] { ... };
mock.Setup(x => x.Method(args));

Then the setup could still be triggered by passing args (the same object[] instance used in the setup expression) when invoking mock.Object.Method.

That is, if we performed structural comparison for enumerables only when that enumerable was specified in the setup expression using new / if we didn't perform structural comparison for enumerables when a captured variable was used in the setup expression, your test case would be fixed.

This would be a breaking change (possibly only a minor one), but the existing unit tests suggest that structual comparison was originally meant to target just such new[] { ... } expressions, so it would probably be fine to make the change.

[dvabuzyarov]

In my opinion it is important that the behavior is consistent. Comparison everywhere should be either by reference only or by value only. Moq should provide robust, clean and predictable behaviour with ability to opt in using logic.

It would be nice if a developer could specify comparasing logic

1. at moq library level, so all newly created mock object will obey it (maybe with a singleton static or with locator pattern)
2. at specific instance of mock object, maybe in construction phase or calling a config method
3. at specific setup level where developer can choose comparison logic for a particular parameter (It.IsEqual())

as default comprising behaviour I would prefer to see the reference logic as it is the default for .net framework, so it more expectable and would create more obvious API.

[stakx]

it is important that the behavior is consistent.

I fully agree.

Comparison everywhere should be either by reference only or by value only. [...] as default comprising behaviour I would prefer to see the reference logic as it is the default for .net framework

That isn't correct. .NET has both reference types and values types. (It wouldn't make sense to e.g. match the integer value 42 against another integer value 42 by reference; they would never match.)

We should be consistent with .NET's standard equality semantics. This means that Moq should (by default) simply delegate to the parameter type's object.Equals implementation.

Moq should provide robust, clean and predictable behaviour with ability to opt in

Ideally, yes. With opt-in meaning the use of matchers such as It.Is* (at least today).

Unfortunately though, Moq isn't brand new anymore, and we need to think twice before making any breaking changes.

For this reason (aside from one other), I think that the structural equality comparison for anonymous collection objects still has its place... as long as it doesn't unnecessarily interfere with the more fundamental matching rule laid out above.

It would be nice if a developer could specify comparasing logic

Moq currently has three levels at which settings can be applied: mock factories, mocks, and setups. Anything more "global" than mock factories probably isn't required (as we could always back even new Mock<...>() objects with a default factory, which would then essentially become the global settings context).

But in this case I think it may be perfectly sufficient to keep things as simple as possible, and only offer matchers at the setup level for custom matching logic. Too many settings and switches doesn't make things easier, esp. for new users.

[dvabuzyarov]

Comparison everywhere should be either by reference only or by value only. [...] as default comprising behaviour I would prefer to see the reference logic as it is the default for .net framework

That isn't correct. .NET has both reference types and values types. (It wouldn't make sense to e.g. match the integer value 42 against another integer value 42 by reference; they would never match.)

Here I meant only reference types. So far I haven't met any comprising issue for value types.

And back to the issue, for now it would be enough to have a way to override the current behaviour. Maybe do you have an idea how to do it?

[stakx]

And back to the issue, for now it would be enough to have a way to override the current behaviour.

In your setup expression, use a It.Is<T>(x => x == collectionObj) matcher in place of just collectionObj.

And if you want to be even clearer about what exactly is going on there, replace == with object.ReferenceEquals.

---

And to summarise my previous two posts: you shouldn't have to actively opt out of the current behavior in the first place; you shouldn't have to do anything differently than you're doing already. Instead, one ought to have to opt in to the structural comparison logic for enumerables... except in the case of setups / verify expressions that make use of "anonymous" collection objects, specified using array or collection initialisers (e.g. new T[0], new T[] { ... }, new List<T> { ... }, etc.). This exception seems sensible to me because such setups would otherwise be completely useless, as they couldn't possibly match any calls.

[github-actions[bot]]

Due to lack of recent activity, this issue has been labeled as 'stale'. It will be closed if no further activity occurs within 30 more days. Any new comment will remove the label.

[github-actions[bot]]

Due to lack of recent activity, this issue has been labeled as 'stale'. It will be closed if no further activity occurs within 30 more days. Any new comment will remove the label.

[github-actions[bot]]

This issue will now be closed since it has been labeled 'stale' without activity for 30 days.