Merge pull request #599 from manfred-brands/issue595_InstancePer

Do not suppress CA1001 if Fixture uses LifeCycle.InstancePerTestCase
nunit · Sep 25, 2023 · f6e3687 · f6e3687
2 parents 3a43fae + 2145034
commit f6e3687
Show file tree

Hide file tree

Showing 12 changed files with 169 additions and 22 deletions.
diff --git a/documentation/NUnit3003.md b/documentation/NUnit3003.md
@@ -8,7 +8,7 @@
 | Severity | Info
 | Enabled  | True
 | Category | Suppressor
-| Code     | [TestFixtureSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs)
+| Code     | [AvoidUninstantiatedInternalClassSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/AvoidUninstantiatedInternalClassSuppressor.cs)
 
 ## Description
 

diff --git a/documentation/NUnit3004.md b/documentation/NUnit3004.md
@@ -8,7 +8,7 @@
 | Severity | Info
 | Enabled  | True
 | Category | Suppressor
-| Code     | [TestFixtureSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/TestFixtureSuppressor.cs)
+| Code     | [TypesThatOwnDisposableFieldsShouldBeDisposableSuppressor](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/DiagnosticSuppressors/TypesThatOwnDisposableFieldsShouldBeDisposableSuppressor.cs)
 
 ## Description
 

diff --git a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs
@@ -155,6 +155,9 @@ public sealed class NUnitFrameworkConstantsTests
             (nameof(NUnitFrameworkConstants.FullNameOfTypeSetUpAttribute), typeof(SetUpAttribute)),
             (nameof(NUnitFrameworkConstants.FullNameOfTypeTearDownAttribute), typeof(TearDownAttribute)),
 
+            (nameof(NUnitFrameworkConstants.FullNameOfFixtureLifeCycleAttribute), typeof(FixtureLifeCycleAttribute)),
+            (nameof(NUnitFrameworkConstants.FullNameOfLifeCycle), typeof(LifeCycle)),
+
             (nameof(NUnitFrameworkConstants.FullNameOfSameAsConstraint), typeof(SameAsConstraint)),
             (nameof(NUnitFrameworkConstants.FullNameOfSomeItemsConstraint), typeof(SomeItemsConstraint)),
             (nameof(NUnitFrameworkConstants.FullNameOfEqualToConstraint), typeof(EqualConstraint)),

diff --git a/...nalyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs b/...nalyzers.tests/DiagnosticSuppressors/AvoidUninstantiatedInternalClassesSuppressorTests.cs
@@ -51,7 +51,7 @@ public void TestSubtract()
             }
         ";
 
-        private static readonly DiagnosticSuppressor suppressor = new TestFixtureSuppressor();
+        private static readonly DiagnosticSuppressor suppressor = new AvoidUninstantiatedInternalClassSuppressor();
         private DiagnosticAnalyzer analyzer;
 
         [OneTimeSetUp]

diff --git a/src/nunit.analyzers.tests/DiagnosticSuppressors/DisposableFieldsSuppressorTests.cs b/src/nunit.analyzers.tests/DiagnosticSuppressors/DisposableFieldsSuppressorTests.cs
@@ -11,7 +11,7 @@ namespace NUnit.Analyzers.Tests.DiagnosticSuppressors
 {
     public class DisposableFieldsSuppressorTests
     {
-        private static readonly DiagnosticSuppressor suppressor = new TestFixtureSuppressor();
+        private static readonly DiagnosticSuppressor suppressor = new TypesThatOwnDisposableFieldsShouldBeDisposableSuppressor();
         private DiagnosticAnalyzer analyzer;
 
         [OneTimeSetUp]
@@ -90,5 +90,36 @@ public void Dispose() {{}}
 
             await TestHelpers.Suppressed(this.analyzer, suppressor, testCode).ConfigureAwait(true);
         }
+
+        [Test]
+        public async Task ShouldNotSuppressWhenInstancePerTestCase()
+        {
+            var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
+            [FixtureLifeCycle(LifeCycle.InstancePerTestCase)]
+            public class TestClass
+            {
+                private readonly IDisposable? field;
+
+                public TestClass()
+                {
+                    field = new DummyDisposable();
+                }
+
+                [Test]
+                public void Test()
+                {
+                    Assert.That(field, Is.Not.Null);
+                }
+
+                private sealed class DummyDisposable : IDisposable
+                {
+                    public void Dispose() {}
+                }
+            }
+            ");
+
+            // InstancePerTestCase mean test should use IDisposable
+            await TestHelpers.NotSuppressed(this.analyzer, suppressor, testCode).ConfigureAwait(true);
+        }
     }
 }
diff --git a/...DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs b/...DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzerTests.cs
@@ -1,3 +1,4 @@
+using System.Threading.Tasks;
 using Gu.Roslyn.Asserts;
 using Microsoft.CodeAnalysis.Diagnostics;
 using NUnit.Analyzers.Constants;
@@ -742,5 +743,33 @@ private sealed class DummyAsyncWriter : IAsyncDisposable
             RoslynAssert.Valid(analyzer, testCode);
         }
 #endif
