CA1515 should ignore test projects

[martincostello]

Analyzer

Diagnostic ID: CA1515

Describe the improvement

Test projects often contain infrastructure code, such as fixtures and input for data driven tests, that are used with unit test classes/methods. These types are often public because test frameworks require the test classes and methods to be public, so by extension classes used as fixtures for these classes also need to be public. Even test classes are hit by this rule, with static test classes raising the warning, which changing would break.

Describe suggestions on how to achieve the rule

Test projects, however such determination is made, should be automatically excluded from the rule. Alternatively, exclude any type exposed by the public API surface of any class that contains at least one test method (e.g. an xunit [Theory] or ``Fact], NUnit [Test]` etc).

Additional context

I typically just enable all rules in my projects by default and opt-out of them where necessary, but with .NET 9 preview 1 I have found that the rule generates a lot of noise to the point where I've just unconditionally disabled it everywhere.

Trying to fix things where I've tried so far just leads down a spiralling path of further changes, most of which is either incorrect as it breaks code, such as the test scenario, or just creates suppression noise:

On one project this lead to this sort of chain of events:

1. Make types internal
2. CA1812 fires because types activated through DI now appear to be unused
3. Tests can't compile because types are no longer public
4. Add internals visible to
5. xunit fails to work properly because types used for tests are no longer public

CA1515 feels far too noisy to be useful at this stage based on its "anything public if it's not a library" heuristic.

[CollinAlpert]

I agree with this, however this is an xunit specific problem, since NUnit allows test classes to be internal.

[CollinAlpert]

@martincostello This is now implemented in xUnit, meaning xUnit will suppress CA1515 in test classes. Can you verify this works for you?

[martincostello]

Sure, I'll take a look when xunit next gets updated in my repos.

[ChrisTorng]

I have never seen CA1515. But now it shows on all test classes of MSTest V2(?).
Microsoft.NET.Test.Sdk 17.9.0
MSTest.TestAdapter 3.3.1
MSTest.TestFramework 3.3.1
This problem seems to start from upgrading to VS 17.12.0 Preview 1.0.

[JohanRoss]

I have got the same issue as @ChrisTorng above here, with MSTest (in Visual Studio 17.12.0 preview 3). So it is now not only limited to xUnit or NUnit.

[Thieum]

It should ignore APIControllers, Blazor pages, signalR hub derived classes, attributes derived from ActionFilterAttribute and Function App classes at least in .net8 too afaik.

[Daeymon]

It should ignore APIControllers, Blazor pages, signalR hub derived classes, attributes derived from ActionFilterAttribute and Function App classes at least in .net8 too afaik.

Also all classes or enums which are associated with a Blazor component parameter, which also have to be public.

[rcdailey]

I just want to add that this analysis warning in general is poor since it can't detect usages outside of a project via DI or any other runtime use case. I ended up having to silence this warning in my global .editorconfig via:

[*.cs]
# Consider making public types internal
dotnet_diagnostic.CA1515.severity = none

[Youssef1313]

This applies to MSTest as well.

[dnperfors]

Even though xunit.analyzers tries to suppress this warning in for test classes, it looks like the refactor suggestion will still perform the refactoring. I think I will have to disable this warning as well 👎

[Youssef1313]

Even though xunit.analyzers tries to suppress this warning in for test classes, it looks like the refactor suggestion will still perform the refactoring.

Not sure if that's expected. Consider opening an issue on dotnet/roslyn in case it's a bug.

cc @CyrusNajmabadi @sharwell