Bugfixes from Xunit.StaFact nuget package not consumed

[weltkante]

.NET Core Version:
5.0 master

Have you experienced this same bug with .NET Framework?:
not applicable

Problem description:

The nuget package Xunit.StaFact used by WinForms had a bug which prevents execution of tests inside VS:

VS test error message

[xUnit.net 00:00:00.47] System.Windows.Forms.Tests: Catastrophic error during deserialization: System.InvalidOperationException: Could not de-serialize type 'Xunit.Sdk.WinFormsTheoryTestCase' because it lacks a parameterless constructor.
   at Xunit.Serialization.XunitSerializationInfo.DeserializeSerializable(Type type, String serializedValue) in C:\Dev\xunit\xunit\src\common\XunitSerializationInfo.cs:line 213
   at Xunit.Serialization.XunitSerializationInfo.Deserialize(Type type, String serializedValue) in C:\Dev\xunit\xunit\src\common\XunitSerializationInfo.cs:line 110
   at Xunit.Sdk.SerializationHelper.Deserialize[T](String serializedValue) in C:\Dev\xunit\xunit\src\common\SerializationHelper.cs:line 40
   at Xunit.Sdk.XunitTestFrameworkExecutor.Deserialize(String value) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\XunitTestFrameworkExecutor.cs:line 84
   at Xunit.DefaultTestCaseBulkDeserializer.<BulkDeserialize>b__2_0(String serialization) in C:\Dev\xunit\xunit\src\xunit.runner.utility\Descriptor\DefaultTestCaseBulkDeserializer.cs:line 22
   at System.Linq.Utilities.<>c__DisplayClass2_0`3.<CombineSelectors>b__0(TSource x)
   at System.Linq.Enumerable.SelectListIterator`2.ToList()
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Xunit.DefaultTestCaseBulkDeserializer.BulkDeserialize(List`1 serializations) in C:\Dev\xunit\xunit\src\xunit.runner.utility\Descriptor\DefaultTestCaseBulkDeserializer.cs:line 22
   at Xunit.Xunit2.BulkDeserialize(List`1 serializations) in C:\Dev\xunit\xunit\src\xunit.runner.utility\Frameworks\v2\Xunit2.cs:line 74
   at Xunit.Runner.VisualStudio.VsTestRunner.RunTestsInAssembly(IRunContext runContext, IFrameworkHandle frameworkHandle, LoggerHelper logger, TestPlatformContext testPlatformContext, RunSettings runSettings, IMessageSinkWithTypes reporterMessageHandler, AssemblyRunInfo runInfo) in C:\Dev\xunit\xunit\src\xunit.runner.visualstudio\VsTestRunner.cs:line 562

The package is referenced in version 0.3.18 and the issue has been brought up and fixed in the origin repository, it is available in nuget package 1.0.19-beta

Unfortunately just updating the nuget package doesn't work, because to properly support WinFormsStaFact they also added a dependency on FrameworkReference on Microsoft.WindowsDesktop.App to the nuget package.

When updating the nuget package this leads to referencing the SDK WinForms in addition to the locally built WinForms, causing conflicts in all test projects.

I have no idea how to properly fix this, but I can work around by dropping all TransitiveFrameworkReference obtained from nuget packages to prevent upgrading the SDK to a DesktopApp. No idea what the proper resolution is to consume the XUnit.StaFact bugfixes, I don't understand the arcade build system.

Expected behavior:
Bugfixes for Xunit.StaFact should be consumed

Minimal repro:
Try to run StaTheory or WinFormsTheory with complex MemberData inside Visual Studio

/cc @hughbe @RussKie FYI

[RussKie]

@AArnott

[AArnott]

I've been complaining about the impact of .NET Core targeting libraries that want to include WPF/WinForms that may be applicable being impossible for a long time. I don't know how to solve this problem. How else can (and should) a nuget package that is built with a WPF/WinForms dependency on .NET Core be built?

[RussKie]

@ericstj @danmosemsft @bradwilson appreciate your input, the desktop is pretty much blocked from running certain kinds of theory tests in Visual Studio.

[bradwilson]

Afraid I'm the wrong person to ask about this.

[weltkante]

It may be possible that this is related to dotnet/sdk#3254 again, except now this is a build time error. The reason I'm saying this is because the situation with System.Drawing and WindowsBase was kinda similar, they were referenced indirectly but we also wanted to reference the locally built one.

It feels similar now, we pull in System.Windows.Forms through the DesktopApp SDK referenced by Xunit.StaFact, but we also ProjectReference it. IMHO the msbuild targets should resolve the conflict in favor of the directly referenced System.Windows.Forms, drop the one from the DesktopApp SDK and note the version inside the deps.json

Just speculating though, I have no idea if thats how its supposed to work but there are similarities.

[AArnott]

While I think it would be interesting to pursue the SDK to get this fixed, IMO your case is so very special (no one else will build WinForms while testing it other than this repo), perhaps it's worth it for you to keep your own source copy of WinFormsFact in your repo rather than consume it via nuget.
I mean, as much as I'd love to see you adopt a library I wrote for testing, maybe it's more trouble than it's worth to consume via NuGet.

[AArnott]

...and maybe you can consume via git submodule.

