MSTEST0004 contradicts MSTEST0002 for nested test classes

[cbersch]

Describe the bug

The analyzer rule MSTEST0004 (PublicTypeShouldBeTestClassAnalyzer) contradicts MSTEST0002 (Test classes should have valid layout). for nested classes.

Consider the following code, which warns with MSTEST0004 for the TopLevelClass class

public sealed class TopLevelClass
{
    [TestClass]
    public sealed class Constructor
    {
        [TestMethod]
        public void TestMethod() { }
    }
}

Expected behavior

The code does not raise an MSTEST0004 analyzer violation

Actual behavior

The code raises an MSTEST0004 analyzer violation.
When changing the TopLevelClass to internal, I get an MSTEST0002 and MSTEST0003 warning, which is correct.

In my opinion, the MSTEST0004 should be raised only if the respective class does not contain nested test classes.

[Evangelink]

It's not entirely clear to me. What's the reasoning behind having the outer class if that's not a test class? You also don't get any warning if you put the outer class as [TestClass].

[cbersch]

The reason for having the outer class is for grouping purpose. So I can have the outer class named according to the class under tests, and then the inner class for the unit of work or method name.

If I put a [TestClass] on the outer class as well, then the MSTEST0004 warning disappears, but SonarQube complains about an empty test class. (This could also be a potential analyzer for the MSTest.Analyzer package).

[Evangelink]

but SonarQube complains about an empty test class. (This could also be a potential analyzer for the MSTest.Analyzer package).

Yes we have some plan to raise on empty test class. I was wondering from what side to tackle the issue and I am wondering if the test class should not be empty rule should be handling this case rather than the rule about public class having to handle this case.

Let me bring that discussion with the team and I'll report our preference.

[Evangelink]

After a few discussions, we will not change the current rules but we will take this case into account when implementing the rule about empty test class.

Our reasoning is that MSTEST0004 is disabled by default and that complying to the rule is not resulting in some invalid state. It is indeed causing Sonar .NET to raise an issue about another rule but we believe this rule should be handling this case rather than the other way around.