Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document MSTEST0039 and MSTEST0040 #44419

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/core/testing/mstest-analyzers/mstest0035.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
title: "[DeploymentItem] can be specified only on test class or test method."
title: "MSTEST0035: [DeploymentItem] can be specified only on test class or test method."
description: "Learn about code analysis rule MSTEST0035: `[DeploymentItem]` can be specified only on test class or test method."
ms.date: 08/02/2024
f1_keywords:
Expand Down
2 changes: 1 addition & 1 deletion docs/core/testing/mstest-analyzers/mstest0036.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
title: "Do not use shadowing inside test class."
title: "MSTEST0036: Do not use shadowing inside test class."
description: "Learn about code analysis rule MSTEST0036: Do not use shadowing inside test class."
ms.date: 08/19/2024
f1_keywords:
Expand Down
2 changes: 1 addition & 1 deletion docs/core/testing/mstest-analyzers/mstest0037.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
title: "Use proper 'Assert' methods"
title: "MSTEST0037: Use proper 'Assert' methods"
description: "Learn about code analysis rule MSTEST0037: Use proper 'Assert' methods."
ms.date: 11/17/2024
f1_keywords:
Expand Down
2 changes: 1 addition & 1 deletion docs/core/testing/mstest-analyzers/mstest0038.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
title: "Don't use 'Assert.AreSame' or 'Assert.AreNotSame' with value types"
title: "MSTEST0038: Don't use 'Assert.AreSame' or 'Assert.AreNotSame' with value types"
description: "Learn about code analysis rule MSTEST0038: Don't use 'Assert.AreSame' or 'Assert.AreNotSame' with value types"
ms.date: 01/06/2025
f1_keywords:
Expand Down
41 changes: 41 additions & 0 deletions docs/core/testing/mstest-analyzers/mstest0039.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
---
title: "MSTEST0039: Use newer 'Assert.Throws' methods"
description: "Learn about code analysis rule MSTEST0039: Use 'Assert.ThrowsExactly' instead of 'Assert.ThrowsException'"
ms.date: 01/17/2025
f1_keywords:
- MSTEST0039
- UseNewerAssertThrowsAnalyzer
helpviewer_keywords:
- UseNewerAssertThrowsAnalyzer
- MSTEST0039
author: Youssef1313
ms.author: ygerges
---
# MSTEST0039: Use newer 'Assert.Throws' methods

| Property | Value |
|-------------------------------------|------------------------------------------------------------------------|
| **Rule ID** | MSTEST0039 |
| **Title** | Use newer 'Assert.Throws' methods |
| **Category** | Usage |
| **Fix is breaking or non-breaking** | Non-breaking |
| **Enabled by default** | Yes |
| **Default severity** | Info |
| **Introduced in version** | 3.8.0 |
| **Is there a code fix** | Yes |

## Cause

The use of <xref:Microsoft.VisualStudio.TestTools.UnitTesting.Assert.ThrowsException*?displayProperty=nameWithType> or <xref:Microsoft.VisualStudio.TestTools.UnitTesting.Assert.ThrowsExceptionAsync*?displayProperty=nameWithType> which are no longer recommended.

## Rule description

<xref:Microsoft.VisualStudio.TestTools.UnitTesting.Assert.ThrowsException*?displayProperty=nameWithType> and <xref:Microsoft.VisualStudio.TestTools.UnitTesting.Assert.ThrowsExceptionAsync*?displayProperty=nameWithType> are not recommended and may be deprecated in the future.

## How to fix violations

Instead, use `Assert.ThrowsExactly` or `Assert.ThrowsExactlyAsync` instead of `Assert.ThrowsException` or `Assert.ThrowsExceptionAsync`.

## When to suppress warnings

Do not suppress a warning from this rule. It's strongly recommended to move from the old APIs to the new ones.
41 changes: 41 additions & 0 deletions docs/core/testing/mstest-analyzers/mstest0040.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
---
title: "MSTEST0040: Do not assert inside 'async void' contexts"
description: "Learn about code analysis rule MSTEST0040: Do not assert inside 'async void' methods, local functions, or lambdas because they may not fail the test"
ms.date: 01/17/2025
f1_keywords:
- MSTEST0040
- AvoidUsingAssertsInAsyncVoidContextAnalyzer
helpviewer_keywords:
- AvoidUsingAssertsInAsyncVoidContextAnalyzer
- MSTEST0040
author: Youssef1313
ms.author: ygerges
---
# MSTEST0040: Do not assert inside 'async void' contexts

