Add code fix for MSTEST0040 — AvoidUsingAssertsInAsyncVoidContext#7892
Add code fix for MSTEST0040 — AvoidUsingAssertsInAsyncVoidContext#7892Copilot wants to merge 11 commits into
Conversation
Adds AvoidUsingAssertsInAsyncVoidContextFixer that offers 'Change return type to Task' for async void methods and async void local functions that contain Assert calls. Lambda/delegate cases are not fixable by this fixer since they require changing the delegate type as well. Also updates the test file to use the new fixer and adds expected fixed code for the fixable test cases. Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/d94e25ce-ce83-4da5-9989-a4d9d8e0b41b Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
…Task Also improve using insertion order (System namespaces before non-System) Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/d94e25ce-ce83-4da5-9989-a4d9d8e0b41b Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Address code review feedback: - Use SyntaxFactory.ElasticCarriageReturnLineFeed instead of hardcoded \r\n - Simplify using insertion logic with clearer default Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/d94e25ce-ce83-4da5-9989-a4d9d8e0b41b Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an MSTest analyzer code fix for MSTEST0040 to automatically remediate Assert usage in async void contexts by changing async void methods/local functions to async Task, plus the necessary localized code-fix title resources and unit test updates.
Changes:
- Introduces
AvoidUsingAssertsInAsyncVoidContextFixerto convertasync voidmethods and local functions toasync Taskand ensureSystem.Threading.Tasksis imported. - Adds the code-fix title string to
CodeFixResources.resxand propagates it to the.xlflocalization files. - Updates analyzer unit tests to use the new code fix provider and validate fixed output for method/local-function scenarios.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/MSTest.Analyzers.UnitTests/AvoidUsingAssertsInAsyncVoidContextAnalyzerTests.cs | Switches verifier to the new fixer and adds fixed-code expectations for method/local-function cases. |
| src/Analyzers/MSTest.Analyzers.CodeFixes/AvoidUsingAssertsInAsyncVoidContextFixer.cs | New code fix provider that changes async void to async Task and injects a System.Threading.Tasks using when needed. |
| src/Analyzers/MSTest.Analyzers.CodeFixes/CodeFixResources.resx | Adds localized string key for the new code fix title. |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.cs.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.de.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.es.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.fr.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.it.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ja.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ko.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pl.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pt-BR.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ru.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.tr.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hans.xlf | Adds localization entry for the new code fix title (state=new). |
| src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hant.xlf | Adds localization entry for the new code fix title (state=new). |
Copilot's findings
- Files reviewed: 16/16 changed files
- Comments generated: 3
There was a problem hiding this comment.
Summary
Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx
Key Findings
The code fix logic, fixer structure, and diagnostic tests are well-organized. The existing tests do verify the happy paths: async void method → async Task, async void local function → async Task, and no-fix for delegate/lambda contexts. The test names follow the established {AssertType}In{Context}_[No]Diagnostic convention.
However, there is one significant coverage gap — a core code path in the fixer is completely untested:
-
Untested import-addition branch —
EnsureSystemThreadingTasksImportAsynchas two branches: (a) early-return when the import exists, and (b) insert theusing System.Threading.Tasks;when absent. Every diagnostic test already includes the import in the input, so branch (b) is never exercised. This means the primary new functionality of the code fix (auto-inserting the missingusing) has no test coverage. -
Untested using-insertion ordering — The fixer specifically places the new
usingafterSystem.*usings but before non-System.*usings. The only test with mixed usings (UseCollectionAssertMethodInAsyncVoidMethod_Diagnostic) already hasSystem.Threading.Tasks, so this ordering code is never reached.
Recommendations
- Add at least one test where the input code has no
using System.Threading.Tasks;(justusing Microsoft.VisualStudio.TestTools.UnitTesting;) and verify the fixer adds it in the correct position. - This single test would cover both the import-addition path and the insertion-ordering logic.
Generated by Test Expert Reviewer
🧪 Test quality reviewed by Test Expert Reviewer 🧪
There was a problem hiding this comment.
Summary
Workflow: Expert Code Reviewer 🧠
Date: 2026-04-29
Repository: microsoft/testfx
Key Findings
-
[Correctness — Critical]
override async voidmethods are not guarded against. Applying the fix to a method with theoverridemodifier changes its return type fromvoidtoTask, breaking the override relationship and producing CS0115 ("no suitable method found to override"). The resulting code does not compile. Fix: add!methodDeclaration.Modifiers.Any(SyntaxKind.OverrideKeyword)to the precondition inRegisterCodeFixesAsync. -
[Correctness — Important]
EnsureSystemThreadingTasksImportAsynconly inspects the file's owncompilationUnit.Usings, missingglobal using System.Threading.Tasks;from other files and SDK implicit usings. On .NET 6+ projects this silently adds a redundant using directive that triggers CS8019 / IDE0005. When warnings are treated as errors the fixed code fails to build. -
[Correctness — Minor] The using-insertion loop places
System.Threading.Tasksafter allSystem.*usings rather than at the correct alphabetical position. This creates an SA1210 violation (e.g. ifSystem.Threading.Tasks.Extensionsis already present), which can become a build error in strict stylecop configurations.
Positive Observations
- The overall architecture is clean and follows the existing Roslyn fixer patterns in this repository.
- The deliberate decision to not fix lambdas/anonymous functions is well-reasoned and correctly implemented via an explicit
AnonymousFunctionExpressionSyntaxguard. ConfigureAwait(false)is correctly applied throughout all async paths.- The trivia-preservation via
.WithTriviaFrom(...)ensures the fix doesn't corrupt surrounding whitespace. - Test coverage is thorough, exercising
Assert,StringAssert, andCollectionAssertacross both methods and local functions.
Generated by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
|
@copilot address review comments |
…extFixer - Only stop at async AnonymousFunctionExpressionSyntax (not non-async lambdas), so Assert inside a non-async lambda nested in async void method gets a fix offer - Exclude alias directives and static usings when checking for System.Threading.Tasks import - Add tests for missing using import and non-async lambda inside async void method Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/a9cefeaa-bf7a-40c5-83e2-0f17e9b91190 Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Addressed all three review comments in d780534:
|
…, and tests - Skip code fix for 'override async void' methods (would break override relationship) - Fix using insertion to respect alphabetical ordering within System.* block (SA1210) - Add test for override async void method (no fix offered) - Add test for missing System.Threading.Tasks using (insertion from scratch) - Add test for alphabetical ordering of System.* usings
Evangelink
left a comment
There was a problem hiding this comment.
Summary
Workflow: Test Expert Reviewer 🧪
Date: 2026-04-30
Repository: microsoft/testfx
Key Findings
The new test coverage is well-structured overall — the switch from EmptyCodeFixProvider to the real fixer is correct, the no-fix delegate/lambda cases are properly verified, and the using-insertion ordering tests (WithoutTaskUsing, UsingsOrderedAlphabetically, MissingTasksUsing) give good confidence in the alphabetical insertion logic.
Three coverage gaps were identified:
-
Local function + missing using — All local function tests already carry
using System.Threading.Tasks;. TheEnsureSystemThreadingTasksImportAsyncpath that adds the using when fixing a local function is never exercised. -
virtual async voidbehavior is unspecified — The fixer excludesoverridemethods but notvirtualones. The existingUseAssertMethodInOverrideAsyncVoidMethod_NoCodeFixtest places a diagnostic only on theoverride, leaving thevirtualbase method's fixability completely unverified. Changing avirtual async voidtovirtual async Taskhas downstream implications (derived class overrides would break), so this path deserves an explicit test. -
Multi-diagnostic (multiple Assert calls) in one
async voidmethod — TheBatchFixeris registered but never exercised with more than one diagnostic per method. A single method with two Assert calls should be tested to verify the batch fix coalesces correctly.
Recommendations
- Add a local function test case without
using System.Threading.Tasks;to cover the using-insertion path for local functions. - Add a test for
virtual async voidmethods to document and enforce whether the fix applies or is intentionally suppressed. - Add a test with multiple Assert calls in a single
async voidmethod to validateBatchFixerbehaviour.
Generated by Test Expert Reviewer
🧪 Test quality reviewed by Test Expert Reviewer 🧪
Evangelink
left a comment
There was a problem hiding this comment.
Summary
Workflow: Expert Code Reviewer
Date: 2026-04-30
Repository: microsoft/testfx
Key Findings
[Correctness – High] virtual methods are not excluded from the fix (line 51). Changing a virtual async void method to virtual async Task without touching its overrides causes CS0508 compile errors across all derived classes. The override guard is present but the symmetric virtual guard is missing.
[Correctness – High] Explicit interface implementations are not excluded (lines 47–51). A method like async void IFoo.Bar() has no override modifier, so the fix will be offered and applied. Changing the return type to Task for a void-returning interface member produces CS0539, silently breaking compilation.
[Correctness – Medium] EnsureSystemThreadingTasksImportAsync only inspects compilationUnit.Usings (file-level directives), missing using directives placed inside a namespace block. Files using namespace-scoped usings will have a duplicate using System.Threading.Tasks; inserted at file scope, changing file style and potentially triggering duplicate-usings warnings.
Positive Observations
- The
overrideexclusion is a good safety guard; the two issues above are close analogues that were missed. - Walking ancestors and breaking cleanly at
AnonymousFunctionExpressionSyntax(async) is correct and avoids the hard lambda/delegate type-change problem. - The alphabetical
usinginsertion logic is well-tested and handles the common cases correctly. BatchFixeris the rightFixAllProviderand the sharedequivalenceKeyensures idempotent Fix All behavior.
Recommendations
- Add
!methodDeclaration.Modifiers.Any(SyntaxKind.VirtualKeyword)to the guard on lines 49–51. - Add
methodDeclaration.ExplicitInterfaceSpecifier is nullto the same guard. - Extend
EnsureSystemThreadingTasksImportAsyncto also check namespace-scoped usings before inserting at file scope.
Generated by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
…tFixer
- Skip fix for virtual methods (changing breaks derived-class overrides with CS0508)
- Skip fix for explicit interface implementations (changing breaks contract with CS0539)
- Check namespace-scoped usings to avoid inserting duplicate System.Threading.Tasks import
- Handle global:: prefix in using names (avoid duplicates for global::System.Threading.Tasks)
- Fix StartsWith("System") → exact "System" or StartsWith("System.") to avoid matching SystemWeb etc.
- Add tests: local function + missing using, virtual method (no fix), explicit interface impl (no fix),
multi-diagnostic batch fix, namespace-scoped using (no extra import added)
Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/4811f646-4f80-4580-9500-cfe4e273c943
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
…espace helper Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/4811f646-4f80-4580-9500-cfe4e273c943 Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Addressed all findings in
All three high/medium findings addressed in
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| // Check file-level usings and namespace-scoped usings for an existing System.Threading.Tasks import. | ||
| if (HasSystemThreadingTasksUsing(compilationUnit.Usings)) | ||
| { | ||
| return document; | ||
| } | ||
|
|
||
| foreach (NamespaceDeclarationSyntax ns in compilationUnit.DescendantNodes().OfType<NamespaceDeclarationSyntax>()) | ||
| { | ||
| if (HasSystemThreadingTasksUsing(ns.Usings)) | ||
| { | ||
| return document; | ||
| } | ||
| } | ||
|
|
| string code = """ | ||
| using System.Collections; | ||
| using System.Web; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
…ompat Addresses two outstanding review comments on PR #7892: 1. Multi-namespace bug (review comment #3233748306): The previous `EnsureSystemThreadingTasksImportAsync` returned early on the first namespace declaration that already contained `using System.Threading.Tasks;`, even when the method being fixed was in a *different* namespace without that import — producing uncompilable code. Replace the manual namespace-walking + import-name string matching with a semantic-model-based check: `LookupNamespacesAndTypes(position, "Task")` correctly resolves whether `Task` is in scope at the method's location, uniformly handling file-scoped, namespace-scoped, global, and SDK-implicit usings (including `global::System.Threading.Tasks` and aliases). When not in scope, the using is added to the smallest enclosing block-scoped namespace, falling back to file scope when none exists (covering the file-scoped namespace case, which the build-time Roslyn 3.11 package doesn't expose syntax APIs for). Detect `global using` directives textually so the build-time package doesn't need to expose `UsingDirectiveSyntax.GlobalKeyword`, ensuring the new non-global using is never inserted before existing globals. 2. Test using `System.Web` (review comment #3233748376): `System.Web` is not present in the `ReferenceAssemblies.Net.Net80` test harness. Replace with `System.Xml`. Adds tests pinning: - multi-namespace scope fix (the new behavior), - file-scoped namespace fallback, - BatchFixer across namespaces, - global-using ordering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSTEST0040 warns when
Assertcalls appear insideasync voidmethods, local functions, or delegates, but offered no automated remediation. This adds a code fix that convertsasync voidtoasync Task.Changes
New fixer
AvoidUsingAssertsInAsyncVoidContextFixer.cs:async voidmethods andasync voidlocal functionsoverridemethods,virtualmethods (changing would break derived-class overrides with CS0508), or explicit interface implementations (changing would break the interface contract with CS0539)async voidmethod/local function — the walk correctly skips non-async lambdas and finds the enclosing contextusing System.Threading.Tasks;if absent, inserted after existingSystem.*usings and before non-System usingsglobal::System.Threading.Tasksand namespace-scopedusingdirectives (no duplicate import inserted)using Tasks = System.Threading.Tasks;) and static imports are not treated as satisfying theSystem.Threading.TasksimportResource string
AvoidUsingAssertsInAsyncVoidContextFixadded toCodeFixResources.resxTest coverage includes:
System.Threading.Tasksusing)async voidmethodoverride,virtual, explicit interface implementation, async delegate/lambdaBatchFixer)using System.Threading.Tasks;— no spurious file-level import addedExample