Add suppressor for CA1515

[CollinAlpert]

This PR adds a suppressor for CA1515 ("Make public types internal"), since test classes must be public.

Fixes dotnet/roslyn-analyzers#7192

[bradwilson]

I'm trying to work out how to unit test this, and I don't see how I can make CA1515 trigger inside of the existing test framework. Any ideas?

[CollinAlpert]

I tried as well and failed. Will give it another go later.

[bradwilson]

I can't make the sample code in the documentation trigger. Is this rule even enabled in .NET 8?

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <AnalysisLevel>latest-All</AnalysisLevel>
    <EnableNETAnalyzers>true</EnableNETAnalyzers>
    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
  </PropertyGroup>

</Project>

namespace Example;

public class Program
{
    public static void Main(string[] args)
    {
    }
}

I get CA1052 because Program isn't static, but that's it.

[CollinAlpert]

It's off by default. You need to opt-in explicitly via .editorconfig or RuleSet.

[CollinAlpert]

I still can't get this to work. The only remaining thing I can think of is to copy the analyzer to xUnit's test project.

Maybe @Youssef1313 can help, since he built a suppressor for EF Core, however that was for a compiler warning, not an analyzer shipped with the SDK.

[bradwilson]

It's off by default. You need to opt-in explicitly via .editorconfig or RuleSet.

I would've thought asking for all rules would've brought all rules. 😁

I did try using .editorconfig and it still doesn't trigger (still only seeing CA1052):

[*.cs]
dotnet_diagnostic.CA1515.severity = warning

[bradwilson]

I'm ready to use this to turn off CA2007, so I hope we can figure out how to test it. 😂

[CollinAlpert]

Don't worry, I'm not going to give up easily :)

It seems the analyzer did not make it into .NET 8, you can install this package version to get it to work.

[bradwilson]

I'll add that and see if I can get some tests that repro, because I've been restructuring the code in anticipation of getting more suppressors. 😄

[bradwilson]

My first try at a test was not successful. Here's the test, just trying to get it to trigger:

using System.Threading.Tasks;
using Xunit;
using Verify = CSharpVerifier<Microsoft.CodeAnalysis.Testing.EmptyDiagnosticAnalyzer>;

public class MakeTypesInternalSuppressorTests
{
	[Fact]
	public async Task OutsideOfTestClass_Triggers()
	{
		var code = @"public class NonTestClass { public static void Main() { } }";

		var expected = Verify.CompilerError("CA1515");

		await Verify.VerifyAnalyzer(code, expected);
	}
}

And I added the Microsoft.CodeAnalysis.NetAnalyzers package version 9.0.0-preview.24072.1 to the reference assemblies for all the tests:

