14

I have run into a weird issue while using NSubstitute a few times and although I know how to work around it I've never been able to explain it.

I've crafted what appears to be the minimum required test to prove the problem and it appears to be something to do with using a method to create a substituted return value.

public interface IMyObject
{
    int Value { get; }
}

public interface IMyInterface
{
    IMyObject MyProperty { get; }
}

[TestMethod]
public void NSubstitute_ReturnsFromMethod_Test()
{
    var sub = Substitute.For<IMyInterface>();

    sub.MyProperty.Returns(MyMethod());
}

private IMyObject MyMethod()
{
    var ob = Substitute.For<IMyObject>();
    ob.Value.Returns(1);
    return ob;
}

When I run the above test I get the following exception:

Test method globalroam.Model.NEM.Test.ViewModel.DelayedAction_Test.NSubstitute_ReturnsFromMethod_Test threw exception: 
NSubstitute.Exceptions.CouldNotSetReturnException: Could not find a call to return from.
Make sure you called Returns() after calling your substitute (for example: mySub.SomeMethod().Returns(value)).
If you substituted for a class rather than an interface, check that the call to your substitute was on a virtual/abstract member.
Return values cannot be configured for non-virtual/non-abstract members.

However, if I change the test method to return this:

sub.MyProperty.Returns((a) => MyMethod());

or this:

var result = MyMethod();
sub.MyProperty.Returns(result);

It works.

I'm just wondering if anyone can explain why this happens?

shytikov
  • 9,155
  • 8
  • 56
  • 103
craftworkgames
  • 9,437
  • 4
  • 41
  • 52

1 Answers1

24

To get the NSubstitute syntax to work there is some messiness going on behind the scenes. This is one of those cases where it bites us. Let's look at a modified version of your example first:

sub.MyProperty.Returns(someValue);

First, sub.MyProperty is called, which returns an IMyObject. The Returns extension method is then called, which needs to somehow work out which call it needs to return someValue for. To do this, NSubstitute records the last call it received in some global state somewhere. Returns in pseudo-ish-code looks something like this:

public static void Returns<T>(this T t, T valueToReturn) {
  var lastSubstitute = bigGlobOfStaticState.GetLastSubstituteCalled();
  lastSubstitute.SetReturnValueForLastCall(valueToReturn);
  bigGlobOfStaticState.ClearLastCall(); // have handled last call now, clear static state
}

So evaluating the entire call looks a bit like this:

sub.MyProperty         // <-- last call is sub.MyProperty
   .Returns(someValue) // <-- make sub.MyProperty return someValue and
                       //     clear last call, as we have already set
                       //     a result for it

Now let's see what happens when we call another substitute while trying to set the return value:

sub.MyProperty.Returns(MyMethod());

Again this evaluates sub.MyProperty, then needs to evaluate Returns. Before it can do that, it needs to evaluate the arguments to Returns, which means running MyMethod(). This evaluation looks more like this:

//Evaluated as:
sub.MyProperty     // <- last call is to sub.MyProperty, as before
   .Returns(
     // Now evaluate arguments to Returns:
     MyMethod()
       var ob = Substitute.For<IMyObject>()
       ob.Value      // <- last call is now to ob.Value, not sub.MyProperty!
         .Returns(1) // <- ok, ob.Value now returns 1, and we have used up the last call
     //Now finish evaluating origin Returns:
     GetLastSubstituteCalled *ugh, can't find one, crash!*

There is another example of the problems this can cause here.

You can work around this by deferring the call to MyMethod(), by using:

sub.MyProperty.Returns(x => MyMethod());

This works because MyMethod() will only execute when it needs to use a return value, so the static GetLastSubstituteCalled method doesn't get confused.

Rather than doing that though, I prefer avoiding other calls to substitutes while I am busy configuring one.

Hope this helps. :)

David Tchepak
  • 9,826
  • 2
  • 56
  • 68
  • Thanks for the detailed answer. It's a lot more complicated than I expected. Do you think this is considered to be a bug in NHibernate? I gather by your answer it's a known issue and rather complex to fix. – craftworkgames May 20 '13 at 23:22
  • 1
    It's a limitation of the choice of syntax. Changing NSub to use explicit, local SubstituteFactories could help (we could keep loads of state around per test and search that, rather than global state), but not sure if people would be willing to complicate the syntax for creating subs. I think I'd prefer that, but I'm not sure if it is a big enough problem to justify changing at this point. – David Tchepak May 21 '13 at 03:22
  • The real problem is that the code looked correct from every angle so it was hard to diagnose the problem. Maybe better error messages might help but I have no idea what they might be. Thanks again. – craftworkgames May 22 '13 at 01:40
  • 1
    Aye, when it goes wrong it is awful. Current repo HEAD detects some cases of this and throws with a descriptive message. I'll try expanding the CouldNotSetReturnException message similarly. Thanks for the suggestion. – David Tchepak May 22 '13 at 03:46
  • I'm probably not understanding what's going on internally (especially since it's simplified), but wouldn't it make more sense to have GetLastSubstituteCalled() revolve around a stack to keep track of calls? Then you can simply pop them off and should allow for us to have ob.Value be the top call, which gets popped off, and then we still have sub.MyProperty on the stack. – Greg B Jul 15 '14 at 16:23
  • @Greg, one prob with that is if for cases where you are not using `Returns` (e.g. normal calls) then the values never get popped off the stack. This would be ok if subs were scoped to local factories (stack cleaned up after test), but with static creation methods it means mem use would continuously grow. (I'd prob switch to local factories if I had my time again.) – David Tchepak Jul 16 '14 at 00:13
  • The problem with this approach is that `MyMethod()` [sometimes gets called by NSubstitute itself](http://stackoverflow.com/questions/6908376/function-is-called-when-i-do-not-expect-it-to-with-nsubstitute) (implicitly, _without_ you explicitly initiating the call) in some cases and then it could break some of your internal logic. So this solution might not work depending on your situation. – Nikita R. Mar 30 '17 at 23:48