NUnit1032 false positive in LifeCycle.InstancePerTestCase test fixture using constructor

[fredjeronimo]

Hi all,

First of all, thank you for the great work you've been doing on this library. It's much appreciated.

It seems the new rule NUnit1032 introduced in v3.7.0 is flagging false positives and the wrong fix suggestion when dealing with test fixtures with LifeCycle.InstancePerTestCase and constructors.

Works as expected with LifeCycle.SingleInstance

It works as expected with LifeCycle.SingleInstance. Given this disposable class:

public class TestDisposable : IDisposable { public void Dispose() { /* do nothing */ } }

and this LifeCycle.SingleInstance failing fixture class:

[FixtureLifeCycle(LifeCycle.SingleInstance)]
internal sealed class FailingNUnit1032TestsSingleInstance
{
    // Correctly flags NUnit1032 with correct fix "The field _testDisposable should be Disposed in a method
    // annotated with [OneTimeTearDownAttribute]"
    private readonly TestDisposable _testDisposable;

    public FailingNUnit1032TestsSingleInstance()
    {
        _testDisposable = new TestDisposable();
    }
    
    [Test]
    public void SomeTest() { /* do something */ /*}
}

The suggested fix works:

[FixtureLifeCycle(LifeCycle.SingleInstance)]
internal sealed class FixedNUnit1032TestsSingleInstance
{
    private readonly TestDisposable _testDisposable;

    public FixedNUnit1032TestsSingleInstance()
    {
        _testDisposable = new TestDisposable();
    }

    [OneTimeTearDown]
    public void TearDown()
    {
        _testDisposable.Dispose();
    }

    [Test]
    public void SomeTest() { /* do something */ }
}

LifeCycle.InstancePerTestCase introduces some issues

However, if we switch to LifeCycle.InstancePerTestCase.

[FixtureLifeCycle(LifeCycle.InstancePerTestCase)]
internal sealed class FailingNUnit1032TestsInstancePerTestCase
{
    // Correctly flags NUnit1032 but with the wrong fix "The field _testDisposable should be Disposed 
    // in a method annotated with [OneTimeTearDownAttribute]"
    private readonly TestDisposable _testDisposable;

    public FailingNUnit1032TestsInstancePerTestCase()
    {
        _testDisposable = new TestDisposable();
    }
    
    [Test]
    public void SomeTest() { /* do something */ /*}
}

The first issue is that a OneTimeTearDownAttribute needs to be static when using LifeCycle.InstancePerTestCase as it only runs once per test fixture, which prevents the disposal of the non-static field as suggested by the fix.

The second problem is that doing a disposal in [TearDown] does not remove the NUnit1032 violation, which I believe it should as it's called after the execution of every test as in Constructor > test > Teardown:

[FixtureLifeCycle(LifeCycle.InstancePerTestCase)]
internal sealed class ShouldWorkNUnit1032TestsInstancePerTestCase
{
    // NUnit1032 still being flagged
    private readonly TestDisposable _testDisposable;

    public ShouldWorkNUnit1032TestsInstancePerTestCase()
    {
        _testDisposable = new TestDisposable();
    }
    
    [TearDown]
    public void TearDown()
    {
        _testDisposable.Dispose();
    }

    [Test]
    public void SomeTest() { /* do something */ }
}

The following does work, but then we lose some of the benefits of the constructor (e.g. readonly fields) and we need to disable nullability warnings (or disable/handle it in the tests or global level in .editorconfig or similar).

[FixtureLifeCycle(LifeCycle.InstancePerTestCase)]
internal sealed class FixedNUnit1032TestsInstancePerTestCase
{
    // NUnit1032 is gone but we have other warnings to handle
#pragma warning disable CS8618
    private TestDisposable _testDisposable;
#pragma warning enable CS8618
    
    [SetUp]
    public void SetUp()
    {
        _testDisposable = new TestDisposable();
    }
    
    [TearDown]
    public void TearDown()
    {
        _testDisposable.Dispose();
    }

    [Test]
    public void SomeTest() { /* do something */ }
}

[manfred-brands]

@fredjeronimo Thanks for reporting.

When you use FixtureLifeCycle the idea is that your class should be IDisposable where you cleanup in the Dispose method instead of in a TearDown method.
Once your class is IDisposable NUnit1032 switches off and the default MS analyzer takes over.

[fredjeronimo]

Thanks @manfred-brands.

That does make sense, although I didn't immediately think of that, rather obvious in hindsight, proper solution.

Any chance, as a feature request, that we could have an analyzer that detects that situation and guides the user to properly implement the IDisposable pattern in the LifeCycle.InstancePerTestCase test fixture when using disposable resources and not doing so?

[manfred-brands]

@fredjeronimo I created a PR which should prompt you to use IDisposable.
It does expect that you have the standard MS CA1001 rule enabled.

[fredjeronimo]

@manfred-brands - That's great news. Thanks for looking at this.

Looking forward to the next version. 😄