UpdateStringFormatToInterpolatableString for non-classic asserts

.github/workflows/ci.yml

@@ -15,5 +15,9 @@ jobs:
         uses: actions/setup-dotnet@main
         with:
           global-json-file: ./global.json
-      - name: dotnet test
-        run: dotnet test --configuration=Release ./src/nunit.analyzers.tests/
+
+      - name: dotnet test (NUnit4)
+        run: dotnet test --configuration=Release -p:NUnitVersion=4 ./src/nunit.analyzers.tests/
+
+      - name: dotnet test (NUnit3)
+        run: dotnet test --configuration=Release -p:NUnitVersion=3 ./src/nunit.analyzers.tests/

documentation/NUnit1033.md

@@ -0,0 +1,107 @@
+# NUnit1033
+
+## NUnit 4 no longer supports string.Format specification
+
+| Topic    | Value
+| :--      | :--
+| Id       | NUnit1033
+| Severity | Error
+| Enabled  | True
+| Category | Structure
+| Code     | [UpdateStringFormatToInterpolatableStringAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzer.cs)
+
+## Description
+
+Replace format specification with interpolated string.
+
+## Motivation
+
+In order to get better failure messages, NUnit4 uses [`CallerArgumentExpression`](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.callerargumentexpressionattribute?view=net-7.0)
+to include the expression passed in for the _actual_ and _constraint_ parameters.
+These are parameters automatically supplied by the compiler.
+
+To facilitate this, we needed to drop support for [composite formatting](https://learn.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting)
+All NUnit4 asserts only allow a single *message* parameter which can be either a simple string literal
+or a [interpolatable string](https://learn.microsoft.com/en-us/dotnet/csharp/tutorials/string-interpolation)
+
+This analyzer needs to be run when still building against NUnit3 as otherwise your code won't compile.
+When usages of the new methods with `params` are detected, the associated CodeFix will convert the format specification
+into an interpolated string.
+
+## How to fix violations
+
+The following code, valid in NUNit3:
+
+```csharp
+[TestCase(4)]
+public void MustBeMultipleOf3(int value)
+{
+    Assert.That(value % 3, Is.Zero, "Expected value ({0}) to be multiple of 3", value);
+}
+```
+
+Will fail with the following message:
+
+```
+Expected value (4) to be multiple of 3
+Expected: 0
+But was:  1
+```
+
+The associated CodeFix for this Analyzer rule will convert the test into:
+
+```csharp
+[TestCase(4)]
+public void MustBeMultipleOf3(int value)
+{
+    Assert.That(value % 3, Is.Zero, $"Expected value ({value}) to be multiple of 3");
+}
+```
+
+The failure message for NUnit4 becomes:
+
+```
+Expected value (4) to be multiple of 3
+Assert.That(value % 3, Is.Zero)
+Expected: 0
+But was:  1
+```
+
+<!-- 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
+# NUnit1033: NUnit 4 no longer supports string.Format specification
+dotnet_diagnostic.NUnit1033.severity = chosenSeverity
+```
+
+where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`.
+
+### Via #pragma directive
+
+```csharp
+#pragma warning disable NUnit1033 // NUnit 4 no longer supports string.Format specification
+Code violating the rule here
+#pragma warning restore NUnit1033 // NUnit 4 no longer supports string.Format specification
+```
+
+Or put this at the top of the file to disable all instances.
+
+```csharp
+#pragma warning disable NUnit1033 // NUnit 4 no longer supports string.Format specification
+```
+
+### Via attribute `[SuppressMessage]`
+
+```csharp
+[System.Diagnostics.CodeAnalysis.SuppressMessage("Structure",
+    "NUnit1033:NUnit 4 no longer supports string.Format specification",
+    Justification = "Reason...")]
+```
+<!-- end generated config severity -->

documentation/index.md

@@ -51,6 +51,7 @@ Rules which enforce structural requirements on the test code.
 | [NUnit1030](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1030.md) | The type of parameter provided by the TestCaseSource does not match the type of the parameter in the Test method | :white_check_mark: | :exclamation: | :x: |
 | [NUnit1031](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1031.md) | The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method | :white_check_mark: | :exclamation: | :x: |
 | [NUnit1032](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1032.md) | An IDisposable field/property should be Disposed in a TearDown method | :white_check_mark: | :exclamation: | :x: |
+| [NUnit1033](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1033.md) | NUnit 4 no longer supports string.Format specification | :white_check_mark: | :exclamation: | :white_check_mark: |
 
 ### Assertion Rules (NUnit2001 - )
 

src/nunit.analyzers.sln

@@ -44,6 +44,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat
 		..\documentation\NUnit1030.md = ..\documentation\NUnit1030.md
 		..\documentation\NUnit1031.md = ..\documentation\NUnit1031.md
 		..\documentation\NUnit1032.md = ..\documentation\NUnit1032.md
+		..\documentation\NUnit1033.md = ..\documentation\NUnit1033.md
 		..\documentation\NUnit2001.md = ..\documentation\NUnit2001.md
 		..\documentation\NUnit2002.md = ..\documentation\NUnit2002.md
 		..\documentation\NUnit2003.md = ..\documentation\NUnit2003.md

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

@@ -66,9 +66,6 @@ public sealed class NUnitFrameworkConstantsTests
             (nameof(NUnitFrameworkConstants.NameOfHasNo), nameof(Has.No)),
 
             (nameof(NUnitFrameworkConstants.NameOfMultiple), nameof(Assert.Multiple)),
-#if NUNIT4
-            (nameof(NUnitFrameworkConstants.NameOfMultipleAsync), nameof(Assert.MultipleAsync)),
-#endif
 
             (nameof(NUnitFrameworkConstants.NameOfThrows), nameof(Throws)),
             (nameof(NUnitFrameworkConstants.NameOfThrowsArgumentException), nameof(Throws.ArgumentException)),

src/nunit.analyzers.tests/DelegateRequired/DelegateRequiredCodeFixTests.cs

@@ -57,10 +57,10 @@ public void VerifyNonDelegateFixWithMessage()
         }
 
         [Test]
-        public void VerifyNonDelegateFixWithMessageAndParams()
+        public void VerifyNonDelegateFixWithFormattableMessage()
         {
             var code = TestUtility.WrapInTestMethod(@"
-                Assert.That(↓MyOperation() + MyOtherOperation(), Throws.InstanceOf<InvalidOperationException>(), ""message-id: {{0}}"", Guid.NewGuid());
+                Assert.That(↓MyOperation() + MyOtherOperation(), Throws.InstanceOf<InvalidOperationException>(), $""message-id: {Guid.NewGuid()}"");
 
                 int MyOperation() => throw new InvalidOperationException();
                 int MyOtherOperation() => throw new InvalidOperationException();

src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzerTests.cs

@@ -14,7 +14,9 @@ namespace NUnit.Analyzers.Tests.UpdateStringFormatToInterpolatableString
     public sealed class UpdateStringFormatToInterpolatableStringAnalyzerTests
     {
         private readonly DiagnosticAnalyzer analyzer = new UpdateStringFormatToInterpolatableStringAnalyzer();
+#if !NUNIT4
         private readonly ExpectedDiagnostic diagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.UpdateStringFormatToInterpolatableString);
+#endif
 
         private static IEnumerable<string> NoArgumentsAsserts { get; } = new[]
         {
@@ -44,6 +46,7 @@ public void AnalyzeWhenOnlyMessageArgumentIsUsed(string method)
             RoslynAssert.Valid(this.analyzer, testCode);
         }
 
+#if !NUNIT4
         [TestCaseSource(nameof(OneArgumentsAsserts))]
         public void AnalyzeWhenFormatAndArgumentsAreUsed(string method)
         {
@@ -62,7 +65,18 @@ public void AnalyzeWhenFormatAndArrayArgumentsAreUsed(string method)
             ");
             RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
         }
+#endif
 
+        [TestCaseSource(nameof(OneArgumentsAsserts))]
+        public void AnalyzeWhenFormattableStringIsUsed(string method)
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                Assert.{method}($""Method: {{""method""}}"");
+            ");
+            RoslynAssert.Valid(this.analyzer, testCode);
+        }
+
+        [Test]
         public void AnalyzeAssertBoolWhenNoArgumentsAreUsed()
         {
             var testCode = TestUtility.WrapInTestMethod(@$"
@@ -80,14 +94,25 @@ public void AnalyzeAssertBoolWhenOnlyMessageArgumentIsUsed()
             RoslynAssert.Valid(this.analyzer, testCode);
         }
 
+#if !NUNIT4
         [Test]
         public void AnalyzeAssertBoolWhenFormatAndArgumentsAreUsed()
         {
             var testCode = TestUtility.WrapInTestMethod(@$"
-                ↓Assert.That(false, ""Method: {{0}}"", false);
+                ↓Assert.That(false, ""Method: {{0}}"", false.ToString());
             ");
             RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
         }
+#endif
+
+        [Test]
+        public void AnalyzeAssertBoolWhenFormattableStringIsUsed()
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                Assert.That(false, $""Method: {{false}}"");
+            ");
+            RoslynAssert.Valid(this.analyzer, testCode);
+        }
 
         [Test]
         public void AnalyzeAssertThatWhenNoArgumentsAreUsed()
@@ -109,6 +134,7 @@ public void AnalyzeAssertThatWhenOnlyMessageArgumentIsUsed()
             RoslynAssert.Valid(this.analyzer, testCode);
         }
 
+#if !NUNIT4
         [Test]
         public void AnalyzeAssertThatWhenFormatAndArgumentsAreUsed()
         {
@@ -118,5 +144,43 @@ public void AnalyzeAssertThatWhenFormatAndArgumentsAreUsed()
             ");
             RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
         }
+#endif
+
+        [Test]
+        public void AnalyzeAssertThatWhenFormatStringIsUsed()
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                double pi = 3.1415;
+                Assert.That(pi, Is.EqualTo(3.1415).Within(0.0001), $""Method: {{pi}}"");
+            ");
+            RoslynAssert.Valid(this.analyzer, testCode);
+        }
+
+        [Test]
+        public void AnalyzeAssertDoesNotThrowWhenFormatAndArgumentsAreUsed()
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                Assert.DoesNotThrow(() => SomeOperation(), ""Expected no exception to be thrown by {{0}}"", nameof(SomeOperation));
+
+                static void SomeOperation()
+                {{
+                }}
+            ");
+            RoslynAssert.Valid(this.analyzer, testCode);
+        }
+
+        [Test]
+        public void AnalyzeAssertThrowsWhenFormatAndArgumentsAreUsed()
+        {
+            var testCode = TestUtility.WrapInTestMethod(@$"
+                Assert.Throws<InvalidOperationException>(() => SomeOperation(),
+                    ""Expected an InvalidOperationException exception to be thrown by {{0}}"", nameof(SomeOperation));
+
+                static void SomeOperation()
+                {{
+                }}
+            ");
+            RoslynAssert.Valid(this.analyzer, testCode);
+        }
     }
 }

