SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
798 stars 229 forks source link

Fix S3433 FP: rule claims generic methods are not executed, but they are (NUnit) #9658

Closed BrightLight closed 2 months ago

BrightLight commented 2 months ago

Description

When using NUnit S3433 reports on generic test methods stating "Make this test method non-generic.". The rule description wrongfully explains (bold by me for emphasis):

A method is detected as test method if marked with one of the following attributes [TestMethod] or [DataTestMethod] (for mstest), [Fact] or [Theory] (for xunit) or [Test], [TestCase], [TestCaseSource] or [Theory] (for nunit). However, whether or not they have a test attribute, non-public methods are not recognized as tests, and therefore not executed. Neither are async void methods, or methods with generics anywhere in their signatures.

This is not true. NUnit does support generic test methods in combination with [TestCaseSource]. See also Sources for generic methods using TestCaseData.

As of NUnit 4.1, it is possible to explicitly specify the generic types to be used for a generic method. [...] When omitted, NUnit will infer the generic type arguments based on the passed values from the TestCaseSource.

Repro steps

A simple example that does work with NUnit (tested with NUnit 3.14.0 and 4.2.1):

public static IEnumerable<TestCaseData> SomeValues
{
  get
  {
    yield return new TestCaseData(10, typeof(int));
    yield return new TestCaseData(99L, typeof(long));
    yield return new TestCaseData(29.3M, typeof(decimal));
    yield return new TestCaseData(12.3F, typeof(float));
    yield return new TestCaseData(300D, typeof(double));
  }
}

// test method with generic argument TValue
[TestCaseSource(nameof(SomeValues))]
public void ValueTypeMatches<TValue>(TValue value, Type typeOfValue)
{
  Assert.That(value, Is.Not.Null);
  Assert.That(value, Is.InstanceOf<TValue>());
  Assert.That(typeof(TValue), Is.EqualTo(typeOfValue));
}

Expected behavior

No S3433 warning.

Actual behavior

S3433 flags this method stating "Make this test method non-generic."

Known workarounds

Deactivate rule S3433.

Related information

CristianAmbrosini commented 2 months ago

Hi @BrightLight! You are correct that NUnit supports generic test methods in combination with the [TestCaseSource] attribute. While the rule description could be enhanced for clarity, it should not trigger for generic test methods with NUnit [TestCaseSource]. I tested the snippet you provided and it did not raise any issue on my side.

Could you please confirm if you are indeed seeing S3433 being raised on your snippet? If not, could you provide a snippet that reproduce the false positive?

Cheers

BrightLight commented 2 months ago

Hello @CristianAmbrosini

apologies. I compared the provided example with the test in our codebase and noticed one crucial difference. In our codebase the test is annotated with both the TestAttribute and the TestCaseSourceAttribute, which is valid, but redundant. Like this:

// test method with generic argument TValue
[Test]
[TestCaseSource(nameof(SomeValues))]
public void ValueTypeMatches<TValue>(TValue value, Type typeOfValue)
{
  Assert.That(value, Is.Not.Null);
  Assert.That(value, Is.InstanceOf<TValue>());
  Assert.That(typeof(TValue), Is.EqualTo(typeOfValue));
}

Without the TestAttribute (just the TestCaseSourceAttribute) S3433 does not report. Which in our case means to fix the warning we can simply remove the [Test] attribute.

I've also now noted that at the very bottom of the rule description (section "Exceptions") it is mentioned that

[...] [TestCase] and [TestCaseSource] test methods in nunit can be generic.

I will add a remark about this special case to the rule description of our local SonarQube server. From my side this ticket can be closed (I'm not closing it myself in case you want to use it to improve the rule description or add handling in case both attributes are used). Thanks.