Support custom awaitables

[jnm2]

Closes #2286, #3023, #3093.
Fixes #2168 and properly fixes #3222.

New features

This enables test and setup methods (and any other methods in user code which NUnit directly invokes) to return objects other than Task which the C# language knows how to await, according to the awaitable expressions section of the C# 5 spec which has not been changed as of 7.3.

It does not matter whether the async keyword is used, so both of these will work:

[Test]
public async ValueTask Test1()
{
    await Task.Yield();
}

[Test]
public ValueTask Test2() => Test1();

Awaitable objects (consumable via the await keyword since C# 5) go beyond tasklike objects (produceable via the async keyword since C# 7). It doesn't matter whether you can use a type as an async method builder return type; it only matters whether you can await it:

[Test]
public System.Runtime.CompilerServices.YieldAwaitable Test3() => Task.Yield();

Behavioral changes

Assert.That now treats all delegates that return an awaitable type the same way it has treated delegates returning Task, by waiting for the result:

Assert.That(() => foo.ValueTaskReturningMethod(), Throws.InstanceOf<FooException>());

No new APIs

This PR does not add new APIs. Therefore, Assert.ThrowsAsync still requires our AsyncTestDelegate type to be passed which requires the delegate to return Task.

If we wanted to support this, we would have to add an Assert.ThrowsAsync(Func<object>) overload. This is as specific as you can be in C# while still allowing custom awaitable return types. We would have to error at runtime (and via analyzer) if the returned value was not in fact awaitable.

Folks using ValueTask have reasonably easy ways to handle this already which work in all versions of NUnit:

[Test]
public void Test4() => Assert.ThrowsAsync(() => foo.ValueTaskReturningMethod().AsTask());

[Test]
public void Test5() => Assert.ThrowsAsync(async () => await foo.ValueTaskReturningMethod());

As well as constraint syntax as of NUnit 3.12, as mentioned above:

[Test]
public void Test7() => Assert.That(foo.ValueTaskReturningMethod, Throws.Exception);

Codebase changes

Because awaitability is pattern-based rather than type-based, I was able to delete almost all occurrences of #if ASYNC (there are only five places left in the framework). Async behavior is now automatically effective in all builds, including net20 and net35.

The remaining occurrences have less to do with async behavior and more to do with whether the .NET 4.0 Task Parallel Library framework types are available. For example, whether we define AsyncTestDelegate which has a return value of Task.
In an earlier discussion we decided on changing ASYNC to TASK_PARALLEL_LIBRARY_API.

[jnm2]

@mikkelbu and @rprouse, would you both be willing to review if you can find the time?

[jnm2]

Travis is showing this failure on the four Mono test runs:

Cancelled : NUnit.Framework.Constraints.DelayedConstraintTests.ThatBlockingDelegateWhichFailsWithoutPolling_FailsAfterDelay
Test cancelled by user
at NUnit.Framework.Internal.Reflect.InvokeMethod

The timeout is 100ms. If this PR pushes it over the threshold, I wonder if we should do any performance testing to compare before and after.

[mikkelbu]

@jnm2 I can take a look at it in the beginning of next week, but I'm not strong in async + await, so I'll take it as a "learning session" 😄.

[jnm2]

@mikkelbu I'm sorry, I was thinking you would be interested in this one, but now I'm not sure what made me think that. I'd covet a quick review but don't trouble yourself too much, please!

[jnm2]

Other DelayedConstraintTests tests are causing continual failures on master with Azure DevOps macOS builds. It's not necessarily unique to this PR.

[jnm2]

Rebased to consume a commit from #3096 to fix a problem with master.

[rprouse]

I've spent about an hour tonight going through this and didn't see anything obvious, but without reading through the specs you linked to and going through the same thought processes as you did working on this code, I'm not sure how much more I can add. I like how your changes clean up the code in so many places and make it much clearer. The number of failing tests worries me though as does the fact that we don't dogfood async/await in NUnit tests.

This is a large and deep change. I'd like to see some additional integration tests of some kind so that we can move forward with it with more confidence. It doesn't need to be part of this solution or repo, I'm just looking for something to prove the code in a real world test suite. It could be something like my platform tests that I used to make sure that NUnit was working on every .NET Core platform when we were making those changes.

[rprouse]

Approving but with some suggestions. There is a lot of new code that is only indirectly tested, for example the new reflection extensions. It would be nice to see some lower level unit tests for some of this code where possible. Other than that, this looks good. It took a long time to review, so it must have taken a very long time to write! Sorry that I didn't finish the review for so long...

[rprouse]

I also kicked off a new Travis build which failed again and the Azure DevOps build no longer exists. You might want to merge latest from master to get the various new build changes?

[jnm2]

Thanks Rob! I'll rebase and add some more direct tests when I get a chance. The rest of my day is pretty full but I will take opportunities to work on it as they come and tomorrow evening is mostly free.

[jnm2]

@rprouse I rebased and then added three commits.

[jnm2]

Not related probably, but debugging the NUnit Console master branch against this PR branch in MonoDevelop (on Mono 5.18.1) shows that _currentWorkItem is null on this line:

nunit/src/NUnitFramework/framework/Internal/Execution/TestWorker.cs

Lines 135 to 137 in 8a6245b

	// Because we execute the current item AFTER the queue state
	// is saved, its children end up in the new queue set.
	_currentWorkItem.Execute();

This can't be good, but I thought we had fixed that. I did check; I'm not on an out-of-date branch.

[jnm2]

Also, it's difficult to break on exceptions on Mono because the framework is constantly triggering System.Runtime.Remoting.RemotingException "Cannot resolve method NUnit.Engine.RunTestsCallbackHandler:RaiseCallbackEvent" on this line:

nunit/src/NUnitFramework/framework/Internal/TestProgressReporter.cs

Line 94 in 8a6245b

handler.RaiseCallbackEvent(node.OuterXml);

Both the framework and the console repos are up to date and compiled in debug mode.

[jnm2]

"Cannot resolve method NUnit.Engine.RunTestsCallbackHandler:RaiseCallbackEvent" stops happening if I check out the v3.10 tag from nunit-console, so something problematic has been introduced between 3.10 and master. I think by sticking with 3.10 I can leave that problem for some other time.

[jnm2]

"Test cancelled by user" reproduces consistently from the command line but not when MonoDevelop is attached. Debugger.Launch() throws NotImplementedException.

[jnm2]

The repro is preserved if you delete all tests except for this one:

nunit/src/NUnitFramework/tests/Attributes/TimeoutTests.cs

Lines 129 to 138 in 5723898

	[Test]
	public void TestTimeoutElapsed()
	{
	TimeoutTestCaseFixture fixture = new TimeoutTestCaseFixture();
	TestSuite suite = TestBuilder.MakeFixture(fixture);
	TestMethod testMethod = (TestMethod)TestFinder.Find("TestTimeOutElapsed", suite, false);
	ITestResult result = TestBuilder.RunTest(testMethod, fixture);
	Assert.That(result.ResultState, Is.EqualTo(ResultState.Failure));
	Assert.That(result.Message, Does.Contain("100ms"));
	}

And any one of these:

nunit/src/NUnitFramework/tests/Constraints/DelayedConstraintTests.cs

Lines 183 to 245 in 5723898

	[Test]
	public void ThatBlockingDelegateWhichFailsWithoutPolling_FailsAfterDelay()
	{
	var watch = new Stopwatch();
	watch.Start();
	Assert.Throws<AssertionException>(() => Assert.That(() =>
	{
	Delay(DELAY);
	return false;
	}, Is.True.After(AFTER)));
	watch.Stop();
	Assert.That(watch.ElapsedMilliseconds, Is.GreaterThanOrEqualTo(MIN));
	}
	[Test]
	public void ThatBlockingDelegateWhichFailsWithPolling_FailsAfterDelay()
	{
	var watch = new Stopwatch();
	watch.Start();
	Assert.Throws<AssertionException>(() => Assert.That(() =>
	{
	Delay(DELAY);
	return false;
	}, Is.True.After(AFTER, POLLING)));
	watch.Stop();
	Assert.That(watch.ElapsedMilliseconds, Is.GreaterThanOrEqualTo(MIN));
	}
	[Test]
	public void ThatBlockingDelegateWhichThrowsWithoutPolling_FailsAfterDelay()
	{
	var watch = new Stopwatch();
	watch.Start();
	Assert.Throws<AssertionException>(() => Assert.That(() =>
	{
	Delay(DELAY);
	throw new InvalidOperationException();
	}, Is.True.After(AFTER)));
	watch.Stop();
	Assert.That(watch.ElapsedMilliseconds, Is.GreaterThanOrEqualTo(MIN));
	}
	[Test]
	public void ThatBlockingDelegateWhichThrowsWithPolling_FailsAfterDelay()
	{
	var watch = new Stopwatch();
	watch.Start();
	Assert.Throws<AssertionException>(() => Assert.That(() =>
	{
	Delay(DELAY);
	throw new InvalidOperationException();
	}, Is.True.After(AFTER, POLLING)));
	watch.Stop();
	Assert.That(watch.ElapsedMilliseconds, Is.GreaterThanOrEqualTo(AFTER));
	}

[jnm2]

Got the repro down to this (the only two tests in my nunit.framework.tests project):

[Test]
public void A()
{
    TimeoutTestCaseFixture fixture = new TimeoutTestCaseFixture();
    TestSuite suite = TestBuilder.MakeFixture(fixture);
    TestMethod testMethod = (TestMethod)TestFinder.Find("TestTimeOutElapsed", suite, false);
    TestBuilder.RunTest(testMethod, fixture);
}

[Test]
public void B()
{
    Assert.Throws<AssertionException>(() => Assert.Fail());
}

A ThreadAbortException is being handled for B.

[jnm2]

And with the above code, the debugger can repro.

[jnm2]

It looks like the problem is that Mono postpones the ThreadAbortException for a while if you catch it and neither rethrow nor call ResetAbort:

nunit/src/NUnitFramework/framework/Internal/Reflect.cs

Lines 272 to 278 in 5723898

	#if THREAD_ABORT
	catch (System.Threading.ThreadAbortException)
	{
	// No need to wrap or rethrow ThreadAbortException
	return null;
	}
	#endif

The ThreadAbortException comes back to life the next time an unrelated exception is thrown and an unrelated catch block ends. The debugger was showing it being resurrected here:

nunit/src/NUnitFramework/framework/Assert.Exceptions.cs

Line 56 in 5723898

}

This seems to have fixed it:

                try
                {
                    return method.Invoke(fixture, args);
                }
                catch (TargetInvocationException e)
                {
                    throw new NUnitException("Rethrown", e.InnerException);
                }
                catch (Exception e)
#if THREAD_ABORT
                    when (!(e is System.Threading.ThreadAbortException))
#endif
                {
                    throw new NUnitException("Rethrown", e);
                }

throw; also fixes it.

[jnm2]

Guess what? This also fixes the TimeoutTests that we've been excluding on Mono for the last ten years! 🎉

@nunit/framework-team Ready for review!

[rprouse]

This was a marathon! Thanks for getting this done, great work.

[jnm2]

Glad to help!