nunit/nunit.analyzers

error NUnit1032 is incorrect when InstancePerTestCase and constructor is used to initialize IDisposible

Closed this issue · 6 comments

trampster commented

If you have the following:

  • InstancePerTestCase
  • An IDisposible created in your constructor
  • [TearDown] method which disposes

Then you get the following error:
error NUnit1032: The field _yourService should be Disposed in a method annotated with [OneTimeTearDownAttribute] (https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1032.md)

However [OneTimeTearDownAttribute] must now be a static method and the IDisposible isn't and shouldn't be so it can't dispose it.

It seems this error doesn't know about InstancePerTestCase and so gets this wrong.

And no I don't want to create the IDisposible in a [Setup] method instead of the constructor. Putting it in the constructor allows me to make in non nullable and readonly. Which means I don't need null checks in every test.

manfred-brands commented

The analyzer explicitly excludes IDisposable types, where it assumes MS CA2000 will pick those up and InstancePerTestCase fixtures where MS CA1001 should be raised if your fixture has IDisposable fields but is not IDisposable itself.

Maybe there is an issue with detecting either of these, can you supply me with a small example?

trampster commented

I'm not getting CA1001, although I think I definitely should be.

I've made my TestFixture IDisposable and I assume Nunit will dispose after running each test if I have InstancePerTestCase.

I suggest the help for this be updated to tell users if they are doing InstancePerTestCase they should make their test fixture disposable

trampster commented

I have InstancePerTestCase specified at the assembly level. Not sure if that would make a difference.

manfred-brands commented

Yes, that would make a difference, the analyzer currently only looks at attributes on the Fixture itself.
However, if your Fixtures are IDisposable that shouldn't matter. That test is done before the other.

trampster commented

Yes I've changed my fixture to be IDisposable and the error disappeared. So I'm happy. Feel free to keep this open if you intend to make it check for assembly level attributes or update the help. Otherwise close it as I'm good to go.

manfred-brands commented

The NUnit.Analyzer complements to the Microsoft NET Analyzers. It knows about [...SetUp] and [...TearDown] which Microsoft doesn't.
When you are using LifeCycle.InstancePerTestCase you are using standard Microsoft pattern where items created in the constructor should be disposed in the Dispose method.

I'm not getting CA1001, although I think I definitely should be.

Unfortunately, neither CA1001 nor CA2213 are enabled by default

You will need to add/edit your .editorconfig file and add:

# CA1001: Types that own disposable fields should be disposable
dotnet_diagnostic.CA1001.severity = warning
# CA2213: Disposable fields should be disposed
dotnet_diagnostic.CA2213.severity = warning

I leave this ticket open to update the documentation with the above.