3

Here is a simplified version of a problem I encountered:

public interface IService
{
    IProvider Provider { get; }
}

public interface IProvider
{
    List<int> Numbers{ get; }
    string Text { get; }
} 

[TestMethod]
public void ServiceTest()
{
    var service = new Mock<IService>();
    var provider = new Mock<IProvider>();

    service.Setup(s => s.Provider).Returns(provider.Object);    // A
    service.Setup(s => s.Provider.Text).Returns("some text");   // B - incorrect

    // they actually meant to do this, instead of 'B'
    // provider.Setup(p => p.Text).Returns("some text"); 

    provider.Setup(p => p.Numbers).Returns(new List<int> { 1, 2, 3 });

    DummyApplicationCode(service.Object);
}

int DummyApplicationCode(IService service)
{
    // will throw, because the Provider was replaced at 'B'
    int shouldBeOne = service.Provider.Numbers.First(); 
    return shouldBeOne;
}

A unit test was failing because way down in the application code under test, the mocked IService was returning the wrong IProvider.

I eventually spotted the line (bear in mind the code I was looking at was not as simple as above) which had caused it, labelled 'B' above, which someone else had added due to misunderstanding the Moq Setup.

I'm aware that subsequent Setups on a mock will override previous ones but I hadn't spotted this issue because the Return of the offending line was for a separate sub-property.

I expect this is by design but it threw me as I hadn't anticipated someone would do this.

My question: Since the Setup at 'B' is only concerned with the return of the provider Text, why does the service 'Provider' property need to replace that which was defined at 'A'?

Nkosi
  • 235,767
  • 35
  • 427
  • 472
mungflesh
  • 776
  • 11
  • 22
  • `If more than one setup is specified for the same method or property, the latest one wins and is the one that will be executed.`, which you already know. The `Provider` is part of the expression tree being executed in your case. So any previous setups that involve that path will also be overridden. – Nkosi Apr 15 '16 at 11:17
  • @Nkosi that's fair enough, thanks, but my query is rather why it really **needs** to do that, rather than bolt the Text return onto the already existing Provider mock. Personally, I would have liked to see that line flagged as an error. – mungflesh Apr 15 '16 at 11:38
  • 2
    I see your point. My guess is that each `Setup` is treated in isolation. A distinct action/function to be invoked. You could create an issue of feature request for it on GitHub. – Nkosi Apr 15 '16 at 11:43

1 Answers1

2

This is clearly intentional when looking at the source:

https://github.com/moq/moq4/blob/master/Source/Mock.cs

https://github.com/moq/moq4/blob/master/Source/Interceptor.cs

Setup creates a "call" by using AddCall on Interceptor. This contains the following block of code which, as long as we're creating a non-conditional setup, removes all previous setups. It's even commented.

if (!call.IsConditional)
            {
                lock (calls)
                {
                    // if it's not a conditional call, we do
                    // all the override setups.
                    // TODO maybe add the conditionals to other
                    // record like calls to be user friendly and display
                    // somethig like: non of this calls were performed.
                    if (calls.ContainsKey(key))
                    {
                        // Remove previous from ordered calls
                        InterceptionContext.RemoveOrderedCall(calls[key]);
                    }

                    calls[key] = call;
}
Owen Pauling
  • 11,349
  • 20
  • 53
  • 64