Detect Arg matcher misuse

[dtchepak]

People often use Arg.Any<int>() in real calls (rather than .Returns stub or .Received assertion) to represent "I don't mind what input is used for this test".

This case is show in the documentation. There is also an example in this post (search for "Another example").

This causes problems for NSubstitute because it tries to use these argument matchers the next time it gets a call (it will consider the next call to be an attempt to stub, rather than a real call), and it can cause confusing test behaviour or test errors.

Proposed title:

Argument matcher used without call to .Returns or .Received

Proposed description (from docs):

Argument matchers should only be used when setting return values or checking received calls. Using Arg.Is or Arg.Any without a call to .Returns or Received() can cause your tests to behave in unexpected ways.

[tpodolak]

Hi @dtchepak
Should I report diagnostic for any usage of methods from Arg class when used outside of NSubstitute context? Or maybe only specific ones cause the troubles? Additionally, I believe that Arg can be used within When method, is there any other case apart from this one and the one you've mentioned where Arg is allowed to be used?

[dtchepak]

@tpodolak Arg.Any is the main one that seems to be used problematically, but Arg.Is can cause similar problems. Arg.Invoke and Arg.Do automatically put the substitute into stub mode so they should be safe.

I think Arg.Any and Arg.Is can be allowed in these contexts:

• In a call before .Returns and .ReturnsForAnyArgs. e.g. sub.Call(Arg.Any<int>()).Returns(42).
• With Received, ReceivedWithAnyArgs, DidNotReceive, DidNotReceiveWithAnyArgs. e.g. sub.Received().Call(Arg.Any<int>()).
• In When and WhenForAnyArgs blocks. e.g. sub.When(x => x.Call(Arg.Any<int>>())).Do(...). Should not be used in the .Do block.
• In the Received.InOrder block. e.g. Received.InOrder(() => { sub.Call(Arg.Any<int>()); })

As an aside, I think it is also an error to use Arg.Is in conjunction with ReturnsForAnyArgs, ReceivedWithAnyArgs, DidNotReceiveWithAnyArgs or WhenForAnyArgs, as those argument matchers will be ignored. I think that is probably a separate issue though, and is definitely not as big an error as the one in this issue as this one can cause different tests to fail unexpected as the state leaks into other tests.

@alexandrnikitin @zvirja Can you think of other cases I've missed here?

[tpodolak]

@dtchepak thanks for the clarification. I think this one will be tricky to implement, the approaches which I was using so far, will probably report false positives. I will take a deeper look at some alternative approaches

[dtchepak]

Thanks @tpodolak. If it is not practical to implement then please close the issue or tag as nice-to-have, or schedule for the-distant-future milestone or similar. 😄

[tpodolak]

@dtchepak as for today I am able to detect incorrect usages of Arg matcher, however, I am still trying to make the analyzer smart enough not to report false positives. For instance

using NSubstitute;

namespace MyNamespace
{
    public abstract class Foo
    {
        public abstract int Bar(int x);
    }

    public class FooTests
    {
        public void Test()
        {
            var substitute = NSubstitute.Substitute.For<Foo>();
            Received.InOrder(() => substitute.Bar(Arg.Is(1)) );
            Received.InOrder(() => { substitute.Bar(Arg.Is(1)); } );
            Received.InOrder(delegate { substitute.Bar(Arg.Any<int>()); });
        }
    }
}

For this piece of code, no diagnostics will be reported (correct behaviour). However this one

using NSubstitute;

namespace MyNamespace
{
    public abstract class Foo
    {
        public abstract int this[int x] { get; }

    }

    public class FooTests
    {
        public void Test()
        {
            var substitute = NSubstitute.Substitute.For<Foo>();
            Received.InOrder(() => Foo(substitute));
        }

        private void Foo(Foo substitute)
        {
            var x = substitute[Arg.Any<int>()];
        }
    }
}

will be considered as invalid usage - even though it is valid. Similary for usages of actual methods (not lambdas or delegates) in Returns/Throws/When etc

[dtchepak]

@tpodolak Sounds very promising! So it is possible?

[tpodolak]

I think/hope so. It requires sort of wider analysis(I basically need to know if given function which uses Arg in the body, is used somewhere in Returns/When/Received statement). I am experimenting with approach presented here. Maybe it will be enough for our needs

[abravodev]

This would be great, because these are the most insidious errors.

[tpodolak]

@dtchepak I am getting closer to finish this one (at least for some beta/alpha release) should we reconsider changing the title and description of diagnostic? You proposed

Proposed title:
Argument matcher used without call to .Returns or .Received

Proposed description (from docs):
Argument matchers should only be used when setting return values or checking received calls. Using Arg.Is or Arg.Any without a call to .Returns or Received() can cause your tests to behave in unexpected ways.

However, as you mentioned in #35 (comment) it is also possible to use arg matchers in Received.InOrder When, WhenForAnyArgs, DidNotReceive etc. Should we change the diagnostic message somehow to indicate the all possible usages?

[dtchepak]

@tpodolak Yes I think so.

Proposed description (take 2):

Argument matchers should only be used when setting return values, checking received calls, or configuring call actions. Using Arg.Is or Arg.Any in other situations can cause your tests to behave in unexpected ways.

Argument matchers are permitted for:

• Specifying a call when using Returns and ReturnsForAnyArgs
• Specifying a call within a When or WhenForAnyArgs block to configure a callback/call action
• Specifying a call to check with Received, DidNotReceive and Received.InOrder
• Configuring a callback with Arg.Do or Arg.Invoke

See NSubstitute's argument matcher documentation for more information.

What do you think?

I'm not sure about title. Here are a few ideas:

• Argument matcher used without specifying a call
• Argument matchers not used for specifying or configuring a call
• Potential argument matcher misuse

[dtchepak]

@tpodolak I've got a PR in for the docs: nsubstitute/NSubstitute#549

[dtchepak]

Another nice repro example in nsubstitute/NSubstitute#587.

@tpodolak I tried the build here: #35 (comment)
It didn't detect that case. Sorry for the delay in trying it out, I lost it off my todo list somehow.