fluentassertions / fluentassertions.analyzers

Analyzers based on the FluentAssertions best practices docs
https://www.fluentassertions.com/tips
MIT License
136 stars 21 forks source link

Request: Give diagnosis when ThrowAsync etc. is not awaited #305

Closed JeppeSN closed 1 month ago

JeppeSN commented 7 months ago

Description

I find that the analyzers should warn people if they say .Should().ThrowAsync<>() and similar, without actually awaiting the task produced by Fluent Assertions.

Complete minimal example reproducing the issue

Consider this code to be tested:

public sealed class C {
    public Task DoWorkAsync() => Task.CompletedTask;
    public Task<int> RetrieveNumberAsync() => Task.FromResult(42);
}

Then consider this attempt at proving that the methods in C will throw:

    [Test]
    public void X() {
        var c = new C();

        c.Invoking(x => x.DoWorkAsync()).Should().ThrowExactlyAsync<InvalidOperationException>();

        c.Invoking(x => x.RetrieveNumberAsync()).Should().ThrowExactlyAsync<InvalidOperationException>();
    }

The problem here is that the test is green. The author of it might think he has written a valid test proving the methods throw.

Actual behavior:

I see no help from FluentAssertions.Analyzers.

Expected behavior:

It would be nice to have some kind of diagnosis from the analyzer saying this is not a good use of Fluent Assertions. It might suggest that the author turns the test method X into an async method, and then uses await in front of the two expression statements.

Of course, the author could do other things, including appending Wait() to the end of the expressions.

Versions

Additional Information

I have seen more than one developer falling in this trap.

In the examples, I used ThrowExactlyAsync<>(). But I think it could be many other methods:

and possibly others.

A couple of years ago, an experienced Stack Overflow user posted the following question: https://stackoverflow.com/questions/70006484/

Meir017 commented 2 months ago

@JeppeSN I think this is already supported by the Microsoft rolsyn analyzers (ex: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs4014?f1url=%3FappId%3Droslyn%26k%3Dk(CS4014)

for example: image

you can enforce this using .editorconfig

JeppeSN commented 2 months ago

@JeppeSN I think this is already supported by the Microsoft rolsyn analyzers (ex: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs4014?f1url=%3FappId%3Droslyn%26k%3Dk(CS4014)

for example: image

you can enforce this using .editorconfig

@Meir017, Your example is different from mine in that your TestMethod AssertThrowExceptionAsync is already declared async. In that situation, it is likely the test author will see the problem. However, in my experience, some people write a synchronous TestMethod public void BlahBlah(), and in that case they can fool themselves with no compiler warning.

Meir017 commented 1 month ago

you can use https://github.com/Microsoft/vs-threading/blob/main/doc/analyzers/VSTHRD110.md

Basically add https://www.nuget.org/packages/Microsoft.VisualStudio.Threading and then you will get diagnostics image