| Property | Value |
|-------------------------------------|------------------------------------------------------------------------|
| **Rule ID** | MSTEST0040 |
| **Title** | Do not assert inside 'async void' contexts |
| **Category** | Usage |
| **Fix is breaking or non-breaking** | Non-breaking |
| **Enabled by default** | Yes |
| **Default severity** | Warning |
| **Introduced in version** | 3.8.0 |
| **Is there a code fix** | No |

## Cause

The use of any assertion method in an `async void` method, local function, or lambda.

## Rule description

Exceptions that are thrown in an `async void` context are unobserved. So, a failing assertion in an `async void` method may go unnoticed. When using VSTest, such exceptions are very likely to be unnoticed. When using Microsoft.Testing.Platform, such exception *may* crash the process
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub I'll appreciate if you can review the wording for accuracy here please.

To share context: in the case of Microsoft.Testing.Platform, we do subscribe to both TaskScheduler.UnobservedTaskException and AppDomain.CurrentDomain.UnhandledException and call FailFast. In VSTest, we don't subscribe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you prevent the crashing in VSTest? Is there a SynchronizationContext that's eating the exception? Are unhandled exceptions on ThreadPool threads configured to not crash?

Exceptions in an async void method are just queued to be thrown on the current SynchronizationContext if there is one, or if there isn't on the ThreadPool.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. VSTest doesn't have synchronization context. Not sure about the second question of configuring ThreadPool threads to not crash. @Evangelink @nohwnd Do you know?

@Evangelink Could it be that the test project where the original issue is noticed is setting their own synchronization context? 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look at VSTest codebase when I'm back at laptop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a quick sample in a VSTest project. The test passes when async void which is kinda expected even for MTP, but I can see the UnhandledException being printed with VSTest.

Maybe it's just about that FailFast in MTP which will simply cause non-zero exit code and things will be failing properly in CI contexts etc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original issue was on netfx so this is pretty sure it's linked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant by "Are unhandled exceptions on ThreadPool threads configured to not crash?" That setting reverts to the .NET Framework 1.x behavior circa 20 years ago where the ThreadPool eats unhandled exceptions rather than allowing them to crash the process.

Copy link
Member Author

@Youssef1313 Youssef1313 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the following wording more accurate then?

Exceptions that are thrown in an `async void` context are unhandled. A failing assertion in an `async void` method will be swallowed and will not crash the process when using VSTest under .NET Framework, and may crash the process when using Microsoft.Testing.Platform or VSTest under modern .NET.

(Note: edited after commenting)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable I'd guess. I'd find the behavior more intuitive if it behaved like xunit does: xunit registers its own SynchronizationContext in order to make async void work, both in terms of knowing when the test completes and knowing when an exception occurs. async void still isn't recommended, but having one doesn't put the whole process in jeopardy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub Interesting. Let me open an issue for that on our side.


## How to fix violations

Refactor the code to not use assertions in `async void`.

## When to suppress warnings

Do not suppress a warning from this rule.

Check failure on line 41 in docs/core/testing/mstest-analyzers/mstest0040.md

View workflow job for this annotation

GitHub Actions / lint

Files should end with a single newline character

docs/core/testing/mstest-analyzers/mstest0040.md:41:41 MD047/single-trailing-newline Files should end with a single newline character https://github.com/DavidAnson/markdownlint/blob/v0.37.2/doc/md047.md
2 changes: 2 additions & 0 deletions docs/core/testing/mstest-analyzers/usage-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,5 @@ Identifier | Name | Description
[MSTEST0035](mstest0035.md) | UseDeploymentItemWithTestMethodOrTestClassTitle | `[DeploymentItem]` can be specified only on test class or test method
[MSTEST0037](mstest0037.md) | UseProperAssertMethodsAnalyzer | Use proper `Assert` methods
[MSTEST0038](mstest0038.md) | AvoidAssertAreSameWithValueTypesAnalyzer | Don't use `Assert.AreSame` or `Assert.AreNotSame` with value types
[MSTEST0039](mstest0039.md) | UseNewerAssertThrowsAnalyzer | Use newer 'Assert.Throws' methods
[MSTEST0040](mstest0040.md) | AvoidUsingAssertsInAsyncVoidContextAnalyzer | Do not assert inside 'async void' contexts
4 changes: 4 additions & 0 deletions docs/navigate/devops-testing/toc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ items:
href: ../../core/testing/mstest-analyzers/mstest0037.md
- name: MSTEST0038
href: ../../core/testing/mstest-analyzers/mstest0038.md
- name: MSTEST0039
href: ../../core/testing/mstest-analyzers/mstest0039.md
- name: MSTEST0040
href: ../../core/testing/mstest-analyzers/mstest0040.md
- name: Microsoft Testing Platform
items:
- name: Overview
Expand Down
Loading