Adopt Microsoft.DotNet.XunitAssert fork #86193

agocke · 2023-05-12T23:55:38Z

Start using Microsoft.DotNet.XUnitAssert instead of assert.xunit.

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.

stephentoub · 2023-05-15T13:59:31Z

...s/System.Text.Json/tests/System.Text.Json.Tests/NewtonsoftTests/ImmutableCollectionsTests.cs

@@ -316,7 +316,7 @@ public void DeserializeSortedSet()
  ""3""
 ]";

-            ImmutableSortedSet<string> data = JsonSerializer.Deserialize<ImmutableSortedSet<string>>(json);
+            ISet<string> data = JsonSerializer.Deserialize<ImmutableSortedSet<string>>(json);


Why did these change?

Ah, interesting case!

So, this is a breaking change, but not one caused by me. The latest source for Xunit.Assert contains an overload for both IReadOnlySet and ISet. That makes this overload ambiguous.

I'm not sure the best way to handle this. This was the easiest way I could think to make progress.

For options I think we could:

Make the change above

Revert the xunit source to an older version which doesn't have this change

Raise the change as an issue in the xunit.assert repo as a potential breaking change.

Do you have any preferences?

Seems reasonable to just accommodate the change.

stephentoub · 2023-05-15T13:59:54Z

src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/XUnitVerifier.cs

@@ -0,0 +1,154 @@
+// Licensed to the .NET Foundation under one or more agreements.


Can you put this file into Common somewhere? It's not specific to regex.

(A comment in the file would also be helpful indicating what it is... it appears to be a copy of a file from the old testing package that was being used?)

Might also be useful to include a link to the original source as a code comment.

Yes, this is from the Roslyn SDK, and I just copied the source over. The Roslyn SDK contains a reference to xunit.assert, which was causing problems. However, my changes in dotnet/arcade#13510 may make this obsolete.

The problems come from xunit.assert and Microsoft.DotNet.XUnitAssert having different assembly names -- so when both are included, all references are ambiguous. The alternative approach I'm proposing in that PR is to share the assembly name. Aside from making the XUnit analyzers work (which I think is a big benefit), it will remove these kinds of conflicts because I will no longer have to remove all shared references, and can instead rely on the Dotnet XunitAssert winning because it is higher version.

I'm ambivalent about the assembly name change, but after trying to fix a ton of these one-off issues, I'm warming up to it.

IMO the assembly name changes is fine - we did the same with the Microsoft.DotNet.XUnitConsoleRunner: https://github.com/dotnet/arcade/blob/2d8d59065b5e090584a8e90c4371fc06ed60bdc5/src/Microsoft.DotNet.XUnitConsoleRunner/src/Microsoft.DotNet.XUnitConsoleRunner.csproj#L6

Avoiding renaming the assembly name between the upstream and the fork should be less breaking.

stephentoub · 2023-05-15T14:01:21Z

src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/XUnitVerifier.cs

@@ -0,0 +1,154 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.


Most of our files use the two line license:

// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license.

Is there a specific reason this one needs to be different? Or is this the license from where the file was copied from?

ViktorHofer · 2023-05-15T15:35:07Z

eng/testing/xunit/xunit.props

+    <!-- Use dotnet/runtime fork of assert.xunit if netcore, otherwise use xunit.assert package. -->
+    <PackageReference Include="Microsoft.DotNet.XUnitAssert" Version="$(MicrosoftDotNetXUnitAssertVersion)" Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'"/>
+    <PackageReference Include="xunit.assert" Version="$(XUnitVersion)" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'" />


The Microsoft.DotNet.XUnitAssert package only works on .NETCoreApp. Therefore I would instead condition on TFI == .NETCoreApp and TFI != .NETCoreApp.

ViktorHofer · 2023-05-15T15:35:47Z

src/libraries/Common/tests/TestUtilities/TestUtilities.csproj

+    <!-- Use dotnet/runtime fork of assert.xunit if netcore, otherwise use xunit.assert package. -->
+    <PackageReference Include="Microsoft.DotNet.XUnitAssert" Version="$(MicrosoftDotNetXUnitAssertVersion)" Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'"/>
+    <PackageReference Include="xunit.assert" Version="$(XUnitVersion)" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'"/>


Same as above, change TFI condition to .NETCoreApp

ViktorHofer · 2023-05-15T15:36:36Z

