ITestLoggerWithParameters | ITestLogger async issue about vstest HOT 5 OPEN

Is there a goal that you are trying to achieve? We cannot redesign the whole way vstest is done, and it was written in a time where async hardly existed as a construct in C#.

from vstest.

I'd like use a lib that I'd like to trigger on messages. But the API surface only supports async methods.

I'm not saying that you should redesign it. Though I would recommend to introduce a new interface which can be used for extensibility that supports async methods. This should apply to Collectors / Adapters / Discoverer / Executer and whatever extensibility point exists within vstest.

For now I would say it would be sufficient to have a Listener upgraded for my case for now... But I would be happy to see that the lookup would look for additional new generation types that are capable to handle async workloads.

From implementation perspective I don't think it would be a huge deal to introduce these types.

from vstest.

Unfortunately that is not so easy. The design where commands (as method calls) flow one way and responses flow as events the other way is common in the whole codebase. For your approach we would have to switch to a message bus or something and then "emulate" the events. I would be up for it, but it would have to be used extensively throughout the whole codebase, and this big refactoring is not something we want to do right now.

from vstest.

I don't think it's something that it's required at least not for the logger scenario.

When getting the implementaion of logger in TestLoggerManager.cs why don't you check for another interface IDisposable() for discovery. If the logger implements it then call it once the testmanager disposes.

The implementation could use that to wait for the internal tasks to be completed (which is not nice) though could be leveraged to add a Task.Wait() on the collected async events that we need to wait for.

I know it's ugly but the implementations do not require synchronization context so deadlock can be avoided.

Or back to my suggestion. Within InternalTestManager you have full control of triggering the events like: LoggerManager.HandleDiscoveredTests(discoveredTestsEvent);
If you introduce a new type like IAsyncTestLogger with a method of:
Task HandleDiscoveredTests(DiscoveredTestsEventArgs e) { return Task.Completed; }
etc.

It can have a default implementation which can be used to wrap the older types to be "backward" compatible like:

/// Partial implementation
public AsyncProxyTestListener : IAsyncTestListener
{
    private readonly ITestListener original;
    
    public AsyncProxyTestListener(ITestListener original)
    {
        this.original = original;
    }

    Task HandleDiscoveredTests(DiscoveredTestsEventArgs e) {
        original.HandleDiscoveredTests(e);
        return Task.Completed; 
    }
}

Then instead of triggering the event call the async method which needs to be waited but it's fine to have LoggerManager.HandleDiscoveredTests(discoveredTestsEvent).ConfigureAwait(false).GetAwaiter().GetResult(); there IMHO.

What do you think?

PS.:
If you don't want to make things sync you can still collect the created tasks and use the DiscoveryRequest _discoveryCompleted manualreset event to only be triggered if the tasks are completed by counting outstanding tasks and on latest task completion trigger the event (or something like that 😆). The same should apply to the TestRunRequest with _runCompletionEvent.

from vstest.

Thanks for the suggestions. We can come up with a lot of workarounds, but I would like to avoid introducing a one off solutions that are not building on a solid foundation. The internal architecture is different and does not mesh well with async workloads.

from vstest.