Replacing ThrowsAsync with a composable async alternative

[jnm2]

Today's Assert.ThrowsAsync and related methods do sync-over-async. This means they convert an async operation into a synchronous one. In other words, they block until the task is complete and return void or the return value directly, rather than returning an awaitable type themselves such as Task ThrowsAsync or Task<Exception> CatchAsync.

What does the problem look like?

public class Assert
{                Problem: not async Task
                    ⬇
    public static void ThrowsAsync(AsyncTestDelegate code) { … }

                Problem: no async Task ThatAsync
                    ⬇
    public static void That(ActualValueDelegate<Task> code) { … }
}

      Problem: not async Task
[Test]   ⬇
public void TestMethod()
{
  Problem: no `await`
    ⬇
    Assert.ThrowsAsync(async () =>
    {
        await Task.Delay(10_000); // Fiddle while Rome burns, why don’t ya 😇
    });

  Problem: no `await`
    ⬇
    Assert.That(async () => await …, Throws.InstanceOf<FooException>());
}

Why is this a problem?

Waiting synchronously for an async operation is a antipattern with real-world consequences. The consequences range from losing scalability (if you're doing async right, there is no thread) to outright deadlocks and other logic errors.

Well, in 3.11, I fixed the deadlock issue if you instantiate Windows Forms or WPF controls in your tests. So what then, why not keep doing sync-over-async in Assert?

• First, deadlocks are still a threat. I only fixed deadlocks on recognized message-pumping SynchronizationContexts, and even that hasn't been stress-tested for reentry with assert delegates nested inside assert delegates. We shouldn't have to go to the length of testing sync-over-async-over-sync-over-async; our APIs shouldn't be allowing it like this.

• Second, without a good reason, it goes in the face of hard-learned good practices with async/await. The async-await paradigm is complex enough for those who are still becoming familiar with it that we should not be requiring them to code their tests around this antipattern. Assert.*Async is doing nothing which justifies an exception. It's one thing for NUnit as a framework to do sync-over-async with the entry point, the test method itself—this is exactly like C# 7.1's async Task Main—but it's quite another thing for NUnit as a library, in Assert.*Async, to be doing sync-over-async as well.

What does the solution look like?

I've had proposals for this nagging me for a year and a half now, and none of them seem like slam dunks.

If we just switch Assert.ThrowsAsync from void to async Task we silently break some people's code. And if we switch Assert.CatchAsync from Exception to Task<Exception>, we cause any code using it to stop compiling. The second instance might be considered good pain, but the first option (silent breakage) is absolutely unacceptable.

➡ This means likely the best course is deprecation warnings for all Assert.*Async methods.
(Assuming we don't ship a special analyzer in the NUnit package to turn the silent errors into compiler errors and provide a "fix all.")

If we do deprecation warnings, these are our options as I see them:

• Pick new names for the async methods. (I have no ideas for names, though.)
• Introduce AssertAsync.Throws and AssertAsync.That where all the methods return Task or Task<>.

@rprouse I would like to fix this in 3.11 as part of the theme, if that seems good to you.

[rprouse]

I would like to get more @nunit/framework-and-engine-team members to weigh in here too. I have always found it odd that we do sync-over-async and don't require people to await async asserts. The original code was written before I joined the team and I think much of it was inherited from 2.6. I always thought that our async asserts would be much simpler if they were async all the way down. I'm not sure, but I think that the original decision might have been to support async void test methods which we no longer support.

Backing out of this mistake is a thorny issue though. We don't want to break existing code. Even deprecation warnings are a pain. Modern C# code has async everywhere and most of the test I write are async. Introducing that many warnings into peoples' unit tests won't be popular.

That said, I like the AssertAsync or maybe AsyncAssert idea. I don't really like the idea of finding new names for our current async asserts.

I would appreciate if other contributors and the NUnit community as a whole could weigh in on this. I think it is a great idea in principle, but I would like to do it in a way that will cause the least pain for people.

[CharliePoole]

@rprouse Throws.Async came in with 3.2. We all had our doubts back when it was added in PR #1142. In fact you made comments about being uncomfortable with sync over async back then. The contributor, @thomaslevesque , initially made it return Task<Exception> so it had to be awaited. It may be useful to review all the discussion that led to its being changed to be synchronous in the final commit.

I think our attitude toward asserts has been formed by our attitude toward async tests. In general, we have to run tests synchronously, even if they are async, in order to be able to capture the result. We applied the same principle to asserts, I think with good reason. Our contract has always been that execution terminates when an assertion fails. Therefore, we had to wait for the assertion to complete in order to know if we should continue or not. Of course, we now have multiple asserts and warnings, so execution doesn't always terminate on failure.

I think we probably should think about true async assertions, but I don't think it can be done cleanly in the current architecture. We would have to put as much thought and design work into it as we did into parallel execution of tests.

[jnm2]

AsyncAssert is a really good suggestion.

I have an idea for what it would look like. If I put up a spike, it might be something to talk about.

We would still run the entry points sync-over-async. Test methods, setups, etc. We'd just provide a new true-async assertion API. The framework architecture stays the same but we gain a new library API. What I'm thinking of might in theory even be able to be done as an outside library extending NUnit.

[jnm2]

We could also consider not providing a deprecation warning and shipping an analyzer with a suggestion, or even leave it to XML docs and release notes.

@JohanLarsson Have you had experience writing a fix-all to update API usage, changing Assert.Throws to await AsyncAssert.Throws and changing the containing method to async Task? I know it can be done in theory.

[jnm2]

Just to be clear: I don't think it would be a bad idea to make the framework's worker infrastructure true-async, but I'm not sure what we'd gain and I don't want to jeopardize our standing in Johan's benchmarks. 😊

[JohanLarsson]

@jnm2 Yes, fix all is a bit of a pain using using the built in stuff in Roslyn but there are ways to make it work.

I'm mixed about async asserts as they add some noise to tests.

[rprouse]

@JohanLarsson just curious, are your images for Rider benchmarks swapped for NUnit and xUnit? It seems odd that they would be opposite on each IDE.

[JohanLarsson]

@rprouse I did not run the Rider benchmarks, was contributed in a PR, so not sure.
The repo is just a quick and dirty hack to get some data to confirm a feeling I had.

[jnm2]

Ran into a deadlock today in a test with a custom synchronization context because Assert.Multiple(AsyncTestDelegate) blocks and returns void rather than returning Task and running asynchronously. It needs to be handled the same as the above:
await AsyncAssert.Multiple(async () => { … });

[jnm2]

I started exploring this in https://github.com/nunit/nunit/compare/jnm2/async_assert. Many of the things I want to do will be easier once I finish my work on non-Task awaitables.

An early question: what is our policy on adding methods to public interfaces, such as Task<ConstraintResult> IContraint.ApplyToAsync(object)? If we add that method, we cause a source-breaking change for anyone who implemented IConstraint without deriving from the base Constraint class. It's also a binary-breaking change if a library is compiled for NUnit 3.10 and is used in a project which updates to 3.11.

If we instead introduce IAsyncConstraint : IConstraint which declares that new method, do we also introduce IResolveAsyncConstraint with IAsyncConstraint Resolve(), and deal with breaking changes for people using the public OperatorStack class by replacing uses of IConstraint with IAsyncConstraint?
Or do we use a runtime cast like this?