Sometimes we cannot convert message+args into a formattable string. (#…

…725) * Sometimes we cannot convert message+args into a formattable string. In those case create a string.Format call. * Remove duplication between 2 ToFormattableString methods * Code review changes
nunit · Apr 19, 2024 · 07d8594 · 07d8594
1 parent d551aac
commit 07d8594
Show file tree

Hide file tree

Showing 13 changed files with 323 additions and 67 deletions.
diff --git a/...it.analyzers.tests/ClassicModelAssertUsage/AreEqualClassicModelAssertUsageCodeFixTests.cs b/...it.analyzers.tests/ClassicModelAssertUsage/AreEqualClassicModelAssertUsageCodeFixTests.cs
@@ -252,6 +252,48 @@ public void TestMethod()
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
         }
 
+        [Test]
+        public void VerifyAreEqualFixWhenWithMessageAndArgsArray()
+        {
+            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(3.0, 3)]
+        public void Test(object actual, object expected)
+        {
+            object[] args = { expected, actual };
+            ↓ClassicAssert.AreEqual(expected, actual, ""Expected: {0} Got: {1}"", args);
+        }");
+            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(3.0, 3)]
+        public void Test(object actual, object expected)
+        {
+            object[] args = { expected, actual };
+            Assert.That(actual, Is.EqualTo(expected), () => string.Format(""Expected: {0} Got: {1}"", args));
+        }");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
+        }
+
+        [Test]
+        public void VerifyAreEqualFixWhenWithMessageVariable()
+        {
+            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(3.0, 3)]
+        public void Test(object actual, object expected)
+        {
+            ↓ClassicAssert.AreEqual(expected, actual, GetLocalizedFormatSpecification(), expected, actual);
+        }
+        private static string GetLocalizedFormatSpecification() => ""Expected: {0} Got: {1}"";
+        ");
+            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(3.0, 3)]
+        public void Test(object actual, object expected)
+        {
+            Assert.That(actual, Is.EqualTo(expected), () => string.Format(GetLocalizedFormatSpecification(), expected, actual));
+        }
+        private static string GetLocalizedFormatSpecification() => ""Expected: {0} Got: {1}"";
+        ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, fixTitle: ClassicModelAssertUsageCodeFix.TransformToConstraintModelDescription);
+        }
+
         [Test]
         public void CodeFixPreservesLineBreakBeforeMessage()
         {

diff --git a/src/nunit.analyzers.tests/CollectionAssertUsage/CollectionAssertUsageCodeFixTests.cs b/src/nunit.analyzers.tests/CollectionAssertUsage/CollectionAssertUsageCodeFixTests.cs
@@ -171,6 +171,42 @@ public void AnalyzeTwoCollectionWhenFormatAndTwoParamsArgumentsAreUsed(string me
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
 
+        [TestCaseSource(nameof(TwoCollectionParameterAsserts))]
+        public void AnalyzeTwoCollectionWhenFormatAndArgsArrayAreUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            var collection1 = new[] {{ 1, 2, 3 }};
+            var collection2 = new[] {{ 2, 4, 6 }};
+            var args = new[] {{ ""first"", ""second"" }};
+            ↓CollectionAssert.{method}(collection1, collection2, ""{{0}}, {{1}}"", args);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            var collection1 = new[] {{ 1, 2, 3 }};
+            var collection2 = new[] {{ 2, 4, 6 }};
+            var args = new[] {{ ""first"", ""second"" }};
+            Assert.That({GetAdjustedTwoCollectionConstraint(method)}, () => string.Format(""{{0}}, {{1}}"", args));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCaseSource(nameof(TwoCollectionParameterAsserts))]
+        public void AnalyzeTwoCollectionWhenFormatVariableAndTwoParamsArgumentsAreUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            var collection1 = new[] {{ 1, 2, 3 }};
+            var collection2 = new[] {{ 2, 4, 6 }};
+            const string formatSpecification = ""{{0}}, {{1}}"";
+            ↓CollectionAssert.{method}(collection1, collection2, formatSpecification, ""first"", ""second"");
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            var collection1 = new[] {{ 1, 2, 3 }};
+            var collection2 = new[] {{ 2, 4, 6 }};
+            const string formatSpecification = ""{{0}}, {{1}}"";
+            Assert.That({GetAdjustedTwoCollectionConstraint(method)}, () => string.Format(formatSpecification, ""first"", ""second""));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
         [TestCaseSource(nameof(TwoCollectionParameterAsserts))]
         public void AnalyzeTwoCollectionWhenFormatAndParamsArgumentsAreUsedOutOfOrder(string method)
         {

diff --git a/src/nunit.analyzers.tests/StringAssertUsage/StringAssertUsageCodeFixTests.cs b/src/nunit.analyzers.tests/StringAssertUsage/StringAssertUsageCodeFixTests.cs
@@ -53,6 +53,34 @@ public void AnalyzeWhenFormatAndArgumentsAreUsed(string method)
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
 
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenFormatAndArgsArrayAreUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            object[] args = {{ ""message"" }};
+            ↓StringAssert.{method}(""expected"", ""actual"", ""Because of {{0}}"", args);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            object[] args = {{ ""message"" }};
+            Assert.That(""actual"", {GetAdjustedConstraint(method)}, () => string.Format(""Because of {{0}}"", args));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCaseSource(nameof(StringAsserts))]
+        public void AnalyzeWhenFormatVariableAndArgumentsAreUsed(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+            ↓StringAssert.{method}(""expected"", ""actual"", GetLocalizedFormatSpecification(), ""message"");
+            static string GetLocalizedFormatSpecification() => ""Because of {{0}}"";
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+            Assert.That(""actual"", {GetAdjustedConstraint(method)}, () => string.Format(GetLocalizedFormatSpecification(), ""message""));
+            static string GetLocalizedFormatSpecification() => ""Because of {{0}}"";
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
         [TestCaseSource(nameof(StringAsserts))]
         public void AnalyzeWhenFormatAndArgumentsAreUsedOutOfOrder(string method)
         {

diff --git a/...tringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs b/...tringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs
@@ -47,6 +47,26 @@ public void AccidentallyUseFormatSpecification(string assertOrAssume)
         }}");
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
+
+        [TestCaseSource(nameof(AssertAndAssume))]
+        public void AccidentallyUseFormatSpecificationWithVariable(string assertOrAssume)
+        {
+            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
+        [TestCase(""NUnit 4.0"", ""NUnit 3.14"")]
+        public void {assertOrAssume}Something(string actual, string expected)
+        {{
+            const string formatSpecification = ""Expected '{{0}}', but got: {{1}}"";
+            ↓{assertOrAssume}.That(actual, Is.EqualTo(expected).IgnoreCase, formatSpecification, expected, actual);
+        }}");
+            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
+        [TestCase(""NUnit 4.0"", ""NUnit 3.14"")]
+        public void {assertOrAssume}Something(string actual, string expected)
+        {{
+            const string formatSpecification = ""Expected '{{0}}', but got: {{1}}"";
+            {assertOrAssume}.That(actual, Is.EqualTo(expected).IgnoreCase, () => string.Format(formatSpecification, expected, actual));
+        }}");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
 #else
         [TestCase(NUnitFrameworkConstants.NameOfAssertIgnore)]
         public void ConvertWhenNoMinimumParameters(string method)
@@ -60,6 +80,34 @@ public void ConvertWhenNoMinimumParameters(string method)
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
 
+        [TestCase(NUnitFrameworkConstants.NameOfAssertIgnore)]
+        public void ConvertWhenNoMinimumParametersAndArgsArray(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+                object[] args = {{ ""{method}"" }};
+                ↓Assert.{method}(""Method: {{0}}"", args);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod($@"
+                object[] args = {{ ""{method}"" }};
+                Assert.{method}(string.Format(""Method: {{0}}"", args));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCase(NUnitFrameworkConstants.NameOfAssertIgnore)]
+        public void ConvertWhenNoMinimumParametersVariableFormatSpecification(string method)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+                const string formatSpecification = ""Method: {{0}}"";
+                ↓Assert.{method}(formatSpecification, ""{method}"");
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod($@"
+                const string formatSpecification = ""Method: {{0}}"";
+                Assert.{method}(string.Format(formatSpecification, ""{method}""));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
         [TestCaseSource(nameof(AssertAndAssume))]
         public void ConvertWhenMinimumParametersIsOne(string assertOrAssume)
         {
@@ -74,6 +122,38 @@ public void ConvertWhenMinimumParametersIsOne(string assertOrAssume)
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
 
+        [TestCaseSource(nameof(AssertAndAssume))]
+        public void ConvertWhenMinimumParametersIsOneAndArgsArray(string assertOrAssume)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+                const bool actual = false;
+                object[] args = {{ actual }};
+                ↓{assertOrAssume}.That(actual, ""Expected actual value to be true, but was: {{0}}"", args);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+                const bool actual = false;
+                object[] args = {{ actual }};
+                {assertOrAssume}.That(actual, () => string.Format(""Expected actual value to be true, but was: {{0}}"", args));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCaseSource(nameof(AssertAndAssume))]
+        public void ConvertWhenMinimumParametersIsOneAndVariableFormatSpecification(string assertOrAssume)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+                const bool actual = false;
+                const string formatSpecification = ""Expected actual value to be true, but was: {{0}}"";
+                ↓{assertOrAssume}.That(actual, formatSpecification, actual);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+                const bool actual = false;
+                const string formatSpecification = ""Expected actual value to be true, but was: {{0}}"";
+                {assertOrAssume}.That(actual, () => string.Format(formatSpecification, actual));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
         [TestCaseSource(nameof(AssertAndAssume))]
         public void ConvertWhenMinimumParametersIsTwo(string assertOrAssume)
         {
@@ -88,6 +168,38 @@ public void ConvertWhenMinimumParametersIsTwo(string assertOrAssume)
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
 
+        [TestCaseSource(nameof(AssertAndAssume))]
+        public void ConvertWhenMinimumParametersIsTwoAndArgsArray(string assertOrAssume)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+                const int actual = 42;
+                object[] args = {{ actual, DateTime.Now }};
+                ↓{assertOrAssume}.That(actual, Is.EqualTo(42), ""Expected actual value to be 42, but was: {{0}} at time {{1:HH:mm:ss}}"", args);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+                const int actual = 42;
+                object[] args = {{ actual, DateTime.Now }};
+                {assertOrAssume}.That(actual, Is.EqualTo(42), () => string.Format(""Expected actual value to be 42, but was: {{0}} at time {{1:HH:mm:ss}}"", args));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [TestCaseSource(nameof(AssertAndAssume))]
+        public void ConvertWhenMinimumParametersIsTwoAndVariableFormatSpecification(string assertOrAssume)
+        {
+            var code = TestUtility.WrapInTestMethod(@$"
+                const int actual = 42;
+                const string formatSpecification = ""Expected actual value to be 42, but was: {{0}} at time {{1:HH:mm:ss}}"";
+                ↓{assertOrAssume}.That(actual, Is.EqualTo(42), formatSpecification, actual, DateTime.Now);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@$"
+                const int actual = 42;
+                const string formatSpecification = ""Expected actual value to be 42, but was: {{0}} at time {{1:HH:mm:ss}}"";
+                {assertOrAssume}.That(actual, Is.EqualTo(42), () => string.Format(formatSpecification, actual, DateTime.Now));
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
         // We need to double the backslashes as the text is formatted and we want the code to contain \n and not LF
         [TestCase("Passed: {0}")]
         [TestCase("Passed: {0} {{")]

diff --git a/src/nunit.analyzers/ClassicModelAssertUsage/ClassicModelAssertUsageAnalyzer.cs b/src/nunit.analyzers/ClassicModelAssertUsage/ClassicModelAssertUsageAnalyzer.cs
@@ -4,6 +4,7 @@
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Operations;
 using NUnit.Analyzers.Constants;
+using NUnit.Analyzers.Helpers;
 using static NUnit.Analyzers.Constants.NUnitFrameworkConstants;
 
 namespace NUnit.Analyzers.ClassicModelAssertUsage
@@ -236,23 +237,15 @@ public sealed class ClassicModelAssertUsageAnalyzer : ClassicAssertionAnalyzer
 
         protected override void AnalyzeAssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation)
         {
-            var methodSymbol = assertOperation.TargetMethod;
+            string methodName = assertOperation.TargetMethod.Name;
 
-            if (ClassicModelAssertUsageAnalyzer.NameToDescriptor.TryGetValue(methodSymbol.Name, out DiagnosticDescriptor? descriptor))
+            if (ClassicModelAssertUsageAnalyzer.NameToDescriptor.TryGetValue(methodName, out DiagnosticDescriptor? descriptor))
             {
                 context.ReportDiagnostic(Diagnostic.Create(
                     descriptor,
                     assertOperation.Syntax.GetLocation(),
-                    ClassicModelAssertUsageAnalyzer.GetProperties(methodSymbol)));
+                    DiagnosticsHelper.GetProperties(methodName, assertOperation.Arguments)));
             }
         }
-
-        private static ImmutableDictionary<string, string?> GetProperties(IMethodSymbol invocationSymbol)
-        {
-            return new Dictionary<string, string?>
-            {
-                [AnalyzerPropertyKeys.ModelName] = invocationSymbol.Name,
-            }.ToImmutableDictionary();
-        }
     }
 }
diff --git a/src/nunit.analyzers/ClassicModelAssertUsage/ClassicModelAssertUsageCodeFix.cs b/src/nunit.analyzers/ClassicModelAssertUsage/ClassicModelAssertUsageCodeFix.cs
@@ -71,8 +71,10 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
                 newArguments.Add(constraintArgument);
 
             // Do the format spec, params to formattable string conversion
+            bool argsIsArray = !string.IsNullOrEmpty(diagnostic.Properties[AnalyzerPropertyKeys.ArgsIsArray]);
+
             argumentNamesToArguments.TryGetValue(NUnitFrameworkConstants.NameOfMessageParameter, out ArgumentSyntax? messageArgument);
-            if (CodeFixHelper.GetInterpolatedMessageArgumentOrDefault(messageArgument, args) is ArgumentSyntax interpolatedMessageArgument)
+            if (CodeFixHelper.GetInterpolatedMessageArgumentOrDefault(messageArgument, args, unconditional: false, argsIsArray) is ArgumentSyntax interpolatedMessageArgument)
                 newArguments.Add(interpolatedMessageArgument);
 
             var newArgumentsList = invocationNode.ArgumentList.WithArguments(newArguments);

diff --git a/src/nunit.analyzers/CollectionAssertUsage/CollectionAssertUsageAnalyzer.cs b/src/nunit.analyzers/CollectionAssertUsage/CollectionAssertUsageAnalyzer.cs
@@ -5,6 +5,7 @@
 using Microsoft.CodeAnalysis.Operations;
 using NUnit.Analyzers.Constants;
 using NUnit.Analyzers.Extensions;
+using NUnit.Analyzers.Helpers;
 using static NUnit.Analyzers.Constants.NUnitFrameworkConstants;
 
 namespace NUnit.Analyzers.CollectionAssertUsage
@@ -61,21 +62,18 @@ protected override bool IsAssert(bool hasClassicAssert, IInvocationOperation inv
 
         protected override void AnalyzeAssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation)
         {
-            var methodSymbol = assertOperation.TargetMethod;
+            string methodName = assertOperation.TargetMethod.Name;
 
-            if (OneCollectionParameterAsserts.TryGetValue(methodSymbol.Name, out string? constraint) ||
-                TwoCollectionParameterAsserts.TryGetValue(methodSymbol.Name, out constraint) ||
-                CollectionAndItemParameterAsserts.TryGetValue(methodSymbol.Name, out constraint))
+            if (OneCollectionParameterAsserts.TryGetValue(methodName, out string? constraint) ||
+                TwoCollectionParameterAsserts.TryGetValue(methodName, out constraint) ||
+                CollectionAndItemParameterAsserts.TryGetValue(methodName, out constraint))
             {
                 context.ReportDiagnostic(Diagnostic.Create(
                     collectionAssertDescriptor,
                     assertOperation.Syntax.GetLocation(),
-                    new Dictionary<string, string?>
-                    {
-                        [AnalyzerPropertyKeys.ModelName] = methodSymbol.Name,
-                    }.ToImmutableDictionary(),
+                    DiagnosticsHelper.GetProperties(methodName, assertOperation.Arguments),
                     constraint,
-                    methodSymbol.Name));
+                    methodName));
             }
         }
     }

diff --git a/src/nunit.analyzers/Constants/AnalyzerPropertyKeys.cs b/src/nunit.analyzers/Constants/AnalyzerPropertyKeys.cs
@@ -3,6 +3,7 @@ namespace NUnit.Analyzers.Constants
     internal static class AnalyzerPropertyKeys
     {
         internal const string ModelName = nameof(AnalyzerPropertyKeys.ModelName);
+        internal const string ArgsIsArray = nameof(AnalyzerPropertyKeys.ArgsIsArray);
         internal const string MinimumNumberOfArguments = nameof(AnalyzerPropertyKeys.MinimumNumberOfArguments);
     }
 }