Detect Arg matcher misuse about nsubstitute.analyzers HOT 21 CLOSED

@tpodolak Sorry for under-specifying this. :( Can't thank you enough for all the work you put into this project.

from nsubstitute.analyzers.

@dtchepak @zvirja I've updated the analyzers to support Arg.Do, Arg.Invoke and Arg.InvokeDelegate
Please test the updated package when you have time
NSubstitute.Analyzers.CSharp.1.1.0-GH-35-arg-matcher1.zip

from nsubstitute.analyzers.

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?

from nsubstitute.analyzers.

@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?

from nsubstitute.analyzers.

@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

from nsubstitute.analyzers.

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. 😄

from nsubstitute.analyzers.

@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

from nsubstitute.analyzers.

@tpodolak Sounds very promising! So it is possible?

from nsubstitute.analyzers.

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

from nsubstitute.analyzers.

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

from nsubstitute.analyzers.

@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?

from nsubstitute.analyzers.

@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

from nsubstitute.analyzers.

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

from nsubstitute.analyzers.

@dtchepak looks good IMO.I've chosen Argument matcher used without specifying a call as a diagnostic title. However I have one question, you wrote that Arg matchers are allowed in

Configuring a callback with Arg.Do or Arg.Invoke

As I've never used Arg.Do and Arg.Invoke, can you provide an example of proper usage of those features combined with Arg.Is and Arg.Any?

When it comes to progress I am close to finishing the analyzer - what is left is basically testing in a real project (and probably support for Arg.Do Arg.Invoke). Everything was pushed to GH-35-arg-matcher branch, so if you want to test it on your own feel free to build the package locally or grab this one NSubstitute.Analyzers.CSharp.1.1.0-GH-35-arg-matcher1.zip. Just remember to change extension from zip to nupkg.

PS
Due to the complexity of the analysis, this analyzer doesn't show warning immediately but only during the compilation.

from nsubstitute.analyzers.

@tpodolak Arg.Invoke and Arg.Do work just like Arg.Any for the purposes of matching a call, but will run actions if the call is matched. For example:

[Fact]
public void WidgetSample() {
    var sub = Substitute.For<IWidgetFactory>();
    WidgetSpec specUsed = null;
    sub.Create(Arg.Do<WidgetSpec>(x => specUsed = x), Arg.Is<int>(x => x < 10));

    sub.Create(new WidgetSpec { StyleCode = 42, NumberOfSprockets = 10 }, 1);
    sub.Create(new WidgetSpec { StyleCode = 5, NumberOfSprockets = 55 }, 999);

    Assert.Equal(42, specUsed.StyleCode);
    Assert.Equal(10, specUsed.NumberOfSprockets);
}
public class Widget { }
public class WidgetSpec {
    public int StyleCode { get; set; }
    public int NumberOfSprockets { get; set; }
}
public interface IWidgetFactory {
    List<Widget> Create(WidgetSpec spec, int quantity);
}

In this example, the WidgetSpec with style 42 is captured as it matches the sub.Create(any, int < 10) specification.

from nsubstitute.analyzers.

Got it (I think). So we can use Arg.Is, Arg.Any without specifying a call, if they are used together with Arg.Invoke or Arg.Do(within given method call), is that correct?

from nsubstitute.analyzers.

Yes. Once we use Arg.Invoke or Arg.Do we are specifying a call (and call actions to run). We can also use Arg.Is or Arg.Any at that time. 👍

from nsubstitute.analyzers.

Ok, in that case, I will have to add support for that in analyzers. Rest of the cases are covered I think. I will try to run the analyzers at work tomorrow to see if they work correctly on a large solution/project

from nsubstitute.analyzers.

@dtchepak I've noticed that there is also Arg.InvokeDelegate method, is this one also allowed to be used with Arg.Any and Arg.Is ?

from nsubstitute.analyzers.

@tpodolak Yes, Arg.InvokeDelegate can be considered the same as Arg.Invoke. I think it is only Arg.Any and Arg.Is that don't automatically switch a call to "call-specification mode".

from nsubstitute.analyzers.

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.

from nsubstitute.analyzers.