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

xUnit.net support broken with xUnit.net 2.5 #1099

Open
bradwilson opened this issue Jun 10, 2023 · 36 comments · May be fixed by #1144 or #1145
Open

xUnit.net support broken with xUnit.net 2.5 #1099

bradwilson opened this issue Jun 10, 2023 · 36 comments · May be fixed by #1144 or #1145
Labels
Area-MS.CA.Testing Microsoft.CodeAnalysis.Testing

Comments

@bradwilson
Copy link

bradwilson commented Jun 10, 2023

In upgrading the xUnit.net analyzers project to use a prerelease 2.5.0 build, I've determined that Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.XUnit (and/or some of its dependencies) are broken when my code uses 2.5.

Any verifier which fails breaks because of a breaking binary dependency on several of the xUnit.net exception classes. (The assertion library, including all the exceptions, has been overhauled for v2 2.5 and v3.) The source of the problem is that you're taking a binary dependency on xunit.assert, which is locking consumers into your version choice when breaking changes are made to anything you consume.

This behavior isn't appropriate, in my view. Your dependency in essence locks users down to only being able to use "compatible" versions of xUnit.net. This is true for any binary dependency you take outside of the core framework: if you ship a hard dependency on a binary that someone else may want to use in their tests, then you are potentially locking them down to only being able to use a version that's compatible with your choice. I ended up removing all binary dependencies from xUnit.net v2 for exactly this reason; for us, the problematic library ended up being JSON.NET, and it broke JSON.NET when they wanted to test themselves with us. Even if we're one of the few or only consumers of this library, we're guaranteed to want to "move on" to newer versions of xUnit.net. 😄

Here is a quick example showing the problem. Given the follow code which should clearly fail:

image

This is the (expected) result using xUnit.net 2.4.2:

image

And this is the (incorrect) result using xUnit.net 2.5.0-pre.22:

image

I can think of at least two suggestions for potential paths forward.

1. Don't use xunit.assert at all.

xUnit.net considers all exceptions to be failing tests. Don't consume the xUnit.net assertion library; instead, create your own exceptions with your own messages and throw those.

2. Use xunit.assert.source (or Git sub-module) instead.

If you wish to consume the library but not ship a binary dependency on it, you can import the source version of our library and use that instead. The project is designed to be consumed in both of these fashions.

By default, importing the source (via NuGet or sub-module) will bring in all the code as public, but you can override that with XUNIT_VISIBILITY_INTERNAL. You can use pre-processor directives to influence which features are enabled or disabled. By leaving everything as internal, you will be able to use the existing assertions without republishing them into your API.

@bradwilson
Copy link
Author

bradwilson commented Jun 10, 2023

For what it's worth, I approached submitting a PR to fix this, but ran across several stumbling blocks.

First, I can build from the command line, but not inside Visual Studio 2022 17.6.2:

image

Second, I was unable to locate where <xunitVersion> is defined, so I'm unsure how to upgrade any dependencies.