src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs

@@ -1,3 +1,5 @@
+#if !NUNIT4
+
 using Gu.Roslyn.Asserts;
 using Microsoft.CodeAnalysis.CodeFixes;
 using Microsoft.CodeAnalysis.Diagnostics;
@@ -7,6 +9,7 @@
 
 namespace NUnit.Analyzers.Tests.UpdateStringFormatToInterpolatableString
 {
+
     [TestFixture]
     public sealed class UpdateStringFormatToInterpolatableStringCodeFixTests
     {
@@ -24,7 +27,7 @@ public void VerifyGetFixableDiagnosticIds()
         }
 
         [TestCase(NUnitFrameworkConstants.NameOfAssertIgnore)]
-        public void VerifyLength(string method)
+        public void ConvertWhenNoMinimumParameters(string method)
         {
             var code = TestUtility.WrapInTestMethod(@$"
                 ↓Assert.{method}(""Method: {{0}}"", ""{method}"");
@@ -34,5 +37,34 @@ public void VerifyLength(string method)
             ");
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
+
+        [Test]
+        public void ConvertWhenMinimumParametersIsOne()
+        {
+            var code = TestUtility.WrapInTestMethod(@"
+                const bool actual = false;
+                ↓Assert.That(actual, ""Expected actual value to be true, but was: {0}"", actual);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@"
+                const bool actual = false;
+                Assert.That(actual, $""Expected actual value to be true, but was: {actual}"");
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+
+        [Test]
+        public void ConvertWhenMinimumParametersIsTwo()
+        {
+            var code = TestUtility.WrapInTestMethod(@"
+                const int actual = 42;
+                ↓Assert.That(actual, Is.EqualTo(42), ""Expected actual value to be 42, but was: {0}"", actual);
+            ");
+            var fixedCode = TestUtility.WrapInTestMethod(@"
+                const int actual = 42;
+                Assert.That(actual, Is.EqualTo(42), $""Expected actual value to be 42, but was: {actual}"");
+            ");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
     }
 }
+#endif

src/nunit.analyzers.tests/nunit.analyzers.tests.csproj

@@ -2,7 +2,7 @@
   <PropertyGroup>
     <RootNamespace>NUnit.Analyzers.Tests</RootNamespace>
     <TargetFrameworks>net6.0;net462</TargetFrameworks>
-    <NUnitVersion Condition="'$(NUnitVersion)'==''">3</NUnitVersion>
+    <NUnitVersion Condition="'$(NUnitVersion)'==''">4</NUnitVersion>
     <NoWarn>$(NoWarn);NUnit1033</NoWarn>
   </PropertyGroup>
 

src/nunit.analyzers/BaseAssertionAnalyzer.cs

@@ -17,25 +17,33 @@ public override void Initialize(AnalysisContext context)
             context.RegisterCompilationStartAction(this.AnalyzeCompilationStart);
         }
 
-        protected abstract void AnalyzeAssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation);
+        protected virtual void AnalyzeAssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation)
+        {
+            throw new NotImplementedException($"You must override one of the {nameof(AnalyzeAssertInvocation)} overloads.");
+        }
+
+        protected virtual void AnalyzeAssertInvocation(Version nunitVersion, OperationAnalysisContext context, IInvocationOperation assertOperation)
+        {
+            this.AnalyzeAssertInvocation(context, assertOperation);
+        }
 
         protected virtual bool IsAssert(bool hasClassicAssert, IInvocationOperation invocationOperation)
         {
             INamedTypeSymbol containingType = invocationOperation.TargetMethod.ContainingType;
             return containingType.IsAssert() || (hasClassicAssert && containingType.IsClassicAssert());
         }
 
-        private void AnalyzeInvocation(bool hasClassicAssert, OperationAnalysisContext context)
+        private void AnalyzeInvocation(Version nunitVersion, OperationAnalysisContext context)
         {
             if (context.Operation is not IInvocationOperation invocationOperation)
                 return;
 
-            if (!this.IsAssert(hasClassicAssert, invocationOperation))
+            if (!this.IsAssert(nunitVersion.Major >= 4, invocationOperation))
                 return;
 
             context.CancellationToken.ThrowIfCancellationRequested();
 
-            this.AnalyzeAssertInvocation(context, invocationOperation);
+            this.AnalyzeAssertInvocation(nunitVersion, context, invocationOperation);
         }
 
         private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
@@ -46,12 +54,11 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
 
             if (nunit is null)
             {
+                // Who would use NUnit.Analyzers without NUnit?
                 return;
             }
 
-            bool hasClassicAssert = nunit.Version.Major >= 4;
-
-            context.RegisterOperationAction((context) => this.AnalyzeInvocation(hasClassicAssert, context), OperationKind.Invocation);
+            context.RegisterOperationAction((context) => this.AnalyzeInvocation(nunit.Version, context), OperationKind.Invocation);
         }
     }
 }