src/libraries/Directory.Build.targets

+  <ItemGroup>
+    <!-- Fixup arcade test references by removing xunit" -->
+    <PackageReference Remove="xunit" Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'" />
+    <PackageReference Remove="xunit.assert" Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'" />
+  </ItemGroup>


Remove this. We don't use Arcade's test framework: https://github.com/dotnet/runtime/blob/80ffe5c713b87524aa87085cff81d09daf8c65b4/src/libraries/Directory.Build.props#LL3C3-L3C3

ViktorHofer · 2023-05-15T15:38:45Z

....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj

+    <!-- Use dotnet/runtime fork of assert.xunit if netcore, otherwise use xunit.assert package. -->
+    <PackageReference Include="Microsoft.DotNet.XUnitAssert" Version="$(MicrosoftDotNetXUnitAssertVersion)" Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'"/>
+    <PackageReference Include="xunit.assert" Version="$(XUnitVersion)" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />


Microsoft.Extensions.DependencyInjection.Specification.Tests is a shipping package which must not rely on non-shipping packages (Microsoft.DotNet.XUnitAssert).

ViktorHofer · 2023-05-15T15:41:03Z

...em.Text.RegularExpressions/tests/FunctionalTests/System.Text.RegularExpressions.Tests.csproj

@@ -72,7 +73,7 @@
    <Compile Include="$(CommonTestPath)System\Diagnostics\DebuggerAttributes.cs" Link="Common\System\Diagnostics\DebuggerAttributes.cs" />

    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisVersion)" PrivateAssets="all" />
-    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.XUnit" Version="$(CompilerPlatformTestingVersion)" />
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing" Version="$(CompilerPlatformTestingVersion)" />


I assume you changed this as the package brings xunit.assert in transitively? What if we would make sure that Microsoft.DotNet.XUnitAssert's assembly identity matches xunit.assert's one. Would we then be able to consume such packages by excluding their transitives?

Yup, this is the approach I took in dotnet/arcade#13510. If that sounds good to you, I will take that change and then retry this PR on top of that. A whole lot of problems will go away.

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.

ghost · 2023-06-01T20:33:28Z

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Start using Microsoft.DotNet.XUnitAssert instead of assert.xunit.

Author:	agocke
Assignees:	agocke
Labels:	`area-Infrastructure`
Milestone:	-

ghost · 2023-07-27T23:01:39Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 12, 2023

ghost assigned agocke May 12, 2023

agocke mentioned this pull request May 13, 2023

Fix some bugs with the new XunitAssert dotnet/arcade#13510

Merged

stephentoub reviewed May 15, 2023

View reviewed changes

stephentoub requested a review from ViktorHofer May 15, 2023 14:01

ViktorHofer reviewed May 15, 2023

View reviewed changes

danmoseley added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 1, 2023

WIP

d383a85

agocke force-pushed the use-msdn-assert branch from 06b8fe1 to d383a85 Compare June 27, 2023 20:48

This was referenced Jun 28, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

Tests.System.TimeProviderTests.TestProviderTimer test failure #87477

Closed

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

ghost closed this Jul 27, 2023

ghost locked as resolved and limited conversation to collaborators Aug 27, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt Microsoft.DotNet.XunitAssert fork #86193

Adopt Microsoft.DotNet.XunitAssert fork #86193

agocke commented May 12, 2023

stephentoub May 15, 2023

agocke May 15, 2023

danmoseley Jun 1, 2023

stephentoub May 15, 2023

stephentoub May 15, 2023

ViktorHofer May 15, 2023

agocke May 15, 2023

ViktorHofer May 15, 2023

stephentoub May 15, 2023

ViktorHofer May 15, 2023

ViktorHofer May 15, 2023

ViktorHofer May 15, 2023

ViktorHofer May 15, 2023

ViktorHofer May 15, 2023 •

edited

Loading

agocke May 15, 2023

ghost commented Jun 1, 2023

ghost commented Jul 27, 2023

		@@ -0,0 +1,154 @@
		// Licensed to the .NET Foundation under one or more agreements.

Adopt Microsoft.DotNet.XunitAssert fork #86193

Adopt Microsoft.DotNet.XunitAssert fork #86193

Conversation

agocke commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 1, 2023

ghost commented Jul 27, 2023

ViktorHofer May 15, 2023 •

edited

Loading