Runtime: Enable xunit analyzers for test projects

[ViktorHofer]

We want to enable xunit analyzers in our test projects but currently don't have enough resources to do so. I'm opening this issue as I'm sure that some our beloved community members would want to help.

EDIT: updated the initial post, see https://github.com/dotnet/corefx/issues/38677#issuecomment-504024301

Common patterns

1. Unused method parameters with discards aren't allowed. Instead use the expected parameter name and assign it to a discard in the method body:

// Not used by this test
_ = overflow;

cc @dotnet/corefx-contrib @safern

[Wraith2]

Do the sqlclient tests need updating even after the branch to dotnet/SqlClient If they do you might as well assign it to me.

[Clockwork-Muse]

For old times' sake, I'll take System.Linq.Parallel

[Gnbrkm41]

Would like to grab System.Memory.Tests.

[Gnbrkm41]

Should various test helpers be turned into internal methods? It looks like the analyzer doesn't like any public methods that aren't marked with Fact/Theory attributes. One of the the code fix suggested is to turn them internal.

Severity    Code    Description Project File    Line    Suppression State
Error   xUnit1013   Public method 'InitalizeBytes' on test class 'Base64TestHelper' should be marked as a Theory.   System.Memory.Tests C:\Users\gotos\Source\Repos\corefx\src\System.Memory\tests\Base64\Base64TestHelper.cs   68  Active

Also there's something to do with overloadable test methods (which also apparently includes object.ToString(), object.Equals()), which there is no way but to change the name:

Severity    Code    Description Project File    Line    Suppression State
Error   xUnit1024   Test method 'ToLower' on test class 'System.Tests.StringTests' has the same name as another method declared on class 'System.Tests.StringTests'.    System.Memory.Tests C:\Users\gotos\Source\Repos\corefx\src\Common\tests\Tests\System\StringTests.cs 5241    Active

[ViktorHofer]

After a discussion with @stephentoub we changed the direction here slightly. I submitted a PR which enables about 10 of the xunit analyzer rules and applies the fixes (manually) on all test projects in the repo.

I think it makes more sense to instead of a per project basis work on a per rule basis. Means to enable additional rules and work on the fixes across the repo: https://xunit.net/xunit.analyzers/rules/

@Gnbrkm41 it seems you already started with the work, I'm sorry for the lost effort. I would recommend to close your PR meanwhile until https://github.com/dotnet/corefx/pull/38676 goes in.

[Gnbrkm41]

As long as it's coming in some way, I'm happy with it 😄

[Gnbrkm41]

Will the most of the analysers be applied repo-wide eventually then? xUnit2000 (Consts/Literals should come at the expected argument) seemed like a nice addition to have.

[ViktorHofer]

Yes, that's the community ask. My PR enables rules repo wide. Additional rules would be great to be enabled by the community. I'll add a list in the top post when my PR is in.

[Gnbrkm41]

Right. Thanks! I'd give it a go after the PR goes in (would be great if you could write some simple guidance as well 😛 )

[Gnbrkm41]

Could I ask which rules have been enabled so far?

[stephentoub]

Could I ask which rules have been enabled so far?

These are the ones that are disabled, so... everything other than these:
https://github.com/dotnet/corefx/blob/b6edaf0ac1ca42b48fd6c836f9bb472090d832a8/src/CodeAnalysis.ruleset#L41-L82

[ViktorHofer]

I disabled these rules not because of a specific reason but to reduce the workload to initially enable the analyzer package. If you think that some of these rules are valuable you are very welcome to send a PR with the test changes.

[ViktorHofer]

I'm closing this meanwhile as there's no further work planned atm.