AmbiguousArgumentsException when upgrading to 4.0

[waynebrantley]

I attempted to upgrade to 4, but ran into the error below. I have a unit test that will pass if run by itself. If I simply copy the unit test and make another version of the same test, the first one ran passes, but the 2nd one always fails.

In the [setup] (run before each test), I create a substitute using ForPartsOf. That looks like this:

var cqrsUow = Substitute.ForPartsOf<CqrsUow>(); cqrsUow.WhenForAnyArgs(x => x.ActuallyCommit(null, false)).DoNotCallBase();

When the 2nd test runs - the error happens when executing the WhenForAnyArgs line above.

NSubstitute.Exceptions.AmbiguousArgumentsException : Cannot determine argument specifications to use. Please use specifications for all arguments of the same type.
Method signature:
    ActuallyCommit(ITransaction, Boolean)
Method arguments (possible arg matchers are indicated with '*'):
    ActuallyCommit(*<null>*, *False*)
All queued specifications:
    any Func<Task>
    any ITransaction
    any Boolean
Matched argument specifications:
    ActuallyCommit(???, ???)

cqrsUow.Configure().ActuallyCommit(Arg.Any<ITransaction>(), Arg.Any<bool>()).Returns(1);
cqrsUow.Configure().ActuallyCommit(null, false).Returns(1);
cqrsUow.Configure().ActuallyCommit(Arg.Any<ITransaction>(), Arg.Any<bool>());
cqrsUow.WhenForAnyArgs(x => x.ActuallyCommit(Arg.Any<ITransaction>(), Arg.Any<bool>())).DoNotCallBase();

I tried to create a simple repo, but could not - simply calling the [setup] method twice does not cause it. Calling [setup] method after I run my command that uses the DI and the above substitution does cause the error...but I could not get something simple for reproducing.

The unit tests described above all pass in NSubstitute 2.x and 3.x without any issues. Any ideas?

[dtchepak]

Hi @waynebrantley ,
NSub 4.0 has improved detection of error cases that is causing this to fail. The error reports this:

All queued specifications:
    any Func<Task>
    any ITransaction
    any Boolean

Notice that there is an Arg.Any<Func<Task>>() queued, so NSubstitute does not know how to use this for the ActuallyCommit(ITransaction, Boolean) call. This may have been queued but not used in a previous test.

Take a quick look around for Func<Task> args nearby. If it is not obvious where the problem is, add the NSubstitute.Analyzers package to your project, which may pick up the issue. Failing that, check the test execution order and narrow down where this arg matcher is queued up.

The silver lining to this is that in future you'll be able to pick up these cases much more quickly and easily as NSubstitute 4.0 will report these sooner. Superfluous argument matchers can cause big problems for tests, so it's important that we start to throw errors in these cases.

[waynebrantley]

the test does this:
_cqrsUow.Received(1).RegisterAfterCommitCallBackItem(Arg.Any<Func<Task>>());

That should be ok, shouldn't it?

RegisterAfterCommitCallBackItem is not specifically mocked on this ForPartsOf call...

[dtchepak]

Is RegisterAfterCommitCallBackItem a virtual method?

[waynebrantley]

No it is not. Reading the docs...again...it does say:

Even without substituting for specific parts of a class, the instance returned by Substitute.ForPartsOf records all calls made to virtual members, so we can check Received() calls made to any partial substitute.

So - it does not record calls to non-virtual members - so I cannot check these? Why does it work (and pass) in NSubstitute 2 and 3?

[dtchepak]

Unfortunately NSubstitute cannot do anything with non-virtual members. The (approximate) way NSubstitute works is to dynamically create a sub-class (or a new class that implements an interface) of the type being substituted for. So we get something like this:

public class Sample {
    public virtual int DoStuffWith(string s) { return s.Length; }
}

// Substitute.For<Sample> returns an instance of:
class SampleProxy : Sample {
    public override int DoStuffWith(string s) {
        // record the call
        // look up configured return value, run when..do actions
        return configured_return_value;
    }
}

When the member is non-virtual, NSubstitute cannot override it, so it ends up being effectively invisible to NSubstitute.

