Add Analyzer/CodeFix for StringAssert

documentation/NUnit2048.md

@@ -0,0 +1,98 @@
+# NUnit2048
+
+## Consider using Assert.That(...) instead of StringAssert(...)
+
+| Topic    | Value
+| :--      | :--
+| Id       | NUnit2048
+| Severity | Warning
+| Enabled  | True
+| Category | Assertion
+| Code     | [StringAssertUsageAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/StringAssertUsage/StringAssertUsageAnalyzer.cs)
+
+## Description
+
+Consider using the constraint model, `Assert.That(actual, {0}(expected))`, instead of the classic model, `StringAssert.{1}(expected, actual)`.
+
+## Motivation
+
+The classic Assert model contains less flexibility than the constraint model and makes it easy to mix the `expected` and the `actual` parameter,
+so this analyzer marks usages of all `StringAssert` methods from the classic Assert model.
+
+```csharp
+[Test]
+public void Test()
+{
+    StringAssert.Contains(expected, actual);
+    StringAssert.DoesNotContain(expected, actual);
+    StringAssert.StartsWith(expected, actual);
+    StringAssert.DoesNotStartWith(expected, actual);
+    StringAssert.EndsWith(expected, actual);
+    StringAssert.DoesNotEndWith(expected, actual);
+    StringAssert.AreEqualIgnoreCase(expected, actual);
+    StringAssert.AreNotEqualIgnoreCase(expected, actual);
+    StringAssert.IsMatch(expected, actual);
+    StringAssert.DoesNotMatch(expected, actual);
+}
+```
+
+## How to fix violations
+
+The analyzer comes with a code fix that will replace `StringAssert.<method>(expected, actual)` with
+`Assert.That(actual, <constraint>(expected))`. So the code block above will be changed into.
+
+```csharp
+[Test]
+public void Test()
+{
+    Assert.That(actual, Does.Contain(expected));
+    Assert.That(actual, Does.Not.Contain(expected));
+    Assert.That(actual, Does.StartWith(expected));
+    Assert.That(actual, Does.Not.StartWith(expected));
+    Assert.That(actual, Does.EndWith(expected));
+    Assert.That(actual, Does.Not.EndWith(expected));
+    Assert.That(actual, Is.EqualTo(expected).IgnoreCase);
+    Assert.That(actual, Is.Not.EqualTo(expected).IgnoreCase);
+    Assert.That(actual, Does.Match(expected));
+    Assert.That(actual, Does.Not.Match(expected));
+}
+```
+
+<!-- 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
+# NUnit2048: Consider using Assert.That(...) instead of StringAssert(...)
+dotnet_diagnostic.NUnit2048.severity = chosenSeverity
+```
+
+where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`.
+
+### Via #pragma directive
+
+```csharp
+#pragma warning disable NUnit2048 // Consider using Assert.That(...) instead of StringAssert(...)
+Code violating the rule here
+#pragma warning restore NUnit2048 // Consider using Assert.That(...) instead of StringAssert(...)
+```
+
+Or put this at the top of the file to disable all instances.
+
+```csharp
+#pragma warning disable NUnit2048 // Consider using Assert.That(...) instead of StringAssert(...)
+```
+
+### Via attribute `[SuppressMessage]`
+
+```csharp
+[System.Diagnostics.CodeAnalysis.SuppressMessage("Assertion",
+    "NUnit2048:Consider using Assert.That(...) instead of StringAssert(...)",
+    Justification = "Reason...")]
+```
+<!-- end generated config severity -->

documentation/index.md

@@ -106,6 +106,7 @@ Rules which improve assertions in the test code.
 | [NUnit2045](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2045.md) | Use Assert.Multiple | :white_check_mark: | :information_source: | :white_check_mark: |
 | [NUnit2046](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2046.md) | Use CollectionConstraint for better assertion messages in case of failure | :white_check_mark: | :information_source: | :white_check_mark: |
 | [NUnit2047](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2047.md) | Incompatible types for Within constraint | :white_check_mark: | :warning: | :white_check_mark: |
+| [NUnit2048](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2048.md) | Consider using Assert.That(...) instead of StringAssert(...) | :white_check_mark: | :warning: | :white_check_mark: |
 
 ### Suppressor Rules (NUnit3001 - )
 

src/nunit.analyzers.sln

