Suppress async void nag in asserts. about asyncusageanalyzers

[sharwell]

@JohanLarsson I believe the problem with that approach is the method can never throw in that form. Even if the expression fails (and throws an exception), the exception will be thrown on a different thread and terminate the test process without properly reporting results. If we were to special case this, it would more likely need to be making it an error since it's not behaving anything close to what the user expects.

[sharwell]

Which case is this?

1. The method does not have an async void signature, but it's reporting a warning anyway (false positive)
2. The method does have an async void signature, but this is a case where you commonly want to use this signature (intentional)

[JohanLarsson]

It is 2 I think, we are stuck with NUnit 2.6.4 for reasons and it gets nasty to suppress these calls all over the place. Don't love suppressing per file.

The case would be to specialcase Assert.Xyz(...) to allow it.

[JohanLarsson]

Try:

[Test]
public void ShouldThrow()
{
    var exception = Assert.Throws<Exception>(async () => await Task.Run(() => { throw new Exception("message"); }).ConfigureAwait(false));
    Assert.AreEqual("message", exception.Message);
}

[Test]
public void ShouldNotThrow()
{
        Assert.DoesNotThrow(async () => await Task.Run(() => { throw new Exception("message"); }).ConfigureAwait(false));
}

Using NUnit.2.6.4

[AArnott]

@JohanLarsson : are you sure NUnit doesn't offer a ThrowsAsync method that you should await, like this?

 var exception = await Assert.ThrowsAsync<Exception>(async () => await Task.Run(() => { throw new Exception("message"); }).ConfigureAwait(false));

Otherwise, as @sharwell said, exceptions are never thrown from methods with async in their signature. If NUnit recognizes Func specifically in its Throws method, then it might work, if NUnit were willing to synchronously block the calling thread while invoking that delegate, but that could cause deadlocks for other reasons, and would slow down concurrent test execution where applicable.

[sharwell]

/cc @jnm2

[jnm2]

Thanks Sam! @JohanLarsson You're using NUnit 2 which actually has this concept of invoking an async void method and then waiting for it to complete by having installed a spying SynchronizationContext. This is a low-fi substitute for awaiting an awaitable object like Task, behaving close enough if you squint. This was removed in NUnit 3. If you were running NUnit 3.0+, that code would error at runtime, telling you to use DoesNotThrowAsync.

My perfectionist side really hates the thought of turning off this insightful nag for all classes named Assert, especially for the sake of such a legacy version of NUnit. If a framework makes you do something yucky (await async void), let's try all options for upgrading the framework before turning off the warning light.

You say you can't upgrade though and that's something we take seriously. In the meantime, how's this workaround?

internal static class AssertUtils
{
    public static void DoesNotThrow(Func<Task> function)
    {
        if (function == null) throw new ArgumentNullException(nameof(function));

        // Suppress once per project.
        NUnit.Framework.Assert.DoesNotThrow(async () => await function.Invoke());
    }
}

Or even more drop-in, if you can handle this (ReSharper will suggest 'simplifying' your access of all the other Assert methods):

internal static class Assert : NUnit.Framework.Assert
{
    public new static void DoesNotThrow(Func<Task> function)
    {
        if (function == null) throw new ArgumentNullException(nameof(function));

        // Suppress once per project.
        NUnit.Framework.Assert.DoesNotThrow(async () => await function.Invoke());
    }
}

[jnm2]

To @AArnott's point:

NUnit's Assert.DoesNotThrowAsync is currently doing sync-over-async for historical reasons. As of NUnit 3.11, this won't deadlock for recognized SynchronizationContexts. But it's one thing for the framework to do sync-over-async with the entry point, the test method itself—this is exactly like async Task Main—and it's quite another thing for Assert.*Async to be doing sync-over-async as well. I've been thinking through ways to fix this; perhaps deprecate these sync-over-async APIs and introduce AssertAsync.

Almost opened an issue on this two weeks ago. Given Sam's ping, I think I'll go ahead and do that.

[jnm2]

If anyone is interested, I've opened the issue.