From 51d5c178869e774d29a19a82f38d69a47ce1c238 Mon Sep 17 00:00:00 2001 From: Brad Wilson Date: Wed, 8 Nov 2023 18:26:10 -0800 Subject: [PATCH] xunit/xunit#2346: Add xUnit2021 to warn when async assertions are not being awaited/stored --- .../X2000/AsyncAssertsShouldBeAwaitedFixer.cs | 73 +++++++++ .../X2000/AsyncAssertsShouldBeAwaitedTests.cs | 138 ++++++++++++++++++ .../AsyncAssertsShouldBeAwaitedFixerTests.cs | 52 +++++++ src/xunit.analyzers/Utility/Constants.cs | 3 + src/xunit.analyzers/Utility/Descriptors.cs | 9 +- .../X2000/AsyncAssertsShouldBeAwaited.cs | 64 ++++++++ 6 files changed, 338 insertions(+), 1 deletion(-) create mode 100644 src/xunit.analyzers.fixes/X2000/AsyncAssertsShouldBeAwaitedFixer.cs create mode 100644 src/xunit.analyzers.tests/Analyzers/X2000/AsyncAssertsShouldBeAwaitedTests.cs create mode 100644 src/xunit.analyzers.tests/Fixes/X2000/AsyncAssertsShouldBeAwaitedFixerTests.cs create mode 100644 src/xunit.analyzers/X2000/AsyncAssertsShouldBeAwaited.cs diff --git a/src/xunit.analyzers.fixes/X2000/AsyncAssertsShouldBeAwaitedFixer.cs b/src/xunit.analyzers.fixes/X2000/AsyncAssertsShouldBeAwaitedFixer.cs new file mode 100644 index 00000000..49b01cf9 --- /dev/null +++ b/src/xunit.analyzers.fixes/X2000/AsyncAssertsShouldBeAwaitedFixer.cs @@ -0,0 +1,73 @@ +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; +using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory; + +namespace Xunit.Analyzers.Fixes; + +[ExportCodeFixProvider(LanguageNames.CSharp), Shared] +public class AsyncAssertsShouldBeAwaitedFixer : BatchedCodeFixProvider +{ + public const string Key_AddAwait = "xUnit2021_AddAwait"; + + public AsyncAssertsShouldBeAwaitedFixer() : + base(Descriptors.X2021_AsyncAssertionsShouldBeAwaited.Id) + { } + + public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) + return; + + var invocation = root.FindNode(context.Span).FirstAncestorOrSelf(); + if (invocation is null) + return; + + var method = invocation.FirstAncestorOrSelf(); + if (method is null) + return; + + var diagnostic = context.Diagnostics.FirstOrDefault(); + if (diagnostic is null) + return; + + context.RegisterCodeFix( + CodeAction.Create( + "Add await", + ct => UseAsyncAwait(context.Document, invocation, method, ct), + Key_AddAwait + ), + context.Diagnostics + ); + } + + static async Task UseAsyncAwait( + Document document, + InvocationExpressionSyntax invocation, + MethodDeclarationSyntax method, + CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + var modifiers = AsyncHelper.GetModifiersWithAsyncKeywordAdded(method); + var returnType = await AsyncHelper.GetReturnType(method, invocation, document, editor, cancellationToken); + var asyncThrowsInvocation = AwaitExpression(invocation.WithoutLeadingTrivia()).WithLeadingTrivia(invocation.GetLeadingTrivia()); + + if (returnType is not null) + editor.ReplaceNode( + method, + method + .ReplaceNode(invocation, asyncThrowsInvocation) + .WithModifiers(modifiers) + .WithReturnType(returnType) + ); + + return editor.GetChangedDocument(); + } +} diff --git a/src/xunit.analyzers.tests/Analyzers/X2000/AsyncAssertsShouldBeAwaitedTests.cs b/src/xunit.analyzers.tests/Analyzers/X2000/AsyncAssertsShouldBeAwaitedTests.cs new file mode 100644 index 00000000..8b10a7cd --- /dev/null +++ b/src/xunit.analyzers.tests/Analyzers/X2000/AsyncAssertsShouldBeAwaitedTests.cs @@ -0,0 +1,138 @@ +using System.Globalization; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Xunit; +using Verify = CSharpVerifier; + +public class AsyncAssertsShouldBeAwaitedTests +{ + [Fact] + public async void UnawaitedNonAssertionDoesNotTrigger() + { + var code = @" +using System.Threading.Tasks; +using Xunit; + +public class TestClass { + [Fact] + public void TestMethod() { + Task.Delay(1); + } +}"; + + await Verify.VerifyAnalyzer(code); + } + + string codeTemplate = @" +using System; +using System.ComponentModel; +using System.Threading.Tasks; +using Xunit; + +public class TestClass : INotifyPropertyChanged {{ + public int Property {{ get; set; }} + + public event PropertyChangedEventHandler? PropertyChanged; + public event EventHandler? SimpleEvent; + public event EventHandler? SimpleIntEvent; + + [Fact] + public async void TestMethod() {{ + {0} + }} +}} + +public static class MyTaskExtensions {{ + public static void ConsumeTask(this Task t) {{ }} +}}"; + + public static TheoryData AsyncAssertions = new() + { + { "PropertyChangedAsync", "Assert.PropertyChangedAsync(this, nameof(Property), async () => throw new DivideByZeroException())" }, + { "RaisesAnyAsync", "Assert.RaisesAnyAsync(eh => SimpleEvent += eh, eh => SimpleEvent -= eh, async () => throw new DivideByZeroException())" }, + { "RaisesAnyAsync", "Assert.RaisesAnyAsync(eh => SimpleIntEvent += eh, eh => SimpleIntEvent -= eh, async () => throw new DivideByZeroException())" }, + { "RaisesAsync", "Assert.RaisesAsync(eh => SimpleIntEvent += eh, eh => SimpleIntEvent -= eh, async () => throw new DivideByZeroException())" }, + { "ThrowsAnyAsync", "Assert.ThrowsAnyAsync(async () => throw new DivideByZeroException())" }, + { "ThrowsAsync", "Assert.ThrowsAsync(typeof(DivideByZeroException), async () => throw new DivideByZeroException())" }, + { "ThrowsAsync", "Assert.ThrowsAsync(async () => throw new DivideByZeroException())" }, + { "ThrowsAsync", "Assert.ThrowsAsync(\"argName\", async () => throw new DivideByZeroException())" }, + }; + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AwaitedAssertDoesNotTrigger( + string _, + string assertion) + { + var code = string.Format(CultureInfo.InvariantCulture, codeTemplate, $"await {assertion};"); + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp8, code); + } + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AssertionWithConsumptionNotTrigger( + string _, + string assertion) + { + var code = string.Format(CultureInfo.InvariantCulture, codeTemplate, $"MyTaskExtensions.ConsumeTask({assertion});"); + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp8, code); + } + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AssertionWithConsumptionViaExtensionNotTrigger( + string _, + string assertion) + { + var code = string.Format(CultureInfo.InvariantCulture, codeTemplate, $"{assertion}.ConsumeTask();"); + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp8, code); + } + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AssertionWithStoredTaskDoesNotTrigger( + string _, + string assertion) + { + var code = string.Format(CultureInfo.InvariantCulture, codeTemplate, $"var task = {assertion};"); + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp8, code); + } + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AssertionWithoutAwaitTriggers( + string assertionName, + string assertion) + { + var code = string.Format(CultureInfo.InvariantCulture, codeTemplate, $"{assertion};"); + var expected = + Verify + .Diagnostic() + .WithSpan(16, 9, 16, 9 + assertion.Length) + .WithSeverity(DiagnosticSeverity.Error) + .WithArguments(assertionName); + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp8, code, expected); + } + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AssertionWithUnawaitedContinuationTriggers( + string assertionName, + string assertion) + { + var code = string.Format(CultureInfo.InvariantCulture, codeTemplate, $"{assertion}.ContinueWith(t => {{ }});"); + var expected = + Verify + .Diagnostic() + .WithSpan(16, 9, 16, 9 + assertion.Length) + .WithSeverity(DiagnosticSeverity.Error) + .WithArguments(assertionName); + + await Verify.VerifyAnalyzer(LanguageVersion.CSharp8, code, expected); + } +} diff --git a/src/xunit.analyzers.tests/Fixes/X2000/AsyncAssertsShouldBeAwaitedFixerTests.cs b/src/xunit.analyzers.tests/Fixes/X2000/AsyncAssertsShouldBeAwaitedFixerTests.cs new file mode 100644 index 00000000..970ebd5a --- /dev/null +++ b/src/xunit.analyzers.tests/Fixes/X2000/AsyncAssertsShouldBeAwaitedFixerTests.cs @@ -0,0 +1,52 @@ +using Microsoft.CodeAnalysis.CSharp; +using Xunit; +using Xunit.Analyzers.Fixes; +using Verify = CSharpVerifier; + +public class AsyncAssertsShouldBeAwaitedFixerTests +{ + string codeTemplate = @" +using System; +using System.ComponentModel; +using System.Threading.Tasks; +using Xunit; + +public class TestClass : INotifyPropertyChanged {{ + public int Property {{ get; set; }} + + public event PropertyChangedEventHandler? PropertyChanged; + public event EventHandler? SimpleEvent; + public event EventHandler? SimpleIntEvent; + + [Fact] + public void TestMethod() {{ + {0} + }} +}} + +public static class MyTaskExtensions {{ + public static void ConsumeTask(this Task t) {{ }} +}}"; + + public static TheoryData AsyncAssertions = new() + { + "Assert.PropertyChangedAsync(this, nameof(Property), async () => throw new DivideByZeroException())", + "Assert.RaisesAnyAsync(eh => SimpleEvent += eh, eh => SimpleEvent -= eh, async () => throw new DivideByZeroException())", + "Assert.RaisesAnyAsync(eh => SimpleIntEvent += eh, eh => SimpleIntEvent -= eh, async () => throw new DivideByZeroException())", + "Assert.RaisesAsync(eh => SimpleIntEvent += eh, eh => SimpleIntEvent -= eh, async () => throw new DivideByZeroException())", + "Assert.ThrowsAnyAsync(async () => throw new DivideByZeroException())", + "Assert.ThrowsAsync(typeof(DivideByZeroException), async () => throw new DivideByZeroException())", + "Assert.ThrowsAsync(async () => throw new DivideByZeroException())", + "Assert.ThrowsAsync(\"argName\", async () => throw new DivideByZeroException())", + }; + + [Theory] + [MemberData(nameof(AsyncAssertions))] + public async void AddsAsyncAndAwait(string assertion) + { + var before = string.Format(codeTemplate, $"[|{assertion}|];"); + var after = string.Format(codeTemplate, $"await {assertion};").Replace("public void TestMethod", "public async Task TestMethod"); + + await Verify.VerifyCodeFix(LanguageVersion.CSharp8, before, after, AsyncAssertsShouldBeAwaitedFixer.Key_AddAwait); + } +} diff --git a/src/xunit.analyzers/Utility/Constants.cs b/src/xunit.analyzers/Utility/Constants.cs index f6348a88..743633c4 100644 --- a/src/xunit.analyzers/Utility/Constants.cs +++ b/src/xunit.analyzers/Utility/Constants.cs @@ -37,6 +37,9 @@ public static class Asserts public const string NotSame = "NotSame"; public const string NotStrictEqual = "NotStrictEqual"; public const string Null = "Null"; + public const string PropertyChangedAsync = "PropertyChangedAsync"; + public const string RaisesAnyAsync = "RaisesAnyAsync"; + public const string RaisesAsync = "RaisesAsync"; public const string Same = "Same"; public const string Single = "Single"; public const string StartsWith = "StartsWith"; diff --git a/src/xunit.analyzers/Utility/Descriptors.cs b/src/xunit.analyzers/Utility/Descriptors.cs index bf25537b..effae1c9 100644 --- a/src/xunit.analyzers/Utility/Descriptors.cs +++ b/src/xunit.analyzers/Utility/Descriptors.cs @@ -606,7 +606,14 @@ public static DiagnosticDescriptor X2019_AssertThrowsShouldNotBeUsedForAsyncThro "Do not use Assert.{0}({1}, message) to fail a test. Use Assert.Fail(message) instead." ); - // Placeholder for rule X2021 + public static DiagnosticDescriptor X2021_AsyncAssertionsShouldBeAwaited { get; } = + Rule( + "xUnit2021", + "Async assertions should be awaited", + Assertions, + Error, + "Assert.{0} is async. The resulting task should be awaited (or stored for later awaiting)." + ); // Placeholder for rule X2022 diff --git a/src/xunit.analyzers/X2000/AsyncAssertsShouldBeAwaited.cs b/src/xunit.analyzers/X2000/AsyncAssertsShouldBeAwaited.cs new file mode 100644 index 00000000..3135471a --- /dev/null +++ b/src/xunit.analyzers/X2000/AsyncAssertsShouldBeAwaited.cs @@ -0,0 +1,64 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Xunit.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class AsyncAssertsShouldBeAwaited : AssertUsageAnalyzerBase +{ + static readonly string[] targetMethods = + { + Constants.Asserts.PropertyChangedAsync, + Constants.Asserts.RaisesAnyAsync, + Constants.Asserts.RaisesAsync, + Constants.Asserts.ThrowsAnyAsync, + Constants.Asserts.ThrowsAsync, + }; + + public AsyncAssertsShouldBeAwaited() : + base(Descriptors.X2021_AsyncAssertionsShouldBeAwaited, targetMethods) + { } + + protected override void AnalyzeInvocation( + OperationAnalysisContext context, + XunitContext xunitContext, + IInvocationOperation invocationOperation, + IMethodSymbol method) + { + var taskType = TypeSymbolFactory.Task(context.Compilation); + var taskOfTType = TypeSymbolFactory.TaskOfT(context.Compilation)?.ConstructUnboundGenericType(); + + for (IOperation? current = invocationOperation; current is not null; current = current?.Parent) + { + // Stop looking when we've hit the enclosing block + if (current is IBlockOperation) + return; + + // Only interested in something that results in an expression to a named type + if (current is not IExpressionStatementOperation expression || expression.Operation.Type is not INamedTypeSymbol namedReturnType) + continue; + + if (namedReturnType.IsGenericType) + { + // Does it return Task? + if (!SymbolEqualityComparer.Default.Equals(namedReturnType.ConstructUnboundGenericType(), taskOfTType)) + continue; + } + else + { + // Does it return Task? + if (!SymbolEqualityComparer.Default.Equals(namedReturnType, taskType)) + continue; + } + + context.ReportDiagnostic( + Diagnostic.Create( + Descriptors.X2021_AsyncAssertionsShouldBeAwaited, + invocationOperation.Syntax.GetLocation(), + method.Name + ) + ); + } + } +}