@@ -92,6 +92,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat
 		..\documentation\NUnit2045.md = ..\documentation\NUnit2045.md
 		..\documentation\NUnit2046.md = ..\documentation\NUnit2046.md
 		..\documentation\NUnit2047.md = ..\documentation\NUnit2047.md
+		..\documentation\NUnit2048.md = ..\documentation\NUnit2048.md
 		..\documentation\NUnit3001.md = ..\documentation\NUnit3001.md
 		..\documentation\NUnit3002.md = ..\documentation\NUnit3002.md
 		..\documentation\NUnit3003.md = ..\documentation\NUnit3003.md

src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs

@@ -56,6 +56,7 @@ public sealed class NUnitFrameworkConstantsTests
             (nameof(NUnitFrameworkConstants.NameOfDoesContain), nameof(Does.Contain)),
             (nameof(NUnitFrameworkConstants.NameOfDoesStartWith), nameof(Does.StartWith)),
             (nameof(NUnitFrameworkConstants.NameOfDoesEndWith), nameof(Does.EndWith)),
+            (nameof(NUnitFrameworkConstants.NameOfDoesMatch), nameof(Does.Match)),
 
             (nameof(NUnitFrameworkConstants.NameOfHas), nameof(Has)),
             (nameof(NUnitFrameworkConstants.NameOfHasProperty), nameof(Has.Property)),
@@ -113,16 +114,16 @@ public sealed class NUnitFrameworkConstantsTests
             (nameof(NUnitFrameworkConstants.NameOfAssertThrowsAsync), nameof(Assert.ThrowsAsync)),
 
             (nameof(NUnitFrameworkConstants.NameOfStringAssert), nameof(StringAssert)),
-            (nameof(NUnitFrameworkConstants.NameOfStringAssertAreEqualIgnoringCase), nameof(StringAssert.AreEqualIgnoringCase)),
-            (nameof(NUnitFrameworkConstants.NameOfStringAssertAreNotEqualIgnoringCase), nameof(StringAssert.AreNotEqualIgnoringCase)),
             (nameof(NUnitFrameworkConstants.NameOfStringAssertContains), nameof(StringAssert.Contains)),
             (nameof(NUnitFrameworkConstants.NameOfStringAssertDoesNotContain), nameof(StringAssert.DoesNotContain)),
-            (nameof(NUnitFrameworkConstants.NameOfStringAssertDoesNotEndWith), nameof(StringAssert.DoesNotEndWith)),
-            (nameof(NUnitFrameworkConstants.NameOfStringAssertDoesNotMatch), nameof(StringAssert.DoesNotMatch)),
+            (nameof(NUnitFrameworkConstants.NameOfStringAssertStartsWith), nameof(StringAssert.StartsWith)),
             (nameof(NUnitFrameworkConstants.NameOfStringAssertDoesNotStartWith), nameof(StringAssert.DoesNotStartWith)),
             (nameof(NUnitFrameworkConstants.NameOfStringAssertEndsWith), nameof(StringAssert.EndsWith)),
+            (nameof(NUnitFrameworkConstants.NameOfStringAssertDoesNotEndWith), nameof(StringAssert.DoesNotEndWith)),
+            (nameof(NUnitFrameworkConstants.NameOfStringAssertAreEqualIgnoringCase), nameof(StringAssert.AreEqualIgnoringCase)),
+            (nameof(NUnitFrameworkConstants.NameOfStringAssertAreNotEqualIgnoringCase), nameof(StringAssert.AreNotEqualIgnoringCase)),
             (nameof(NUnitFrameworkConstants.NameOfStringAssertIsMatch), nameof(StringAssert.IsMatch)),
-            (nameof(NUnitFrameworkConstants.NameOfStringAssertStartsWith), nameof(StringAssert.StartsWith)),
+            (nameof(NUnitFrameworkConstants.NameOfStringAssertDoesNotMatch), nameof(StringAssert.DoesNotMatch)),
 
             (nameof(NUnitFrameworkConstants.NameOfConstraint), nameof(Constraint)),
 

src/nunit.analyzers.tests/StringAssertUsage/StringAssertUsageAnalyzerTests.cs

