Async delegate assertions

[StephenCleary]

All assertions that take TestDelegate should have correlating methods that take an asynchronous equivalent (Task AsyncTestDelegate()). E.g., Assert.Throws(TestDelegate) should have a matching Assert.ThrowsAsync(AsyncTestDelegate) or Assert.Throws(AsyncTestDelegate). This would enable this kind of usage:

await Assert.ThrowsAsync<Exception>(async () =>
{
  await Task.Yield();
  throw new Exception();
});

Also, some synchronous assertion code has outdated code; e.g., Assert.Throws will try to create an AsyncInvocationRegion that will never succeed, since AIR (correctly) rejects async void methods and TestDelegate returns void. Ideally, AIR should only be used when executing the [Test] methods themselves, not within any other code.

[thomaslevesque]

What's the story for async void void in NUnit 3.0 ? In 2.6.4, it was possible to write async void test methods, and to pass async delegates to Assert.Throws. In 3.0, I can still write async void test methods, but passing async delegates to Assert.Throws is invalid. This seems inconsistent; it should either accept both, or none.

[rprouse]

Async void tests were supposed to be removed from NUnit 3. Are the tests
running? Are there no errors?
On Feb 1, 2015 4:44 PM, "Thomas Levesque" notifications@github.com wrote:

What's the story for async void void in NUnit 3.0 ? In 2.6.4, it was
possible to write async void test methods, and to pass async delegates to
Assert.Throws. In 3.0, I can still write async void tests, but passing
async delegates to Assert.Throws is invalid. This seems inconsistent; it
should either accept both, or none.

—
Reply to this email directly or view it on GitHub
#464 (comment).

[thomaslevesque]

OK, I understand now... I had referenced NUnit 3.0 alpha5 in my test project, but I was still using ReSharper's runner, which is 2.6.3, so it didn't reject the async void tests. Assert.Throws was failing, because it's in NUnit.Framework, not in the runner. If I use the 3.0 runner, I get the error on the async void tests as well.

[CharliePoole]

@StephenCleary Since Assert.Throws is capable of calling an async method, I'm not clear about why a separate ThrowsAsync is wanted. The only thing it seems to add is the ability for you to await the assert itself, but if you're calling an async method, you would generally await that inside the method, just as you do in the example.

[StephenCleary]

When Assert.Throws is used with an async method, it first sets up a context to execute that method. This context changes the behavior of await within the system under test. An Assert.ThrowsAsync would not establish a context and would not change the behavior of await.

[CharliePoole]

Can you post an example that shows Assert.Throws not working as you would wish the async version to work? Maybe a test that fails with Assert.Throws but would hypothetically pass with Assert.ThrowsAsync.

[StephenCleary]

Tried to write one today, but I couldn't get Assert.Throws to work with an async method at all; it always says "System.ArgumentException : 'async void' methods are not supported, please use 'async Task' instead". I mentioned this in my original comment: "Assert.Throws will try to create an AsyncInvocationRegion that will never succeed, since AIR (correctly) rejects async void methods and TestDelegate returns void."

Using 3.0.0-beta-1 (3.0.5562.31848).

[CharliePoole]

OK, we'll see what we can do with this.

[sjwoodard]

It would make sense to be able to write a test like this:
Assert.Throws<Exception>(async () => await obj.ThrowAsync());

The easiest alternative I found was this, but I would much prefer to use async/await.
Assert.Throws<AggregateException>(() => obj.ThrowAsync().Wait());

3.0.0-beta-3 (NuGet)

[StephenCleary]

The problem with Assert.Throws<Exception>(async () => await obj.ThrowAsync()); is that it requires sync-over-async behavior. If you're unit testing ViewModels or other code that needs to run within a single-threaded context, then there's no way for NUnit to do sync-over-async and avoid deadlocks.

