Recursive mocks: Strange behavior with argument matchers.

[kevkeller]

Describe the bug Arg.Is seems to override behavior of Arg.Any for recursive mocks.

To Reproduce

namespace ConsoleApp1
{
    using System;
    using NSubstitute;

    class Program
    {
        static void Main(string[] args)
        {
            var foo = Substitute.For<IFoo>();

            //Recusrive
            foo.CreateBar(Arg.Any<string>()).Number.Returns(25);
            foo.CreateBar(Arg.Is<string>(s => s.Contains("b"))).Number.Returns(50);

            Console.WriteLine(foo.CreateBar("123").Number); // -> Number: 25, actually returns 50
            Console.WriteLine(foo.CreateBar("abc").Number); // -> Number: 50

            //Non recursive
            foo.Test(Arg.Any<string>()).Returns(5);
            foo.Test(Arg.Is<string>(s => s.Contains("b"))).Returns(10);

            Console.WriteLine(foo.Test("123")); // -> 5
            Console.WriteLine(foo.Test("abc")); // -> 10

            Console.ReadLine();
        }
    }

    public interface IFoo
    {
        int Test(string value);
        IBar CreateBar(string value);

    }

    public interface IBar
    {
        int Number { get; }
    }
}

Expected behaviour So, either I don't understand how recursive mocks are supposed to work or there really is a bug here. The first call to CreateBar should return 25 for the Number property (matched by Arg.Any<string>() ). The second call should return 50 for the Number property (matched by Arg.Is<string>(s => s.Contains("b")) ). In practice, however, Arg.Is seems to override Arg.Any so both calls return 50 for the Number property. It perfectly works with non recusrive mocks though (see calls to foo.Test("123"), foo.Test("abc")). This is why I figured that it's supposed to behave in a similar fashion for recursive mocks.

Environment:

• NSubstitute version: 3.1.0
• Platform: net471, Windows 7 x64

[dtchepak]

Thanks very much for raising this, and thanks for the very useful example and explanation. I also reproduced this with 4.0-rc1.

I need to look into this further, but for future investigation here are some preliminary tests that explore the issue a little:

[Fact]
public void NSubIssue492() {
    var foo = Substitute.For<IFoo>();

    foo.CreateBar(Arg.Any<string>()).Number.Returns(25);
    foo.CreateBar(Arg.Is<string>(s => ContainsB(s))).Number.Returns(50);

    Assert.Equal(50, foo.CreateBar("abc").Number); // PASSES
    Assert.Equal(25, foo.CreateBar("123").Number); // -> FAILS: expected 25, actually returns 50
}

[Fact]
public void NSubIssue492_ExplicitlyStubProperty() {
    var foo = Substitute.For<IFoo>();

    var bar1 = Substitute.For<IBar>();
    bar1.Number.Returns(25);
    var bar2 = Substitute.For<IBar>();
    bar2.Number.Returns(50);
    foo.CreateBar(Arg.Any<string>()).Returns(bar1);
    foo.CreateBar(Arg.Is<string>(s => ContainsB(s))).Returns(bar2);

    Assert.Equal(50, foo.CreateBar("abc").Number); // PASSES
    Assert.Equal(25, foo.CreateBar("123").Number); // PASSES
}

[Fact]
public void NSubIssue492_ShouldGetDifferentAutoMocks() {
    var foo = Substitute.For<IFoo>();

    var bar1 = foo.CreateBar(Arg.Any<string>());
    var bar2 = foo.CreateBar(Arg.Is<string>(s => ContainsB(s)));
    Assert.NotSame(bar1, bar2); // -> FAILS
}

public bool ContainsB(string s) {
    Console.WriteLine("!!!" + s);
    return s.Contains("b");
}

[dtchepak]

@alexandrnikitin Does this behaviour seem familiar to you? I have a vague recollection of you looking in to a similar issue previously, but I can find it in the issue log. Please let me know if you remember anything.

[alexandrnikitin]

@dtchepak Unfortunately I don't remember 😞 We have #56 "Nested property calls" issue but I don't think it's related.

