diff --git a/documentation/NUnit2050.md b/documentation/NUnit2050.md index 2ec0da35..f39cc3e8 100644 --- a/documentation/NUnit2050.md +++ b/documentation/NUnit2050.md @@ -28,9 +28,15 @@ This analyzer needs to be run when still building against NUnit3 as otherwise yo When usages of the new methods with `params` are detected, the associated CodeFix will convert the format specification into an interpolated string. +Once you moved to NUnit4 the analyzer has some limited functionality as there are a few +cases where your NUnit3 code will compile on NUnit4, but not the way you want it. +Here what you think are parameters to a format specification are actually interpreted as +the _actual_ and _constraint_ expression strings. +Unfortunately you only find that out when the test fails, which could be never. + ## How to fix violations -The following code, valid in NUNit3: +The following code, valid in NUnit3: ```csharp [TestCase(4)] @@ -60,13 +66,68 @@ public void MustBeMultipleOf3(int value) The failure message for NUnit4 becomes: -``` +```csharp Expected value (4) to be multiple of 3 Assert.That(value % 3, Is.Zero) Expected: 0 But was: 1 ``` +As the `[CallerMemberExpression]` parameters are `string`, some of NUnit 3.x code compiles, but when failing show the wrong message: + +```csharp +[TestCase("NUnit 4", "NUnit 3")] +public void TestMessage(string actual, string expected) +{ + Assert.That(actual, Is.EqualTo(expected), "Expected '{0}', but got: '{1}'", expected, actual); +} +``` + +When using NUnit3, this results in: + +``` + Expected 'NUnit 3', but got: 'NUnit 4' + String lengths are both 7. Strings differ at index 6. + Expected: "NUnit 3" + But was: "NUnit 4" + -----------------^ +``` + +But when using NUnit4, we get: + +``` + Message:  + Expected '{0}', but got: '{1}' +Assert.That(NUnit 3, NUnit 4) + String lengths are both 7. Strings differ at index 6. + Expected: "NUnit 3" + But was: "NUnit 4" + -----------------^ +``` + +Where the format string is treated as the _message_ and its arguments are interpreted as the _actual_ and _expected_ expressions! +After applying the code fix the code looks like: + +```csharp +[TestCase("NUnit 4", "NUnit 3")] +public void TestMessage(string actual, string expected) +{ + Assert.That(actual, Is.EqualTo(expected), $"Expected '{expected}', but got: '{actual}'"); +} +``` + +and the output: + +``` + Message:  + Expected 'NUnit 3', but got: 'NUnit 4' +Assert.That(actual, Is.EqualTo(expected)) + String lengths are both 7. Strings differ at index 6. + Expected: "NUnit 3" + But was: "NUnit 4" + -----------------^ +``` + ## Configure severity diff --git a/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzerTests.cs b/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzerTests.cs index 8f927f2e..7b0ae1d0 100644 --- a/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzerTests.cs +++ b/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzerTests.cs @@ -12,9 +12,7 @@ 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 NoArgumentsAsserts { get; } = new[] { @@ -92,7 +90,6 @@ public void AnalyzeAssertBoolWhenOnlyMessageArgumentIsUsed() RoslynAssert.Valid(this.analyzer, testCode); } -#if !NUNIT4 [Test] public void AnalyzeAssertBoolWhenFormatAndArgumentsAreUsed() { @@ -101,7 +98,6 @@ public void AnalyzeAssertBoolWhenFormatAndArgumentsAreUsed() "); RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode); } -#endif [Test] public void AnalyzeAssertBoolWhenFormattableStringIsUsed() @@ -132,6 +128,19 @@ public void AnalyzeAssertThatWhenOnlyMessageArgumentIsUsed() RoslynAssert.Valid(this.analyzer, testCode); } + [Test] + public void AnalyzeAssertThatWhenFormatAndStringArgumentsAreUsed() + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@" + [TestCase(""NUnit 4.0"", ""NUnit 3.14"")] + public void AssertSomething(string actual, string expected) + { + ↓Assert.That(actual, Is.EqualTo(expected).IgnoreCase, ""Expected '{0}', but got: {1}"", expected, actual); + }"); + + RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode); + } + #if !NUNIT4 [Test] public void AnalyzeAssertThatWhenFormatAndArgumentsAreUsed() diff --git a/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs b/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs index 179f3103..2aeb0e6d 100644 --- a/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs +++ b/src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs @@ -1,5 +1,3 @@ -#if !NUNIT4 - using Gu.Roslyn.Asserts; using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.Diagnostics; @@ -25,6 +23,25 @@ public void VerifyGetFixableDiagnosticIds() Assert.That(ids, Is.EquivalentTo(new[] { AnalyzerIdentifiers.UpdateStringFormatToInterpolatableString })); } +#if NUNIT4 + [Test] + public void AccidentallyUseFormatSpecification() + { + var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@" + [TestCase(""NUnit 4.0"", ""NUnit 3.14"")] + public void AssertSomething(string actual, string expected) + { + ↓Assert.That(actual, Is.EqualTo(expected).IgnoreCase, ""Expected '{0}', but got: {1}"", expected, actual); + }"); + var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@" + [TestCase(""NUnit 4.0"", ""NUnit 3.14"")] + public void AssertSomething(string actual, string expected) + { + Assert.That(actual, Is.EqualTo(expected).IgnoreCase, $""Expected '{expected}', but got: {actual}""); + }"); + RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode); + } +#else [TestCase(NUnitFrameworkConstants.NameOfAssertIgnore)] public void ConvertWhenNoMinimumParameters(string method) { @@ -107,6 +124,7 @@ public void TestEscapedBraces() RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode); } +#endif + } } -#endif diff --git a/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj b/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj index 14e0124f..54022203 100644 --- a/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj +++ b/src/nunit.analyzers.tests/nunit.analyzers.tests.csproj @@ -26,7 +26,7 @@ - + diff --git a/src/nunit.analyzers/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzer.cs b/src/nunit.analyzers/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzer.cs index 3f4efa4d..980e6e1d 100644 --- a/src/nunit.analyzers/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzer.cs +++ b/src/nunit.analyzers/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzer.cs @@ -42,13 +42,21 @@ protected override bool IsAssert(bool hasClassicAssert, IInvocationOperation inv protected override void AnalyzeAssertInvocation(Version nunitVersion, OperationAnalysisContext context, IInvocationOperation assertOperation) { - if (nunitVersion.Major >= 4) + if (nunitVersion.Major < 4) { - // Too late, this won't work as the method with the `params` parameter doesn't exists - // and won't be resolved by the compiler. - return; + AnalyzeNUnit3AssertInvocation(context, assertOperation); + } + else + { + AnalyzeNUnit4AssertInvocation(context, assertOperation); } + } + /// + /// This looks to see if the `params` overload is called. + /// + private static void AnalyzeNUnit3AssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation) + { int lastParameterIndex = assertOperation.TargetMethod.Parameters.Length - 1; if (lastParameterIndex > 0 && assertOperation.TargetMethod.Parameters[lastParameterIndex].IsParams) { @@ -57,21 +65,10 @@ protected override void AnalyzeAssertInvocation(Version nunitVersion, OperationA { string methodName = assertOperation.TargetMethod.Name; - if (!ObsoleteParamsMethods.Contains(methodName)) + if (ObsoleteParamsMethods.Contains(methodName)) { - return; + ReportDiagnostic(context, assertOperation, methodName, lastParameterIndex - 1); } - - int minimumNumberOfArguments = lastParameterIndex - 1; - - context.ReportDiagnostic(Diagnostic.Create( - updateStringFormatToInterpolatableString, - assertOperation.Syntax.GetLocation(), - new Dictionary - { - [AnalyzerPropertyKeys.ModelName] = methodName, - [AnalyzerPropertyKeys.MinimumNumberOfArguments] = minimumNumberOfArguments.ToString(CultureInfo.InvariantCulture), - }.ToImmutableDictionary())); } } @@ -87,5 +84,60 @@ static bool IsNonEmptyParamsArrayArgument(IArgumentOperation argument) return argument.ArgumentKind == ArgumentKind.Explicit; } } + + /// + /// This looks to see if the `CallerMemberExpression` parameters are explicitly specified. + /// + private static void AnalyzeNUnit4AssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation) + { + string methodName = assertOperation.TargetMethod.Name; + + if (methodName != NUnitFrameworkConstants.NameOfAssertThat) + { + // Only Assert.That has CallerMemberExpression that could be accidentally used as format paramaters + return; + } + + // Find the 'message' parameter. + ImmutableArray parameters = assertOperation.TargetMethod.Parameters; + int formatParameterIndex = 1; + for (; formatParameterIndex < parameters.Length; formatParameterIndex++) + { + IParameterSymbol parameter = parameters[formatParameterIndex]; + if (parameter.IsOptional) + { + if (parameter.Name == "message") + break; + + // Overload with FormattableString or Func overload + return; + } + } + + ImmutableArray arguments = assertOperation.Arguments; + if (arguments.Length > formatParameterIndex && arguments[formatParameterIndex + 1].ArgumentKind == ArgumentKind.Explicit) + { + // The argument after the message is explicitly specified + // Most likely the user thought it was using a format specification with a parameter. + // Or it copied code from some NUnit 3.x source into an NUNit 4 project. + ReportDiagnostic(context, assertOperation, methodName, formatParameterIndex); + } + } + + private static void ReportDiagnostic( + OperationAnalysisContext context, + IInvocationOperation assertOperation, + string methodName, + int minimumNumberOfArguments) + { + context.ReportDiagnostic(Diagnostic.Create( + updateStringFormatToInterpolatableString, + assertOperation.Syntax.GetLocation(), + new Dictionary + { + [AnalyzerPropertyKeys.ModelName] = methodName, + [AnalyzerPropertyKeys.MinimumNumberOfArguments] = minimumNumberOfArguments.ToString(CultureInfo.InvariantCulture), + }.ToImmutableDictionary())); + } } }