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

Use Assert.ThatAsync #804

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
73 changes: 73 additions & 0 deletions documentation/NUnit2055.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# NUnit2055

## Use Assert.ThatAsync

| Topic | Value
| :-- | :--
| Id | NUnit2055
| Severity | Info
| Enabled | True
| Category | Assertion
| Code | [UseAssertThatAsyncAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/UseAssertThatAsync/UseAssertThatAsyncAnalyzer.cs)

## Description

You can use `Assert.ThatAsync` to assert asynchronously.

## Motivation

`Assert.That` runs synchronously, even if pass an asynchronous delegate. This "sync-over-async" pattern blocks
the calling thread, preventing it from doing anything else in the meantime.

`Assert.ThatAsync` allows for a proper async/await. This allows for a better utilization of threads while waiting for the
asynchronous operation to finish.

## How to fix violations

Convert the asynchronous method call with a lambda expression and `await` the `Assert.ThatAsync` instead of the
asynchronous method call.

```csharp
Assert.That(await DoAsync(), Is.EqualTo(expected)); // bad (sync-over-async)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Assert.That(await DoAsync(), Is.EqualTo(expected)); // bad (sync-over-async)
Assert.That(async () => await DoAsync(), Is.EqualTo(expected)); // bad (sync-over-async)

await Assert.ThatAsync(() => DoAsync(), Is.EqualTo(expected)); // good (proper async/await)
```

<!-- start generated config severity -->
## Configure severity

### Via ruleset file

Configure the severity per project, for more info see
[MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022).

### Via .editorconfig file

```ini
# NUnit2055: Use Assert.ThatAsync
dotnet_diagnostic.NUnit2055.severity = chosenSeverity
```

