jcansdale / TestDriven.Net-Issues

Issue tracking for TestDriven.Net
https://github.com/jcansdale/TestDriven.Net-Issues/issues
24 stars 2 forks source link

[STAThread] async Task hangs when executed with Test With > In-Proc (VS SDK) #110

Closed jcansdale closed 6 years ago

jcansdale commented 6 years ago

The following currently hangs when executed using Test With > In-Proc (VS SDK).

[STAThread]
async Task DoStuffWithDelay()
{
   await Task.Delay(1000);
}

This is unfortunate because sometimes you want to do stuff on the Main thread between long running async operations.

Related

jnm2 commented 6 years ago

@jcansdale I wouldn't expect hanging unless a WindowsFormsSynchronizationContext or DispatcherSynchronizationContext was installed; I'd just expect the rest of the method to execute on a non-STA thread.

To help me make sure that I properly covered common deadlock situations (https://github.com/nunit/nunit/pull/2776), can you let me know what the type of SynchronizationContext.Current is right before the await? I appreciate it!

jcansdale commented 6 years ago

@jnm2, thanks for dropping by. 😄

This is actually a feature of TestDriven.Net that lets execute arbitrary methods inside the inside the VS process. The target assembly mustn't have any dependencies except for assemblies that are resolvable from inside VS. It therefore isn't using any particular test framework. I'm reusing the [STAThread] attribute to indicate when a target method should be executed on the Main thread.

For example, I can reference EnvDTE and Test With > In-Proc (VS SDK) on this:

        void DumpProjects(DTE dte)
        {
            foreach (Project project in dte.Solution)
            {
                Console.WriteLine(project.Name);
            }
        }

Unfortunately, it turned out the following would deadlock:

[STAThread]
async Task DoStuffWithDelay()
{
   await Task.Delay(1000);
}

This is now fixed by executing the async method using JoinableTaskFactory (which apparently isn't just for use inside VS). https://github.com/Microsoft/vs-threading

I'm now struggling to test this code using NUnit.

I've had some success using Xunit.StaFact (https://github.com/AArnott/Xunit.StaFact) and some code he was using to test JoinableTaskFactory itself.

Here's an example project with a single test called CanSwitchToStaThread. ThreadingTests.zip

It works fine with the [StaFact] attribute, but it fails if I try to use [Test, Apartment(ApartmentState.STA)]. 😭

Test 'ThreadingTests.MyTests.CanSwitchToStaThread' failed: System.InvalidOperationException:
   We can only simulate the UI thread if you're already on it (the starting thread for the test).
    at Microsoft.Verify.Operation(Boolean condition, String message)
    JoinableTaskTestBase.cs(56,0)

I'd be interested to know if there's a way to get it working with your NUnit PR?

If you're curious, here's a recent build of TestDriven.Net: TestDriven.VSPackage.zip

jnm2 commented 6 years ago

Oh, thanks for the explanation! 😃

What's going on at https://github.com/AArnott/Xunit.StaFact/tree/master/src/Xunit.StaFact looks eerily similar to the changes I've just made in NUnit. What happens if you try using https://ci.appveyor.com/api/buildjobs/rgabiufm7klv1lc0/artifacts/NUnit.3.11.0-ci-05551-jnm2-sta-sy.nupkg?

I'm not quite gathering the steps I need to go through to reproduce the issue you're seeing. You're saying the issue is specific to using NUnit to test TestDriven.NET?

jcansdale commented 6 years ago

@jnm2,

You're saying the issue is specific to using NUnit to test TestDriven.NET?

Yes. It seems to work with [StaFact], but not [Test, Apartment(ApartmentState.STA)]. Is that what your PR modifies?

Here is a repro:

    public class MyTests : JoinableTaskTestBase
    {
        //[StaFact]
        [NUnit.Framework.Test]
        [NUnit.Framework.Apartment(ApartmentState.STA)]
        public void CanSwitchToStaThread()
        {
            SimulateUIThread(async () =>
            {
                var jtf = new JoinableTaskFactory(context);
                await jtf.RunAsync(async () =>
                {
                    // Get off the UI thread first so that we can transition (back) to it.
                    await TaskScheduler.Default;
                    Assert.Equal(ApartmentState.MTA, Thread.CurrentThread.GetApartmentState());
                    await jtf.SwitchToMainThreadAsync();
                    Assert.Equal(ApartmentState.STA, Thread.CurrentThread.GetApartmentState());
                });

            });
        }
    }

You can find the example solution here (with the NUnit build you linked in the lib folder): ThreadingTests.zip

When I run with xUnit, I get: image

When I run with the version of NUnit you linked, I get: image

If I run the individual method using TestDriven.Net (without any test attributes), it also passes: image

Any idea why the Environment.CurrentManagedThreadId changing when executed with NUnit but not with the [StaFact] attribute? I'd really like to get this working with NUnit because all my other tests use NUnit.

jnm2 commented 6 years ago

Any idea why the Environment.CurrentManagedThreadId changing when executed with NUnit

Yes, it's because the fixture is constructed in a normal worker thread but the test method is executed on an STA worker thread. NUnit currently uses the same fixture instance for all test cases inside the fixture. I do have an issue somewhere for an option to construct a fixture per test case, but I'm not sure if we would end up executing the constructor on the same thread.

In the meantime, the fix is to apply [Apartment(ApartmentState.STA)] to the fixture class instead of the method. This causes the constructor to run on the STA thread as well as automatically applying to every test in the fixture.

jnm2 commented 6 years ago

In fact, once you make sure the constructor and test method are running on the same thread by doing that, NUnit 3.10.1 passes the test. JoinableTaskFactory.RunAsync already does the deadlock mitigation that I've added for NUnit 3.11, and you're awaiting things that coerce the thread, so my STA fix isn't needed for this sample code either.

jcansdale commented 6 years ago

NUnit currently uses the same fixture instance for all test cases inside the fixture.

Thanks for investigating. I'd forgotten about the difference in fixture lifecycle between NUnit and xUnit. The difference in behavior makes sense now.

JoinableTaskFactory.RunAsync already does the deadlock mitigation that I've added for NUnit 3.11, and you're awaiting things that coerce the thread, so my STA fix isn't needed for this sample code either.

Does your deadlock mitigation install a message pump for the Main thread? Is there any special attribute required or is it simply a case of [Apartment(ApartmentState.STA)]? It would be nice if I could test my code without any special boilerplate. 😄

jnm2 commented 6 years ago

Yes. With the PR DLL you were testing against, NUnit installs a single-threaded message pump before executing STA tests, so you don't need any boilerplate.

Everyone is so strapped for time recently at NUnit, I'm just waiting for someone to reapprove, but I'm confident it's going into 3.11 as-is.

jcansdale commented 6 years ago

This PR turns out to be exactly what I need!

For my tests to run as expected, I need the following to pass:

[Test]
[Apartment(ApartmentState.STA)]
public async Task ApartmentStateAfterYield()
{
    await Task.Yield();
    Assert.That(Thread.CurrentThread.GetApartmentState(), Is.EqualTo(ApartmentState.STA));
}

This passes with your PR, but not the most recent NUnit release.

Everyone is so strapped for time recently at NUnit, I'm just waiting for someone to reapprove, but I'm confident it's going into 3.11 as-is.

I'll mention the above test as a comment on that PR. Thanks for your work on this. 👍

jnm2 commented 6 years ago

Yes, that is almost identical to one of the tests added in that PR. You are precisely who this PR is for! 😃 No problem, thanks for commenting!