Skip to content

Commit 4cee336

Browse files
manfred-brandsmanfred-brands
authored
Merge pull request #848 from nunit/Issue840
New rule to ensure that base class TestFixtures are abstract.
2 parents cf2040d + 68eb7ef commit 4cee336

File tree

13 files changed

+466
-1
lines changed

13 files changed

+466
-1
lines changed

‎documentation/NUnit1034.md‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# NUnit1034
2+
3+
## Base TestFixtures should be abstract
4+
5+
| Topic | Value
6+
| :-- | :--
7+
| Id | NUnit1034
8+
| Severity | Warning
9+
| Enabled | True
10+
| Category | Structure
11+
| Code | [TestFixtureShouldBeAbstractAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/TestFixtureShouldBeAbstract/TestFixtureShouldBeAbstractAnalyzer.cs)
12+
13+
## Description
14+
15+
Base TestFixtures should be abstract to prevent base class tests executing separately.
16+
17+
## Motivation
18+
19+
When a base class is not `abstract` it will also be run as a standalone test which is most times not the intention.
20+
21+
```csharp
22+
namespace Tests
23+
{
24+
internal class ParentFixture
25+
{
26+
[Test]
27+
public void ParentTest()
28+
{
29+
Assert.Pass($"Run {nameof(ParentTest)} from class {GetType().Name}");
30+
}
31+
}
32+
33+
internal class ChildFixture : ParentFixture
34+
{
35+
[Test]
36+
public void ChildTest()
37+
{
38+
Assert.Pass($"Run {nameof(ChildTest)} from class {GetType().Name}");
39+
}
40+
}
41+
}
42+
```
43+
44+
As the `Parent` class is valid as a standalone `TestFixture` it will be instantiated and run separately.
45+
46+
```text
47+
ChildTest: Run ChildTest from class ChildFixture
48+
ParentTest: Run ParentTest from class ChildFixture
49+
ParentTest: Run ParentTest from class ParentFixture
50+
```
51+
52+
This rule only fires when a class is found to be used as a base class in the current compilation.
53+
54+
## How to fix violations
55+
56+
Mark any base class test fixture as `abstract`:
57+
58+
```csharp
59+
internal abstract class Parent
60+
{
61+
[Test]
62+
public void ParentRun()
63+
{
64+
Assert.Pass($"Run {nameof(ParentRun)} from {GetType().Name}");
65+
}
66+
}
67+
```
68+
69+
<!-- start generated config severity -->
70+
## Configure severity
71+
72+
### Via ruleset file
73+
74+
Configure the severity per project, for more info see
75+
[MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022).
76+
77+
### Via .editorconfig file
78+
79+
```ini
80+
# NUnit1034: Base TestFixtures should be abstract
81+
dotnet_diagnostic.NUnit1034.severity = chosenSeverity
82+
```
83+
84+
where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`.
85+
86+
### Via #pragma directive
87+
88+
```csharp
89+
#pragma warning disable NUnit1034 // Base TestFixtures should be abstract
90+
Code violating the rule here
91+
#pragma warning restore NUnit1034 // Base TestFixtures should be abstract
92+
```
93+
94+
Or put this at the top of the file to disable all instances.
95+
96+
```csharp
97+
#pragma warning disable NUnit1034 // Base TestFixtures should be abstract
98+
```
99+
100+
### Via attribute `[SuppressMessage]`
101+
102+
```csharp
103+
[System.Diagnostics.CodeAnalysis.SuppressMessage("Structure",
104+
"NUnit1034:Base TestFixtures should be abstract",
105+
Justification = "Reason...")]
106+
```
107+
<!-- end generated config severity -->

‎documentation/index.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ Rules which enforce structural requirements on the test code.
5252
| [NUnit1031](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1031.md) | The individual arguments provided by a ValuesAttribute must match the type of the corresponding parameter of the method | :white_check_mark: | :exclamation: | :x: |
5353
| [NUnit1032](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1032.md) | An IDisposable field/property should be Disposed in a TearDown method | :white_check_mark: | :exclamation: | :x: |
5454
| [NUnit1033](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1033.md) | The Write methods on TestContext will be marked as Obsolete and eventually removed | :white_check_mark: | :warning: | :white_check_mark: |
55+
| [NUnit1034](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1034.md) | Base TestFixtures should be abstract | :white_check_mark: | :warning: | :white_check_mark: |
5556

5657
## Assertion Rules (NUnit2001 - )
5758

‎src/nunit.analyzers.codefixes/TestFixtureShouldBeAbstract/TestFixtureShouldBeAbstractCodeFix.cs‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
using System.Collections.Immutable;
2+
using System.Composition;
3+
using System.Threading.Tasks;
4+
using Microsoft.CodeAnalysis;
5+
using Microsoft.CodeAnalysis.CodeActions;
6+
using Microsoft.CodeAnalysis.CodeFixes;
7+
using Microsoft.CodeAnalysis.CSharp;
8+
using Microsoft.CodeAnalysis.CSharp.Syntax;
9+
using NUnit.Analyzers.Constants;
10+
11+
namespace NUnit.Analyzers.TestFixtureShouldBeAbstract
12+
{
13+
[ExportCodeFixProvider(LanguageNames.CSharp)]
14+
[Shared]
15+
public class TestFixtureShouldBeAbstractCodeFix : CodeFixProvider
16+
{
17+
internal const string MakeBaseTestFixtureAbstract = "Make base test fixture abstract";
18+
19+
public override ImmutableArray<string> FixableDiagnosticIds
20+
=> ImmutableArray.Create(AnalyzerIdentifiers.BaseTestFixtureIsNotAbstract);
21+
22+
public sealed override FixAllProvider GetFixAllProvider()
23+
{
24+
return WellKnownFixAllProviders.BatchFixer;
25+
}
26+
27+
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
28+
{
29+
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
30+
31+
if (root is null)
32+
{
33+
return;
34+
}
35+
36+
context.CancellationToken.ThrowIfCancellationRequested();
37+
38+
var node = root.FindNode(context.Span);
39+
40+
if (node is not ClassDeclarationSyntax originalExpression)
41+
return;
42+
43+
// Add the abstract modifier
44+
SyntaxTokenList updatedModifiers = AddAbstractModifier(originalExpression);
45+
46+
ClassDeclarationSyntax newExpression = originalExpression.WithoutLeadingTrivia()
47+
.WithModifiers(updatedModifiers);
48+
49+
var newRoot = root.ReplaceNode(originalExpression, newExpression);
50+
51+
var codeAction = CodeAction.Create(
52+
MakeBaseTestFixtureAbstract,
53+
_ => Task.FromResult(context.Document.WithSyntaxRoot(newRoot)),
54+
MakeBaseTestFixtureAbstract);
55+
56+
context.RegisterCodeFix(codeAction, context.Diagnostics);
57+
}
58+
59+
private static SyntaxTokenList AddAbstractModifier(ClassDeclarationSyntax originalExpression)
60+
{
61+
var modifiers = originalExpression.Modifiers;
62+
63+
var abstractSyntax = SyntaxFactory.Token(SyntaxKind.AbstractKeyword)
64+
.WithTrailingTrivia(SyntaxTriviaList.Create(SyntaxFactory.Whitespace(" ")));
65+
66+
if (!modifiers.Any())
67+
{
68+
// Get leading trivia from declaration
69+
abstractSyntax = abstractSyntax.WithLeadingTrivia(originalExpression.GetLeadingTrivia());
70+
}
71+
72+
return modifiers.Add(abstractSyntax);
73+
}
74+
}
75+
}

‎src/nunit.analyzers.sln‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "documentation", "documentat
4343
..\documentation\NUnit1031.md = ..\documentation\NUnit1031.md
4444
..\documentation\NUnit1032.md = ..\documentation\NUnit1032.md
4545
..\documentation\NUnit1033.md = ..\documentation\NUnit1033.md
46+
..\documentation\NUnit1034.md = ..\documentation\NUnit1034.md
4647
..\documentation\NUnit2001.md = ..\documentation\NUnit2001.md
4748
..\documentation\NUnit2002.md = ..\documentation\NUnit2002.md
4849
..\documentation\NUnit2003.md = ..\documentation\NUnit2003.md

‎src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ public static readonly (string Constant, Type Type)[] FullNameOfTypeSource =
128128
(nameof(NUnitFrameworkConstants.FullNameOfTypeValuesAttribute), typeof(ValuesAttribute)),
129129
(nameof(NUnitFrameworkConstants.FullNameOfTypeITestBuilder), typeof(ITestBuilder)),
130130
(nameof(NUnitFrameworkConstants.FullNameOfTypeISimpleTestBuilder), typeof(ISimpleTestBuilder)),
131+
(nameof(NUnitFrameworkConstants.FullNameOfTypeIFixtureBuilder), typeof(IFixtureBuilder)),
131132
(nameof(NUnitFrameworkConstants.FullNameOfTypeIParameterDataSource), typeof(IParameterDataSource)),
132133
(nameof(NUnitFrameworkConstants.FullNameOfTypeTestCaseData), typeof(TestCaseData)),
133134
(nameof(NUnitFrameworkConstants.FullNameOfTypeTestCaseParameters), typeof(TestCaseParameters)),

‎src/nunit.analyzers.tests/TestFixtureShouldBeAbstract/TestFixtureShouldBeAbstractAnalyzerTests.cs‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
using System.Collections.Generic;
2+
using Gu.Roslyn.Asserts;
3+
using Microsoft.CodeAnalysis.Diagnostics;
4+
using NUnit.Analyzers.Constants;
5+
using NUnit.Analyzers.TestFixtureShouldBeAbstract;
6+
using NUnit.Framework;
7+
8+
namespace NUnit.Analyzers.Tests.TestFixtureShouldBeAbstract
9+
{
10+
public class TestFixtureShouldBeAbstractAnalyzerTests
11+
{
12+
private static readonly DiagnosticAnalyzer analyzer = new TestFixtureShouldBeAbstractAnalyzer();
13+
private static readonly ExpectedDiagnostic expectedDiagnostic =
14+
ExpectedDiagnostic.Create(AnalyzerIdentifiers.BaseTestFixtureIsNotAbstract);
15+
16+
private static readonly IEnumerable<string> testMethodRelatedAttributes =
17+
[
18+
"OneTimeSetUp",
19+
"OneTimeTearDown",
20+
"SetUp",
21+
"TearDown",
22+
"Test",
23+
];
24+
25+
[TestCaseSource(nameof(testMethodRelatedAttributes))]
26+
public void AnalyzeWhenBaseFixtureIsAbstract(string attribute)
27+
{
28+
var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@"
29+
public abstract class BaseFixture
30+
{{
31+
[{attribute}]
32+
public void BaseFixtureMethod() {{ }}
33+
}}
34+
35+
public class DerivedFixture : BaseFixture
36+
{{
37+
[Test]
38+
public void DerivedFixtureMethod() {{ }}
39+
}}");
40+
41+
RoslynAssert.Valid(analyzer, testCode);
42+
}
43+
44+
[TestCaseSource(nameof(testMethodRelatedAttributes))]
45+
public void AnalyzeWhenBaseFixtureIsNotAbstract(string attribute)
46+
{
47+
var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@"
48+
public class ↓BaseFixture
49+
{{
50+
[{attribute}]
51+
public void BaseFixtureMethod() {{ }}
52+
}}
53+
54+
public class DerivedFixture : BaseFixture
55+
{{
56+
[Test]
57+
public void DerivedFixtureMethod() {{ }}
58+
}}");
59+
60+
RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode);
61+
}
62+
63+
[Test]
64+
public void AnalyzeWhenGenericFixtureIsAbstract()
65+
{
66+
var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
67+
public abstract class BaseFixture<T>
68+
{
69+
[Test]
70+
public void BaseFixtureMethod() { }
71+
}
72+
73+
public class IntFixture : BaseFixture<int>
74+
{
75+
}
76+
77+
public class DoubleFixture : BaseFixture<double>
78+
{
79+
}");
80+
81+
RoslynAssert.Valid(analyzer, testCode);
82+
}
83+
84+
[Test]
85+
public void AnalyzeWhenGenericFixtureIsNotAbstract()
86+
{
87+
var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@"
88+
public class ↓BaseFixture<T>
89+
{
90+
[Test]
91+
public void BaseFixtureMethod() { }
92+
}
93+
94+
public class IntFixture : BaseFixture<int>
95+
{
96+
}
97+
98+
public class DoubleFixture : BaseFixture<double>
99+
{
100+
}");
101+
102+
RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode);
103+
}
104+
}
105+
}

‎src/nunit.analyzers.tests/TestFixtureShouldBeAbstract/TestFixtureShouldBeAbstractCodeFixTests.cs‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
using System.Collections.Generic;
2+
using Gu.Roslyn.Asserts;
3+
using Microsoft.CodeAnalysis.CodeFixes;
4+
using Microsoft.CodeAnalysis.Diagnostics;
5+
using NUnit.Analyzers.Constants;
6+
using NUnit.Analyzers.TestFixtureShouldBeAbstract;
7+
using NUnit.Framework;
8+
9+
namespace NUnit.Analyzers.Tests.TestFixtureShouldBeAbstract
10+
{
11+
public class TestFixtureShouldBeAbstractCodeFixTests
12+
{
13+
private static readonly DiagnosticAnalyzer analyzer = new TestFixtureShouldBeAbstractAnalyzer();
14+
private static readonly CodeFixProvider fix = new TestFixtureShouldBeAbstractCodeFix();
15+
private static readonly ExpectedDiagnostic expectedDiagnostic =
16+
ExpectedDiagnostic.Create(AnalyzerIdentifiers.BaseTestFixtureIsNotAbstract);
17+
18+
private static readonly IEnumerable<string[]> testMethodRelatedAttributes =
19+
[
20+
["", "OneTimeSetUp"],
21+
["internal ", "OneTimeTearDown"],
22+
["public ", "SetUp"],
23+
["partial ", "TearDown"],
24+
["internal partial ", "Test"],
25+
];
26+
27+
[TestCaseSource(nameof(testMethodRelatedAttributes))]
28+
public void FixWhenBaseFixtureIsNotAbstract(string modifiers, string attribute)
29+
{
30+
var testCode = TestUtility.WrapClassInNamespaceAndAddUsing($@"
31+
// Base Fixture
32+
{modifiers}class ↓BaseFixture
33+
{{
34+
[{attribute}]
35+
public void BaseFixtureMethod() {{ }}
36+
}}
37+
38+
{modifiers}class DerivedFixture : BaseFixture
39+
{{
40+
[Test]
41+
public void DerivedFixtureMethod() {{ }}
42+
}}");
43+
44+
var fixedCode = TestUtility.WrapClassInNamespaceAndAddUsing($@"
45+
// Base Fixture
46+
{modifiers}abstract class BaseFixture
47+
{{
48+
[{attribute}]
49+
public void BaseFixtureMethod() {{ }}
50+
}}
51+
52+
{modifiers}class DerivedFixture : BaseFixture
53+
{{
54+
[Test]
55+
public void DerivedFixtureMethod() {{ }}
56+
}}");
57+
58+
RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, testCode, fixedCode);
59+
}
60+
}
61+
}

‎src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ internal static class AnalyzerIdentifiers
3737
internal const string ValuesParameterTypeMismatchUsage = "NUnit1031";
3838
internal const string FieldIsNotDisposedInTearDown = "NUnit1032";
3939
internal const string TestContextWriteIsObsolete = "NUnit1033";
40+
internal const string BaseTestFixtureIsNotAbstract = "NUnit1034";
4041

4142
#endregion Structure
4243

‎src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ internal abstract class NUnitFrameworkConstants
8686
public const string FullNameOfTypeTestAttribute = "NUnit.Framework.TestAttribute";
8787
public const string FullNameOfTypeParallelizableAttribute = "NUnit.Framework.ParallelizableAttribute";
8888
public const string FullNameOfTypeITestBuilder = "NUnit.Framework.Interfaces.ITestBuilder";
89+
public const string FullNameOfTypeIFixtureBuilder = "NUnit.Framework.Interfaces.IFixtureBuilder";
8990
public const string FullNameOfTypeISimpleTestBuilder = "NUnit.Framework.Interfaces.ISimpleTestBuilder";
9091
public const string FullNameOfTypeValuesAttribute = "NUnit.Framework.ValuesAttribute";
9192
public const string FullNameOfTypeValueSourceAttribute = "NUnit.Framework.ValueSourceAttribute";

0 commit comments

Comments
 (0)