Assert.Ignore breaks when a Task is returned w/o using async/await

[SeanFeldman]

Writing a test that invokes Assert.Ignore() and it fails with the following exception:

Stacktrace

NUnit.Framework.Internal.NUnitException : Rethrown
  ----> NUnit.Framework.IgnoreException : should not run
   at NUnit.Framework.Internal.Reflect.InvokeMethod(MethodInfo method, Object fixture, Object[] args) in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\Reflect.cs:line 281
   at NUnit.Framework.Internal.AsyncToSyncAdapter.Await(Func`1 invoke) in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\AsyncToSyncAdapter.cs:line 77
   at NUnit.Framework.Internal.Commands.TestMethodCommand.RunTestMethod(TestExecutionContext context) in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\Commands\TestMethodCommand.cs:line 84
--IgnoreException
   at NUnit.Framework.Assert.Ignore(String message, Object[] args) in C:\src\nunit\nunit\src\NUnitFramework\framework\Assert.cs:line 214
   at TestNUnit.Class1.Failing() in C:\Users\Sean\Desktop\TestNUnit\TestNUnit\Class1.cs:line 16

The failing test looks as the following:

        [Test]
        public Task Failing()
        {
            Assert.Ignore("should not run");

            return Task.FromResult(1);
        }

Changing the test to include the async keyword with await fixes this test:

        [Test]
        public async Task Working()
        {
            Assert.Ignore("should not run");

            await Task.FromResult(1);
        }

This fails with NUnit 3.11.0, but works with earlier versions.
Tested with .NET Framework 4.5.2

[CharliePoole]

Added a fence to display your stacktrace more cleanly

[rprouse]

Can you point us to the tests that were failing to give us a real world example? Test that return a Task but not being async isn't really supported. It may have worked, or maybe it only appeared to work in earlier versions of NUnit and test failures were masked? @jnm2 you made the recent async changes, any comments?

[SeanFeldman]

We have a set of tests (Acceptance tests) that are executed to ensure there's no regression. Usually those tests are quite sophisticated and would have an await. In some cases, when the test is simple, we'll return the Task rather than await it. The issue we've ran into is that the tests are on the support branches and cannot be modified. The tooling (nunit) can be and that's how the issue was discovered.

Real world examples below that has started failing with 3.11.0:

• Example 1
• Example 2

[rprouse]

@SeanFeldman you are not updating the support branches but you are updating the version of NUnit on the support branches?

I would argue that the behavior in the tests you linked to are using undocumented behavior that we fixed. Our documentation isn't very explicit with async tests which I think we should address, but the only examples I've seen all await the results and are all marked async, https://github.com/nunit/docs/wiki/Test-Attribute

[SeanFeldman]

Support branches are updated. Package containing test source code cannot be bumped up.

From the linked documentation:

Test methods targeting .Net 4.0 or higher may be marked as async and NUnit will wait for the method to complete before recording the result and moving on to the next test.

There's absolutely nothing in the documentation that would rule out using Task. In fact, from an API point of view, Task and async Task is the same.

[CharliePoole]

The highlighting in your citation of the docs is yours and, arguably, changes the meaning of the sentence. To my knowledge, there has never been any intention of allowing a non-async test method to return Task, although it's possible that changes have occured over time.

It would be very interesting to try out this code on various versions of the framework. In particular, I'm surprised that the test is not marked as "not runnable" due to the non-async Task return. Is there a reason you want to return a task? NUnit has not been told what to do with the return value, which is one reason why I would expect the signature to be invalid.

[SeanFeldman]

Is there a reason you want to return a task?

Brevity. From C# perspective returning a Task or having a method marked as async and awaiting for the Task is the same.

[jnm2]

@SeanFeldman

There's absolutely nothing in the documentation that would rule out using Task. In fact, from an API point of view, Task and async Task is the **same.

You're right that NUnit shouldn't care if the method is Task or async Task because async is purely an implementation detail. However, your two examples are observably different from an API point of view. This is fine:

public async Task ReturnsTask()
{
    throw new SomeException();
}

var task = ReturnsTask();
await task; // Can be combined with Task.WhenAll, etc, and only throws when awaited

But this is frowned upon, at least in shipping code:

public Task ThrowsBeforeReturningTask()
{
    throw new SomeException();
}

var task = ThrowsBeforeReturningTask(); // Throws in an unexpected location
await task;

To avoid the async state machine, the proper implementation would be:

public Task ReturnsTask()
{
    return Task.FromException(new SomeException());
}

Indeed, it's frowned upon if you indirectly cause an exception (say, ObjectDisposedException) and don't catch and wrap in Task.FromException or use async. (The same applies to ValueTask.) More on this: https://stackoverflow.com/a/25018946

Now, I think there are good reasons that you shouldn't have to care when writing test code.

1. The fact that Assert.Ignore() throws an exception might be considered by the @nunit/framework-team to be an NUnit implementation detail the same way it is for Assert.Fail(). If that's the case, the last thing we'd want is for you to couple your tests to the assumption that it does throw an exception.

2. The only caller of your test method is NUnit. There's no reason to make you write extra code like there would be if the method was part of your shipped code.

Therefore, my view is that this is a regression of an undocumented but useful feature.

[jnm2]

And by the way, async is usually the best option because the debugger experience is a lot better and because it's a good tradeoff to gain simplicity. It saves typing and cognitive overhead. The only time the simplicity is not worth the slight performance tradeoff is in a measured hot path.

The only time I wouldn't use async is when there is no await within the method and the compiler generates a warning (or error if you have /warnaserror):

CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls

And I'm not comfortable proposing that everyone should ignore that warning or that everyone should add <NoWarn>CS1998</NoWarn> to their csproj. The path of least resistance is to make the method non-async and therefore I think it's the best tradeoff when there is no await within the method.

[jnm2]

I can see by reading it that #3095 doesn't affect this scenario. I'll try to get a fix through for this first.

[jnm2]

@SeanFeldman If you'd like to try this out before 3.12.0 is released, you can merge this with your nuget.config and look for version 3.12.0-dev-06133 or greater:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <add key="NUnit MyGet" value="https://www.myget.org/F/nunit/api/v3/index.json" />
  </packageSources>
</configuration>

[SeanFeldman]

@jnm2 I'll give it a try and report back. Thank you.

[SeanFeldman]

Sorry for delay. Going to test it today/tomorrow and let know.

[SeanFeldman]

@jnm2 confirmed. 3.12.x fixes the problem. Thank you 🙂

[jnm2]

Thanks for the confirmation!

[SeanFeldman]

@jnm2 how far 3.12.x from being finalized?

[jnm2]

We are behind schedule, but I'm not sure if there's any reason not to release soon. @rprouse, what are you thinking here?

[rprouse]

@jnm2 I would like to get the release out this month if possible. I'd ask the @nunit/framework-team to move any outstanding issues or pull requests to the milestone that we should include in the release so that we can track them.

[jnm2]

@rprouse I threw in #3145 because the PR is nearly merged, but nothing is particularly on my radar. The release date also came up at https://gitter.im/nunit/nunit?at=5c8f99679d9cc8114ade435d.

[SeanFeldman]

Hi folks, any news on 3.12 coming out? Thanks.

[jnm2]

@rprouse Can I help?

[rprouse]

@SeanFeldman thanks for prompting, I'll try to get the release out soon.

@jnm2 I've gone through the issues in the milestone and cleaned them up. Nearly everything will be resolved once I review your PR #3095 except for #3085. That looks somewhat serious, but do you think it is enough to hold up a release for? Also, is there anything else out there that should be in the release?

[jnm2]

@rprouse Nothing is on my radar except for #3085 now. I agree, we should fix it. It's as simple as changing this line to : assemblyPath; and adding a test, right?

nunit/src/NUnitFramework/framework/Api/DefaultTestAssemblyBuilder.cs

Line 88 in 9ff47eb

: Path.GetFileName(assemblyPath);

I'll try to make time this evening if no one else gets there first.

[mikkelbu]

@rprouse, @jnm2 Perhaps #3036 should be in the milestone (although I've only skimmed the comments).

[jnm2]

#3216 is up to fix #3085.

@mikkelbu I agree, that one would be really good to get in now that the WinForms and WPF crowd (myself included) are going to be moving to .NET Core. What happened there, anyway? I totally failed to see the mentions. 🤦‍♂️ Looks like it's ready to merge, so I'll get it done.

[jnm2]

@rprouse Okay, #3036 and #3085 are both fixed in master. I threw in the issues for #3214 and #3098 because they just need a second review and they look low-risk, but feel free to pull them out if you see something.

There's a chance I'll want to get #3205 and #3206 in too, but I haven't opened either one of them yet. It's late and I'll have time tomorrow.

[jnm2]

@rprouse #3205 and #3206 seem like they'll need more time.
Anything else before release?