-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exception thrown from async Received.InOrder() crashes NUnit #604
Comments
Just checked by quickly adding an awaitable version 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()); |
@grzesiek-galezowski Thank you for reporting the issue. I'll try to explain the motivation behind As for the issue and the mitigation. I don't have strong opinion whether we need to introduce one more helper method
|
Hi, thank you for the explanation. So if I understand correctly, I should not be passing Please allow me to explain my motivation in reporting this issue.
I think other people will try passing async blocks inside 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. |
Hi @grzesiek-galezowski ,
IIRC using
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. 🤔 |
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. |
@dtchepak @grzesiek-galezowski as for now there is no analyzer which would detect non-substitute calls in |
Hi, @tpodolak. Thanks for the info. I don't remember another case where the analyzers did not pick non-substitute right now. |
@zvirja Yes I think you are right. I was thinking of adding a |
@dtchepak, @zvirja does it make sense to add analyzer for that as well? |
@tpodolak Yes I think so. I will write up some draft |
That would be great, thanks. |
The following test crashes NUnit:
If I remove the
async
keyword, then the exception is correctly propagated to the test.Exception stack trace:
Project dependencies:
The text was updated successfully, but these errors were encountered: