Consider running on the UI thread by default

[sharwell]

Currently the library starts tests on background threads by default. The behavior can be changed for an assembly by the following:

[assembly: VsTestSettings(UIThread = true)]

I would propose running tests on the UI thread by default instead, using one of the following:

1. Change the default for UIThread to true, or
2. Remove the UIThread property, and force the value was true

Overall, I believe this change better represents the requirements of code users are planning to execute as integration tests.

• Many VS APIs require are affinitized to the main thread, so they can be used directly in tests
• Integration tests typically cannot run concurrently due to state collisions within the IDE environment, so affinitizing them to the UI thread is not constraining
• While xunit in general is used for unit testing which does not require a main thread context, integration tests typically run in a more restricted environment. I would argue that it's acceptable to use a default which differs from other xunit frameworks because the host environment itself is "special".

This change would impact users in the following ways:

1. The IAsyncLifetime interface can be used to implement asynchronous initialization and cleanup code, since the UI thread will be blocked for the execution of constructors and Dispose
2. Integration test methods should be asynchronous if they need to avoid blocking the UI thread
3. Integration tests which need to run on the background thread can use await TaskScheduler.Default to switch to a background thread

[jcansdale]

My main concern with making, UIThread = true the default is that it could hide potential issues when code is executed on a background thread.

There is now a requirement to use:

    [PackageRegistration(UseManagedResourcesOnly = true, AllowsBackgroundLoading = true)]
    [ProvideAutoLoad(Constants.vsContextNoSolution, PackageAutoLoadFlags.BackgroundLoad)]

This means that initialization is sometimes done the UI thread (when package load is triggered by a command) and sometimes on a background thread (when package load is triggered by a context). I've had plenty of situations where my my extension has sometimes worked and sometimes not worked depending on initialization order.

For this reason, I'd maybe prefer the default to be the situation that is less likely to work (running on a background thread). This would force me to be explicit when a test or some library code requires the UI thread.

Perhaps the most common scenario would be testing the creation of MEF components on a background thread (and initializing asynchronously where they're do any initialization that requires the UI thread).

I'd prefer being able to do the following:

[VsFact]
public void MyComponent()
{
   var componentModel = (IComponentModel)(await GetServiceAsync(typeof(SComponentModel)));
   var exports = componentModel.DefaultExportProvider;
   var myComponent = exports.GetExportedValue<MyComponent>();
   await myComponent.InitializeAsync();
}

...without having to remember to do await TaskScheduler.Default.

Does that make sense?

[sharwell]

@jcansdale If the UIThread being false could guarantee that code in your scenario worked, I would certainly be understanding. However, the integration test library does not reset the package initialization state between tests by default, so you already have to "jump through hoops" to write a test for these situations. Starting the test on the UI thread would have little to no impact on the scenario.

(The hoops required is the test must execute in a fresh VS instance to guarantee that the type is not already initialized. For a test with a focus on specific initialization conditions, explicitly stating the thread affinity is a valuable clarification regardless of what the default is.)

[jcansdale]

@sharwell,

It was a bit of a rushed example. You're right it wouldn't work consistently in its current form. With ReuseInstance = false it would be terribly inefficient.

Here is a more realistic example:

        [VsFact(UIThread = false)]
        public async Task TestMyComponentAsync()
        {
            var componentModel = (IComponentModel)await AsyncServiceProvider.GlobalProvider.GetServiceAsync(typeof(SComponentModel));
            var catalog = new TypeCatalog(typeof(MyService));
            var container = new CompositionContainer(catalog, componentModel.DefaultExportProvider);

            var service = container.GetExportedValue<MyService>();
            await service.InitializeAsync();

            Assert.NotNull(service);
        }

Alternatively you could do:

        [VsFact(UIThread = true)]
        public async Task TestMyComponentAsync()
        {
            var componentModel = (IComponentModel)await AsyncServiceProvider.GlobalProvider.GetServiceAsync(typeof(SComponentModel));
            var catalog = new TypeCatalog(typeof(MyService));
            var container = new CompositionContainer(catalog, componentModel.DefaultExportProvider);

            await TaskScheduler.Default;
            var service = container.GetExportedValue<MyService>();
            await service.InitializeAsync();

            Assert.NotNull(service);
        }

I do find await TaskScheduler.Default to be a little obscure and hard to remember (IntelliSense / Ctrl+. doesn't help me find it 😉).

Unfortunately the first example doesn't work on Visual Studio 2017 because AsyncServiceProvider.GlobalProvider appears to have the same issue as ServiceProvider.GlobalProvider.

Whether the default is UIThread = true or UIThread = false, I think it would be best if both of these tests worked as expected. Currently tracking down the issue and coming up with a workaround is a bit convoluted. I expect other developers would soon bump into it as well.

Re: what the default should be , I'm leaning slightly towards UIThread = false. While I agree this might be the less common scenario, I think having a default of UIThread = true is more likely to hide potential issues. It will make developers less likely to test their code being initialized on a background thread.

Here's an example PR that I would have liked to test like this, but we tested using some rather convoluted manual testing: https://github.com/github/VisualStudio/pull/1689