nunit.analyzers icon indicating copy to clipboard operation
nunit.analyzers copied to clipboard

NUnit1032 on `MsSqlContainer.DisposeAsync()`

Open perlun opened this issue 1 year ago • 5 comments

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 avatar Sep 09 '24 10:09 perlun

(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)

perlun avatar Sep 09 '24 10:09 perlun

I thought the rule recognized DisposeAsync? Can you try

public Task OneTimeTearDown() {
       await _msSqlContainer.DisposeAsync();
    }

manfred-brands avatar Sep 09 '24 11:09 manfred-brands

I should have read your second email before responding.

manfred-brands avatar Sep 09 '24 11:09 manfred-brands

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:

perlun avatar Sep 10 '24 06:09 perlun

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

manfred-brands avatar Sep 11 '24 02:09 manfred-brands