Fix some bugs with the new XunitAssert #13510
Conversation
I found a few bugs in trying to enable dotnet/runtime#86193. One is that there's no way to flip Arcade to pull in the fork package instead of xunit.assert. The second is that when both are pulled in, because the fork has a different assembly name, the references are ambiguous and produce compile-time breaks. Matching names seems like it will get rid of that problem, and it also has the side-effect of letting the analyzer work properly against the fork.
| @@ -3,7 +3,6 @@ | |||
|
|
|||
| <!-- This project provides xunit.assert, so we can't bring the package in, as it would conflict. --> | |||
| <ItemGroup> | |||
| <PackageReference Remove="xunit" /> | |||
There was a problem hiding this comment.
Don't you need to set UseDotNetXUnitAssert to true somewhere here for the tests to use the right xunit.assert? But aside, don't you want to use the live reference here and not a PackageReference to Microsoft.DotNet.XUnitAssert?
I guess you now need to PackageReference Remove="xunit.assert" and "Microsoft.DotNet.XUnitAssert" here?
There was a problem hiding this comment.
Or do you want rely on conflict resolution here to prefer xunit.assert from Microsoft.DotNet.XUnitAssert over the one coming from the xunit.assert package?
There was a problem hiding this comment.
I was doing the latter, but the former sounds more reliable. I'll change to doing that.
There was a problem hiding this comment.
Turns out the test is going to pull from the old version of Arcade (not a live build) so replacing is annoying. I'll rely on conflict resolution for now.
src/Microsoft.DotNet.XUnitAssert/tests/Microsoft.DotNet.XUnitAssert.Tests.csproj
Outdated
Show resolved
Hide resolved
| @@ -3,6 +3,9 @@ | |||
| <PropertyGroup> | |||
| <TargetFramework>$(NetCurrent)</TargetFramework> | |||
| <Nullable>enable</Nullable> | |||
| <!-- Baseline analyzer warnings. These warnings are present in the upstream xunit.assert tests. --> | |||
| <NoWarn>$(NoWarn);xUnit2000;xUnit2003;xUnit2005;xUnit2007;xUnit2011;xUnit2015;xUnit2017</NoWarn> | |||
| <UseDotNetXUnitAssert>true</UseDotNetXUnitAssert> | |||
There was a problem hiding this comment.
So with the next Arcade SDK update you will have a PackageReference to Microsoft.DotNet.XUnitAssert and a ProjectReference to the live build. Are you sure that the right version will be picked up? Can you please verify that locally?
There was a problem hiding this comment.
Confirmed. The copy of xunit.assert in the test directory is the source-built version.
I found a few bugs in trying to enable dotnet/runtime#86193. One is that there's no way to flip Arcade to pull in the fork package instead of xunit.assert. The second is that when both are pulled in, because the fork has a different assembly name, the references are ambiguous and produce compile-time breaks. Matching names seems like it will get rid of that problem, and it also has the side-effect of letting the analyzer work properly against the fork.