Exception thrown from async Received.InOrder() crashes NUnit

[grzesiek-galezowski]

The following test crashes NUnit:

    [Test]
    public void Test1()
    {
      Received.InOrder(async () =>
      {
        throw new Exception();
      });
    }

If I remove the async keyword, then the exception is correctly propagated to the test.

Exception stack trace:

Unhandled exception. System.Exception: Exception of type 'System.Exception' was thrown.
   at NUnitTestProject1.Tests.<>c.<<Test1>b__0_0>d.MoveNext() in ...\ConsoleApp1\NUnitTestProject1\UnitTest1.cs:line 15
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__139_1(Object state)
   at System.Threading.QueueUserWorkItemCallback.<>c.<.cctor>b__6_0(QueueUserWorkItemCallback quwi)
   at System.Threading.ExecutionContext.RunForThreadPoolUnsafe[TState](ExecutionContext executionContext, Action`1 callback, TState& state)
   at System.Threading.QueueUserWorkItemCallback.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()

Project dependencies:

<PackageReference Include="NSubstitute" Version="4.2.1" />
    <PackageReference Include="nunit" Version="3.12.0" />
    <PackageReference Include="NUnit3TestAdapter" Version="3.16.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.4.0" />

[grzesiek-galezowski]

Just checked by quickly adding an awaitable version of Received.InOrder() that uses the following variant of

public async Task RunInQueryContext(Func<Task> calls, IQuery query)
{
    _currentQuery.Value = query;
    try
    {
        await calls();
    }
    finally
    {
        _currentQuery.Value = null;
    }
}

and it seems to propagate the exception correctly when used like this:

await Received.InOrder(async () => throw new Exception());

[alexandrnikitin]

@grzesiek-galezowski Thank you for reporting the issue. I'll try to explain the motivation behind Received.InOrder helper. Received.InOrder(Action calls) and its parameter Action calls isn't meant to return anything and be async or awaitable. The sole purpose of it is to wrap calls into one context/unit-to-verify and "capture" their sequence. Since NSub works with virtual methods only, it doesn't execute them so that you don't need to care about their returns. So the param and the "InOrder" calls inside don't need to be async and awaitable.

As for the issue and the mitigation. I don't have strong opinion whether we need to introduce one more helper method Received.InOrder() that accepts Func<Task> or not. E.g.

    public class Received
    {
        public static void InOrder(Func<Task> calls)
        {
            var query = new Query(SubstitutionContext.Current.CallSpecificationFactory);
            SubstitutionContext.Current.ThreadContext.RunInQueryContext(calls, query).Wait();
            new SequenceInOrderAssertion().Assert(query.Result());
        }
    }

[grzesiek-galezowski]

Hi, thank you for the explanation. So if I understand correctly, I should not be passing async blocks inside Received.InOrder().

Please allow me to explain my motivation in reporting this issue.

1. When calls are checked in the Received.InOrder(), I typically do not put .Received(xxx) on each of them.
2. But I noticed, that sometimes, I get false positives when I think I validate a call on a substitute while in reality, I check on a real instance. With .Received(), I would get a NotASubstitute exception, but not here.
3. AFAIK NSubstitute itself cannot detect whether something is "substitute/not a substitute" in the Received.InOrder() as it first executes the calls and then queries a sequence registered by substitutes. So I thought, maybe if I tried to call Received() or something like that, in the Received.InOrder() block, then I will get an exception.
4. And then I noticed that when I pass an async action, the exceptions are "swallowed" by NCrunch and Resharper reports the test as inconclusive. dotnet test crashes.

I think other people will try passing async blocks inside Received.InOrder() so they may stumble upon this issue. When prototyping my own awaitable InOrder(), I made it return task only to skip the AggregateException, but that's hardly something critical for me. If an overload can resolve the exception issue without breaking someone's code, I am all for it. Or maybe there is another way, like creating that overload and straight away throwing an exception from it to help someone avoid it?

Anyway, now that I know I do not need to pass async blocks and everything will work, I understand if you decide to just close the issue.

[dtchepak]

Hi @grzesiek-galezowski ,

But I noticed, that sometimes, I get false positives when I think I validate a call on a substitute while in reality, I check on a real instance. With .Received(), I would get a NotASubstitute exception, but not here.

IIRC using Received() in a Received.InOrder block can cause other issues. To detect non-substitute calls, have you tried NSubstitute Analyzers? If it is not detecting this case please let us know.

I think other people will try passing async blocks inside Received.InOrder() so they may stumble upon this issue.

Yes I agree this is an easy thing to stumble upon. We can try to fix it in the API and/or detect the case in NSubstitute.Analyzers. 🤔

[grzesiek-galezowski]

Hi, @dtchepak

I don't think analyzers handle the non-substitute case, I have them installed and have never seen such warning.

Thanks for considering fixing this. my preference would be of course to fix it in the API, although this probably means more maintenance work for you. Analyzers should be fine as well.

[tpodolak]

@dtchepak @grzesiek-galezowski as for now there is no analyzer which would detect non-substitute calls in Receive.InOrder - this is still waiting for implementation https://github.com/nsubstitute/NSubstitute.Analyzers/issues/108. @grzesiek-galezowski is this the only case when analyzers didn't pick up the non-substitute call for you? If no please provide us with sample repro code so we can improve the analyzers

[grzesiek-galezowski]

Hi, @tpodolak. Thanks for the info. I don't remember another case where the analyzers did not pick non-substitute right now.

[zvirja]

@tpodolak @dtchepak Does it also make sense to not allow async callback for Received.InOrder? It looks like we are not handling them correctly anyway and there is no good reason for using them at all.

[dtchepak]

Does it also make sense to not allow async callback for Received.InOrder?

@zvirja Yes I think you are right. I was thinking of adding a Received.InOrder for Tasks so people could use await in the block if they wanted, but now you mention this it doesn't make sense. And I definitely don't want to encourage people to copy/paste their async code-under-test straight into the test.

[tpodolak]

@dtchepak @zvirja as you seem to agree not to introduce an additional Received.InOrder overload, I will add an analyzer to prevent people from making the Received.InOrder lambda async.

[tpodolak]

IIRC using Received() in a Received.InOrder block can cause other issues. To detect non-substitute calls, have you tried NSubstitute Analyzers? If it is not detecting this case please let us know.

@dtchepak, @zvirja does it make sense to add analyzer for that as well?

[dtchepak]

@tpodolak Yes I think so. I will write up some draft NSxxxx documentation for it if you like?

[tpodolak]

@tpodolak Yes I think so. I will write up some draft NSxxxx documentation for it if you like?

That would be great, thanks.