Convert MSTest to async pattern

[Evangelink]

Description

Currently MSTest is based on sync pattern on all the code and we fake async behavior through .Wait(), .GetAwaiter().GetResult() or Task.Run. It would be better to have MSTest fully async.

Note that on the adapter to Test Platform level, our entry points are not async and it won't be possible to convert these. It's ok as we can consider these methods as our main.

[SimonCropp]

@Evangelink i dont think this is possible to properly fix without changes to ITestExecutor. since we would still need to use .GetAwaiter().GetResult() in MSTestExecutor

[Evangelink]

We can't "fully" fix but my idea is that everything under the test platform interface is done "async". We know that the implementation detail of Test Platform allows us to have a "single thread" so that's ok.

[SimonCropp]

wont that still have the possibility of deadlocks?

[Evangelink]

It will help solve most of them but for sure it won't solve everything because of some implementation choices that were made. I am still ramping up in the knowledge of the codebase and I am often surprised by some things that were done (e.g. using TaskScheduler.Default and expecting to have a different thread for each task while there is no guarantee from runtime - yes most of the time that's quite true but that's not the contract).

[SimonCropp]

forgive my ignorance. but why cant ITestExecutor be changed in a new major? or obsoleted (with warning) and IAsyncTestExecutor added?

[Evangelink]

Technically it's possible but Test Platform is similar to mstest and far away from supporting async/await. The platform is in stabilization phase at the moment so it'd be hard to push for this change there.

[dotMorten]

I'm excited for this. One idea that might not be breaking: Add a new test attribute / interface to use instead. Example:

public class AsyncTestMethodAttribute : TestMethodAttribute, IAsyncTestMethod
{
    public override TestResult[] Execute(ITestMethod testMethod) => throw new NotSupportedException("Call ExecuteAsync");
    public virtual Task<TestResult[]> ExecuteAsync(ITestMethod testMethod);
}
public interface IAsyncTestMethod
{
    Task<TestResult[]> ExecuteAsync(ITestMethod testMethod);
}

The test runner will then just look for IAsyncTestMethod and call ExecuteAsync without the 'wait' call. This allows anyone to switch to it, and also any custom TestMethodAttributes can implement the interface. This could then be achieved in 3.x without breaking.

[Evangelink]

The test runner will then just look for IAsyncTestMethod and call ExecuteAsync without the 'wait' call. This allows anyone to switch to it, and also any custom TestMethodAttributes can implement the interface. This could then be achieved in 3.x without breaking.

@dotMorten thanks for the suggestion! It would definitely help with some part but I feel there might still be some issues as we would need to flow the async/await up to the "entry point" to be sure to have a correct behavior. To do so, we would need to change some public API (honestly I don't know if they are used outside or not) hence the need to wait for v4.

I'll try to make a test to list exactly what would be breaking because maybe we can apply similar pattern in multiple places.

[dotMorten]

having a second thought: It's kind of annoying having to use different attributes depending on whether the testmethod is async or not. We already know that from the signature, and it's probably more changes updating all your tests to use the new attribute, than it is to deal with the breaking changes.

[butameron]

I've noticed the following line causes deadlocks when testing async method that internally calls Dispatcher.Invoke() under DispatcherSynchronizationContext,

https://github.com/microsoft/testfx/blob/9647a997408daa56d43bc83121887ff78d95d832/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs#L164C9-L164C40

I think this is a duplicate issue with this issue so I'm posting it here.

In my opinion, it would be better if it were aware of SynchronizationContext like this (please note this is pseudo code, and I'm not certain it functions correctly):

if(Application.Current.Dispatcher != null && SynchronizationContext.Current is DispatcherSynchronizationContext)
{
    var frame = new DispatcherFrame();
    Application.Current.Dispatcher.BeginInvoke(async()=>
    {
        await (task ?? Task.CompletedTask);
        frame.Continue = false;
    });
    Dispatcher.PushFrame(frame);
}
else
{
    task?.GetAwaiter().GetResult();
}

If this is successful, there would be no need to change the signature of TestMethodAttribute.