+
+        [Test]
+        public void ShouldNotAnalyzeWhenInstancePerTestCase()
+        {
+            var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@$"
+            [FixtureLifeCycle(LifeCycle.InstancePerTestCase)]
+            public class TestClass
+            {{
+                private readonly IDisposable? field;
+
+                public TestClass()
+                {{
+                    field = new DummyDisposable();
+                }}
+
+                [Test]
+                public void Test()
+                {{
+                    Assert.That(field, Is.Not.Null);
+                }}
+
+                {DummyDisposable}
+            }}
+            ");
+
+            // InstancePerTestCase mean test should use IDisposable
+            RoslynAssert.Valid(analyzer, testCode);
+        }
     }
 }
diff --git a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs
@@ -117,6 +117,9 @@ public static class NUnitFrameworkConstants
         public const string FullNameOfTypeSetUpAttribute = "NUnit.Framework.SetUpAttribute";
         public const string FullNameOfTypeTearDownAttribute = "NUnit.Framework.TearDownAttribute";
 
+        public const string FullNameOfFixtureLifeCycleAttribute = "NUnit.Framework.FixtureLifeCycleAttribute";
+        public const string FullNameOfLifeCycle = "NUnit.Framework.LifeCycle";
+
         public const string NameOfConstraint = "Constraint";
 
         public const string FullNameOfSameAsConstraint = "NUnit.Framework.Constraints.SameAsConstraint";

diff --git a/...osticSuppressors/TestFixtureSuppressor.cs → ...dUninstantiatedInternalClassSuppressor.cs b/...osticSuppressors/TestFixtureSuppressor.cs → ...dUninstantiatedInternalClassSuppressor.cs
@@ -11,20 +11,15 @@
 namespace NUnit.Analyzers.DiagnosticSuppressors
 {
     [DiagnosticAnalyzer(LanguageNames.CSharp)]
-    public sealed class TestFixtureSuppressor : DiagnosticSuppressor
+    public sealed class AvoidUninstantiatedInternalClassSuppressor : DiagnosticSuppressor
     {
         internal static readonly SuppressionDescriptor AvoidUninstantiatedInternalTestFixtureClasses = new(
             id: AnalyzerIdentifiers.AvoidUninstantiatedInternalClasses,
             suppressedDiagnosticId: "CA1812",
             justification: "Class is an NUnit TestFixture and is instantiated using reflection");
 
-        internal static readonly SuppressionDescriptor TypesThatOwnDisposableFieldsShouldHaveATearDown = new(
-            id: AnalyzerIdentifiers.TypesThatOwnDisposableFieldsShouldBeDisposable,
-            suppressedDiagnosticId: "CA1001",
-            justification: "Field should be Disposed in TearDown or OneTimeTearDown method");
-
         public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions { get; } =
-            ImmutableArray.Create(AvoidUninstantiatedInternalTestFixtureClasses, TypesThatOwnDisposableFieldsShouldHaveATearDown);
+            ImmutableArray.Create(AvoidUninstantiatedInternalTestFixtureClasses);
 
         public override void ReportSuppressions(SuppressionAnalysisContext context)
         {
@@ -48,17 +43,10 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
                 SemanticModel semanticModel = context.GetSemanticModel(sourceTree);
                 INamedTypeSymbol? typeSymbol = (INamedTypeSymbol?)semanticModel.GetDeclaredSymbol(classDeclaration, context.CancellationToken);
 
-                // Does the class have any Test related methods
                 if (typeSymbol is not null &&
-                    typeSymbol.GetMembers().OfType<IMethodSymbol>().Any(m => m.IsTestRelatedMethod(context.Compilation)))
+                    typeSymbol.IsTestFixture(context.Compilation))
                 {
-                    foreach (var suppression in this.SupportedSuppressions)
-                    {
-                        if (diagnostic.Descriptor.Id == suppression.SuppressedDiagnosticId)
-                        {
-                            context.ReportSuppression(Suppression.Create(suppression, diagnostic));
-                        }
-                    }
+                    context.ReportSuppression(Suppression.Create(AvoidUninstantiatedInternalTestFixtureClasses, diagnostic));
                 }
             }
         }

