Exceptions by NSubstitute affect the next test

[tndata]

Describe the bug
It is really hard to debug when errors/exceptions in one test affects the next test. Tests should be independent and not depend on previous tests. This really makes debugging impossible when a bug in one test makes the next test fail.

To Reproduce
The minimal code below reproduces the problem where the first test fails due to exception in the second test.

Yes, the code below is not really correct, but still the effect is unexpected. Yes, the analyzer picks it up, but I think the test should fail regardless:

public class UnitTest1
{
    public class User
    {
        public string UserName { get; set; }
    }

    public interface IDatabase
    {
        int CreateUser(string userName);
        User LoadUser(int userId);
    }

    [Fact]
    public void OKTestThatFail()
    {
        var stub = Substitute.For<IDatabase>();
    }

    [Fact]
    public void BadTestThatPass()
    {
        var stub = Substitute.For<IDatabase>();
        stub.Received(Arg.Any<int>());       
    }
}

Expected behaviour
I would expect that the second test in the code above fails due to bad usage of NSubstitute.

Environment:

• NSubstitute version: [e.g. 4.2.1]

Additional context
Add any other context about the problem here.

[dtchepak]

Hi @tndata ,

Unfortunately this is always going to be an issue for NSubstitute's syntax. We do some awful things to get the readable syntax, and the trade-off is having cases like this.

For this particular example, if you think about how stub.Received().LoadUser(42); works, first Received() gets called, which tells stub that the next call it receives should be used for an assertion, then LoadUser(42) is called, so the stub checks that it has received that call and throws if not. (A small example of the pseudocode generated for a method like LoadUser is in the How NSubstitute Works documentation.)

The problem for us is that when Received() is called we have no way of knowing at runtime if it is going to be followed by the required call to for the assertion.

The good news is that thanks to some awesome work from @tpodolak with Roslyn, we can add the NSubstitute.Analyzers package for C# or VB to detect these (and other) cases that can cause problems at runtime. For this case it gives us the following warning on the stub.Received(Arg.Any<int>()); line:

warning NS5000: Unused received check. To fix, make sure there is a call after "Received". Correct: "sub.Received().SomeCall();". Incorrect: "sub.Received();"

I highly recommend installing NSubstitute.Analyzers in any project where NSubstitute is used.

Hope this helps.

Regards,
David

[tndata]

Thanks, I understand. In my case the bad tests throws an NSubstitute exception, but then shows up in the OK test.. The lack of isolation really caught my by surprise as I expect one Stub would not interefer with stubs in other tests.

[dtchepak]

The lack of isolation really caught my by surprise as I expect one Stub would not interefer with stubs in other tests.

Yes it is definitely a big drawback to NSubstitute. When we were first writing it we thought it was worth this to get a concise, easy-to-read (in our opinion) syntax. This is further mitigated by writing tests first (or at least not long after) the production code, making sure each tests fails as expected then passes as expected, which tends to pick up any problems before the bleed out to other tests. The more recent invention of NSubstitute.Analyzers has dramatically improved this situation (although there are still cases that can slip through).

If that is a bit too uncomfortable for you or your team (which I completely understand), I heartily endorse Moq and FakeItEasy as alternatives that don't have this drawback (at the expense of ever-so-slightly more syntax IMO, YMMV :) ).

I'll close this for now as I think NSubstitute.Analyzers covers this particular case. Feel free to re-open if you have further questions, or to open new issues if you find problems NSubstitute.Analyzers does not cover.