Exception on test cleanup

[ilengyel]

Fix the following exception on test cleanup: InvalidComObjectException - COM object that has been separated from its underlying RCW cannot be used.

[bradphelan]

Looks good. Was it necessary to submit the other "StyleCop" package upgrades at the same time? @AArnott might be more inclined to accept if there are only changes with respects to the issue at hand. Otherwise I hope it gets merged ASAP as I have exactly the same problem.

[ilengyel]

Hi @bradphelan , The stylecop changes are maybe not 100% necessary but seems benign, but I did need to change the unit test project packages, as it was not working with the VS2015 installation I have. And the change I did there was to bring it inline with the other projects.

One good thing about this project is that the CI build will produce the nuget packages as artifacts https://ci.appveyor.com/project/AArnott/xunit-stafact/build/1.0.37/artifacts so you can publish them on your local staging nuget server or directory like we did.

I have not heard from @AArnott so he may be on a long vacation, or not receiving notifications..

[AArnott]

Thanks for your contribution. I was going to go along with the package updates (although as @bradphelan suggested, I would have liked them to be separated at least by commit if not by PR), but the revert of one of the projects from project.json to packages.config gives me pause because that's going backwards. I'd rather migrate all the projects forward to project.json to be consistent, or just leave it inconsistently alone for now till we have time to fix it.
@ilengyel do you have time to remove the package format changes from the meat of what you're fixing here? If not, I'll try to find time over the next day or so.

[ilengyel]

Ah, I did not realize the package.json was the newer format.. I suppose I would need to update the nuget addin in my visual studio to support that.. I will try to do the changes with the online editor.. otherwise would have to be tomorrow afternoon.

[ilengyel]

Hi @AArnott , It looks like I am losing the battle with project.json format... I am almost convinced that AppVeyor may have dropped support for it.. Any chance to run a build on the master branch? This would confirm that point.

I have cut down this pull request to the bare minimum, so should you clean up the project structure, there would be less chance of conflicts.

Regards,

[ilengyel]

@AArnott also FYI, according to http://blog.nuget.org/20161121/introducing-nuget4.0.html the uwp-style project.json looks like will be deprecated in the future too.

[bradphelan]

I just realized that this code is probably wrong depending on which thread it gets run on. InvokeShutdown is async so depending on how fast XUnit tears everything down and how fast the dispatcher event queue can be flushed you might still see the exception.

One way to synchronise is to use a TaskCompletionSource

        var tc = new TaskCompletionSource<bool>();
        var h = (EventHandler)((sender,e) => tc.SetResult(true));
        Dispatcher.CurrentDispatcher.ShutdownFinished += h;
        Dispatcher.CurrentDispatcher.InvokeShutdown();
        tc.Task.Wait();
        Dispatcher.CurrentDispatcher.ShutdownFinished -= h;

It's ugly and I think it will deadlock if run on the dispatcher thread and there are items in the dispatcher queue. But some technique for waiting for the dispatcher to shutdown is required I think.

[ilengyel]

@bradphelan actually I am not so sure.. I see that all WPF tests start their own thread created in the RunOnSTA method(https://github.com/ilengyel/Xunit.StaFact/blob/4e4f0c35cfce030a08df22a958b95c10c3e0b398/src/Xunit.StaFact.Shared/Sdk/UITestInvoker.cs#L83), and have their own dispatcher.. which I assume is Dispatcher.CurrentDispatcher

[ilengyel]

And from what I see in the the MSDN doc, BeginInvokeShutdown is the async version where as InvokeShutdown is the synchronous version

[bradphelan]

I'm not sure as well. Threading is hard. When I see an async method and the calling code assumes the job is done after it returns then I am suspicious :)

…

On Mon, 6 Feb 2017, 17:00 Imre Lengyel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Xunit.StaFact.Desktop/Sdk/DispatcherSynchronizationContextAdapter.cs <#5>: > @@ -41,5 +41,11 @@ internal override void Run(Func<Task> work) this.PumpTill(task); task.GetAwaiter().GetResult(); } + + internal override void Cleanup() + { + Dispatcher.CurrentDispatcher.InvokeShutdown(); @bradphelan <https://github.com/bradphelan> actually I am not so sure.. I see that all WPF tests start their own thread created in the RunOnSTA method( https://github.com/ilengyel/Xunit.StaFact/blob/4e4f0c35cfce030a08df22a958b95c10c3e0b398/src/Xunit.StaFact.Shared/Sdk/UITestInvoker.cs#L83), and have their own dispatcher.. which I assume is Dispatcher.CurrentDispatcher — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABE8pA2Pf4MB356HL2jGrmgRjgfo0b5ks5rZ0ObgaJpZM4LwFc3> .

[bradphelan]

You are right. Internally it calls Invoke which is synchronous. My bad. Sorry

…

On Mon, 6 Feb 2017, 17:22 Imre Lengyel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Xunit.StaFact.Desktop/Sdk/DispatcherSynchronizationContextAdapter.cs <#5>: > @@ -41,5 +41,11 @@ internal override void Run(Func<Task> work) this.PumpTill(task); task.GetAwaiter().GetResult(); } + + internal override void Cleanup() + { + Dispatcher.CurrentDispatcher.InvokeShutdown(); And from what I see in the the MSDN doc, BeginInvokeShutdown <https://msdn.microsoft.com/en-us/library/system.windows.threading.dispatcher.begininvokeshutdown(v=vs.110).aspx> is the async version where as InvokeShutdown <https://msdn.microsoft.com/en-us/library/system.windows.threading.dispatcher.invokeshutdown(v=vs.110).aspx> is the synchronous version — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABE8gxeR4Rpkm3BkaFVQMNrkxmjtOHVks5rZ0iogaJpZM4LwFc3> .

[ilengyel]

Hi @AArnott any verdict to my queries above? Shed some light to this project.json nuget format, or revert to classical mode as per original PR?

[AArnott]

project.json looks like will be deprecated in the future too.

@ilengyel: that blog post you refer to talks about improvements to project.json. Only the comments talk about migrating folks. And yes, it's fine to discuss migrating folks, but that's not the same as deprecation yet. Anyway, until PackageReference is an RTW shipped feature, there's no way I'm moving to it (that's something AppVeyor wouldn't support anyway).

It looks like I am losing the battle with project.json format... I am almost convinced that AppVeyor may have dropped support for it..

No, appveyor can't have dropped support for it. It's a VS2015 feature and this project is building on VS2015 build agents. And I have dozens of projects that use project.json that build successfully on appveyor every day.

[AArnott]

Ah, the issue you're seeing on appveyor is probably that a newer version of nuget stopped supporting the net-cf target framework (if they ever did at all). The fix for that is independent of this one so I'll prepare that change.

[ilengyel]

Hi @AArnott Thanks for fixing the build 😄 .. good to know what was the root cause.

With regard to project.json I was only speculating earlier as the current information on the web regarding the format future seems inconsistent.. eg https://www.stevejgordon.co.uk/project-json-replaced-by-csproj (i know this is an extrapolation). It would have been interesting if csproj was reformatted to project.json for all .net project types. But as long as they do 1-way migrations as a last resort. I very much appreciate to be able to use VS2017rc seamlessly while the rest of the team are still on VS2015.