nunit / nunit.analyzers

Roslyn analyzers for writing unit tests with NUnit
MIT License
88 stars 32 forks source link

NUnit1032 on `MsSqlContainer.DisposeAsync()` #781

Open perlun opened 1 month ago

perlun commented 1 month ago

Hi,

With the following test class:

public class MsSqlContainerTest {
    private MsSqlContainer _msSqlContainer;

    [OneTimeSetUp]
    public void OneTimeSetup() {
        _msSqlContainer = new MsSqlBuilder().Build();
    }

    [OneTimeTearDown]
    public void OneTimeTearDown() {
        _msSqlContainer.DisposeAsync().AsTask().Wait();
    }
}

...I get a warning/error saying 17>SomeControllerTests.cs(84,28): Error NUnit1032 : The field _msSqlContainer should be Disposed in a method annotated with [OneTimeTearDownAttribute] (https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1032.md)

I realize why this happens; the MsSqlContainer (from https://dotnet.testcontainers.org/modules/mssql/) doesn't implement the IDisposable interface but only IAsyncDisposable, which is why we have to do it this way in the OneTimeTearDown method.

Should we consider this a shortcoming/deficiency in NUnit(.Analyzers) or should this be reported upstream to TestContainers.NET, WDYT? :thinking:

perlun commented 1 month ago

(Hmm, I realize that making the OneTimeTearDown method be public async Task OneTimeTearDown() and just calling _msSqlContainer.DisposeAsync() works fine. :see_no_evil: Perhaps this is just to be considered as user error on my part? :grin:

I saw an exception message about async void methods not working as setup/teardown methods in NUnit but incorrectly thought it meant that all async was forbidden... which is not the case, only async void, which is very understandable)

manfred-brands commented 1 month ago

I thought the rule recognized DisposeAsync? Can you try


public Task OneTimeTearDown() {
       await _msSqlContainer.DisposeAsync();
    }
manfred-brands commented 1 month ago

I should have read your second email before responding.

perlun commented 1 month ago

Yeah, but no worries. :+1: If we really wanted to make it perfect, I guess we could make the rule (or another rule...) warn about DisposeAsync() being used in a non-async OneTimeTearDown-method or something... but perhaps that's overly ambitious. :slightly_smiling_face:

manfred-brands commented 1 month ago

warn about DisposeAsync() being used in a non-async OneTimeTearDown-method

That is more generic than NUnit and hold for all Async inside non-Async method.

The Microsoft.VisualStudio.Threading.Analyzers does raise a warning for this:

image