Simplify the ValuesAttribute (#758)

* Close #755: Simplify the ValuesAttribute * Replace the expression-bodied property FixableDiagnosticIds with a read-only property with an initializer * Simplify AnalyzeParameter in SimplifyValuesAnalyzer * Rephrase SimplifyValuesDescription * Apply suggestions from code review Co-authored-by: Manfred Brands <manfred-brands@users.noreply.github.com> --------- Co-authored-by: Manfred Brands <manfred-brands@users.noreply.github.com>
nunit · Jun 20, 2024 · 16e159c · 16e159c
1 parent 8e5a34b
commit 16e159c
Show file tree

Hide file tree

Showing 9 changed files with 706 additions and 0 deletions.
diff --git a/documentation/NUnit4001.md b/documentation/NUnit4001.md
@@ -0,0 +1,78 @@
+# NUnit4001
+
+## Simplify the Values attribute
+
+| Topic    | Value
+| :--      | :--
+| Id       | NUnit4001
+| Severity | Info
+| Enabled  | True
+| Category | Style
+| Code     | [SimplifyValuesAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/SimplifyValues/SimplifyValuesAnalyzer.cs)
+
+## Description
+
+Consider removing unnecessary parameters from the ValuesAttribute.
+
+## Motivation
+
+When used without any arguments, the [Values] attribute on a (nullable) boolean or an (nullable) enum parameter
+will automatically include all possible values.
+
+Therefore the `Values` attribute like
+
+```csharp
+[Test]
+public void MyBoolTest([Values(true, false)] bool value) { /* ... */ } 
+```
+
+can be simplified to
+
+```csharp
+[Test]
+public void MyBoolTest([Values] bool value) { /* ... */ } 
+```
+
+## How to fix violations
+
+Remove all arguments of the `Values` attribute.
+
+<!-- start generated config severity -->
+## Configure severity
+
+### Via ruleset file
+
+Configure the severity per project, for more info see
+[MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022).
+
+### Via .editorconfig file
+
+```ini
+# NUnit4001: Simplify the Values attribute
+dotnet_diagnostic.NUnit4001.severity = chosenSeverity
+```
+
+where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`.
+
+### Via #pragma directive
+
+```csharp
+#pragma warning disable NUnit4001 // Simplify the Values attribute
+Code violating the rule here
+#pragma warning restore NUnit4001 // Simplify the Values attribute
+```
+
+Or put this at the top of the file to disable all instances.
+
+```csharp
+#pragma warning disable NUnit4001 // Simplify the Values attribute
+```
+
+### Via attribute `[SuppressMessage]`
+
+```csharp
+[System.Diagnostics.CodeAnalysis.SuppressMessage("Style",
+    "NUnit4001:Simplify the Values attribute",
+    Justification = "Reason...")]
+```
+<!-- end generated config severity -->
diff --git a/documentation/index.md b/documentation/index.md
@@ -120,3 +120,11 @@ builds (version 3.0.0 and above) which require Visual Studio 2019 (version 16.3)
 | [NUnit3002](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3002.md) | Field/Property is initialized in SetUp or OneTimeSetUp method | :white_check_mark: | :information_source: | :x: |
 | [NUnit3003](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3003.md) | Class is an NUnit TestFixture and is instantiated using reflection | :white_check_mark: | :information_source: | :x: |
 | [NUnit3004](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit3004.md) | Field should be Disposed in TearDown or OneTimeTearDown method | :white_check_mark: | :information_source: | :x: |
+
+## Style Rules (NUnit4001 - )
+
+Rules which help you write concise and readable NUnit test code.
+
+| Id       | Title       | :mag: | :memo: | :bulb: |
+| :--      | :--         | :--:  | :--:   | :--:   |
+| [NUnit4001](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit4001.md) | Simplify the Values attribute | :white_check_mark: | :information_source: | :x: |
diff --git a/src/nunit.analyzers.tests/SimplifyValues/SimplifyValuesAnalyzerTests.cs b/src/nunit.analyzers.tests/SimplifyValues/SimplifyValuesAnalyzerTests.cs
@@ -0,0 +1,253 @@
+using System.Globalization;
+using Gu.Roslyn.Asserts;
+using Microsoft.CodeAnalysis;
+using NUnit.Analyzers.Constants;
+using NUnit.Analyzers.SimplifyValues;
+using NUnit.Framework;
+
+namespace NUnit.Analyzers.Tests.SimplifyValues;
+
+public class SimplifyValuesAnalyzerTests
+{
+    private readonly SimplifyValuesAnalyzer analyzer = new();
+
+    [Test]
+    public void VerifySupportedDiagnostics()
+    {
+        var diagnostics = this.analyzer.SupportedDiagnostics;
+
+        Assert.That(diagnostics, Has.Length.EqualTo(1));
+        var diagnostic = diagnostics[0];
+        Assert.Multiple(() =>
+        {
+            Assert.That(diagnostic.Id, Is.EqualTo(AnalyzerIdentifiers.SimplifyValues));
+            Assert.That(diagnostic.Title.ToString(CultureInfo.InvariantCulture), Is.Not.Empty);
+            Assert.That(diagnostic.Category, Is.EqualTo(Categories.Style));
+            Assert.That(diagnostic.DefaultSeverity, Is.EqualTo(DiagnosticSeverity.Info));
+        });
+    }
+
+    [Test]
+    public void AnalyzeWhenAttributeIsNotInNUnit()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenAttributeIsNotInNUnit
+    {
+        [Test]
+        public void ATest([Values] bool b) { }
+
+        private sealed class ValuesAttribute : Attribute
+        { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenCombinatorialStrategyIsNotUsed(
+        [Values("Sequential", "Pairwise")] string nonCombinatorialAttribute,
+        [Values] bool fullyQualify,
+        [Values] bool omitAttribute)
+    {
+        var prefix = fullyQualify ? "NUnit.Framework." : string.Empty;
+        var suffix = omitAttribute ? string.Empty : "Attribute";
+        var attribute = $"{prefix}{nonCombinatorialAttribute}{suffix}";
+
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@"
+    public class AnalyzeWhenCombinatorialStrategyIsNotUsed
+    {{
+        public enum TestEnum {{ A, B, C }}
+
+        [Test]
+        [{attribute}]
+        public void Test([Values(TestEnum.A, TestEnum.B, TestEnum.C)] TestEnum e) {{ }}
+    }}");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenAttributeHasNoArguments()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenAttributeHasNoArguments
+    {
+        [Test]
+        public void ATest([Values] bool b) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenOneBooleanWasUsed()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenOneBooleanWasUsed
+    {
+        [Test]
+        public void ATest([Values(true)] bool b) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenTwoBooleanWereUsedForNullableBoolean()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenTwoBooleanWereUsedForNullableBoolean
+    {
+        [Test]
+        public void ATest([Values(true, false)] bool? b) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenNotAllEnumValuesWereUsed()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenNotAllEnumValuesWereUsed
+    {
+        public enum TestEnum { A, B, C }
+
+        [Test]
+        public void Test([Values(TestEnum.A, TestEnum.B)] TestEnum e) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenNotAllEnumValuesWereUsedForNullableBoolean()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenNotAllEnumValuesWereUsedForNullableBoolean
+    {
+        public enum TestEnum { A, B, C }
+
+        [Test]
+        public void Test([Values(TestEnum.A, TestEnum.B, TestEnum.C)] TestEnum? e) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeNotAllBooleanValuesAreInParameters()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeNotAllBooleanValuesAreInParameters
+    {
+        public void Test([Values(new object[] { true })] bool b) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeNotAllNullableBooleanValuesAreInParameters()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeNotAllNullableBooleanValuesAreInParameters
+    {
+        public void Test([Values(new object[] { true, false })] bool? b) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeNotAllEnumValuesAreInParameters()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeNotAllEnumValuesAreInParameters
+    {
+        public enum TestEnum { A, B, C }
+
+        public void Test([Values(new object[] { TestEnum.A, TestEnum.B })] TestEnum e) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeNotAllNullableEnumValuesAreInParameters()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeNotAllNullableEnumValuesAreInParameters
+    {
+        public enum TestEnum { A, B, C }
+
+        public void Test([Values(new object[] { TestEnum.A, TestEnum.B, TestEnum.C })] TestEnum? e) { }
+    }");
+        RoslynAssert.Valid(this.analyzer, testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenAllBooleanValuesWereUsed()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenAllBooleanValuesWereUsed
+    {
+        [Test]
+        public void Test([↓Values(true, false)] bool b) { }
+    }");
+        RoslynAssert.Diagnostics(this.analyzer,
+            ExpectedDiagnostic.Create(AnalyzerIdentifiers.SimplifyValues),
+            testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenAllNullableBooleanValuesWereUsed()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenAllNullableBooleanValuesWereUsed
+    {
+        [Test]
+        public void Test([↓Values(true, false, null)] bool? b) { }
+    }");
+        RoslynAssert.Diagnostics(this.analyzer,
+            ExpectedDiagnostic.Create(AnalyzerIdentifiers.SimplifyValues),
+            testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenAllEnumValuesWereUsed()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenAllEnumValuesWereUsed
+    {
+        public enum TestEnum { A, B, C }
+
+        [Test]
+        public void Test([↓Values(TestEnum.A, TestEnum.B, TestEnum.C)] TestEnum e) { }
+    }");
+        RoslynAssert.Diagnostics(this.analyzer,
+            ExpectedDiagnostic.Create(AnalyzerIdentifiers.SimplifyValues),
+            testCode);
+    }
+
+    [Test]
+    public void AnalyzeWhenAllNullableEnumValuesWereUsed()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeWhenAllNullableEnumValuesWereUsed
+    {
+        public enum TestEnum { A, B, C }
+
+        [Test]
+        public void Test([↓Values(TestEnum.A, TestEnum.B, TestEnum.C, null)] TestEnum? e) { }
+    }");
+        RoslynAssert.Diagnostics(this.analyzer,
+            ExpectedDiagnostic.Create(AnalyzerIdentifiers.SimplifyValues),
+            testCode);
+    }
+
+    [Test]
+    public void AnalyzeAllEnumValuesAreInParameters()
+    {
+        var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+    public class AnalyzeAllEnumValuesAreInParameters
+    {
+        public enum TestEnum { A, B, C }
+
+        public void Test([↓Values(new object[] { TestEnum.A, TestEnum.B, TestEnum.C })] TestEnum e) { }
+    }");
+        RoslynAssert.Diagnostics(this.analyzer,
+            ExpectedDiagnostic.Create(AnalyzerIdentifiers.SimplifyValues),
+            testCode);
+    }
+}