@@ -0,0 +1,45 @@
+using System.Collections.Generic;
+using Gu.Roslyn.Asserts;
+using Microsoft.CodeAnalysis.Diagnostics;
+using NUnit.Analyzers.Constants;
+using NUnit.Analyzers.StringAssertUsage;
+using NUnit.Framework;
+
+namespace NUnit.Analyzers.Tests.StringAssertUsage
+{
+    [TestFixture]
+    internal class StringAssertUsageAnalyzerTests
+    {
+        private readonly DiagnosticAnalyzer analyzer = new StringAssertUsageAnalyzer();
+        private readonly ExpectedDiagnostic diagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.StringAssertUsage);
+
+        private static IEnumerable<string> StringAsserts => StringAssertUsageAnalyzer.StringAssertToConstraint.Keys;
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenNoArgumentsAreUsed(string method)
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                ↓StringAssert.{method}(""expected"", ""actual"");
+            ");
+            RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
+        }
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenOnlyMessageArgumentIsUsed(string method)
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                ↓StringAssert.{method}(""expected"", ""actual"", ""message"");
+            ");
+            RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
+        }
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenFormatAndArgumentsAreUsed(string method)
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                ↓StringAssert.{method}(""expected"", ""actual"", ""Because of {{0}}"", ""message"");
+            ");
+            RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
+        }
+    }
+}

src/nunit.analyzers.tests/StringAssertUsage/StringAssertUsageCodeFixTests.cs

@@ -0,0 +1,62 @@
+using System.Collections.Generic;
+using Gu.Roslyn.Asserts;
+using Microsoft.CodeAnalysis.CodeFixes;
+using Microsoft.CodeAnalysis.Diagnostics;
+using NUnit.Analyzers.Constants;
+using NUnit.Analyzers.StringAssertUsage;
+using NUnit.Framework;
+
+namespace NUnit.Analyzers.Tests.StringAssertUsage
+{
+    [TestFixture]
+    internal sealed class StringAssertUsageCodeFixTests
+    {
+        private static readonly DiagnosticAnalyzer analyzer = new StringAssertUsageAnalyzer();
+        private static readonly CodeFixProvider fix = new StringAssertUsageCodeFix();
+        private static readonly ExpectedDiagnostic expectedDiagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.StringAssertUsage);
+
+        private static IEnumerable<string> StringAsserts => StringAssertUsageAnalyzer.StringAssertToConstraint.Keys;
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenNoArgumentsAreUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            ↓StringAssert.{method}(""expected"", ""actual"");
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            Assert.That(""actual"", {GetAdjustedConstraint(method)});
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenOnlyMessageArgumentIsUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            ↓StringAssert.{method}(""expected"", ""actual"", ""message"");
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            Assert.That(""actual"", {GetAdjustedConstraint(method)}, ""message"");
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenFormatAndArgumentsAreUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            ↓StringAssert.{method}(""expected"", ""actual"", ""Because of {{0}}"", ""message"");
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            Assert.That(""actual"", {GetAdjustedConstraint(method)}, $""Because of {{""message""}}"");
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        private static string GetAdjustedConstraint(string method)
+        {
+            return StringAssertUsageAnalyzer.StringAssertToConstraint[method]
+                                            .Replace("expected", "\"expected\"");
+        }
+    }
+}

src/nunit.analyzers/ClassicModelAssertUsage/ClassicModelAssertUsageAnalyzer.cs

@@ -252,14 +252,11 @@ protected override void AnalyzeAssertInvocation(OperationAnalysisContext context
         {
             return new Dictionary<string, string?>
             {
-                { AnalyzerPropertyKeys.ModelName, invocationSymbol.Name },
-                {
-                    AnalyzerPropertyKeys.HasToleranceValue,
+                [AnalyzerPropertyKeys.ModelName] = invocationSymbol.Name,
+                [AnalyzerPropertyKeys.HasToleranceValue] =
                     (invocationSymbol.Name == NameOfAssertAreEqual &&
                         invocationSymbol.Parameters.Length >= 3 &&
-                        invocationSymbol.Parameters[2].Type.SpecialType == SpecialType.System_Double).ToString()
-                },
-                { AnalyzerPropertyKeys.IsGenericMethod, invocationSymbol.IsGenericMethod.ToString() },
+                        invocationSymbol.Parameters[2].Type.SpecialType == SpecialType.System_Double).ToString(),
             }.ToImmutableDictionary();
         }
     }

src/nunit.analyzers/ClassicModelAssertUsage/ClassicModelAssertUsageCodeFix.cs

@@ -45,30 +45,8 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
             if (invocationNode is null)
                 return;
 
