Upgrade xUnit #75068

jaredpar · 2024-09-12T01:11:16Z

One of our NuGet audit alerts was rooted in our xUnit packages as they only had netstandard1.3 libraries. To resolve this alert I moved us to newer versions of the package which had netstandard2.0 libraries.

Upgrading the package revealed two problems in our code base:

The xUnit Assert APIs took advantage of the unmanaged constraint in a fwe places. That is not supported by Visual Basic and leads to errors on import. To work around this I inserted AssertEx thunks that our Visual Basic layer can call through. Longer term we will need to consider adding support for the constraint (issue)
The xUnit APIs significantly increased their nullable reference type annotations. This caught a number of places where the tests were passing potentially null values to APIs that don't accept null. There were enough hits that I added AssertEx methods that handle the nullable case. In a few places though I just suppressed the warning.
Several xUnit APIs are ambiguous in the face of collection expressions and I needed to add casts to disambiguate.

The vulnerability: GHSA-cmhx-cq75-c4mj

jaredpar · 2024-09-13T15:48:25Z

@dotnet/roslyn-compiler, @dotnet/roslyn-ide PTAL

jaredpar · 2024-09-13T15:51:34Z

src/Compilers/Test/Core/Assert/AssertEx.cs

@@ -160,6 +161,9 @@ public static void AreEqual<T>(T expected, T actual, string message = null, IEqu
            }
        }

+        public static void Equal<T>(ImmutableArray<T> expected, IEnumerable<T> actual)


This change added Assert.Equals<T>(T expected, T actual). This created an ambiguity with the following pattern in our code base:

AssertEx.Equals([1, 2], someImmutableArray);

That call matches both the new overload and the existing overloads that have ImmutableArray<T> as a parameter. Unfortunately, the number of optional parameters is a tie breaker in overload resolution. That meant that the new overload was winning which is undesirable. Added the overloads without optional parameters to get the desired binding behavior.

If that's the pattern, should we perhaps add AssertEx.Equals<T>(ReadOnlySpan<T>, ImmutableArray<T>) instead?

Can you help me understand how that will help?

Just avoid creating the immutable array for the expected value.

This feeds into the equality that takes IEnumerable<T> though. I could rewrite that as well. IS that what you're suggesting?

I was specifically going off of the pattern you showed above, which is collectionExpression, ImmutableArray. Is that not the pattern?

.vscode/tasks.json

eng/Directory.Packages.props

src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs

src/Compilers/Server/VBCSCompilerTests/NamedPipeClientConnectionHostTests.cs

333fred · 2024-09-13T17:06:03Z

src/Compilers/Test/Core/Assert/AssertEx.cs

@@ -160,6 +161,9 @@ public static void AreEqual<T>(T expected, T actual, string message = null, IEqu
            }
        }

+        public static void Equal<T>(ImmutableArray<T> expected, IEnumerable<T> actual)


If that's the pattern, should we perhaps add AssertEx.Equals<T>(ReadOnlySpan<T>, ImmutableArray<T>) instead?

CyrusNajmabadi · 2024-09-13T23:09:39Z

src/EditorFeatures/CSharpTest/Debugging/ProximityExpressionsGetterTests.cs

@@ -61,7 +61,7 @@ static void Main(string[] args)
 }");
        var terms = CSharpProximityExpressionsService.GetProximityExpressions(tree, 245, cancellationToken: default);
        Assert.NotNull(terms);
-        AssertEx.Equal(["yy", "xx"], terms);
+        AssertEx.Equal((string[])["yy", "xx"], terms);


what's going on here? not loving that these IDE tests just got a lot more verbose (which goes against moving to collection exprs in the first place) :)

Unfortunately xUnit introduced overloads that make these ambiguous now. I'm open to suggestions on how to approach this but it's hard to see how to reconcile the platform issues.

This is a good example of where Equals(ROSpan<T> span, T[] array) won't fix all our issues. The terms value in this case is IList<T>.

Do you know what it was using before? Was this an IEnumerable<T> vs IEnumerable<T> case?

So, this is an AssertEx helper, not an Assert helper. What methods are actually ambiguous here?

This will not be helped by the new overload because the RHS is IReadOnlyList<T>

Sure, but this is not a general xUnit API, this is our own extension. So we can just add whatever helpers we need to make it work.

CyrusNajmabadi · 2024-09-13T23:10:17Z

src/LanguageServer/ProtocolUnitTests/Diagnostics/AdditionalFileDiagnosticsTests.cs

@@ -63,10 +63,10 @@ public async Task TestWorkspaceDiagnosticsWithRemovedAdditionalFile(bool useVSDi
        var results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics);
        Assert.Equal(3, results.Length);

-        Assert.Empty(results[0].Diagnostics);
+        AssertEx.Empty(results[0].Diagnostics);


curious why Assert.Empty doesn't wor khere? (or in all the other places changed).

