Automatically failing when an unexpected call happens.

[Silvie Sotto]

Hi,

Is there a way to automatically fail (throw an exception) when a substitute is called with unexpected (undefined) parameters?

For instance if I set up a substitute with calculator.Add(1, 2).Returns(3); 

I want it to succeed only for Add(1,2). Any other calls to any methods (e.g. Add(10,20), Sub(4,2), etc.) should fail. 

Normally in that situation the substitute returns a default object so the tested object is in an undefined state.

Setting up calculator.Add(1, 2).Received() would not catch any other issues as long as Add(1,2) was called at least once. 

This assertion is also done after the fact, so there is a high chance of the tested code failing before it and making it hard to find the actual place when the undefined call is being made.

Thank you

[David Tchepak]

NSubstitute is set up specifically to try and avoid "strict mocks" like this, as they are often blamed for over-specified tests and making code difficult to change. Even so, we can still force NSubstitute to do this with a bit of work.

One approach is to setup a default handler for all calls, then setup the specific calls we need, then change the default handler to throw.

    var calc = Substitute.For<ICalculator>();

    Func<int> defaultHandler = () => 0;

    calc.Add(Arg.Any<int>(), Arg.Any<int>())

        .Returns(x => defaultHandler());

    calc.Add(1, 2).Returns(3);

    defaultHandler = () => { throw new Exception("Unexpected call"); };

    Assert.AreEqual(3, calc.Add(1, 2));

    Assert.Throws<Exception>(() => calc.Add(3, 2));

We can also assert this using combinations of `Received`, `DidNotReceive` and/or `ReceivedCalls()`, although as you mentioned this is after the fact. (If you'd like some examples of this let me know.)

Hope this helps.

Regards,

David

--
You received this message because you are subscribed to the Google Groups "NSubstitute" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nsubstitute...@googlegroups.com.
To post to this group, send email to nsubs...@googlegroups.com.
Visit this group at http://groups.google.com/group/nsubstitute.
For more options, visit https://groups.google.com/d/optout.

[Silvie Sotto]

Thanks for the reply.

1. I'm curious why the handlers have to be set up through this 2-step process. Why having something like below doesn't work and always uses the Any version. Why isn't the specific version used first to match?

    calc.Add(1, 2).Returns(3);

    calc.Add(Arg.Any<int>(), Arg.Any<int>())

        .Returns(x => { throw new Exception("Unexpected call"); });

Is there any way to know what order the handlers are going to be selected?

In addition, this doesn't really address the issue of calling functions for which mocks haven't been defined at all. If an interface has dozens of functions, you'd have to manually create these default exception throwing handlers for all of them. 

2. I'm not sure I understand the philosophy of 'strict' mocks being bad, especially since the documentation itself mentions using function handlers as bad practice (http://nsubstitute.github.io/help/return-from-function/). I'd assume from that that a good practice would be to specify only specific (strict) mocks with simple pairs of request-response.

I'd be grateful if you could shed some light on how to write code that can be tested using the non-strict mocks, when any mistake can result in the mock returning some undefined result. 

Let's say I have the following code:

int delay = 5;

bool result = mockedObject.SetupDelay(delay);

if (result)

{

 // do some stuff.

}

/// <other code>

and I add a handler by mistake for mockedObject.SetupDelays(5).Returns(true). Suddenly the simple code doesn't work because the conditional block is not executed. There is no way to immediately tell why the test failed because the failure happens somewhere in the <other code>. You need to spend your time debugging to see what's going on. This is obviously a simplified example, but you can see what could happen with a more complicated logic.

There are other problems with default values. I know for instance that NSubstitute returns some objects instead of nulls whenever the return value is not defined. I don't know however for instance what happens in this example (mockedObject is a substitute with no returns defined):

Delay d1 = mockedObject.Undefined(1);

Delay d2 = mockedObject.Undefined(1);

if (d1 == d2)

{

 // Is this executed? Are d1 and d2 the same object? Different, but equal objects? Non-equal objects?

}

Is the philosophy of non-strict mocks to just go with the flow? I.e. you don't set up mocks most of the time, but instead rely on and dynamically check what you get out of the mock and adjust your assertions accordingly?

Thank you for any advice.

[David Tchepak]

In answer to 1), NSubstitute tries to find return values by looking at the most recently configured call, through to the least recent (last in, first out). So in this example:

    calc.Add(1, 2).Returns(3);

    calc.Add(Arg.Any<int>(), Arg.Any<int>())

        .Returns(x => { throw new Exception("Unexpected call"); });

the most recent call configuration matches all calls, so it will always throw instead of returning 3. This lets us override previously spec'd calls where required.

It is possible to make sure calls to other methods aren't received by checking `ReceivedCalls()`, but this is a bit of a hack. NSubstitute isn't designed for this scenario.

For 2), strict mocks aren't necessarily bad, it's just that NSubstitute is designed for a different style of testing and design.