The reason it passes with NSubstitute 2 and 3 is because of this invisibility. For example:

// If we write:
_cqrsUow.Received(1).RegisterAfterCommitCallBackItem(Arg.Any<Func<Task>>());

// at runtime, NSubstitute will only see:
_cqrsUow.Received(1);
Arg.Any<Func<Task>>();

Because (as far as NSub is concerned) we haven't said what call we want to check has been received, the assertion will never get triggered and therefore will never fail.

This problem with virtual members has always been the significant downside of the syntax we have selected. But the concise syntax is nice, so we've thought it worth the cost. In particular, if doing a test-first style approach, we end up testing the test as we write it. For example: write a test to assert a call was received, run it and watch it fail, then implement the code to pass the test. If the second steps passes unexpectedly, we've immediately found an issue with the test. (This was the main scenario NSub was written for. It also works fine with test-after approaches, if we tweak the test/code after the fact to ensure it fails/tests what we think it does).

We've been steadily changing to detect more of these cases at an earlier stage to help avoid misleading tests, so the problem caused updating to NSub 4 in this case has been a good thing (although I'm very sorry for the inconvenience in the first place!). The culmination of these efforts has been @tpodolak's NSubstitute.Analyzers, which will pick out a large number of these problematic cases at compile time using Roslyn. I strongly recommend installing this package wherever people have NSubstitute installed.

I'm really sorry for the previous NSub versions resulting in a misleading test and for the confusion caused when upgrading. Hopefully this has given a good explanation of why we decided to make this breaking change.

Regards,
David

[waynebrantley]

Makes sense...while 'breaking change'....my prior code was actually broken...so I guess it is ok!
I installed the analyzer and it is perfectly happy with my 'bad calls' though.
In fact, I see no warnings from it at all...FYI

[tpodolak]

Hi @waynebrantley I did a quick check of analyzers based on previous comments, and the usages of Received, When and WhenForAnyArgs against non-virtual members are marked as warnings

Can you provide a full sample of code for which analyzers are not showing the warnings?
PS
Visual Studio sometimes doesn't pick up the analyzers after installation and restart is required. Unfortunately, there is nothing I can do about it as this is a Visual Studio quirk :(

[waynebrantley]

So the difference is the CqrsUow implements an interface IUnitOfWorkCoreCqrs.
The substitute variable is that interface (and under normal circumstances is provided through DI - hence the interface). So this code will not show the warning (and consequently installing your analyzer resulted in no warnings of any kind.

            IUnitOfWorkCoreCqrs cqrsUow = Substitute.ForPartsOf<CqrsUow>();
            cqrsUow.Received(1).RegisterAfterCommitCallBackItem(Arg.Any<Func<Task>>());

[tpodolak]

@waynebrantley thanks for providing the snippet, now I know what is going on.
Because you are assigning CqrsUow to IUnitOfWorkCoreCqrs, from the Roslyn semantic model point of view, you are calling Received method on the interface member - so no warnings were reported. As for today our analyzers are not able to correctly figure out what was the "real" assignment done during substitute creation. This is non trivial task which requires a data flow analysis to travers the syntax tree and find the real type assigned to variable. I've created a task in NSubstitute.Analazyers repo nsubstitute/NSubstitute.Analyzers#68 to investigate possibilities of improving non-virtual member detection. For now I can only suggest to use var

var cqrsUow = Substitute.ForPartsOf<CqrsUow>();
cqrsUow.Received(1).RegisterAfterCommitCallBackItem(Arg.Any<Func<Task>>());

instead

[waynebrantley]

This is non trivial task

Yes, figured that

For now I can only suggest to use var

Cannot really do that as the value we are using comes from DI, etc.... No worries

Now that I am through all this - I really do appreciate the fact that NSubstitute 4 gives errors in this case. Previously it just 'appeared' the test was working - when in fact it was not! So while figuring out why and such was a bit of pain - this is a better solution.

FYI: The only use case not really handled would be if I had only had ONE test - it does happily pass each time! It is only when I create a 2nd substitute that it fails.

Thanks for your help on this and all your work on a really great product!