diff --git a/...alyzers/DiagnosticSuppressors/TypesThatOwnDisposableFieldsShouldBeDisposableSuppressor.cs b/...alyzers/DiagnosticSuppressors/TypesThatOwnDisposableFieldsShouldBeDisposableSuppressor.cs
@@ -0,0 +1,64 @@
+#if !NETSTANDARD1_6
+
+using System.Collections.Immutable;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+using NUnit.Analyzers.Constants;
+using NUnit.Analyzers.Extensions;
+
+namespace NUnit.Analyzers.DiagnosticSuppressors
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class TypesThatOwnDisposableFieldsShouldBeDisposableSuppressor : DiagnosticSuppressor
+    {
+        internal static readonly SuppressionDescriptor TypesThatOwnDisposableFieldsShouldHaveATearDown = new(
+            id: AnalyzerIdentifiers.TypesThatOwnDisposableFieldsShouldBeDisposable,
+            suppressedDiagnosticId: "CA1001",
+            justification: "Field should be Disposed in TearDown or OneTimeTearDown method");
+
+        public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions { get; } =
+            ImmutableArray.Create(TypesThatOwnDisposableFieldsShouldHaveATearDown);
+
+        public override void ReportSuppressions(SuppressionAnalysisContext context)
+        {
+            foreach (var diagnostic in context.ReportedDiagnostics)
+            {
+                SyntaxTree? sourceTree = diagnostic.Location.SourceTree;
+
+                if (sourceTree is null)
+                {
+                    continue;
+                }
+
+                SyntaxNode node = sourceTree.GetRoot(context.CancellationToken)
+                                            .FindNode(diagnostic.Location.SourceSpan);
+
+                if (node is not ClassDeclarationSyntax classDeclaration)
+                {
+                    continue;
+                }
+
+                SemanticModel semanticModel = context.GetSemanticModel(sourceTree);
+                INamedTypeSymbol? typeSymbol = (INamedTypeSymbol?)semanticModel.GetDeclaredSymbol(classDeclaration, context.CancellationToken);
+
+                if (typeSymbol is not null)
+                {
+                    if (typeSymbol.IsInstancePerTestCaseFixture(context.Compilation))
+                    {
+                        // If a TestFixture used InstancePerTestCase, it should be IDisposable
+                        continue;
+                    }
+
+                    if (typeSymbol.IsTestFixture(context.Compilation))
+                    {
+                        context.ReportSuppression(Suppression.Create(TypesThatOwnDisposableFieldsShouldHaveATearDown, diagnostic));
+                    }
+                }
+            }
+        }
+    }
+}
+
+#endif
diff --git a/...zers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs b/...zers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs
@@ -69,9 +69,10 @@ private static void AnalyzeDisposableFields(SyntaxNodeAnalysisContext context)
                 return;
             }
 
-            if (!typeSymbol.GetMembers().OfType<IMethodSymbol>().Any(m => m.IsTestRelatedMethod(context.Compilation)))
+            if (typeSymbol.IsInstancePerTestCaseFixture(context.Compilation) ||
+                !typeSymbol.IsTestFixture(context.Compilation))
             {
-                // Not a TestFixture, CA1812 should have picked this up.
+                // CA1001 should picked this up. Assuming it is enabled.
                 return;
             }
 

diff --git a/src/nunit.analyzers/Extensions/AttributeDataExtensions.cs b/src/nunit.analyzers/Extensions/AttributeDataExtensions.cs
@@ -43,6 +43,16 @@ public static bool IsSetUpOrTearDownMethodAttribute(this AttributeData @this, Co
                 || attributeType.IsType(NUnitFrameworkConstants.FullNameOfTypeTearDownAttribute, compilation);
         }
 
+        public static bool IsFixtureLifeCycleAttribute(this AttributeData @this, Compilation compilation)
+        {
+            var attributeType = @this.AttributeClass;
+
+            if (attributeType is null)
+                return false;
+
+            return attributeType.IsType(NUnitFrameworkConstants.FullNameOfFixtureLifeCycleAttribute, compilation);
+        }
+
         public static AttributeArgumentSyntax? GetConstructorArgumentSyntax(this AttributeData @this, int position,
             CancellationToken cancellationToken = default)
         {

diff --git a/src/nunit.analyzers/Extensions/IMethodSymbolExtensions.cs b/src/nunit.analyzers/Extensions/IMethodSymbolExtensions.cs
@@ -1,5 +1,6 @@
 using System.Linq;
 using Microsoft.CodeAnalysis;
+using NUnit.Analyzers.Constants;
 
 namespace NUnit.Analyzers.Extensions
 {
@@ -67,5 +68,22 @@ internal static bool HasTestRelatedAttributes(this IMethodSymbol methodSymbol, C
             return methodSymbol.GetAttributes().Any(
                 a => a.IsTestMethodAttribute(compilation) || a.IsSetUpOrTearDownMethodAttribute(compilation));
         }
+
+        internal static bool IsTestFixture(this ITypeSymbol typeSymbol, Compilation compilation)
+        {
+            return typeSymbol.GetMembers().OfType<IMethodSymbol>().Any(m => m.IsTestRelatedMethod(compilation));
+        }
+
+        internal static bool IsInstancePerTestCaseFixture(this ITypeSymbol typeSymbol, Compilation compilation)
+        {
+            // Is there a FixtureLifeCycleAttribute?
+            AttributeData? fixtureLifeCycleAttribute = typeSymbol.GetAllAttributes().FirstOrDefault(x => x.IsFixtureLifeCycleAttribute(compilation));
+            return fixtureLifeCycleAttribute is not null &&
+                fixtureLifeCycleAttribute.ConstructorArguments.Length == 1 &&
+                fixtureLifeCycleAttribute.ConstructorArguments[0] is TypedConstant typeConstant &&
+                typeConstant.Kind == TypedConstantKind.Enum &&
+                typeConstant.Type.IsType(NUnitFrameworkConstants.FullNameOfLifeCycle, compilation) &&
+                typeConstant.Value is 1 /* LifeCycle.InstancePerTestCase */;
+        }
     }
 }