The usual complaint against strict mocks is it can lead to brittle tests. Hadi Hariri gives an example [1] showing how adding an unrelated call to production code can cause an entire test fixture to fail (he mentions one test, but this could break all existing tests to in a fixture) because now an unexpected call has been received. Mark Seemann also advises against them, see the "Strict Mocks are not the solution" heading in [2].

[1]: http://hadihariri.com/2009/04/15/strict-use-of-strict-mocks/

[2]: http://blog.ploeh.dk/2013/10/23/mocks-for-commands-stubs-for-queries/

Function handlers are advised against in the NSub docs as it can be a sign of replicating implementation details of dependencies, rather than testing against an interface.

I'm not sure I quite follow your SetupDelay or `d1 == d2` examples. Is that test code, or code being tested?

As I see it, the main idea behind non-strict mocks is that you test your code behaves a certain way given specific values returned by its dependencies. If you leave a call unspecified, it is regarded as unimportant for the test (i.e. the code being tested should behave the same for all values of unspecified calls). In terms of NSubstitute defaults, this means we should specify the result even if the default matches what we need, so that our test explicitly describes the scenario being tested. 

I get the feeling I haven't quite managed to address your questions adequately, so please ask for clarifications where required.

Regards,

David

[Silvie Sotto]

LIFO makes sense when overwriting, thanks for the clarification. Is there a way to clean up all previously set up returns, without the need to overwrite them all?

Thanks for the article links. From my perspective however both Hadi and Mark talk only about issues with the strict mocks without mentioning any issues with the dynamic ones.

My main concern is debugging. Consider the following code:

public bool DoesUserExist(int userId)

{

    var user = this.userRepository.Read(userId);

    if (user.Id == 0)

{        

return false;

}

    return true;

}

[TestMethod]

public void UserExistsTest1()

{

UserRepository userRepository = Substitute.For<UserRepository>();

userRepository.Read(1000).Returns(new User() { Id = 1000 });

bool actual = DoesUserExist(10000);

Assert.True(actual);

}

[TestMethod]

public void UserExistsTest2()

{

UserRepository userRepository = Substitute.For<UserRepository>();

userRepository.Reads(1000).Returns(new User() { Id = 1000 });

bool actual = DoesUserExist(1000);

Assert.True(actual);

}

Both tests will fail on assertions. Since the tests are actually executing fine, but fail on assertions, your first thought would most likely be that the tested code is the problem - after all, it doesn't satisfy the tests. Once you spend your time overanalyzing DoesUserExist(), you can't really find any issue with it. You then turn to each test, spend more time analyzing them to find that you made some trivial typos. If the mocks were strict, you'd immediately know where the problem was. 

A similar situation would occur if you modified the code being tested to use some undefined variation of the Read method.

If the assertion passes, it's even worse. The dynamic nature of the mock hides the fact that the test is buggy, and that it succeeds every time.

I'm thinking it would be a good idea to use ReceivedCalls() to debug such cases, though you need to know about the dynamic nature of the framework, which is not always apparent to e.g. new developers working on a legacy system.

My previous examples with SetupDelay and 'd1 == d2' are both code being tested. I'm especially interested in what the framework returns for the unexpected calls. If I have something like:

IMockedClass mockedObject = Substitute.For<IMockedObject>();

IMockedClass o1 = mockedObject.ReturnObject(1);

IMockedClass o2 = mockedObject.ReturnObject(1);

IMockedClass o3 = mockedObject.ReturnObject(2);

IMockedClass o4 = mockedObject.ReturnDifferentObject();

What are the relations between the returned objects? Are they the same objects? Different, but equal objects? Non-equal objects?

Thank you.

[David Tchepak]

Thanks for the clarifications. Replies inline.

LIFO makes sense when overwriting, thanks for the clarification. Is there a way to clean up all previously set up returns, without the need to overwrite them all?

There isn't at the moment. It was raised in issue #157 (https://github.com/nsubstitute/NSubstitute/issues/157#issuecomment-51280086) as a possible addition.

 

Thanks for the article links. From my perspective however both Hadi and Mark talk only about issues with the strict mocks without mentioning any issues with the dynamic ones.

Agreed.

My main concern is debugging. ... Once you spend your time overanalyzing DoesUserExist(), you can't really find any issue with it. You then turn to each test, spend more time analyzing them to find that you made some trivial typos. If the mocks were strict, you'd immediately know where the problem was. 

A similar situation would occur if you modified the code being tested to use some undefined variation of the Read method.

If the assertion passes, it's even worse. The dynamic nature of the mock hides the fact that the test is buggy, and that it succeeds every time.