@CyrusNajmabadi tried to summarize a lot of the reasons like this in the PR description + doc comments on the methods. In this case it's because xUnit NRT annotated their APIs and Empty, Single, etc ... do not accept null. Given that diagnostics assigns different meaning to null and Empty I didn't want to make broad changes to the test implementation. Instead I added null annotated versions of the methods to AssertEx and redirected the impacted sites to those.

CyrusNajmabadi · 2024-09-13T23:11:11Z

src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs

@@ -2138,7 +2138,7 @@ public void RemoveAnalyzerReference()
            solution = solution.WithAnalyzerReferences([analyzerRef1, analyzerRef2]);

            var solution2 = solution.RemoveAnalyzerReference(analyzerRef1);
-            AssertEx.Equal([analyzerRef2], solution2.AnalyzerReferences);
+            AssertEx.Equal((AnalyzerReference[])[analyzerRef2], solution2.AnalyzerReferences);


:'(

is assertex something owned by roslyn? can we just fixup the signatures here so this works? (or make it do a runtime check to do the right thing)?

This will not be fixed by the new overload because the RHS is IReadOnlyList<T>

CyrusNajmabadi · 2024-09-13T23:13:06Z

Several xUnit APIs are ambiguous in the face of collection expressions and I needed to add casts to disambiguate.

This seems... bad :)

In particular, i validated much of the collection expr work we did against the xunit api to make sure people could use collection exprs with apis like that without an issue. Could you clarify what is not ambiguous there? I'm def worried as that seems like there's a trap where collection exprs can work well with an api, but then easily change into a form that causes problems, causing source-level breaks.