-            var invocationIdentifier = diagnostic.Properties[AnalyzerPropertyKeys.ModelName];
-            var isGenericMethod = diagnostic.Properties[AnalyzerPropertyKeys.IsGenericMethod] == true.ToString();
-
             var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
 
-            string? GetUserDefinedImplicitTypeConversion(ExpressionSyntax expression)
-            {
-                var typeInfo = semanticModel.GetTypeInfo(expression, context.CancellationToken);
-                var convertedType = typeInfo.ConvertedType;
-                if (convertedType is null)
-                {
-                    return null;
-                }
-
-                var conversion = semanticModel.ClassifyConversion(expression, convertedType);
-
-                if (!conversion.IsUserDefined)
-                {
-                    return null;
-                }
-
-                return convertedType.ToString();
-            }
-
             // Replace the original ClassicAssert.<Method> invocation name into Assert.That
             var newInvocationNode = invocationNode.UpdateClassicAssertToAssertThat(out TypeArgumentListSyntax? typeArguments);
 
@@ -78,19 +56,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
             // Now, replace the arguments.
             List<ArgumentSyntax> arguments = invocationNode.ArgumentList.Arguments.ToList();
 
-            ArgumentSyntax CastIfNecessary(ArgumentSyntax argument)
-            {
-                string? implicitTypeConversion = GetUserDefinedImplicitTypeConversion(argument.Expression);
-                if (implicitTypeConversion is null)
-                    return argument;
-
-                // Assert.That only expects objects whilst the classic methods have overloads
-                // Add an explicit cast operation for the first argument.
-                return SyntaxFactory.Argument(SyntaxFactory.CastExpression(
-                    SyntaxFactory.ParseTypeName(implicitTypeConversion),
-                    argument.Expression));
-            }
-
             // See if we need to cast the arguments when they were using a specific classic overload.
             arguments[0] = CastIfNecessary(arguments[0]);
             if (arguments.Count > 1)
@@ -117,6 +82,38 @@ ArgumentSyntax CastIfNecessary(ArgumentSyntax argument)
                     this.Title,
                     _ => Task.FromResult(context.Document.WithSyntaxRoot(newRoot)),
                     this.Title), diagnostic);
+
+            ArgumentSyntax CastIfNecessary(ArgumentSyntax argument)
+            {
+                string? implicitTypeConversion = GetUserDefinedImplicitTypeConversion(argument.Expression);
+                if (implicitTypeConversion is null)
+                    return argument;
+
+                // Assert.That only expects objects whilst the classic methods have overloads
+                // Add an explicit cast operation for the first argument.
+                return SyntaxFactory.Argument(SyntaxFactory.CastExpression(
+                    SyntaxFactory.ParseTypeName(implicitTypeConversion),
+                    argument.Expression));
+            }
+
+            string? GetUserDefinedImplicitTypeConversion(ExpressionSyntax expression)
+            {
+                var typeInfo = semanticModel.GetTypeInfo(expression, context.CancellationToken);
+                var convertedType = typeInfo.ConvertedType;
+                if (convertedType is null)
+                {
+                    return null;
+                }
+
+                var conversion = semanticModel.ClassifyConversion(expression, convertedType);
+
+                if (!conversion.IsUserDefined)
+                {
+                    return null;
+                }
+
+                return convertedType.ToString();
+            }
         }
 
         protected virtual void UpdateArguments(Diagnostic diagnostic, List<ArgumentSyntax> arguments, TypeArgumentListSyntax typeArguments)

src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs

@@ -89,6 +89,7 @@ internal static class AnalyzerIdentifiers
         internal const string UseAssertMultiple = "NUnit2045";
         internal const string UsePropertyConstraint = "NUnit2046";
         internal const string WithinIncompatibleTypes = "NUnit2047";
+        internal const string StringAssertUsage = "NUnit2048";
 
         #endregion Assertion
 

src/nunit.analyzers/Constants/AnalyzerPropertyKeys.cs

@@ -4,7 +4,6 @@ internal static class AnalyzerPropertyKeys
     {
         internal const string HasToleranceValue = nameof(AnalyzerPropertyKeys.HasToleranceValue);
         internal const string ModelName = nameof(AnalyzerPropertyKeys.ModelName);
-        internal const string IsGenericMethod = nameof(AnalyzerPropertyKeys.IsGenericMethod);
         internal const string MinimumNumberOfArguments = nameof(AnalyzerPropertyKeys.MinimumNumberOfArguments);
     }
 }