That's why I prefer await Assert.Throws<Exception>(() => obj.ThrowAsync()); - it extends the asynchrony to include the unit test method, and NUnit's job is significantly easier (doesn't have to mess with contexts or avoiding deadlocks at all).

(note that await Assert.Throws... isn't currently supported either; I'm just pointing out why I think it's a better pattern).

[dustinmoris]

Why is this marked as a feature? It is a bug because currently I am not able to use Assert.Throws with an async delegate, which worked before and there is no alternative now either?

[CharliePoole]

@dustinmorris it's a feature because it's a new signature that didn't exist before.

If something worked before, it was something different from what is contemplated here.

[PaulARoy]

It's actually quite blocking, I have a lot of tests using Assert.Throws<MyException>(async () => await SomethingAsync());, and I think I'm not the only one. The await signature could indeed be pretty nice.

The AggregateException / Wait is a workaround, but it removes the possibility to filter the exception, so it may include errors.
I can't see any other possibility to test exception throwing for async methods. If there is, I'll gladly change my code! But I think it's just not possible as is.

If it's not considered as a bug (I get that), this is still a regression from version 2.0 in my opinion.

[CharliePoole]

I guess our original "support" was an unintended side-effect of supporting async void. Actually a good side effect in this case. :-)

I upped the priority.

[joypeterson]

This change in behavior from 2.0 is also blocking my upgrade to 3.0 as I also have a lot of tests using Assert.Throws<MyException>(async () => await SomethingAsync());

[appel1]

Using this:

private Task SomethingAsync()
{
  return Task.Factory.StartNew(
    () => { throw new InvalidOperationException("Failed"); }, 
    CancellationToken.None, 
    TaskCreationOptions.None, 
    TaskScheduler.Default);
}

Assert.That works:

Assert.That(() => SomethingAsync(), Throws.TypeOf<InvalidOperationException>());

Assert.Throws doesn't:

Assert.Throws<InvalidOperationException>(() => SomethingAsync());

Result Message: 
Expected: <System.InvalidOperationException>
  But was:  null

[PaulARoy]

Pretty nice workaround! Thanks!

[thomaslevesque]

Pretty nice workaround! Thanks!

Yes, but still a workaround, not a fix...

There should be a version of Assert.Throws that accepts a Func<Task> (or let's call it AsyncTestDelegate to match the existing TestDelegate). Actually, I don't think we can name it Throws, because there would be an ambiguity when passing a method group (e.g. when writing Assert.Throws(Foo) where Foo is a method), so perhaps it should be called ThrowsAsync.

[CharliePoole]

@StephenCleary I can't agree with you there: ThrowsAsync is not async. All the asynchronicity takes place inside it and the test writer can't await it.

If we could figure out a way to make Throws work for both cases without conflict, I would favor that.

[thomaslevesque]

@StephenCleary I can't agree with you there: ThrowsAsync is not async. All the asynchronicity takes place inside it and the test writer can't await it.

The problem is that if ThrowsAsync is not async, then it has to wait synchronously for the async delegate to complete ("sync over async"). This is usually considered bad practice in application code, because it can lead to deadlocks. Or course, in unit tests it's different because we don't have a SynchronizationContext, so it won't deadlock, but I don't think it's a good idea to encourage people to do something in test code that they shouldn't do in application code.

On the other hand, I just realized that synchronously waiting for the task to complete is exactly what AsyncInvocationRegion is doing for async test methods (basically, it just calls Task.Wait()).

(in my PR, ThrowsAsync returns a task, and should be awaited)

If we could figure out a way to make Throws work for both cases without conflict, I would favor that.

As far as I can tell, it's not possible; if the new overload is named Throws as well, it will work with lambda expressions, but not with method groups. In the example I gave earlier, Assert.Throws<Exception>(Foo) wouldn't compile, but Assert.Throws<Exception>(() => Foo()) would work. This would almost certainly break existing code.

(interestingly, Assert.Throws<Exception>(FooAsync) would compile, because overload resolution picks the async signature over the synchronous one)

[StephenCleary]

in unit tests it's different because we don't have a SynchronizationContext, so it won't deadlock

I have in many situations written unit tests with a SyncCtx. The AsyncContext in my AsyncEx library was originally written with unit tests in mind.

Having code resume in the same thread is sometimes necessary. I commonly use SyncCtx in unit tests when testing ViewModels. I also have some code that assumes it will execute in a synchronized fashion (i.e., no more than one continuation at a time), which is the case if run in a UI or ASP.NET context, but (obviously) not in a thread pool context.

On a side note, xUnit has made the interesting choice to always run their unit tests within a single-threaded SyncCtx. Previous (prerelease) versions of NUnit v3 would attempt to detect if a SyncCtx would be helpful (i.e., the async Throws support) and automatically inject one if necessary. I objected to this design because it can be confusing - part of the test would run without a SyncCtx and part with one.

[CharliePoole]

@thomaslevesque I'm afraid that ship has sailed. :-) NUnit's entire approach to async tests is "sync over async" - the test methods may be async but NUnit has to wait for them to complete in order to know whether they passed or failed. That blocks the thread on which the test is running, but other tests continue to run on other threads so it's usually not a problem, especially as async tests are usually not needed.

In this case, we are one level lower dealing with Asserts but the same is true of them: NUnit has to know whether they failed or passed.

[StephenCleary]

NUnit's entire approach to async tests is "sync over async" - the test methods may be async but NUnit has to wait for them to complete in order to know whether they passed or failed.

I sort of agree. It's abstracted away enough from the unit test writer that they don't see the sync-over-async behavior. Right now NUnit blocks one thread per async unit test, but it's entirely conceivable that a future version may asynchronously wait for it to complete (more similar to how the ASP.NET framework works), and only have a single sync-over-async point for all async unit tests in a test run.

NUnit has to know whether they failed or passed.

Yes, but this can be done in either a sync-over-async way, or an async-all-the-way way. The current PR is taking the async-all-the-way way, which I personally prefer but there are arguments to be made both ways. Both synchronous and asynchronous patterns are equally capable of propagating exceptions.

[thomaslevesque]

In this case, we are one level lower dealing with Asserts but the same is true of them: NUnit has to know whether they failed or passed.

Of course, but it can wait asynchronously for the delegate to complete...

Anyway, you're the boss @CharliePoole, so I'll do as you say 😉
I'll update my PR to make it "sync over async", which will probably make the code simpler.

Does the name ThrowsAsync still make sense? It's not great because the method can't be awaited, but it has to have a different name... or be in a different class. Perhaps we could do something like Assert.Async.Throws(...) ?

Both synchronous and asynchronous patterns are equally capable of propagating exceptions.

True, but it's more awkward with the synchronous pattern, because we need to unwrap the AggregateException and return the inner exception

[CharliePoole]

@thomaslevesque I'll have to get back to this and do more than an email review - i.e. run the code. Also, I'd like to hear from other committers. @rprouse ? @simoneb ?

To put it clearly, the implementation can be either synchronous or asynchronous. It has to be synchronous from the user point of view in order to be consistent with other choices in NUnit.

[appel1]

Perhaps a bit too verbose but you could do something similar to the signature of Assert.That.

public static void Throws<TException, TResult>(Func<TResult> testMethod) { ... }

I'm a bit undecided on whether it is a good idea to return the task since that might give the wrong impression that you have to wait on it.

[thomaslevesque]

@appel1, this would work, but then you'd have to specify the return type (Task) as well as the exception type, even though you only care about the exception.

I'm a bit undecided on whether it is a good idea to return the task since that might give the wrong impression that you have to wait on it.

Well, you can't return the task anyway, since Throws already returns the exception.

[gerektoolhy]

14 tests fail out of 27 after upgrade from 2.6.4 to 3.01, all of those 14 tests fail with this:

System.ArgumentException : 'async void' methods are not supported, please use 'async Task' instead
   at NUnit.Framework.Internal.AsyncInvocationRegion.Create(MethodInfo method)
   at NUnit.Framework.Assert.Throws(IResolveConstraint expression, TestDelegate code, String message, Object[] args)
   at NUnit.Framework.Assert.Throws(IResolveConstraint expression, TestDelegate code)

Downgrading to 2.6.4.

[PaulARoy]

Do not downgrade just for this, you just have to change the signature of your test methods from async void to async Task.

This problem (which is an evolution) is not directly related to this issue.

EDIT: ignore this message I was completely mistaken on the issue

[thomaslevesque]

@PaulARoy, I think @dariusdamalakas is using Assert.Throws, so it's not a matter of switching from async void to async Task. Basically he needs the Assert.ThrowsAsync added in my PR.

@rprouse @CharliePoole, did you have time to review my PR?

[gerektoolhy]

@thomaslevesque , correct, we are using Assert.Throws to check that correct exceptions are being thrown from async methods. Here's an example of failing test.

@PaulARoy, if there's something wrong with the tests or a way to rewrite them, let me know. But it looks like this is breaking change between v2.6 and v3.

[PaulARoy]

My bad I was wrong on my message. Sorry for that.

A solution that works:

[Test]
public void MyTest()
{
    Assert.That(() => myobject.myMethodAsync(), Throws.TypeOf<MyException>());
}

Applied to your test:

 Assert.That(() => underTest.ValidateAsync(request), Throws.TypeOf<SecurityTokenExpiredException>()
                .And.Message.Contains("NotOnOrAfter is missing"));

It might not be perfect but it should work

This is indeed a breaking change.

[gerektoolhy]

@PaulARoy , hm, interesting. This does work, and this is the way i've ve used to write tests before figured out there is Assert.Throws method. Will keep this in mind.

I'll leave my project pointing to 2.6.4 for the moment, will upgrade to v3 when I have time.

Thanks.

[PaulARoy]

No problem!
This is just an alternative, I was also using Assert.Throws. I had the same issue so I am always glad to help :)

But indeed, Assert.Throws with async support would be better. I prefer this approach.

[huysentruitw]

Any idea when this fix is available on NuGet?

[rprouse]

This will be in the 3.2 release which we hope to get out within the month. We are a bit behind schedule on the release because @CharliePoole and I have been busy with other projects recently.