Fixes xunit/xunit#1423: Analyzers should not just tell you you're doi…

…ng it wrong but what to do instead
xunit · Sep 18, 2021 · 39aa196 · 39aa196
1 parent c5586d4
commit 39aa196
Show file tree

Hide file tree

Showing 37 changed files with 363 additions and 265 deletions.
diff --git a/src/xunit.analyzers.fixes/AssertCollectionContainsShouldNotUseBoolCheckFixer.cs b/src/xunit.analyzers.fixes/AssertCollectionContainsShouldNotUseBoolCheckFixer.cs
@@ -28,8 +28,8 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
 			var invocation = root.FindNode(context.Span).FirstAncestorOrSelf<InvocationExpressionSyntax>();
 			var diagnostic = context.Diagnostics.First();
-			var assertMethodName = diagnostic.Properties[Constants.Properties.MethodName];
-			var replacement = assertMethodName == Constants.Asserts.True ? Constants.Asserts.Contains : Constants.Asserts.DoesNotContain;
+			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 			var title = string.Format(titleTemplate, replacement);
 
 			context.RegisterCodeFix(

diff --git a/...analyzers.fixes/AssertEnumerableAnyCheckShouldNotBeUsedForCollectionContainsCheckFixer.cs b/...analyzers.fixes/AssertEnumerableAnyCheckShouldNotBeUsedForCollectionContainsCheckFixer.cs
@@ -29,7 +29,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var invocation = root.FindNode(context.Span).FirstAncestorOrSelf<InvocationExpressionSyntax>();
 			var diagnostic = context.Diagnostics.First();
 			var assertMethodName = diagnostic.Properties[Constants.Properties.AssertMethodName];
-			var replacement = assertMethodName == Constants.Asserts.True ? Constants.Asserts.Contains : Constants.Asserts.DoesNotContain;
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 
 			var title = string.Format(titleTemplate, replacement);
 			context.RegisterCodeFix(

diff --git a/src/xunit.analyzers.fixes/AssertEqualShouldNotBeUsedForBoolLiteralCheckFixer.cs b/src/xunit.analyzers.fixes/AssertEqualShouldNotBeUsedForBoolLiteralCheckFixer.cs
@@ -30,7 +30,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var diagnostic = context.Diagnostics.First();
 			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
 			var literalValue = diagnostic.Properties[Constants.Properties.LiteralValue];
-			var replacement = GetReplacementMethodName(methodName, literalValue);
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 
 			if (replacement != null && invocation.Expression is MemberAccessExpressionSyntax)
 			{
@@ -46,18 +46,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			}
 		}
 
-		static string GetReplacementMethodName(
-			string methodName,
-			string literalValue)
-		{
-			if (AssertEqualShouldNotBeUsedForBoolLiteralCheck.EqualMethods.Contains(methodName))
-				return literalValue == bool.TrueString ? Constants.Asserts.True : Constants.Asserts.False;
-			if (AssertEqualShouldNotBeUsedForBoolLiteralCheck.NotEqualMethods.Contains(methodName))
-				return literalValue == bool.TrueString ? Constants.Asserts.False : Constants.Asserts.True;
-
-			return null;
-		}
-
 		static async Task<Document> UseBoolCheckAsync(
 			Document document,
 			InvocationExpressionSyntax invocation,

diff --git a/src/xunit.analyzers.fixes/AssertEqualShouldNotBeUsedForCollectionSizeCheckFixer.cs b/src/xunit.analyzers.fixes/AssertEqualShouldNotBeUsedForCollectionSizeCheckFixer.cs
@@ -30,7 +30,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var diagnostic = context.Diagnostics.First();
 			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
 			var sizeValue = diagnostic.Properties[Constants.Properties.SizeValue];
-			var replacement = GetReplacementMethodName(methodName, sizeValue);
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 			var title = string.Format(titleTemplate, replacement);
 
 			context.RegisterCodeFix(
@@ -43,16 +43,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			);
 		}
 
-		static string GetReplacementMethodName(
-			string methodName,
-			string literalValue)
-		{
-			if (literalValue == "1")
-				return Constants.Asserts.Single;
-
-			return methodName == Constants.Asserts.Equal ? Constants.Asserts.Empty : Constants.Asserts.NotEmpty;
-		}
-
 		static async Task<Document> UseCollectionSizeAssertionAsync(
 			Document document,
 			InvocationExpressionSyntax invocation,

diff --git a/src/xunit.analyzers.fixes/AssertEqualShouldNotBeUsedForNullCheckFixer.cs b/src/xunit.analyzers.fixes/AssertEqualShouldNotBeUsedForNullCheckFixer.cs
@@ -29,7 +29,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var invocation = root.FindNode(context.Span).FirstAncestorOrSelf<InvocationExpressionSyntax>();
 			var diagnostic = context.Diagnostics.First();
 			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
-			var replacement = GetReplacementMethod(methodName);
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 
 			if (replacement != null && invocation.Expression is MemberAccessExpressionSyntax)
 			{
@@ -45,16 +45,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			}
 		}
 
-		static string GetReplacementMethod(string methodName)
-		{
-			if (AssertEqualShouldNotBeUsedForNullCheck.EqualMethods.Contains(methodName))
-				return Constants.Asserts.Null;
-			if (AssertEqualShouldNotBeUsedForNullCheck.NotEqualMethods.Contains(methodName))
-				return Constants.Asserts.NotNull;
-
-			return null;
-		}
-
 		static async Task<Document> UseNullCheckAsync(
 			Document document,
 			InvocationExpressionSyntax invocation,

diff --git a/src/xunit.analyzers.fixes/AssertEqualsShouldNotBeUsedFixer.cs b/src/xunit.analyzers.fixes/AssertEqualsShouldNotBeUsedFixer.cs
@@ -25,12 +25,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
 			var invocation = root.FindNode(context.Span).FirstAncestorOrSelf<InvocationExpressionSyntax>();
 			var diagnostic = context.Diagnostics.First();
-			var replacement = diagnostic.Properties[Constants.Properties.MethodName] switch
-			{
-				nameof(object.Equals) => Constants.Asserts.Equal,
-				nameof(object.ReferenceEquals) => Constants.Asserts.Same,
-				_ => null,
-			};
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 
 			if (replacement != null && invocation.Expression is MemberAccessExpressionSyntax)
 			{

diff --git a/src/xunit.analyzers.fixes/AssertRegexMatchShouldNotUseBoolLiteralCheckFixer.cs b/src/xunit.analyzers.fixes/AssertRegexMatchShouldNotUseBoolLiteralCheckFixer.cs
@@ -30,7 +30,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var diagnostic = context.Diagnostics.First();
 			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
 			var isStatic = diagnostic.Properties[Constants.Properties.IsStatic];
-			var replacementMethod = methodName == Constants.Asserts.True ? Constants.Asserts.Matches : Constants.Asserts.DoesNotMatch;
+			var replacementMethod = diagnostic.Properties[Constants.Properties.Replacement];
 			var title = string.Format(titleTemplate, replacementMethod);
 
 			context.RegisterCodeFix(

diff --git a/src/xunit.analyzers.fixes/AssertSameShouldNotBeCalledOnValueTypesFixer.cs b/src/xunit.analyzers.fixes/AssertSameShouldNotBeCalledOnValueTypesFixer.cs
@@ -23,14 +23,10 @@ public sealed override FixAllProvider GetFixAllProvider() =>
 		public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 		{
 			var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
-			var methodName = context.Diagnostics.First().Properties[Constants.Properties.MethodName];
+			var diagnostic = context.Diagnostics.First();
+			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
 			var invocation = root.FindNode(context.Span).FirstAncestorOrSelf<InvocationExpressionSyntax>();
-			var replacement = methodName switch
-			{
-				Constants.Asserts.Same => Constants.Asserts.Equal,
-				Constants.Asserts.NotSame => Constants.Asserts.NotEqual,
-				_ => null,
-			};
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 
 			if (replacement != null && invocation.Expression is MemberAccessExpressionSyntax)
 			{

diff --git a/src/xunit.analyzers.fixes/AssertStringEqualityCheckShouldNotUseBoolCheckFixer.cs b/src/xunit.analyzers.fixes/AssertStringEqualityCheckShouldNotUseBoolCheckFixer.cs
@@ -33,7 +33,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var assertMethodName = diagnostic.Properties[Constants.Properties.AssertMethodName];
 			var isStaticMethodCall = diagnostic.Properties[Constants.Properties.IsStaticMethodCall];
 			var ignoreCase = diagnostic.Properties[Constants.Properties.IgnoreCase];
-			var replacement = GetReplacementMethodName(assertMethodName);
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 
 			context.RegisterCodeFix(
 				CodeAction.Create(
@@ -45,9 +45,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			);
 		}
 
-		static string GetReplacementMethodName(string assertMethodName) =>
-			assertMethodName == Constants.Asserts.True ? Constants.Asserts.Equal : Constants.Asserts.NotEqual;
-
 		static async Task<Document> UseEqualCheck(
 			Document document,
 			InvocationExpressionSyntax invocation,

diff --git a/src/xunit.analyzers.fixes/AssertSubstringCheckShouldNotUseBoolCheckFixer.cs b/src/xunit.analyzers.fixes/AssertSubstringCheckShouldNotUseBoolCheckFixer.cs
@@ -30,7 +30,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var diagnostic = context.Diagnostics.First();
 			var assertMethodName = diagnostic.Properties[Constants.Properties.AssertMethodName];
 			var substringMethodName = diagnostic.Properties[Constants.Properties.SubstringMethodName];
-			var replacement = GetReplacementMethodName(assertMethodName, substringMethodName);
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
 			var title = string.Format(titleTemplate, replacement);
 
 			context.RegisterCodeFix(
@@ -43,16 +43,6 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			);
 		}
 
-		static string GetReplacementMethodName(
-			string assertMethodName,
-			string substringMethodName)
-		{
-			if (substringMethodName == nameof(string.Contains))
-				return assertMethodName == Constants.Asserts.True ? Constants.Asserts.Contains : Constants.Asserts.DoesNotContain;
-
-			return substringMethodName;
-		}
-
 		static async Task<Document> UseSubstringCheckAsync(
 			Document document,
 			InvocationExpressionSyntax invocation,

diff --git a/src/xunit.analyzers.fixes/AssertThrowsShouldNotBeUsedForAsyncThrowsCheckFixer.cs b/src/xunit.analyzers.fixes/AssertThrowsShouldNotBeUsedForAsyncThrowsCheckFixer.cs
@@ -31,12 +31,13 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
 			var method = invocation.FirstAncestorOrSelf<MethodDeclarationSyntax>();
 			var diagnostic = context.Diagnostics.First();
 			var methodName = diagnostic.Properties[Constants.Properties.MethodName];
-			var title = string.Format(TitleTemplate, methodName + "Async");
+			var replacement = diagnostic.Properties[Constants.Properties.Replacement];
+			var title = string.Format(TitleTemplate, replacement);
 
 			context.RegisterCodeFix(
 				CodeAction.Create(
 					title,
-					createChangedDocument: ct => UseAsyncThrowsCheck(context.Document, invocation, method, methodName + "Async", ct),
+					createChangedDocument: ct => UseAsyncThrowsCheck(context.Document, invocation, method, replacement, ct),
 					equivalenceKey: title
 				),
 				context.Diagnostics

diff --git a/src/xunit.analyzers.tests/Analyzers/AssertCollectionContainsShouldNotUseBoolCheckTests.cs b/src/xunit.analyzers.tests/Analyzers/AssertCollectionContainsShouldNotUseBoolCheckTests.cs
@@ -1,4 +1,5 @@
 using Xunit;
+using Xunit.Analyzers;
 using Verify = CSharpVerifier<Xunit.Analyzers.AssertCollectionContainsShouldNotUseBoolCheck>;
 
 public class AssertCollectionContainsShouldNotUseBoolCheckTests
@@ -29,7 +30,7 @@ void TestMethod() {{
 			Verify
 				.Diagnostic()
 				.WithSpan(4, 9, 4, 40 + collection.Length)
-				.WithArguments("Assert.True()");
+				.WithArguments("Assert.True()", Constants.Asserts.Contains);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -48,7 +49,7 @@ void TestMethod() {{
 			Verify
 				.Diagnostic()
 				.WithSpan(4, 9, 4, 41 + collection.Length)
-				.WithArguments("Assert.False()");
+				.WithArguments("Assert.False()", Constants.Asserts.DoesNotContain);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -69,7 +70,7 @@ void TestMethod() {{
 			Verify
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 40 + enumerable.Length)
-				.WithArguments("Assert.True()");
+				.WithArguments("Assert.True()", Constants.Asserts.Contains);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -90,7 +91,7 @@ void TestMethod() {{
 			Verify
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 98 + enumerable.Length)
-				.WithArguments("Assert.True()");
+				.WithArguments("Assert.True()", Constants.Asserts.Contains);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -111,7 +112,7 @@ void TestMethod() {{
 			Verify
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 41 + enumerable.Length)
-				.WithArguments("Assert.False()");
+				.WithArguments("Assert.False()", Constants.Asserts.DoesNotContain);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -132,7 +133,7 @@ void TestMethod() {{
 			Verify
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 99 + enumerable.Length)
-				.WithArguments("Assert.False()");
+				.WithArguments("Assert.False()", Constants.Asserts.DoesNotContain);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}

diff --git a/src/xunit.analyzers.tests/Analyzers/AssertEqualShouldNotBeUsedForBoolLiteralCheckTests.cs b/src/xunit.analyzers.tests/Analyzers/AssertEqualShouldNotBeUsedForBoolLiteralCheckTests.cs
@@ -14,8 +14,13 @@ public class AssertEqualShouldNotBeUsedForBoolLiteralCheckTests
 	};
 
 	[Theory]
-	[MemberData(nameof(Methods))]
-	public async void FindsWarning_ForFirstBoolLiteral(string method)
+	[InlineData(Constants.Asserts.Equal, Constants.Asserts.True)]
+	[InlineData(Constants.Asserts.StrictEqual, Constants.Asserts.True)]
+	[InlineData(Constants.Asserts.NotEqual, Constants.Asserts.False)]
+	[InlineData(Constants.Asserts.NotStrictEqual, Constants.Asserts.False)]
+	public async void FindsWarning_ForFirstBoolLiteral(
+		string method,
+		string replacement)
 	{
 		var source = $@"
 class TestClass {{
@@ -29,15 +34,17 @@ void TestMethod() {{
 				.Diagnostic()
 				.WithSpan(5, 9, 5, 33 + method.Length)
 				.WithSeverity(DiagnosticSeverity.Warning)
-				.WithArguments($"Assert.{method}()");
+				.WithArguments($"Assert.{method}()", replacement);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
 
 	[Theory]
-	[InlineData(Constants.Asserts.Equal)]
-	[InlineData(Constants.Asserts.NotEqual)]
-	public async void FindsWarning_ForFirstBoolLiteral_WithCustomComparer(string method)
+	[InlineData(Constants.Asserts.Equal, Constants.Asserts.False)]
+	[InlineData(Constants.Asserts.NotEqual, Constants.Asserts.True)]
+	public async void FindsWarning_ForFirstBoolLiteral_WithCustomComparer(
+		string method,
+		string replacement)
 	{
 		var source = $@"
 class TestClass {{
@@ -51,7 +58,7 @@ void TestMethod() {{
 				.Diagnostic()
 				.WithSpan(5, 9, 5, 93 + method.Length)
 				.WithSeverity(DiagnosticSeverity.Warning)
-				.WithArguments($"Assert.{method}()");
+				.WithArguments($"Assert.{method}()", replacement);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}

diff --git a/src/xunit.analyzers.tests/Analyzers/AssertEqualShouldNotBeUsedForCollectionSizeCheckTests.cs b/src/xunit.analyzers.tests/Analyzers/AssertEqualShouldNotBeUsedForCollectionSizeCheckTests.cs
@@ -1,5 +1,6 @@
 using Microsoft.CodeAnalysis;
 using Xunit;
+using Xunit.Analyzers;
 using Verify = CSharpVerifier<Xunit.Analyzers.AssertEqualShouldNotBeUsedForCollectionSizeCheck>;
 
 public class AssertEqualShouldNotBeUsedForCollectionSizeCheckTests
@@ -66,7 +67,7 @@ void TestMethod() {{
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 32 + collection.Length)
 				.WithSeverity(DiagnosticSeverity.Warning)
-				.WithArguments("Assert.Equal()");
+				.WithArguments("Assert.Equal()", Constants.Asserts.Empty);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -88,7 +89,7 @@ void TestMethod() {{
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 35 + collection.Length)
 				.WithSeverity(DiagnosticSeverity.Warning)
-				.WithArguments("Assert.NotEqual()");
+				.WithArguments("Assert.NotEqual()", Constants.Asserts.NotEmpty);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -110,7 +111,7 @@ void TestMethod() {{
 				.Diagnostic()
 				.WithSpan(6, 9, 6, 32 + collection.Length)
 				.WithSeverity(DiagnosticSeverity.Warning)
-				.WithArguments("Assert.Equal()");
+				.WithArguments("Assert.Equal()", Constants.Asserts.Single);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}
@@ -145,7 +146,7 @@ void TestMethod() {
 				.Diagnostic()
 				.WithSpan(20, 9, 20, 51)
 				.WithSeverity(DiagnosticSeverity.Warning)
-				.WithArguments("Assert.Equal()");
+				.WithArguments("Assert.Equal()", Constants.Asserts.Single);
 
 		await Verify.VerifyAnalyzerAsync(source, expected);
 	}