diff --git a/src/xunit.analyzers.tests/Utility/CodeAnalyzerHelper.cs b/src/xunit.analyzers.tests/Utility/CodeAnalyzerHelper.cs
index 5e09e08..447ecf6 100644
--- a/src/xunit.analyzers.tests/Utility/CodeAnalyzerHelper.cs
+++ b/src/xunit.analyzers.tests/Utility/CodeAnalyzerHelper.cs
@@ -34,6 +34,7 @@ static CodeAnalyzerHelper()
 
 		CurrentXunitV2 = defaultAssemblies.AddPackages(
 			ImmutableArray.Create(
+				new PackageIdentity("Microsoft.CodeAnalysis.NetAnalyzers", "9.0.0-preview.24072.1"),
 				new PackageIdentity("Microsoft.Extensions.Primitives", "8.0.0"),
 				new PackageIdentity("System.Collections.Immutable", "1.6.0"),
 				new PackageIdentity("System.Threading.Tasks.Extensions", "4.5.4"),
@@ -45,6 +46,7 @@ static CodeAnalyzerHelper()
 
 		CurrentXunitV2RunnerUtility = defaultAssemblies.AddPackages(
 			ImmutableArray.Create(
+				new PackageIdentity("Microsoft.CodeAnalysis.NetAnalyzers", "9.0.0-preview.24072.1"),
 				new PackageIdentity("Microsoft.Extensions.Primitives", "8.0.0"),
 				new PackageIdentity("System.Collections.Immutable", "1.6.0"),
 				new PackageIdentity("System.Threading.Tasks.Extensions", "4.5.4"),
@@ -56,6 +58,7 @@ static CodeAnalyzerHelper()
 		CurrentXunitV3 = defaultAssemblies.AddPackages(
 			ImmutableArray.Create(
 				new PackageIdentity("Microsoft.Bcl.AsyncInterfaces", "8.0.0"),
+				new PackageIdentity("Microsoft.CodeAnalysis.NetAnalyzers", "9.0.0-preview.24072.1"),
 				new PackageIdentity("Microsoft.Extensions.Primitives", "8.0.0"),
 				new PackageIdentity("System.Threading.Tasks.Extensions", "4.5.4"),
 				new PackageIdentity("System.Text.Json", "8.0.0"),
@@ -68,6 +71,7 @@ static CodeAnalyzerHelper()
 		CurrentXunitV3RunnerUtility = defaultAssemblies.AddPackages(
 			ImmutableArray.Create(
 				new PackageIdentity("Microsoft.Bcl.AsyncInterfaces", "8.0.0"),
+				new PackageIdentity("Microsoft.CodeAnalysis.NetAnalyzers", "9.0.0-preview.24072.1"),
 				new PackageIdentity("Microsoft.Extensions.Primitives", "8.0.0"),
 				new PackageIdentity("System.Threading.Tasks.Extensions", "4.5.4"),
 				new PackageIdentity("System.Text.Json", "8.0.0"),

[bradwilson]

I suspect there's something fundamentally missing here. Maybe the NuGet package relies on some MSBuild magic to get enabled that isn't happening when using the Roslyn testing framework. (Presumably because this isn't a library, but an analyzer.)

[bradwilson]

It's also not clear to me yet how I can inject <AnalysisLevel>latest-All</AnalysisLevel> as a property into the build process for the testing framework.

[bradwilson]

Maybe @sharwell has some thoughts about testing suppressors and getting all of this stuff working. 🤞🏼

[CollinAlpert]

I got this to work now, but the solution is extremely hacky.

I added a dependency to Microsoft.CodeAnalysis.NetAnalyzers and Microsoft.CodeAnalysis.Workspaces.Common (because Microsoft.CodeAnalysis.NetAnalyzers depends on it) to the test project to ensure the assemblies are in the NuGet cache. Then I load the assemblies the Roslyn Analyzer is in via reflection and add it to the custom CSharpCodeFixTest. Of course this will not work if someone has a custom NuGet folder.

Apart from that the tests are relatively straightforward. A small note in regards to the WithIsSuppressed method: it accepts a tri-state boolean; null meaning the suppresion state is not tested, so simply omitting WithIsSuppressed to test that a diagnostic is not suppressed does not work.

[CollinAlpert]

Oh, another thing is the test can't run on anything but .NET (Core), due to AssemblyLoadContext being used to merge the dependent assemblies. If you know a different way to load the dependencies, so the tests would be able to run in net472 as well, that would be great.

Other than that I just want to say that of course this is just a initial proof of concept. If you are OK with this approach, and I could completely understand if you are not, I would still need to do some cleanup and performance improvements.

[bradwilson]

I'll take a look at the tests and let you know. As I mentioned previously I was planning to change the main code anyway because I'm extracting a base class so I can write more suppressors.

[bradwilson]

Yep, I can make this work. 👍🏼

[bradwilson]

I made a bunch of cleanup and prep in main so that this PR could look much more focused, and I pushed things into re-usability as much as possible. The end result is the suppressor is quite compact in code, and the tests are as well (and they now run against both xUnit.net v2 and v3).

There is a bit of conditional code in the test because Roslyn 3.11 handles suppression differently than Roslyn 4.x (the former leaves the diagnostic in the list, but marked as suppressed; the latter removes suppressed diagnostics entirely from the output).

Let me know if you have any comments on what I've done, but I otherwise consider this ready to merge.

[bradwilson]

If you know a different way to load the dependencies, so the tests would be able to run in net472 as well, that would be great.

I do, but I'm not sure it's worth it.

Essentially the kind of load you need to do (to ensure you end up in the default context) is complicated by the fact that the assembly isn't in the output folder (we used <PacakgeDownload> rather than <PackageReference> since the latter conflicts with all of our Roslyn 4.x test projects because of the 3.11 version of Microsoft.CodeAnalysis.Workspaces.Common that is required as the dependency for Microsoft.CodeAnalysis.NetAnalyzers).

You set up an assembly resolver hook on the AppDomain and then attempt to load the assembly by name (rather than loading from file), and the resolver hook can go find the assembly in the NuGet cache instead of depending on being locally on disk.

This is fundamentally an operation that xUnit.net already has to do to support testing on .NET Framework, and it's janky code that I don't want to bring over here if I can help it. I'm going to rely on just hand-testing this with .NET Framework and let the unit tests on .NET run as standard coverage. I might reconsider it if a large number of issues arise from suppressors on .NET Framework, but I don't actually expect that so I'm comfortable with this risk.

[CollinAlpert]

Looks great! I like the architecture a lot.

[bradwilson]

With 1.12.0-pre.4, prior to this change:

With 1.12.0-pre.5, including this change:

🎉

[bradwilson]

It's also probably worth mentioning that you can pick up new xunit.analyzers without having to upgrade xunit, since the version that xunit brings in is a >= version. https://xunit.net/docs/using-ci-builds#newer

[CollinAlpert]

This is awesome!

[bradwilson]

Next up, CA2007. 😉

[CollinAlpert]

Are you looking to suppress that in the xunit codebase or to suppress it for unit tests? Would you like me to contribute that suppressor as well?

If not, please tag me in the PR/commit, I'd be interested in the result.

[bradwilson]

Suppress it for test methods (since calling .ConfigureAwait(false) in a test method throws you off our run-limiting thread pool). But I've already beat you to it, I'm afraid. Just waiting for the build now. 2aca2ae

[CollinAlpert]

Very nice! I'm glad this works so well.

[bradwilson]

I definitely appreciate you digging in and figuring out how to test it all. It makes sense in retrospect now that I see the technique, but I doubt this would've ever occurred to me!

[CollinAlpert]

Yeah, I'm also still a bit perplexed that I managed to figure it out. I had a lot of fun though :)