Disable VSTHRD200 for test methods

[cremor]

Is your feature request related to a problem? Please describe.

Test methods should not trigger VSTHRD200. But I also don't want to disable the rule for the whole test project like suggested in the recommended .editorconfig files.

Describe the solution you'd like

VSTHRD200 should ignore methods that have any of the following attributes applied:

• [TestMethod] (MSTest)
• [Fact] (xUnit)
• [Test] (NUnit)
• [Theory] (xUnit and NUnit)

When this is implemented the recommended .editorconfig files should be updated too.

Describe alternatives you've considered

Disabling VSTHRD200 for the whole test project, which I don't want.

[zdenek-jelinek]

Disabling VSTHRD200 for the whole test project, which I don't want.

There's a less strict option to only disable it in specific files. For example, if a convention is that all files containing tests end with Test.cs, an .editorconfig rule can be set up as follows:

[*Test.cs]
dotnet_diagnostic.VSTHRD200.severity = none

This still has the issue that some methods may not be subject to the rule since not all methods in test files are test methods, but at least is not as bad as disabling it for entire project.

Maybe the recommended test project .editorconfig file could be updated with this?

[cremor]

MSTest added a diagnostic suppressor for this rule in its last release, see microsoft/testfx#2926

While this is nice, it shouldn't be necessary for each test framework to add a diagnostic suppressor. So I still think this issue should be fixed here.

[AArnott]

While this is nice, it shouldn't be necessary for each test framework to add a diagnostic suppressor. So I still think this issue should be fixed here.

Interesting. I've never seen a diagnostic suppressor before.

As for fixing it here or there, it's a toss-up. Either vs-threading has to hard-code knowledge of every test framework's attribute names, or each test framework has to hard-code knowledge of our analyzer in order to suppress it in some way. I don't know of a generally agreed upon precedent for resolving who should make the change.
At the end of the day, the analyzer is doing what it was activated to do. The fact that you don't want to require that policy for specific methods in specific files is totally reasonable, but feels to me like it's a special case that may not necessarily merit a fix to the core analyzer.

At the moment though, sure I could go either way. And it would be less work for one location to account for all the (known) test frameworks than for each test framework to make their own changes to suppress the analyzer. It's just a matter of time and which teams have it, because at the moment, this issue has mostly been left active because it's a reasonable proposal but we don't have time to fund it yet.

[bradwilson]

We now have 3 suppressors (CA1515, CA2007, and as of today in a pre-release build, VSTHRD200).

I definitely agree that there is zero end-user benefit to adding Async onto the end of their test names, and counter-benefit when it comes to reporting. I feel like this is on us when we violate (and/or suggest users violate) the norms to add suppressors when the rule when it's not appropriate.

I don't feel like it should be the responsibility for vs-threading to look for exceptional situations across the universe of code; even if they wanted to, they shouldn't be responsible for determining the nuance of the implementation. Just to pick on us, you could look explicitly for [Fact] and [Theory], but also that's an extension point, and our rules sometimes apply to all derived attributes and sometimes only to direct usage of Fact and Theory. It really depends on the analyzer. I don't want you to have to make that decision, or even know that that's a decision that needs to be made.

As such, if I had a vote, I'd say you should close this.

If you do, @cremor may want to open up an issue against NUnit, assuming they don't have a suppressor for this.

[bradwilson]

Also, if you're interested, the implementation of the suppressor is actually pretty straight forward, as is the test:

https://github.com/xunit/xunit.analyzers/blob/main/src/xunit.analyzers/Suppressors/UseAsyncSuffixForAsyncMethodsSuppressor.cs

https://github.com/xunit/xunit.analyzers/blob/main/src/xunit.analyzers.tests/Suppressors/UseAsyncSuffixForAsyncMethodsSuppressorTests.cs

[cremor]

I don't feel like it should be the responsibility for vs-threading to look for exceptional situations across the universe of code; even if they wanted to, they shouldn't be responsible for determining the nuance of the implementation. Just to pick on us, you could look explicitly for [Fact] and [Theory], but also that's an extension point, and our rules sometimes apply to all derived attributes and sometimes only to direct usage of Fact and Theory. It really depends on the analyzer. I don't want you to have to make that decision, or even know that that's a decision that needs to be made.

As such, if I had a vote, I'd say you should close this.

Good point, I'll close this issue.
And thanks for adding the suppressor to xUnit!