nunit: Replacing ThrowsAsync with a composable async alternative

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.

About this issue

Original URL
State: closed
Created 6 years ago
Reactions: 7
Comments: 40 (26 by maintainers)

Most upvoted comments

Is this causing pain to enough people to make big changes now? I expect that if we change the behaviour of Assert.That to throw an ArgumentException now, it would be a lot of work for people with a lot of async in their tests to go through and fix them all. It isn’t a straight search and replace, and it is a runtime failure, not a compile-time failure.

I think we’re in a bit of a corner here and I don’t know what the right answer is. We can’t change Assert.That to be awaitable because it would break too many people. I also don’t think that most people would appreciate why we are doing it because our current async tests work fine 99% of the time.

So, I’m okay with introducing true async asserts with new Assert.ThatAsync calls, but I’d be uncomfortable throwing exceptions if people try to use our current Assert.That. I even think that might be a big enough breaking change that I’d be reluctant to do it in a major upgrade. Most people don’t read the breaking changes on major upgrades or aren’t able to do the work needed.

I would like to see analyzers gentle pushing people to better alternatives.

rprouse on Dec 20, 2019

No you misunderstand my question. You’ve told me the special case method for accessing Message, and the less special case method for accessing a given Property. Thank you. I’m now asking if there is an even more general way to map the With object, in this case the exception, using an arbitrary function, to a value (which would then be further automatically wrapped ready to apply the That tests to it).

Other than Property itself (and various collection-related accessors), I don’t think so. Typically you’d use Matches or a custom constraint for that sort of thing, or capture the value external to the assertion so that you can perform multiple assertions on the same value (without just using And combinators).

uecasm on Apr 15, 2023

@mikebeaton The NUnit3TestAdapter works with NUnit version 4. We have a bit of name and version mangling here. If we could have changed the name it would be without the number 3 there, it doesn’t mean anything anymore. And, if you guys find something that says it doesn’t anymore, we need to fix that.

OsirisTerje on Apr 14, 2023

@jnm2 @Daniel-Svensson @uecasm @trampster @Dreamescaper @mikebeaton @JohanLarsson Can you guys confirm that https://www.myget.org/feed/nunit/package/nuget/NUnit/4.0.0-dev-07733 is working for you?

OsirisTerje on Apr 6, 2023

Because of Assert.That(async () => await SomethingAsync(), Throws.TypeOf<FooException>()) needing to be awaitable, v4 is certainly the easiest way to go because we can change public interfaces.

Even Assert.That(async () => { await Task.Delay(1000); return 4; }, Is.EqualTo(4)) would need to be an awaitable call.

I think we should do Assert.ThatAsync as the most obvious awaitable version, and throw ArgumentException if you use Assert.That when anything async is going on with a message to use Assert.ThatAsync. How does that sound?

jnm2 on Nov 13, 2019

@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.

JohanLarsson on May 8, 2018

@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.

CharliePoole on May 3, 2018

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.

rprouse on May 3, 2018