Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Target xUnit v3 for our v2 release #118

Merged
merged 28 commits into from
Jan 18, 2025
Merged

Target xUnit v3 for our v2 release #118

merged 28 commits into from
Jan 18, 2025

Conversation

AArnott
Copy link
Owner

@AArnott AArnott commented Jan 17, 2025

Huge callout to @reduckted, who did most of the work on this. 🎉🎉🎉

With xUnit v3 now available with breaking changes from v2, see the following table for how we support both versions:

xUnit Xunit.STAFact
For xUnit v2 support Use Xunit.STAFact 1.x
For xUnit v3 support Use Xunit.STAFact 2.x+

Closes #103

@AArnott AArnott linked an issue Jan 17, 2025 that may be closed by this pull request
@AArnott AArnott added this to the v2.0 milestone Jan 17, 2025
@AArnott AArnott changed the title Target Xunit v3 for our v2 release Target xUnit v3 for our v2 release Jan 17, 2025
@AArnott
Copy link
Owner Author

AArnott commented Jan 17, 2025

There's a fatal error in the runner that doesn't happen in VS, but does happen when running tests from the command line. It's causing PR validation to fail. I got this from the logs:

TpTrace Error: 0 : 2576, 15, 2025/01/17, 14:08:15.807, 98420275812, testhost.dll, [xUnit.net 00:00:01.01]     [FATAL ERROR] System.InvalidCastException
TpTrace Information: 0 : 2576, 15, 2025/01/17, 14:08:15.810, 98422606649, testhost.dll, [xUnit.net 00:00:01.02]       System.InvalidCastException : Unable to cast object of type 'SyncContextType' to type 'System.String'.
TpTrace Information: 0 : 2576, 15, 2025/01/17, 14:08:15.811, 98423747472, testhost.dll, [xUnit.net 00:00:01.02]       Stack Trace:
TpTrace Information: 0 : 2576, 15, 2025/01/17, 14:08:15.812, 98424917689, testhost.dll, [xUnit.net 00:00:01.02]            at System.Linq.Enumerable.SelectListIterator`2.MoveNext()
TpTrace Information: 0 : 2576, 15, 2025/01/17, 14:08:15.813, 98425591108, testhost.dll, [xUnit.net 00:00:01.02]            at System.Linq.Enumerable.WhereEnumerableIterator`1.ToArray()
TpTrace Error: 0 : 2576, 15, 2025/01/17, 14:08:15.813, 98426574326, testhost.dll, [xUnit.net 00:00:01.02] Catastrophic failure: System.InvalidCastException : Unable to cast object of type 'SyncContextType' to type 'System.String'.

This can be repro'd through the tools\dotnet-test-cloud.ps1 script (which automatically excludes the tests that are expected to fail).

Unfortunately, the callstack must have been partially stomped on because it doesn't tell exactly where the bug is. But I suspect you did some serialization work, and I wonder if this is due to serialization of test discovery results or something.
@reduckted, can you investigate?

@reduckted
Copy link
Contributor

reduckted commented Jan 18, 2025

@AArnott You were right about it being a serialization problem. Fixed in reduckted@3b4dc05 and reduckted@4de628f

@reduckted
Copy link
Contributor

I'm seeing 35 warnings about duplicate test cases being skipped. Those warnings didn't exist on the main branch, so I'll see if I can work out what's going on.

@reduckted
Copy link
Contributor

I'm seeing 35 warnings about duplicate test cases being skipped.

Fixed in reduckted@24d2aa8

The data rows and test method arguments were not being used when generating unique test IDs, so each data row for a theory test was appearing as a duplicate.

@AArnott
Copy link
Owner Author

AArnott commented Jan 18, 2025

Looks like xunit v3 made another breaking change to their console runner that's breaking our mac CocoaFact tests. I hope xunit has some docs for how to run tests in a cocoa process on mac. I'm guessing the prior v2 way of doing it was documented somewhere and we copied it, so maybe v3 has updated docs.

I'm off to bed. @reduckted if you want to work on it in the meantime, please feel free. Otherwise I'll pick this up tomorrow.

@reduckted
Copy link
Contributor

😅 Fixed in reduckted@c56047b

After some trial and error, and some digging through props and targets files, I worked it out.

First, the xUnit auto-generated entry point shows what is needed to replace the old ConsoleClient. A very simple replacement. 🎉 This also means we can remove the reference to xunit.v3.runner.console.

We also need to disable xUnit's auto-generated entry point. Easily done with a build property.

A couple of tweaks to the command line arguments too. The -appdomains argument no longer exists, and we don't need to pass the path of the assembly to test.

Attempting to run the tests then led to a rather cryptic error:

/Users/runner/work/Xunit.StaFact/Xunit.StaFact/.nuget/packages/microsoft.testing.platform.msbuild/1.4.3/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets(320,5): error : Full path tool calculation failed. Runner 'dotnet' [/Users/runner/work/Xunit.StaFact/Xunit.StaFact/test/Xunit.StaFact.Tests.Mac/Xunit.StaFact.Tests.Mac.csproj]

Microsoft.Testing.Platform.MSBuild is a transitive dependency of xUnit. That package has build files that define a Test target. So running dotnet build -t:Test was resulting in that target running instead of the custom target in the property file. The fix to this was to move the custom test target out of the project file and into a Directory.Build.targets file in the same directory so that the Test target will override the one from Microsoft.Testing.Platform.MSBuild instead of the other way around.

One last change was to set IsTestingPlatformApplication to false. When this is true, Microsoft.Testing.Platform.MSBuild.targets will generate a few files to enable Microsoft Testing Platform support. Since we don't want to use the Microsoft Testing Platform, we can turn that setting off so that those files are not generated.

@AArnott
Copy link
Owner Author

AArnott commented Jan 18, 2025

Awesome. That sounds much more complicated than I anticipated. Thanks so much for diagnosing and fixing that.

Since we don't want to use the Microsoft Testing Platform

Do you think we should suppress the transitive package reference? Would doing so simplify anything?

@AArnott AArnott merged commit 4a13264 into main Jan 18, 2025
6 checks passed
@AArnott AArnott deleted the xunitv3 branch January 18, 2025 15:59
@reduckted
Copy link
Contributor

Do you think we should suppress the transitive package reference? Would doing so simplify anything?

Oh, if it's possible to do that then it would simplify things a bit. We could move the Test target back into the project file and remove the IsTestingPlatformApplication property since it would have no effect.

@AArnott
Copy link
Owner Author

AArnott commented Jan 19, 2025

Moving targets into the project file itself isn't actually a positive move, IMO. And removal of a property definition itself isn't worth it, I'd say. But as this technique can be useful in avoiding some of the discovery costs you incurred to resolve issues, here's how it would be done:

<PackageReference Include="Microsoft.Testing.Platform.MSBuild" ExcludeAssets="all" />

Of course this means too that this package ID must be directly specified in Directory.Packages.props, which gives one more version to maintain, even though it's an "anti-reference".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to xunit v3
2 participants