where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`.

### Via #pragma directive

```csharp
#pragma warning disable NUnit2055 // Use Assert.ThatAsync
Code violating the rule here
#pragma warning restore NUnit2055 // Use Assert.ThatAsync
```

Or put this at the top of the file to disable all instances.

```csharp
#pragma warning disable NUnit2055 // Use Assert.ThatAsync
```

### Via attribute `[SuppressMessage]`

```csharp
[System.Diagnostics.CodeAnalysis.SuppressMessage("Assertion",
"NUnit2055:Use Assert.ThatAsync",
Justification = "Reason...")]
```
<!-- end generated config severity -->
1 change: 1 addition & 0 deletions documentation/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ Rules which improve assertions in the test code.
| [NUnit2052](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2052.md) | Consider using Assert.That(expr, Is.Negative) instead of ClassicAssert.Negative(expr) | :white_check_mark: | :information_source: | :white_check_mark: |
| [NUnit2053](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2053.md) | Consider using Assert.That(actual, Is.AssignableFrom(expected)) instead of ClassicAssert.IsAssignableFrom(expected, actual) | :white_check_mark: | :information_source: | :white_check_mark: |
| [NUnit2054](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2054.md) | Consider using Assert.That(actual, Is.Not.AssignableFrom(expected)) instead of ClassicAssert.IsNotAssignableFrom(expected, actual) | :white_check_mark: | :information_source: | :white_check_mark: |
| [NUnit2055](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2055.md) | Use Assert.ThatAsync | :white_check_mark: | :information_source: | :white_check_mark: |

## Suppressor Rules (NUnit3001 - )

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ public sealed class NUnitFrameworkConstantsTests
(nameof(NUnitFrameworkConstants.NameOfAssertIsNotNull), nameof(ClassicAssert.IsNotNull)),
(nameof(NUnitFrameworkConstants.NameOfAssertNotNull), nameof(ClassicAssert.NotNull)),
(nameof(NUnitFrameworkConstants.NameOfAssertThat), nameof(ClassicAssert.That)),
#if NUNIT4
(nameof(NUnitFrameworkConstants.NameOfAssertThatAsync), nameof(ClassicAssert.ThatAsync)),
#else
(nameof(NUnitFrameworkConstants.NameOfAssertThatAsync), "ThatAsync"),
#endif
Comment on lines +113 to +117
Copy link
Member

Choose a reason for hiding this comment

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

I know I used this pattern myself for MultipleAsync but I don't like it.
The problem is that the name ThatAsync is defined in the analyzer because it does support both version 3 and 4.
Maybe I need to add an NUnitFrameworkConstantsV4 class and only test this against V4.

(nameof(NUnitFrameworkConstants.NameOfAssertGreater), nameof(ClassicAssert.Greater)),
(nameof(NUnitFrameworkConstants.NameOfAssertGreaterOrEqual), nameof(ClassicAssert.GreaterOrEqual)),
(nameof(NUnitFrameworkConstants.NameOfAssertLess), nameof(ClassicAssert.Less)),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
#if NUNIT4
using Gu.Roslyn.Asserts;
using Microsoft.CodeAnalysis.Diagnostics;
using NUnit.Analyzers.Constants;
using NUnit.Analyzers.UseAssertThatAsync;
using NUnit.Framework;

namespace NUnit.Analyzers.Tests.UseAssertThatAsync;

[TestFixture]
public sealed class UseAssertThatAsyncAnalyzerTests
{
private static readonly DiagnosticAnalyzer analyzer = new UseAssertThatAsyncAnalyzer();
private static readonly ExpectedDiagnostic diagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.UseAssertThatAsync);
private static readonly string[] configureAwaitValues =
{
"",
".ConfigureAwait(true)",
".ConfigureAwait(false)",
};

[Test]
public void AnalyzeWhenIntResultIsUsed()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public void Test()
{
Assert.That(GetIntAsync().Result, Is.EqualTo(42));
}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.Valid(analyzer, testCode);
}

[Test]
public void AnalyzeWhenBoolResultIsUsed()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public void Test()
{
Assert.That(GetBoolAsync().Result);
}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.Valid(analyzer, testCode);
}

[Test]
public void AnalyzeWhenAwaitIsNotUsedInLineForInt()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public async Task Test()
{
var fourtyTwo = await GetIntAsync();
Assert.That(fourtyTwo, Is.EqualTo(42));
}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.Valid(analyzer, testCode);
}

// do not touch because there is no ThatAsync equivalent
[Test]
public void AnalyzeWhenExceptionMessageIsFuncString()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public async Task Test()
{
Assert.That(await GetBoolAsync(), () => ""message"");
}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.Valid(analyzer, testCode);
}

[Test]
public void AnalyzeWhenAwaitIsNotUsedInLineForBool()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public async Task Test()
{
var myBool = await GetBoolAsync();
Assert.That(myBool, Is.True);
}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.Valid(analyzer, testCode);
}

[Test]
public void AnalyzeWhenAwaitIsUsedInLineForInt([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasMessage)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
Assert.That(await GetIntAsync(){configureAwait}, Is.EqualTo(42){(hasMessage ? @", ""message""" : "")});
Copy link
Member

Choose a reason for hiding this comment

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

This is fully async. Yes it be written as Assert.ThatAsync(GetIntAsync, Is.EqualTo(42)) but that is a style choice.
Users might find it more logic to pass results to Assert than delegates.

}}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.Diagnostics(analyzer, diagnostic, testCode);
}

[Test]
public void AnalyzeWhenAwaitIsUsedInLineForBool([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasConstraint, [Values] bool hasMessage)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
Assert.That(await GetBoolAsync(){configureAwait}{(hasConstraint ? ", Is.True" : "")}{(hasMessage ? @", ""message""" : "")});
}}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.Diagnostics(analyzer, diagnostic, testCode);
}

[Test]
public void AnalyzeWhenAwaitIsUsedAsSecondArgument([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasMessage)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
↓Assert.That(expression: Is.EqualTo(42), actual: await GetIntAsync(){configureAwait}{(hasMessage ? @", message: ""message""" : "")});
}}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.Diagnostics(analyzer, diagnostic, testCode);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Add practical examples that really need it like:

            var i = 0;
            Task<int> GetResult() => Task.FromResult(i++);

            Assert.That(GetResult, Is.GreaterThan(0).After(1000, 10));

The last line could also be:
Assert.That(async () => await GetResult(), Is.GreaterThan(0).After(1000, 10));

Similar one for throwing:
Assert.That(() => Task.FromException<int>(new InvalidOperationException()), Throws.InvalidOperationException);

#endif
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
#if NUNIT4
using Gu.Roslyn.Asserts;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Diagnostics;
using NUnit.Analyzers.Constants;
using NUnit.Analyzers.UseAssertThatAsync;
using NUnit.Framework;

namespace NUnit.Analyzers.Tests.UseAssertThatAsync;

[TestFixture]
public sealed class UseAssertThatAsyncCodeFixTests
{
private static readonly DiagnosticAnalyzer analyzer = new UseAssertThatAsyncAnalyzer();
private static readonly CodeFixProvider fix = new UseAssertThatAsyncCodeFix();
private static readonly ExpectedDiagnostic diagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.UseAssertThatAsync);
private static readonly string[] configureAwaitValues =
{
"",
".ConfigureAwait(true)",
".ConfigureAwait(false)",
};

[Test]
public void VerifyGetFixableDiagnosticIds()
{
var fix = new UseAssertThatAsyncCodeFix();
var ids = fix.FixableDiagnosticIds;

Assert.That(ids, Is.EquivalentTo(new[] { AnalyzerIdentifiers.UseAssertThatAsync }));
}

[Test]
public void VerifyIntAndConstraint([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasMessage)
{
var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
public async Task Test()
{{
Assert.That(await GetIntAsync(){configureAwait}, Is.EqualTo(42){(hasMessage ? @", ""message""" : "")});
}}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
await Assert.ThatAsync(() => GetIntAsync(), Is.EqualTo(42){(hasMessage ? @", ""message""" : "")});
}}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode);
}

[Test]
public void VerifyTaskIntReturningInstanceMethodAndConstraint([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasMessage)
{
var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
public async Task Test()
{{
Assert.That(await this.GetIntAsync(){configureAwait}, Is.EqualTo(42){(hasMessage ? @", ""message""" : "")});
}}

private Task<int> GetIntAsync() => Task.FromResult(42);");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
await Assert.ThatAsync(() => this.GetIntAsync(), Is.EqualTo(42){(hasMessage ? @", ""message""" : "")});
}}

private Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode);
}

[Test]
public void VerifyBoolAndConstraint([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasMessage)
{
var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
public async Task Test()
{{
Assert.That(await GetBoolAsync(){configureAwait}, Is.EqualTo(true){(hasMessage ? @", ""message""" : "")});
}}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
await Assert.ThatAsync(() => GetBoolAsync(), Is.EqualTo(true){(hasMessage ? @", ""message""" : "")});
}}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode);
}

// Assert.That(bool) is supported, but there is no overload of Assert.ThatAsync that only takes a single bool.
[Test]
public void VerifyBoolOnly([ValueSource(nameof(configureAwaitValues))] string configureAwait, [Values] bool hasMessage)
{
var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
public async Task Test()
{{
Assert.That(await GetBoolAsync(){configureAwait}{(hasMessage ? @", ""message""" : "")});
}}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@"
public async Task Test()
{{
await Assert.ThatAsync(() => GetBoolAsync(), Is.True{(hasMessage ? @", ""message""" : "")});
}}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode);
}

[Test]
public void VerifyIntAsSecondArgumentAndConstraint([ValueSource(nameof(configureAwaitValues))] string configureAwait)
{
var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
public async Task Test()
{{
↓Assert.That(expression: Is.EqualTo(42), actual: await GetIntAsync(){configureAwait});
}}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public async Task Test()
{
await Assert.ThatAsync(() => GetIntAsync(), Is.EqualTo(42));
}

private static Task<int> GetIntAsync() => Task.FromResult(42);");
RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode);
}

[Test]
public void VerifyBoolAsSecondArgumentAndConstraint([ValueSource(nameof(configureAwaitValues))] string configureAwait)
{
var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
public async Task Test()
{{
↓Assert.That(message: ""message"", condition: await GetBoolAsync(){configureAwait});
}}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
public async Task Test()
{
await Assert.ThatAsync(() => GetBoolAsync(), Is.True, ""message"");
}

private static Task<bool> GetBoolAsync() => Task.FromResult(true);");
RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add an example for detecting:

public void Test()
{
    Assert.That(() => Task.FromException<int>(new InvalidOperationException()), Throws.InvalidOperationException);
}

With a code fix looking like:

public async Task Test()
{
    await Assert.ThatAsync(() => Task.FromException<int>(new InvalidOperationException()), Throws.InvalidOperationException);
}

Especially notice the need to change the return type of the test from void to async Task.

#endif
1 change: 1 addition & 0 deletions src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ internal static class AnalyzerIdentifiers
internal const string NegativeUsage = "NUnit2052";
internal const string IsAssignableFromUsage = "NUnit2053";
internal const string IsNotAssignableFromUsage = "NUnit2054";
internal const string UseAssertThatAsync = "NUnit2055";

#endregion Assertion

Expand Down
1 change: 1 addition & 0 deletions src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public static class NUnitFrameworkConstants
public const string NameOfAssertNotNull = "NotNull";
public const string NameOfAssertIsNotNull = "IsNotNull";
public const string NameOfAssertThat = "That";
public const string NameOfAssertThatAsync = "ThatAsync";
public const string NameOfAssertGreater = "Greater";
public const string NameOfAssertGreaterOrEqual = "GreaterOrEqual";
public const string NameOfAssertLess = "Less";
Expand Down
Loading
Loading