[merriemcgaw]

Thanks @AArnott ! I think that's the approach we're going to need to take. Putting this up for grabs for the moment.

[weltkante]

I'll make an attempt for next week

[weltkante]

I've tested locally and got a few general questions before I create a PR:

• should we keep a local copy of the code we actually use and make changes as required or start using submodules and let the source be maintained in its own repo?
  • do submodules work out of the box with arcade and CI infrastructure? if not then this probably requires assistance by someone who knows how this works
  • if we want to use submodules I need to do a PR against the original repo to conditionally compile without WPF support, as WinForms obviously does not have the WPF assemblies available to reference
• where to put the source (or the submodule)? candidates are
  • src\Xunit.StaFact (used that in my WIP branch)
  • src\Common\tests\Xunit.StaFact
• should we reference the source from an existing csproj or create a new one?
  • in the WIP branch I've been linking the source into InternalUtilitiesForTests.csproj
• how do we deal with the different copyright headers in the foreign source? They cause build failures and I have suppressed SA1636 for the WIP branch

[edit]

• also noticed the licenses are different, MIT vs. MS-PL, probably needs adressing in some form or another as the top level license file (referenced by the copyright headers in source) doesn't match

[AArnott]

how do we deal with the different copyright headers in the foreign source? They cause build failures and I have suppressed SA1636 for the WIP branch

I hit this recently too. The answers weren't great though. The crux of the fix was to choose one top-level directory under which to create submodules, and I dropped an .editorconfig file there to suppress all analyzer warnings:

https://github.com/AArnott/Nerdbank.Streams/pull/174/files#diff-27e3827ced01b814eb1ec2a994e5b7a5

But since that didn't work as it should due to dotnet/roslyn#42762, I also had to do this:

https://github.com/AArnott/Nerdbank.Streams/pull/174/files#diff-ef6e265f0bd6383042733844e4892a20

[RussKie]

@weltkante very good questions.
I suppose there will be a combination of things, something that we've dealt with in https://github.com/gitextensions/gitextensions repo - all (but one) submodules live under "Externals" folder and have their own set of targets and props files overriding the solution's settings.

Please also be aware of #3013

Once I'm done with #3064 I can help you with this one.

[RussKie]

I had a chat with @vladimir-krestov and he said he had success with using https://www.nuget.org/packages/Xunit.StaFact/1.0.31-beta version.
Posting here so I don't forget, I'll try to find some time to verify.

[vladimir-krestov]

Yes, I have updated xUnit in the project locally.
What I made:

• Installed package using PM: Install-Package Xunit.StaFact -Version 1.0.31-beta
• Changed xUnit version manually in Versions.props file

The build is successful for me. There are no errors.
WinFormsTheory worked correctly with MemberData.

I'm not sure that I got the correct result because I have problems with running tests in VisualStudio.
So, @weltkante, please check this to make sure.

[weltkante]

I think this PR (AArnott/Xunit.StaFact#39) fixed it for WinForms as well, nice!

I can confirm the framework reference is gone from nuspec and tests work correctly in VS. This basically resolved this issue externally, except you still have to do the actual update :-)

WinFormsTheory worked correctly with MemberData.

I'm not sure that I got the correct result because I have problems with running tests in VisualStudio.

The errors for WinFormsTheory+MemberData were only visible inside VS. Not sure why you can't run tests, works for me. I have to execute build-local.ps1 in a powershell console first, then I can run tests.

[vladimir-krestov]

I had platform issues when I tested the new xUnit version. I used some workaround to run tests in VS. But I have fixed my issues and now it isn't necessary.
The build is correct with the new xUnit for me too.
@weltkante, could you please create a new PR to master with updating xUnit?

[AArnott]

Given the success with the latest version of Xunit.StaFact 1.0.31-beta, I may release as stable. I'll give it a little longer so more folks on this repo can validate and close this issue.

[RussKie]

@weltkante and I observing a new wave of deadlocks. Trying to assert whether these have anything to do with Xunit.StaFact 1.0.31-beta.

[weltkante]

There are two issues with stafact surfacing here, haven't been noticing them since I've been running tests from VS (now that its working)

• ThreadRental at some point doesn't seem to recognize its task finished, looks like UITheoryTestCase doesn't get disposed. This happens with the EnsureEditorsTests tests, they are running 3 threads and all seem to have finished their tests, yet stafact is still polling task status in its message loop. Running tests from VS or using dotnet test does not cause this, I've been able to isolate the tests and setup a standalone repro and created Hanging UITheoryTestCase AArnott/Xunit.StaFact#44 for further investigation.

• WinFormsSynchronizationContextAdapter.PumpTill is using a polling message loop without waiting for messages (created CPU usage in polling message loop AArnott/Xunit.StaFact#43). It doesn't affect the hang but having high CPU usage when idle can be confusing and its easy to fix.

[AArnott]

The xunit.stafact bugs @weltkante reported are fixed and available on https://www.nuget.org/packages/xunit.stafact

[weltkante]

Took me a while to figure out but some tests are suddenly executing in parallel when they shouldn't be, created AArnott/Xunit.StaFact#48

[edit] looks like that was a red herring, no idea what it was that was causing the failures, but its gone without having to change anything