[zvirja]

@dtchepak @alexandrnikitin Finally found time to take a look 😊

Essentially, as it has been discovered by David, the issue is caused by the following behavior:

[Fact]
public void NSubIssue492_ShouldGetDifferentAutoMocks() {
    var foo = Substitute.For<IFoo>();

    var bar1 = foo.CreateBar(Arg.Any<string>());
    var bar2 = foo.CreateBar(Arg.Is<string>(s => ContainsB(s)));
    Assert.NotSame(bar1, bar2); // -> FAILS
}

It appears that NSubstitute is not ready to support this feature on all the layers. Originally, the issue happens when we try to test whether we already have result for the call: https://github.com/nsubstitute/NSubstitute/blob/47366a9b9b0fdc241662c291547ce431e9536c9d/src/NSubstitute/Core/CallSpecification.cs#L24-L30

Even if incoming ICall instance has pending specifications, we don't check them here - we care about raw argument values only. That is not the only place which doesn't support that - the IArgumentSpecification type don't support equality comparison itself. Same happens for the IArgumentMatcher type used under the hood. So the fix will require subtle amount of additional work.

In addition to the technical challenge we have, I see some confusion around the feature itself. Consider the following code:

sut.Echo(Arg.Any<string>()).Name.Returns("Blah");
sut.Echo(Arg.Is(str => str.StartsWith("Hello"))).Id.Returns("WelcomeId");

One might expect that for the Hello Alex string the result will be { Name = "Blah", Id = "WelcomeId" } -but that's not true. Of course, it's pretty obvious why it doesn't work, however the feature itself makes it very intuitive to feel like that's supported.

I'm not sure how to proceed on this. I have a bright idea to tweak the CallSpecification.IsSatisfiedBy() method so that we never match calls which has pending specifications (the call.GetArgumentSpecifications() is not empty). Currently we are not able to match them anyway, so let's be hones and not pretend we can do it 😄 The only implication will be that the following code will not work as you might expect - result will be { Id = "SUPER_ID" }:

sut.Echo(Arg.Any<string>()).Number.Returns(42);
sut.Echo(Arg.Any<string>()).Id.Returns("SUPER_ID");

Might be not a big problem - if you have such a complex setup, it's fine to create intermediate substitute... 🤔

@dtchepak @alexandrnikitin Your thoughts?

[dtchepak]

Thanks for looking into this @zvirja.

I was actually surprised to find that the "SUPER_ID" version "worked" with the current code! (gives { Id = "SUPER_ID"; Number = 42 }) 😂

I think this issue gets very confusing very quickly. NSubstitute's API encourages not thinking too hard about the underlying mechanics (effectively we do 2.Returns(42) and similar absurdities all the time 😂), and so I think it is fairly natural to carry on that thinking to assume something like:

sut.Echo(Arg.Any<string>()).Name.Returns("Blah");
sut.Echo(Arg.Is(str => str.StartsWith("Hello"))).Id.Returns("WelcomeId");

var result = sut.Echo("Hello there");

will cause result to be { Id = "WelcomeId"; Name = "Blah" } (we have an argument that matches both conditions).

I think I prefer your suggestion of breaking the "SUPER_ID" behaviour in favour of having a more consistent logic behind this: if you stub a call it will overwrite previous, overlapping stubs; and, be careful with recursive subs!

But this will be a breaking change, breaking previously passing tests.

Might be not a big problem - if you have such a complex setup, it's fine to create intermediate substitute... 🤔

Are you saying maybe we should leave it like this, and instead suggest people be more explicit in setups that have this problem? Or that the changed "SUPER_ID" behaviour from fixing CallSpecification.IsSatisfiedBy() is not a big problem because they can create intermediate sub?

[zvirja]

Or that the changed "SUPER_ID" behaviour from fixing CallSpecification.IsSatisfiedBy() is not a big problem because they can create intermediate sub?

Yes, that's what I suggested 😉

But this will be a breaking change, breaking previously passing tests.

So let's plan this change for v5 🎈 Our first reason to create the next major version 😅