Third, when trying to shift Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.csproj from xunit.assert to xunit.assert.source, I end up with a ton of analyzer issues (since my code doesn't conform to your standards).

This is the diff to make it "work":

diff --git a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EmptyWithMessageException.cs b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EmptyWithMessageException.cs
index d91b951..2fa7297 100644
--- a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EmptyWithMessageException.cs
+++ b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EmptyWithMessageException.cs
@@ -8,7 +8,7 @@ using Xunit.Sdk;
 
 namespace Microsoft.CodeAnalysis.Testing.Verifiers
 {
-    public class EmptyWithMessageException : EmptyException
+    internal class EmptyWithMessageException : EmptyException
     {
         public EmptyWithMessageException(IEnumerable collection, string userMessage)
             : base(collection)
diff --git a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EqualWithMessageException.cs b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EqualWithMessageException.cs
index 3a14e78..e2d10c7 100644
--- a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EqualWithMessageException.cs
+++ b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/EqualWithMessageException.cs
@@ -7,7 +7,7 @@ using Xunit.Sdk;
 
 namespace Microsoft.CodeAnalysis.Testing.Verifiers
 {
-    public class EqualWithMessageException : EqualException
+    internal class EqualWithMessageException : EqualException
     {
         public EqualWithMessageException(object? expected, object? actual, string userMessage)
             : base(expected, actual)
diff --git a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.csproj b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.csproj
index 2947cb5..134c771 100644
--- a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.csproj
+++ b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.csproj
@@ -4,6 +4,23 @@
     <TargetFrameworks>$(TestingLibraryTargetFrameworks)</TargetFrameworks>
     <AssemblyName>Microsoft.CodeAnalysis.Testing.Verifiers.XUnit</AssemblyName>
     <RootNamespace>Microsoft.CodeAnalysis.Testing.Verifiers</RootNamespace>
+    <DefineConstants>$(DefineConstants);XUNIT_NULLABLE;XUNIT_VISIBILITY_INTERNAL</DefineConstants>
+    <NoWarn>
+      $(NoWarn);
+      AD0001;
+      CS8604;
+      IDE0055;
+      RS0016; RS0027;
+      SA1003; SA1009; SA1027;
+      SA1108; SA1111; SA1117; SA1121; SA1122; SA1127; SA1128;
+      SA1206;
+      SA1300; SA1310; SA1313; SA1314;
+      SA1400; SA1412; SA1413;
+      SA1502; SA1503; SA1505; SA1507; SA1513; SA1515; SA1519; SA1520;
+      SA1611; SA1614; SA1615; SA1616; SA1617; SA1618; SA1623; SA1633; SA1642; SA1648; SA1649;
+      SX1101;
+      SX1309
+  </NoWarn>
   </PropertyGroup>
 
   <PropertyGroup>
@@ -14,7 +31,7 @@
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="xunit.assert" Version="2.3.0" />
+    <PackageReference Include="xunit.assert.source" Version="2.4.2" />
   </ItemGroup>
 
   <ItemGroup>
diff --git a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/NotEmptyWithMessageException.cs b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/NotEmptyWithMessageException.cs
index 9204a8b..f5b8e83 100644
--- a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/NotEmptyWithMessageException.cs
+++ b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/NotEmptyWithMessageException.cs
@@ -7,7 +7,7 @@ using Xunit.Sdk;
 
 namespace Microsoft.CodeAnalysis.Testing.Verifiers
 {
-    public class NotEmptyWithMessageException : NotEmptyException
+    internal class NotEmptyWithMessageException : NotEmptyException
     {
         public NotEmptyWithMessageException(string userMessage)
         {
diff --git a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/PublicAPI.Unshipped.txt b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/PublicAPI.Unshipped.txt
index 2745b01..3b935e8 100644
--- a/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/PublicAPI.Unshipped.txt
+++ b/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/PublicAPI.Unshipped.txt
@@ -1,10 +1,3 @@
-Microsoft.CodeAnalysis.Testing.Verifiers.EmptyWithMessageException
-Microsoft.CodeAnalysis.Testing.Verifiers.EmptyWithMessageException.EmptyWithMessageException(System.Collections.IEnumerable collection, string userMessage) -> void
-Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException
-Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException.EqualWithMessageException(object expected, object actual, string userMessage) -> void
-Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException.EqualWithMessageException(string expected, string actual, int expectedIndex, int actualIndex, string userMessage) -> void
-Microsoft.CodeAnalysis.Testing.Verifiers.NotEmptyWithMessageException
-Microsoft.CodeAnalysis.Testing.Verifiers.NotEmptyWithMessageException.NotEmptyWithMessageException(string userMessage) -> void
 Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier
 Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.Context.get -> System.Collections.Immutable.ImmutableStack<string>
 Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.XUnitVerifier() -> void
@@ -19,6 +12,3 @@ virtual Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.NotEmpty<T>(strin
 virtual Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.PushContext(string context) -> Microsoft.CodeAnalysis.Testing.IVerifier
 virtual Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.SequenceEqual<T>(System.Collections.Generic.IEnumerable<T> expected, System.Collections.Generic.IEnumerable<T> actual, System.Collections.Generic.IEqualityComparer<T> equalityComparer = null, string message = null) -> void
 virtual Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.True(bool assert, string message = null) -> void
-override Microsoft.CodeAnalysis.Testing.Verifiers.EmptyWithMessageException.Message.get -> string
-override Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException.Message.get -> string
-override Microsoft.CodeAnalysis.Testing.Verifiers.NotEmptyWithMessageException.Message.get -> string

The warning disablement is extensive, and it didn't feel appropriate to PR with that in place; it would make more sense to figure out how to exclude external code (via a source-based NuGet package) from code analysis. I was unable to figure that out in the short time I gave myself. 😁

There are test failures here, because the unit tests you've put in place take hard dependencies on the exception types (which are no longer type-matched, since you're throwing internal versions). Here's one example (of the 26 failures):

Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifierTests.TestEmptyMessage

Assert.Throws() Failure
Expected: typeof(Xunit.Sdk.EmptyException)
Actual:   typeof(Microsoft.CodeAnalysis.Testing.Verifiers.EmptyWithMessageException): 'someCollectionName' is not empty
Assert.Empty() Failure
Expected: <empty>
Actual:   [0]
---- Microsoft.CodeAnalysis.Testing.Verifiers.EmptyWithMessageException : 'someCollectionName' is not empty
Assert.Empty() Failure
Expected: <empty>
Actual:   [0]

The simple fix here would be to stop the strong type testing (and remove all the checks against properties you shouldn't really care about, like Expected and Actual) and simply rely on testing the message to ensure that it matches. I did verify by fixing this one test that this strategy would work fine:

diff --git a/tests/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.UnitTests/XUnitVerifierTests.cs b/tests/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.UnitTests/XUnitVerifierTests.cs
index 849e62c..9742a2e 100644
--- a/tests/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.UnitTests/XUnitVerifierTests.cs
+++ b/tests/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.UnitTests/XUnitVerifierTests.cs
@@ -15,7 +15,7 @@ namespace Microsoft.CodeAnalysis.Testing.Verifiers
         {
             var actual = new int[1];
             var verifier = new XUnitVerifier();
-            var exception = Assert.ThrowsAny<EmptyException>(() => verifier.Empty("someCollectionName", actual));
+            var exception = Assert.ThrowsAny<Exception>(() => verifier.Empty("someCollectionName", actual));
             Assert.Equal($"'someCollectionName' is not empty{Environment.NewLine}Assert.Empty() Failure{Environment.NewLine}Expected: <empty>{Environment.NewLine}Actual:   [0]", exception.Message);
         }
 

Also, one of your analyzers was repeatedly throwing an exception, which is why I had to also exclude AD0001:

CSC : error AD0001: Analyzer 'Microsoft.CodeAnalysis.CSharp.Analyzers.MetaAnalyzers.CSharpDiagnosticAnalyzerApiUsageAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'. [C:\Dev\xunit\roslyn-sdk\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit.csproj::TargetFramework=netstandard2.0]

@sharwell
Copy link
Member

@bradwilson The expectation is for xunit.assert to retain binary compatibility of the public API surface for 2.x releases, where x >= 4. If xunit.assert does not provide binary compatibility that works with binding redirection, the SDK provides generic packages (without the Xunit suffix) which allow a custom IVerifier implementation to be provided.

@sharwell
Copy link
Member

@bradwilson if you have any questions on the analyzer project update feel free to reach out

@bradwilson
Copy link
Author

@sharwell We are breaking binary compatibility with 2.5.

@sharwell
Copy link
Member

I think the only significant impact of that will be users will need to provide their own implementation of XUnitVerifier when updating to 2.5.

Is there a need to break compatibility in 2.5 knowing 3.0 is on the horizon and has more flexibility to break things?

@bradwilson
Copy link
Author

The same assertion library is shipped in 2.5 that is currently part of 3.0.

Even if we didn't ship this with 2.5 (which, to be clear, is not an option), you'd still be broken for anybody who wanted to use 3.0. Your binary dependency is the problem, regardless of which exact version breaks you.

@sharwell
Copy link
Member

sharwell commented Jun 16, 2023

Right, but the binary dependency is optional (removed by switching from Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.Xunit → Microsoft.CodeAnalysis.CSharp.CodeFix.Testing). I expect we'll come up with a more concrete plan after 2.5 and/or 3.0 is released in a stable version.

bradwilson added a commit to xunit/xunit.analyzers that referenced this issue Jun 17, 2023
@bradwilson
Copy link
Author

I've used this to work around the issue: xunit/xunit.analyzers@1b8cf8a?w=1

If I'm the only consumer of Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.Xunit can you can probably just delete it outright, as I'll just use this moving forward. If you have other consumers, you can use my commit as a template for your work.

@bradwilson
Copy link
Author

I've also fixed the issue where xunit.analyzers was triggering xUnit2007 and xUnit2015 on the assert source itself (examples of "I have to break the rules, but you shouldn't" 😂).

xunit/assert.xunit@205442f
xunit/xunit.analyzers@c6b8c48

I still am experiencing occasional AD0001 (with NullRef exceptions) from Microsoft.CodeAnalysis.CSharp.Analyzers.MetaAnalyzers.CSharpDiagnosticAnalyzerApiUsageAnalyzer, usually on clean builds. Any idea how to collect information to track a bug for that?

CSC : error AD0001: Analyzer 'Microsoft.CodeAnalysis.CSharp.Analyzers.MetaAnalyzers.CSharpDiagnosticAnalyzerApiUsageAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'. [C:\Dev\xunit\analyzers\src\xunit.analyzers.tests\xunit.analyzers.tests.csproj::TargetFramework=net472]

@sharwell
Copy link
Member

Any idea how to collect information to track a bug for that?

A full stack trace and context information is provided in the Visual Studio error list (design time errors) or in the .binlog (command line builds).

CSharpDiagnosticAnalyzerApiUsageAnalyzer

This analyzer is disabled by default starting with dotnet/roslyn-analyzers#6705. A stricter/faster analyzer will be provided instead that requires analyzers be implemented in an assembly that does not reference Microsoft.CodeAnalysis.Workspaces, and code fixes are implemented in a separate assembly (typically shipping in the same package).

@bradwilson
Copy link
Author

@sharwell Thanks, reported dotnet/roslyn-analyzers#6708

@bradwilson
Copy link
Author

FWIW, the problematic assembly is the test assembly, not either of the production assemblies. It seems like they get the analyzers run because of the project reference from the unit tests to the production assemblies. There's no reason they should be run at all, but I don't think I can disable it (short of just turning off every analyzer in the test project via NoWarn or .editorconfig).

@bradwilson
Copy link
Author

FYI, 2.5.0 shipped today. https://fosstodon.org/@xunit/110669666679697253

@sharwell sharwell added the Area-MS.CA.Testing Microsoft.CodeAnalysis.Testing label Jul 7, 2023
b-straub added a commit to b-straub/DexieNET that referenced this issue Aug 19, 2023
.NET, degrade XUnit until dotnet/roslyn-sdk#1099 has been fixed
@b-straub
Copy link

b-straub commented Aug 19, 2023

I've used this to work around the issue: xunit/xunit.analyzers@1b8cf8a?w=1

If I'm the only consumer of Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.Xunit can you can probably just delete it outright, as I'll just use this moving forward. If you have other consumers, you can use my commit as a template for your work.

I'm using Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.Xunit and Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.Xunit as well. It would be nice if you could open a pull request for the dotnet/roslyn-sdk/

@bradwilson
Copy link
Author

@b-straub Given the compiler issues I ran across, switching to the source-based version of our assertion library will not be trivial for them (because our source does not conform to their source rules, and I have no idea whether it's even possible to exclude source-via-NuGet from getting the rules run against it). As such, I am not able to effectively create a PR.

I don't have a good recommendation, either. If their rules cannot be excluded from xUnit.net's source, then likely they'll need to stop using our assertion library outright (or perhaps fork it, and rewrite the bits they need to conform to their coding style).

@Shane32
Copy link

Shane32 commented Sep 5, 2023

I don't have a good recommendation, either. If their rules cannot be excluded from xUnit.net's source, then likely they'll need to stop using our assertion library outright (or perhaps fork it, and rewrite the bits they need to conform to their coding style).

The xUnit.net source-only nuget package should probably have // <auto-generated /> added to the top of every file so that rules and analysis is ignored on these files during compilation. I believe an alternative is to name the files .g.cs. I did a quick test with the Nullability.Source library and it seemed to work, where after adding the comment, compiler warnings that were configured as errors were now ignored by the compiler.

@bradwilson
Copy link
Author

@Shane32 Are the rules about this written down somewhere? I wouldn't mind modifying the source to add headers to fix the issue if that's what it takes. It would certainly make it more widely usable.

@Shane32
Copy link

Shane32 commented Sep 5, 2023

I don't know. See: DotNetAnalyzers/StyleCopAnalyzers#3299

@Shane32
Copy link

Shane32 commented Sep 5, 2023

It does seem to make sense, or else source generation (such as System.Text.Json AOT support) would likely break compilation for many repositories where the generated files did not match the rules configured for the project.

@gao-artur
Copy link

The problem with annotating all files with // <auto-generated /> is that it will also disable all the rules in the source repo. Alternatively, you can drop the .editorconfig file with the following content into the nuget when packing source files

[*]
generated_code = true

I just tried adding it to the %userprofile%\.nuget\packages\xunit.assert.source\2.5.0\contentFiles folder and it worked.
Before
image

After
image

@bradwilson
Copy link
Author

@gao-artur That sounds like a simpler solution than adding the headers before packing. 😄

@bradwilson
Copy link
Author

By applying the changes suggested by @gao-artur (and fixing up a couple issues that popped up as a result), I'm able to get the following changes to build Microsoft.CodeAnalysis.Testing.Verifiers.XUnit, and the changes are fairly minimal. This will remove the binary dependency and un-break xUnit.net users post 2.4.2.


There remains compiler errors from Roslyn.SDK.IntegrationTests. First, filenames have been renamed:

CSC : error CS2001: Source file 'C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\..\..\..\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\EqualWithMessageException.cs' could not be found. [C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\Roslyn.SDK.IntegrationTests.csproj]
CSC : error CS2001: Source file 'C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\..\..\..\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\EmptyWithMessageException.cs' could not be found. [C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\Roslyn.SDK.IntegrationTests.csproj]
CSC : error CS2001: Source file 'C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\..\..\..\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\NotEmptyWithMessageException.cs' could not be found. [C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\Roslyn.SDK.IntegrationTests.csproj]

Fixing up the filenames causes a host of incompatibility errors from the conflicts from the xunit.assert that's being imported here vs. xunit.assert.source that's required to make types like XUnitVerifier compile properly. One example out of a dozen or so:

C:\Dev\roslyn-sdk\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\NotEmptyException.cs(14,23): error CS0436: The type 'NotEmptyException' in 'C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\..\..\..\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\NotEmptyException.cs' conflicts with the imported type 'NotEmptyException' in 'xunit.assert, Version=2.4.2.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c'. Using the type defined in 'C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\..\..\..\src\Microsoft.CodeAnalysis.Testing\Microsoft.CodeAnalysis.Testing.Verifiers.XUnit\NotEmptyException.cs'. [C:\Dev\roslyn-sdk\tests\VisualStudio.Roslyn.SDK\Roslyn.SDK.IntegrationTests\Roslyn.SDK.IntegrationTests.csproj]

It's at this point that I bow out. The build magic that's being applied here (starting with being unable to work inside of Visual Studio, through "magic" imports and variables like $(XUnitVersion), which has no obvious definition) tells me to leave this to the experts like @sharwell.

I will leave the fork available, but I will not be opening a PR to fix this issue.

Although the Roslyn team should wait for a 2.5.1 RTM before doing any final merges, I strongly urge you to attempt to validate your fix with a pre-release build, in the event that you find any further issues that I hadn't uncovered which may require any further changes to xUnit.net. As always, there are no guarantee of any future 2.x releases for xUnit.net, so any breaking issues discovered after 2.5.1 ships may not be fixed for the 2.x tree. The time to verify this approach is now, or else you risk being stranded indefinitely on xUnit.net <= 2.4.2 (forcing any consumers of Microsoft.CodeAnalysis.Testing.Verifiers.XUnit to be stranded along with you).

b-straub pushed a commit to b-straub/DexieNET that referenced this issue Sep 8, 2023
@elringus
Copy link

I'm using Microsoft.CodeAnalysis.CSharp.SourceGenerators.Testing.XUnit v1.1.2-beta1.23431.1 with xUnit 2.5.1 and have the same issue. Is downgrading to xUnit 2.4 is the only workaround for now?

@sharwell
Copy link
Member

sharwell commented Sep 18, 2023

@elringus you can also switch to Microsoft.CodeAnalysis.CSharp.SourceGenerators.Testing (remove the .XUnit suffix) and use DefaultVerifier instead of the xUnit 2.4 default XUnitVerifier. The only real difference is the exception type which is thrown. If you want to keep the same exception types, you can define a new class XUnitVerifier based on the source code for XUnitVerifier in this repository and use that.

@elringus
Copy link

@sharwell This works, thank you!

@AArnott
Copy link
Contributor

AArnott commented Sep 27, 2023

That workaround unblocked me on xunit 2.5.1 as well.

@jasonswearingen
Copy link

i was also bit by this. fix mentioned by sharwell also works for me.

akoeplinger added a commit to akoeplinger/runtime that referenced this issue Jan 19, 2024
Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.Xunit brings in an older xunit reference that is not compatible in some cases, we've hit issues with it while upgrading our xunit reference in the past.

As mentioned in dotnet/roslyn-sdk#1099 (comment) we can use the generic testing package instead.
The only difference is the exception type thrown when an assert fails, e.g. the test throws `InvalidOperationException` instead of `Xunit.Sdk.EqualException` but we still get actual vs. expected exception messages so this is enough for us.
ViktorHofer pushed a commit to dotnet/runtime that referenced this issue Jan 19, 2024
Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.Xunit brings in an older xunit reference that is not compatible in some cases, we've hit issues with it while upgrading our xunit reference in the past.

As mentioned in dotnet/roslyn-sdk#1099 (comment) we can use the generic testing package instead.
The only difference is the exception type thrown when an assert fails, e.g. the test throws `InvalidOperationException` instead of `Xunit.Sdk.EqualException` but we still get actual vs. expected exception messages so this is enough for us.
tmds pushed a commit to tmds/runtime that referenced this issue Jan 23, 2024
Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.Xunit brings in an older xunit reference that is not compatible in some cases, we've hit issues with it while upgrading our xunit reference in the past.

As mentioned in dotnet/roslyn-sdk#1099 (comment) we can use the generic testing package instead.
The only difference is the exception type thrown when an assert fails, e.g. the test throws `InvalidOperationException` instead of `Xunit.Sdk.EqualException` but we still get actual vs. expected exception messages so this is enough for us.
@MonstraG
Copy link

MonstraG commented Jan 26, 2024

What if I cannot follow the workaround because DefaultVerifier seems to lack Diagnostic()?

I basically have https://github.dev/dotnet/roslyn-sdk/samples/CSharp/Analyzers/Analyzers.Test/Tests/CodeBlockAnalyzerUnitTests.cs in my codebase

that is crashing with:

Method not found: 'Void Xunit.Sdk.EqualException..ctor(System.Object, System.Object)'.
   at Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException..ctor(Object expected, Object actual, String userMessage)
   at Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.Equal[T](T expected, T actual, String message) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/XUnitVerifier.cs:line 49
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticResults(IEnumerable`1 actualResults, ImmutableArray`1 analyzers, DiagnosticResult[] expectedResults, IVerifier verifier) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 360
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticsAsync(EvaluatedProjectState primaryProject, ImmutableArray`1 additionalProjects, DiagnosticResult[] expected, IVerifier verifier, CancellationToken cancellationToken) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 273
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.RunImplAsync(CancellationToken cancellationToken) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 207
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.RunAsync(CancellationToken cancellationToken) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 181
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_0.<<InvokeTestMethodAsync>b__1>d.MoveNext() in /_/src/xunit.execution/Sdk/Frameworks/Runners/TestInvoker.cs:line 285
--- End of stack trace from previous location ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in /_/src/xunit.execution/Sdk/Frameworks/ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in /_/src/xunit.core/Sdk/ExceptionAggregator.cs:line 90

csproj:

<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit" Version="1.1.1"/>
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.XUnit" Version="1.1.1"/>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" />
<PackageReference Include="xunit" Version="2.6.6" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.5.6">
    <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    <PrivateAssets>all</PrivateAssets>
</PackageReference>

@sharwell
Copy link
Member

What if I cannot follow the workaround because DefaultVerifier seems to lack Diagnostic()?

Diagnostic() is a member of CSharpAnalyzerVerifier<,>, so the IVerifier instance you choose has no impact on that.

@sharwell
Copy link
Member

@bradwilson I was reviewing today, and was not able to find a way to make xunit 2.5 compatible with our goals. Would it be possible to re-add the following method APIs to restore binary compatibility?

public class EmptyException
{
  [Obsolete($"Use '{nameof(ForNonEmptyCollection)}' instead.")]
  public EmptyException(IEnumerable collection)
    : this(
      string.Format(
        CultureInfo.CurrentCulture,
        "Assert.Empty() Failure: Collection was not empty{0}Collection: {1}",
        Environment.NewLine,
        CollectionTracker<object?>.FormatStart(Assert.GuardArgumentNotNull(nameof(collection), collection.Cast<object?>())))
  {
  }
}

public class NotEmptyException
{
  [Obsolete($"Use '{nameof(ForNonEmptyCollection)}' instead.")]
  public NotEmptyException()
    : this("Assert.NotEmpty() Failure: Collection was empty")
  {
  }
}

public class EqualException
{
  [Obsolete($"Use '{nameof(ForMismatchedValues)}' instead.")]
  public EqualException(object? expected, object? actual)
    : this(.. /* refactor ForMismatchedValuesWithError string generation */)
  {
  }

  [Obsolete($"Use '{nameof(ForMismatchedStrings)}' instead.")]
  public EqualException(string? expected, string? actual, int expectedIndex, int actualIndex)
    : this(.. /* refactor ForMismatchedStrings string generation */)
  {
  }
}

public class XunitException
{
  [Obsolete("No longer used.")]
  public string? UserMessage { get; protected set; }
}

@bradwilson
Copy link
Author

@sharwell You should not be taking a binary dependency against our assertion library. Either stop using our assertion exceptions, or import our assertions as source so you don't have a binary dependency.

@bradwilson
Copy link
Author

Use this as a starting point: #1099 (comment)

@sharwell
Copy link
Member

... Either stop using our assertion exceptions, or import our assertions as source ...

Both of these options will penalize users by making the framework not throw XunitException (at least not the one that could be caught by the user's code) when an assertion fails. The only value provided by the xunit packages produced by this repository comes from the binary dependency against the xunit assertion library.

@bradwilson
Copy link
Author

@sharwell The only code that should care what exceptions you throw is your tests, not everybody else's tests.

Anybody who's writing a test to catch and verify XunitException who isn't me is doing it wrong. 😂

@bradwilson
Copy link
Author

If I'm being blunt: the *.Xunit projects provide effectively zero benefit. Our test framework--like every test framework for .NET that I'm aware of--considers exceptions to be failures. All the code that matters is in Microsoft.CodeAnalysis.Analyzer.Testing and everything else is unnecessary window dressing.

I haven't looked at the MSTest and NUnit variants because I don't honestly care about what you did there, but I suspect they're just as empty as the Xunit variants are.

If I had my way, I'd deprecate all the test-framework-specific versions and point people to the base project.

jm2c

@sharwell
Copy link
Member

If I had my way, I'd deprecate all the test-framework-specific versions and point people to the base project.

OK, that was the plan if we couldn't figure out the compat issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-MS.CA.Testing Microsoft.CodeAnalysis.Testing
Projects
None yet
9 participants