Checking received calls changed after NSubstitute 1.10.0.0

[John Schroeder]

Hello, I am trying to update from NSubstitute 1.10.0.0 to the latest 3.1.0 and I noticed a change in how "Checking received calls" behaves. Several of my unit tests have behavior that combine the "Setting out and ref args" and "Checking received calls". When I try to do to a Received() call check with NSubstitute 3.1.0 with an "out" parameter, the test now fails.

This can be reproduced with the "Setting out and ref args" unit test from the NSubstitute Docs and adding the one line for checking a received call:

public interface ILookup {
    bool TryLookup(string key, out string value);
}

//Arrange
var value = "";
var lookup = Substitute.For<ILookup>();
lookup
    .TryLookup("hello", out value)
    .Returns(x => { 
        x[1] = "world!";
        return true;
    });

//Act
var result = lookup.TryLookup("hello", out value);

//Assert
Assert.True(result);
Assert.AreEqual(value, "world!");

//Assert
lookup.Received().TryLookup("hello", out value);

In NSubstitute 1.10.0.0 this last assert passes but in 3.1.0 it fails. Is this the expected behavior? If so, how would I update my unit tests to keep the same behavior? (Note: setting value to "" before the receive check works. However, in 1.10.0.0, value has to match. I would prefer to keep the check for value since my unit tests currently just have the Received check and not the check for AreEqual with value.)

[David Tchepak]

Hi John,

If I recall correctly (that's a big "if"), out/ref params used to try to look at the reference of the variable passed, but this did not work for value types so we had to change how it worked. Now it should match based on the original value passed to the call, not to the actual variable passed. The 3.1+ behaviour is expected from my perspective, but not ideal. 

So as you mentioned, this will fix it:

            //Assert

            Assert.True(result);

            Assert.AreEqual(value, "world!");

            //Assert

            var emptyValue = ""; // <-- New line, matches the original value of `value` variable

            lookup.Received().TryLookup("hello", out emptyValue);

When you say you'd like to avoid the `AreEqual` check with `value`, do you mean you would use the one assertion to check both the received call and the output value of the call? As a general rule I think the use of the value coming back from a sub (be it via return value or out/ref) should be tested separately to how a particular call was received, but I think we'd need to switch to a more realistic example to discuss that further (as there is no point in checking the output from a substitute we've just stubbed out anyway :) ). 

Please let me know if you need more info or have any trouble getting this to work.

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+unsubscribe@googlegroups.com.
To post to this group, send email to nsubs...@googlegroups.com.
Visit this group at https://groups.google.com/group/nsubstitute.
For more options, visit https://groups.google.com/d/optout.

[John Schroeder]

Hi David,

Thank you for the reply. I replied yesterday but either the message didn't post or it got deleted. Either way, I think I was off track with what are tests are doing and where the confusion comes in.

Our tests are pretty close to the example. The only major difference is that we are calling a method that evokes the substitution so we are not calling the substitution directly (hence why we are checking the received call). Since we aren't calling it directly, we can't check that value has been set.

Here's a better example:

public interface ILookup {
    bool TryLookup(string key, out int value);
}

public class LookupAdapter {
    private ILookup lookup;

    /// <summary/>
    public LookupAdapter(ILookup lookup){
        this.lookup = lookup;
    }

    public bool Run(string key){
        bool success = false;
        int response;
        if (key == "hello")
            success = lookup.TryLookup(key, out response);
        return success;
    }
}

//Arrange
int value;
int expectedValue = 1234;

var lookup = Substitute.For<ILookup>();
lookup
    .TryLookup("hello", out value)
    .Returns(x =>
    {

        x[1] = expectedValue;
        return true;
    });
var LookupAdapter = new LookupAdapter(lookup);

//Act
var result = LookupAdapter.Run("hello");

//Assert
Assert.True(result);

//Assert
lookup.Received().TryLookup("hello", out expectedValue);

So, since expectedValue can no longer be used, I wasn't sure the best way to fix this. Setting expectedValue to 0 would work but it wouldn't be as clear. I could use "value" instead here which might actually make the most sense. Any thoughts or improvements?

As long as the underlying code gets the 1234 value correctly, our units should still be fine. Again, just looking for the best approach to update our unit tests and to make sure NSubstitute behaviour is as expected. Thanks!

[David Tchepak]

Thanks for clarifying. Just checked the group and it looks like it flagged your earlier response for some reason (and didn't notify me :( ). Sorry about that!

In this case I don't think we need to test that the underlying code gets the 1234 value correctly. It can be assumed that NSubstitute will take care of that via the `x[1] = expectedValue;` line (we have our own tests that assert this works. See [1]). So once we've configured the substitute to set this value, we don't need to test that this is the value that comes back to our calling code.

In terms of the `Received` call, passing `expectedValue` of `1234` fails because the actual call was `lookup.TryLookup("hello", out {reference to an uninitialised int}`. It did not receive `lookup.TryLookup("hello", out {reference to an int holding 1234}`. This becomes more important when we consider values passed using `ref`, as the initial value given can be used by the called method so we need to make sure that value is correct (we use the same mechanism for `out`, which is why we have the same behaviour there).

In terms of updating this test, the fix is to pass an argument that better resembles the one it actually received:

    int blankValue;

    lookup.Received().TryLookup("hello", out blankValue);

This matches the `LookupAdapter` call very closely. Or, with the current C# compiler,  just:

    lookup.Received().TryLookup("hello", out int blankValue);

If we expand the example with an additional step of using the value from the lookup then I think it becomes a bit simpler:

public class LookupAdapter {

    private ILookup lookup;

    private INotification notify;

    // ... 

    public bool Run(string key){

        bool success = false;

        int response;

        if (key == "hello") {

            success = lookup.TryLookup(key, out response);

            if (success) notify.Push(response);

        }

        return success;

    }

}

Then we can test:

[Test] public void Sample() {

    //Arrange

    int value;

    int expectedValue = 1234;

    var lookup = Substitute.For<ILookup>();

    lookup

        .TryLookup("hello", out value)

        .Returns(x =>

        {

            x[1] = expectedValue;

            return true;

        });

    var notify = Substitute.For<INotification>();

    var LookupAdapter = new LookupAdapter(lookup, notify);

    //Act

    var result = LookupAdapter.Run("hello");

    //Assert

    Assert.True(result);

    //Assert

    notify.Received().Push(expectedValue);

    lookup.Received().TryLookup("hello", out int blank); // <-- Do NOT actually need this.

}

Now we're testing that the `LookupAdapter` correctly used the stubbed value from the `ILookup` dependency by passing it to the `INotification` instance when the key is "hello" and the lookup was successful. (We should also test when the lookup fails, and when the key is not "hello").

Note that we no longer even need to explicitly test `lookup.Received().TryLookup(...)`, as it can be inferred by the fact that `expectedValue` ended up getting passed to the `INotification` instance. I almost always avoid "double testing" in this way in an attempt to avoid over-specifying the tests (which makes them more brittle).

Hope this helps. If I haven't answered your question sufficiently please let me know and I'll go in to more detail. :)

Regards,

David

[1]: https://github.com/nsubstitute/NSubstitute/blob/e3b4bcadda12aebe1254d2f242f9c36d88fe1472/tests/NSubstitute.Acceptance.Specs/OutAndRefParameters.cs

--