While the user can work around them, the code is def no longer in the desirable form. Leading to worst of both worlds :(

333fred · 2024-09-13T23:21:44Z

Clarified with @CyrusNajmabadi offline. In particular, these ambiguities will be solved in xUnit 2.9.1, when that comes out: xunit/xunit#3021.

CyrusNajmabadi · 2024-09-13T23:21:45Z

Ok. Fred walked me through this. Unpleasant. but this seems like an acceptable path forward until we get upgraded to xunit 2.9.1. Would we want to just wait on htat?

333fred · 2024-09-13T23:25:21Z

Would we want to just wait on htat?

This is a CG alert lying in wait. We need to solve it now, unfortunately.

jaredpar · 2024-09-13T23:27:52Z

Yeah unfortunately this is all motivated by getting us CG clean.

CyrusNajmabadi · 2024-09-14T09:21:13Z

"CG"?

CyrusNajmabadi · 2024-09-14T09:22:00Z

Would we want to just wait on htat?

This is a CG alert lying in wait. We need to solve it now, unfortunately.

Could we add an assertex overload akin to the one brad is adding to 2.9.1? Then we can call that, instead of adding all the cats.

jaredpar · 2024-09-14T21:53:45Z

"CG"?

Component governance. Basically security issues.

Could we add an assertex overload akin to the one brad is adding to 2.9.1? Then we can call that, instead of adding all the cats.

If there is a pattern that works better here I'm happy to add it.

333fred · 2024-09-14T22:43:00Z

Could we add an assertex overload akin to the one brad is adding to 2.9.1? Then we can call that, instead of adding all the cats.

If there is a pattern that works better here I'm happy to add it.

That's what I suggested here: #75068 (comment)

jjonescz · 2024-09-16T08:02:47Z

Clarified with @CyrusNajmabadi offline. In particular, these ambiguities will be solved in xUnit 2.9.1, when that comes out: xunit/xunit#3021.

I didn't follow how the linked issue is related. The first-class span feature isn't merged into main, so it cannot cause ambiguities here.

CyrusNajmabadi · 2024-09-16T09:14:48Z

Isn't this just an ambiguity involving collection expressions? The overload we want to add (which will also come in 2.9.1) will be seen as better than the current ambiguous overloads.

jjonescz · 2024-09-16T09:17:20Z

Oh yeah, I see, the xunit fix will help with some cases that became ambiguous even without the "first-class span" feature.

CyrusNajmabadi · 2024-09-16T13:21:32Z

Yup yup. So my preference is that we add the overload, update our code to call that, then upgrade xunit again, then remove the overload and update again.

333fred · 2024-09-16T15:40:38Z

Oh yeah, I see, the xunit fix will help with some cases that became ambiguous even without the "first-class span" feature.

Yup, first-class spans made this slightly more breaking, but it was already broken.

src/Compilers/Test/Core/Assert/AssertEx.cs

jcouv

Done with review pass (iteration 5)

jasonmalinowski · 2024-09-16T21:10:47Z

.editorconfig

@@ -151,6 +151,16 @@ dotnet_public_api_analyzer.require_api_files = true
 # Workaround for https://github.com/dotnet/roslyn/issues/70570
 dotnet_diagnostic.IDE0055.severity = warning

+# Enable the xUnit analyzer rules


Comment says enable, but this is only disabling things.

Yes but it's referencing an issue that says enable the rules.

jcouv

LGTM Thanks (iteration 5)

nohwnd · 2024-09-17T19:00:29Z

@jaredpar revert 97e3b and define

<MicrosoftTestPlatformVersion>17.11.1</MicrosoftTestPlatformVersion>
<MicrosoftNETTestSdkVersion>$(MicrosoftTestPlatformVersion)</MicrosoftNETTestSdkVersion>

To update the version of test.sdk from the outdated 17.5 that arcade ships. That fixes the language server tests (details here microsoft/vstest#10355)

And hopefully does not break any other tests.

jaredpar · 2024-09-18T19:55:40Z

@333fred feedback addressed and tests are passing. Can you give it another look?

333fred · 2024-09-19T20:08:47Z

src/Compilers/Test/Core/Assert/AssertEx.cs

@@ -160,6 +161,12 @@ public static void AreEqual<T>(T expected, T actual, string message = null, IEqu
            }
        }

+        public static void Equal<T>(ReadOnlySpan<T> expected, T[] actual) =>
+            Equal<T>(expected.ToArray(), actual);


Nit: since we know allocations are a problem in tests, perhaps instead call Equal<T>(expected, actual.AsSpan())?

One of our NuGet audit alerts was rooted in our xUnit packages as they only had `netstandard1.3` libraries. To resolve this alert I moved us to newer versions of the package which had `netstandard2.0` libraries. Upgrading the package revealed two problems in our code base: 1. The xUnit `Assert` APIs took advantage of the `unmanaged` constraint in a fwe places. That is not supported by Visual Basic and leads to errors on import. To work around this I inserted `AssertEx` thunks that our Visual Basic layer can call through. Longer term we will need to consider adding support for the constraint ([issue](dotnet#75063)) 2. The xUnit APIs significantly increased their nullable reference type annotations. This caught a number of places where the tests were passing potentially null values to APIs that don't accept null. There were enough hits that I added `AssertEx` methods that handle the nullable case. In a few places though I just suppressed the warning. The vulnerability: GHSA-cmhx-cq75-c4mj

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 12, 2024

dotnet-policy-service bot added the VSCode label Sep 12, 2024

jaredpar mentioned this pull request Sep 12, 2024

Enable new xUnit analyzers #75093

Open

7 tasks

jaredpar marked this pull request as ready for review September 13, 2024 15:48

jaredpar requested review from a team as code owners September 13, 2024 15:48

jaredpar commented Sep 13, 2024

View reviewed changes

333fred reviewed Sep 13, 2024

View reviewed changes

jcouv self-assigned this Sep 13, 2024

CyrusNajmabadi reviewed Sep 13, 2024

View reviewed changes

jcouv reviewed Sep 16, 2024

View reviewed changes

src/Compilers/Test/Core/Assert/AssertEx.cs Show resolved Hide resolved

jcouv reviewed Sep 16, 2024

View reviewed changes

jasonmalinowski reviewed Sep 16, 2024

View reviewed changes

jcouv approved these changes Sep 16, 2024

View reviewed changes

build-analysis bot mentioned this pull request Sep 18, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

333fred approved these changes Sep 19, 2024

View reviewed changes

jaredpar added 11 commits September 25, 2024 15:36

more

96045bd

more

9228c15

more

02908cd

more

e1adb8b

see if this fixes the tests

db8f936

test fix

62ac904

fix overload

f2af9e9

fixup for new xunit version

a9c728a

fix

e45edb9

more

8d9ee0b

jaredpar force-pushed the xunit branch from f7019a0 to 8d9ee0b Compare September 25, 2024 22:37

Fix

0d749b9

jaredpar merged commit 0b058f2 into dotnet:main Sep 26, 2024
28 checks passed

jaredpar deleted the xunit branch September 26, 2024 20:09

dotnet-policy-service bot added this to the Next milestone Sep 26, 2024

akhera99 modified the milestones: Next, 17.12 P3 Sep 26, 2024

This was referenced Oct 1, 2024

[Automated] PRs inserted in VS build main-35330.119 #75310

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35401.07 #75313

Closed

dotnet-bot mentioned this pull request Oct 11, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35410.215 #75481

Closed

Upgrade xUnit #75068

Upgrade xUnit #75068

Conversation

jaredpar commented Sep 12, 2024 • edited Loading

jaredpar commented Sep 13, 2024

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

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

CyrusNajmabadi commented Sep 13, 2024

333fred commented Sep 13, 2024

CyrusNajmabadi commented Sep 13, 2024

333fred commented Sep 13, 2024

jaredpar commented Sep 13, 2024

CyrusNajmabadi commented Sep 14, 2024

CyrusNajmabadi commented Sep 14, 2024

jaredpar commented Sep 14, 2024

333fred commented Sep 14, 2024

jjonescz commented Sep 16, 2024 • edited Loading

CyrusNajmabadi commented Sep 16, 2024

jjonescz commented Sep 16, 2024

CyrusNajmabadi commented Sep 16, 2024

333fred commented Sep 16, 2024

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

nohwnd commented Sep 17, 2024

jaredpar commented Sep 18, 2024

Choose a reason for hiding this comment

jaredpar commented Sep 12, 2024 •

edited

Loading

jjonescz commented Sep 16, 2024 •

edited

Loading