I think dynamic mocks lean quite a bit on sticking to small units for tests, and TDD. Small units means that the time required to analyse DoesUsersExist() and its tests is relatively small. (On the flip side of the coin, lots of small units means loads of tests which makes any brittleness much more painful.) TDD/test-first is meant to reduce the risk of buggy tests. If we write a test first and make sure it fails, then write code to pass the test, but the test still fails, this should alert us to either the code or test being incorrect, and we need only search a very small amount of code to find the problem.

 

I'm thinking it would be a good idea to use ReceivedCalls() to debug such cases, though you need to know about the dynamic nature of the framework, which is not always apparent to e.g. new developers working on a legacy system.

`ReceivedCalls()` is probably ok for debugging, but I think it is too much of a hack to try and make it a replacement for strict mocks. If strict mocks work better for you and your team, I think you'd be best off with a mocking library that supports them directly, like FakeItEasy or Moq (both awesome libraries!).

 

My previous examples with SetupDelay and 'd1 == d2' are both code being tested. I'm especially interested in what the framework returns for the unexpected calls. If I have something like:

IMockedClass mockedObject = Substitute.For<IMockedObject>();

IMockedClass o1 = mockedObject.ReturnObject(1);

IMockedClass o2 = mockedObject.ReturnObject(1);

IMockedClass o3 = mockedObject.ReturnObject(2);

IMockedClass o4 = mockedObject.ReturnDifferentObject();

What are the relations between the returned objects? Are they the same objects? Different, but equal objects? Non-equal objects?

NSubstitute will have specific defaults for these cases but I would advise against relying on them. If the results are important, I'd make them explicit:

    IMockedClass anObj = Substitute.For<IMockedClass>();

    IMockedClass differentObj = Substitute.For<IMockedClass>();

    mockedObject.ReturnObject(1).Returns(anObj);

    mockedObject.ReturnObject(2).Returns(differentObj);

    IMockedClass o1 = mockedObject.ReturnObject(1);

    IMockedClass o2 = mockedObject.ReturnObject(1);

    IMockedClass o3 = mockedObject.ReturnObject(2);

Now it seems apparent that o1 and o2 will be the same objects, where o3 will be a different object. If we want different references but equal objects, I'd create those explicitly (probably not substitutes; if I'm relying on equality I think I'd want real, value objects) and return them from calls as required.

 

Hope this helps. I appreciate the discussion. :)

Regards,

David

[Silvie Sotto]

I think dynamic mocks lean quite a bit on sticking to small units for tests, and TDD. Small units means that the time required to analyse DoesUsersExist() and its tests is relatively small. (On the flip side of the coin, lots of small units means loads of tests which makes any brittleness much more painful.) TDD/test-first is meant to reduce the risk of buggy tests. If we write a test first and make sure it fails, then write code to pass the test, but the test still fails, this should alert us to either the code or test being incorrect, and we need only search a very small amount of code to find the problem.

I agree, though in my opinion this is a nature of unit testing itself where units are kept small and isolated, regardless of whether you write tests first or not.

Given the dynamic nature of these mocks, the tests could also return false positives, which would not alert you until you introduce new changes. The main concern is really with reengineering existing code, which already has a bunch of tests. This can make it much harder to find the bug.

Granted, I don't see an elegant solution to the problem, both strict and loose mocks have their pros and cons.

 

I would suggest to consider introducing a strict option to NSubstitute. I may be wrong, but I assume you could have a property that indicates whether the substitute should create default objects or not. This property would be set to dynamic by default (create default objects). Then in the code that creates the default objects (or determines that you have to create a default object) you'd check that property and throw an exception if it was set to strict. It sounds like a small change. Hope this makes sense.

NSubstitute will have specific defaults for these cases but I would advise against relying on them. If the results are important, I'd make them explicit:

Of course this would be my choice as well, especially given that I lean towards strictness. I was asking this in case somebody made a mistake of setting a return for incorrect parameters, thus leaving the creation of the objects to the framework. If I understand correctly, the framework will create something default if there's a bug in your test setup, but you can't rely on the values e.g. being equal for calls to a single method with the same parameter.

[David Tchepak]

I would suggest to consider introducing a strict option to NSubstitute. I may be wrong, but I assume you could have a property that indicates whether the substitute should create default objects or not. This property would be set to dynamic by default (create default objects). Then in the code that creates the default objects (or determines that you have to create a default object) you'd check that property and throw an exception if it was set to strict. It sounds like a small change. Hope this makes sense.

Along these lines, i'm interested in providing ways for people to hook their own logic into substitutes (so they can set up things like strict mocks if they like). I haven't found a nice way of doing this yet though, largely due to NSubstitute's syntax relying on some awful tricks that would end up leaking out through these hooks.

 

...If I understand correctly, the framework will create something default if there's a bug in your test setup, but you can't rely on the values e.g. being equal for calls to a single method with the same parameter.

If NSub creates a default for a call, it will return the same instance for all calls with the same parameters.

e.g. Assert.AreSame(sub.Call(1,2), sub.Call(1, 2));