[RFC] Add extensibility to the async pipeline to allow custom implementation of waiting strategy

[garuma]

/cc @jnm2

Related to:

• Apply an IWrapSetUpTearDown attribute to all tests in a fixture #2220
• Provide a way to use a custom IWorkItemDispatcher #3013
• Extensibility of MessagePumpStrategy #2917

This PR is an example of extensibility mechanism for the async machinery allowing external people to customize its behavior via attributes.

By leveraging the existing IWrapSetUpTearDown interface, one can now fully execute unit-test inside any sort of mainloop (GUI-based or not).

For instance here is the set of custom attribute created to support running tests with XWT:

// Run wrapper
[System.AttributeUsage (AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
public class XwtCommandWrapperAttribute : Attribute, IWrapSetUpTearDown
{
	public TestCommand Wrap (TestCommand command) => new RunOnUICommand (command);
	class RunOnUICommand : DelegatingTestCommand
	{
		public RunOnUICommand (TestCommand innerCommand)
			: base (innerCommand)
		{
		}

		public override TestResult Execute (TestExecutionContext context)
		{
			TestResult result = null;
			var evt = new ManualResetEventSlim ();
			Xwt.Application.Invoke (() => {
				try {
					result = innerCommand.Execute (context);
				} catch (Exception e) {
					Console.WriteLine ("Catastrophic fail", e);
				}
				evt.Set ();
			});
			evt.Wait ();
			return result;
		}
	}
}

// Custom wait strategy
[System.AttributeUsage (AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
public class XwtAsyncWaitStrategyAttribute : Attribute, IAsyncCompletionStrategy
{
	public void WaitForCompletion (IAwaiterObject awaitable)
	{
		while (!awaitable.IsCompleted)
			Xwt.Application.MainLoop.DispatchPendingEvents ();
	}
}

Additionally, this PR adds a GetCombinedCustomAttributes method to support looking up attribute definitions on both the test method and the containing fixture so that those two attributes above can be applied at the fixture level thus ensuring all the tests automatically inherit the behavior.

(Note that this way of doing things work for our own test suite with minimal changes to the nunit codebase but I don't have any strong feeling about going in that specific direction)

[CharliePoole]

"Test adapter" has a very specific meaning in the NUnit world. It's something outside the framework that adapts the representation of tests in NUnit to some other form. The most obvious example is the VS Test Adapter.
In this context (i.e. within the framework itself) it has no particular meaning for me. Sorry not to suggest an alternative, but I don't know what you are trying to say.

[CharliePoole]

Looking down farther, I see somebody has added an AwaitAdapter in the past... I still don't like the term but I see that you aren't the one who introduced it. 😕

[garuma]

Yep, "adapter" there refers to AwaitAdapter and AsyncToSyncAdapter. I can change to something else if the wording is troublesome.

[jnm2]

Thanks @garuma. This PR is a great proof of concept because it shows that it doesn't take much code at all to get something basic working. I apologize for letting the time slip by. I want to work on this in two phases:

• Allowing user code to take control of test execution (Apply an IWrapSetUpTearDown attribute to all tests in a fixture #2220)
• Enabling sync-over-async strategies for message pumps other than .NET's built-in ones (Extensibility of MessagePumpStrategy #2917)

I'm going to comment in each of those places, for now.

[jnm2]

@garuma More on #2917 (comment). I would like to avoid having this ever be necessary for an NUnit extender to implement:

// Custom wait strategy
[System.AttributeUsage (AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
public class XwtAsyncWaitStrategyAttribute : Attribute, IAsyncCompletionStrategy
{
    public void WaitForCompletion (IAwaiterObject awaitable)
    {
        while (!awaitable.IsCompleted)
            Xwt.Application.MainLoop.DispatchPendingEvents ();
    }
}

Instead, you'd modify your first code sample to do something like this (I want to come up with an API to help make this simpler):

// Run wrapper
[System.AttributeUsage(AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
public sealed class XwtCommandWrapperAttribute : Attribute, IWrapSetUpTearDown
{
    public TestCommand Wrap (TestCommand command) => new RunOnUICommand (command);

    private sealed class RunOnUICommand : DelegatingTestCommand
    {
        public RunOnUICommand(TestCommand innerCommand)
            : base (innerCommand)
        {
        }

        public override TestResult Execute(TestExecutionContext context)
        {
            TestResult result = null;
            
            Xwt.Application.Invoke(() =>
            {
                try
                {
                    result = innerCommand.Execute(context);
                }
                catch (Exception e)
                {
                    Console.WriteLine("Catastrophic fail", e);
                }
                
                Xwt.Application.Exit();
            });
            
            Xwt.Application.Run();
            
            return result;
        }
    }
}

This avoids the anti-pattern of calling DispatchPendingEvents() in a busy loop which I carefully avoided for the Windows Forms and WPF message loops.

Rather, the underlying framework should be using MsgWaitForMultipleObjectsEx or similar so that no CPU is consumed until an event occurs such as an event loop message or a synchronization context post.

[jnm2]

(moved my comment to #2917 (comment))

[OsirisTerje]

@jnm2 What is the state of this now, and the corresponding issues? This is now 5 years old, but it may still have merit, right?

[jnm2]

The problem is that a DoEvents loop doesn't behave the same as an Application.Run() call, with a queued action and queued Exit() following the action. I wanted the API to be a different shape so that people don't go down the DoEvents loop route.

[OsirisTerje]

@jnm2 Do you believe this can be made to work, or should this PR be abandoned ? It is now 5 years old......

[CharliePoole]

This seems like a reason for concern...

This branch is 1 commit ahead of, 1326 commits behind nunit/nunit:master.