Use Assert.ThatAsync #804
Use Assert.ThatAsync #804
Conversation
There was a problem hiding this comment.
Although the PR fixes what #759 describes it is for the wrong reason.
None of the examples do async-over-sync.
It fixes a style issue and at most should be a suggestion or even disabled by default.
This allows you to use this rule, but don't force it on others.
If you were mistaken and you thought this was doing synchronous async we could drop it.
What we could use is a rule that detects the real use-cases where we do synchronous async calls which is the examples mentioned in my comments.
This could be a separate diagnostic supported by the same Analyzer/CodeFix pair.
| asynchronous method call. | ||
|
|
||
| ```csharp | ||
| Assert.That(await DoAsync(), Is.EqualTo(expected)); // bad (sync-over-async) |
There was a problem hiding this comment.
| Assert.That(await DoAsync(), Is.EqualTo(expected)); // bad (sync-over-async) | |
| Assert.That(async () => await DoAsync(), Is.EqualTo(expected)); // bad (sync-over-async) |
| #if NUNIT4 | ||
| (nameof(NUnitFrameworkConstants.NameOfAssertThatAsync), nameof(ClassicAssert.ThatAsync)), | ||
| #else | ||
| (nameof(NUnitFrameworkConstants.NameOfAssertThatAsync), "ThatAsync"), | ||
| #endif |
There was a problem hiding this comment.
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.
| var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" | ||
| public async Task Test() | ||
| {{ | ||
| Assert.That(await GetIntAsync(){configureAwait}, Is.EqualTo(42){(hasMessage ? @", ""message""" : "")}); |
There was a problem hiding this comment.
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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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);
| private static Task<bool> GetBoolAsync() => Task.FromResult(true);"); | ||
| RoslynAssert.CodeFix(analyzer, fix, diagnostic, code, fixedCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| .Where(a => a.ArgumentKind == ArgumentKind.Explicit) // filter out arguments that were not explicitly passed in | ||
| .ToArray(); | ||
|
|
||
| // The first parameter is usually the "actual" parameter, but sometimes it's the "condition" parameter. |
There was a problem hiding this comment.
The only argument name we want to catch is actually called del as used in:
public static void That<TActual>(ActualValueDelegate<TActual> del, IResolveConstraint expr
| // Since the order is not guaranteed, let's just call it "actualArgument" here. | ||
| var actualArgument = arguments.SingleOrDefault(a => firstParameterCandidates.Contains(a.Parameter?.Name)) | ||
| ?? arguments[0]; | ||
| if (actualArgument.Syntax is not ArgumentSyntax argumentSyntax || argumentSyntax.Expression is not AwaitExpressionSyntax awaitExpression) |
There was a problem hiding this comment.
When the argument is awaited then NUnit doesn't do async-over-sync.
|
Thanks for the quick feedback @manfred-brands! Besides the overload you mentioned here, am I correct in assuming that the following overloads should also be converted? public static void That(Func<bool> condition, NUnitString message, string actualExpression)
public static void That(TestDelegate code, IResolveConstraint constraint, NUnitString message, string actualExpression, string constraintExpression) |
The first no. As NUnit calls |
This closes #759.
Please assign the issue and the PR to me!