Nunit1007 - ignore [Test] attribute when [TestCaseSource] is present

[pyrocumulus]

Consider the following sample:

public static IEnumerable MyTestSource
{
    get
    {
        yield return new TestCaseData(1).Returns(true);
        yield return new TestCaseData(0).Returns(false);
    }
}

[Test]
[TestCaseSource(typeof(MyClass), nameof(MyTestSource))]
public bool Value_ShouldEqual_One(int value)
{
    return value == 1;
}

The above code generates the following error:
NUnit1007 "Method has non-void return type 'bool', but no result is expected in ExpectedResult"

Problem
But this is a false positive in this scenario. The ExpectedResult is actually provided through the TestCaseSource but the analyzer only looks at the [Test] attribute.

Possible solution
The NUnit1007 analyzer could just ignore the [Test] attribute when a [TestCaseSource] is present.

An alternative would be to raise an error when you add both [Test] and [TestCaseSource] but that seems overly restrictive because there's nothing really wrong with it (as I see it).

[mikkelbu]

I think you are right regarding ignoring the [Test] attribute when a [TestCaseSource] is present. But I think we should also have a "warning" or "info" if one combines a [Test] attribute and a [TestCaseSource] (or [TestCase]) attribute. I'll create another issue for this.

[siprbaum]

@mikkelbu I completely agree with your reasoning that [Test] should be ignored if [TestCaseSource] is present.

Furthermore, I want to hint that the same reasing applies to the [TestCase], as it also makes the [Test] attribute redundant.

What I would not like to have is a Warning message about cases where both attributes are specified. From nunits point of view it is perfectly fine to have them both specified, so IMHO nothing which warants warning.

Maybe an Info message, but even that I would probably deactivate, considering that the codebase (very large codebase, hundreds of developers) I am working with have these redundant occurences on tests in the range of several thousands.

It is still a small fraction on the overall test number, but nevertheless a huge code churn for just stylistic changes.

[pyrocumulus]

@siprbaum that was my initial reaction as well. From Nunit's perspective there is nothing wrong with supplying both, it's just unnecessary.

So in my opinion both Error and Warning are too much in that regard. I could work with Information but I do think that it would be best to make it an optional one. Some code bases are full of both and do work fine.

[mikkelbu]

Thanks for the feedback and the initial report. You are both right that it should be an optional diagnostic (with Info level). Then the user can activate it locally - and also tweak the severity so it fits their setup.