diff --git a/Documentation/Diagnostics/PH2162.md b/Documentation/Diagnostics/PH2162.md new file mode 100644 index 000000000..537872ee6 --- /dev/null +++ b/Documentation/Diagnostics/PH2162.md @@ -0,0 +1,141 @@ +# PH2162: TestTimeouts class should be internal + +| Property | Value | +|--|--| +| Package | [Philips.CodeAnalysis.MsTestAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MsTestAnalyzers) | +| Diagnostic ID | PH2162 | +| Category | [MsTest](../MsTest.md) | +| Analyzer | [TestTimeoutsClassAccessibilityAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityAnalyzer.cs) | +| CodeFix | Yes | +| Severity | Warning | +| Enabled By Default | Yes | + +## Introduction + +Projects commonly define a `TestTimeouts` class that centralizes timeout constants (e.g., `CiAppropriate`, `Integration`). This analyzer enforces that any class named `TestTimeouts` is declared as `internal` and not `static`. Public exposure or use of `static` on this coordination class is discouraged. + +This rule helps avoid conflicts with the MSTest analyzer that restricts all classes in a test project from being public unless they are annotated with `[TestClass]`. + +## Reason + +Keeping `TestTimeouts` internal: + +1. Prevents leaking test-only infrastructure into the public API surface of a library. +2. Avoids external projects taking a compile-time dependency on timeout values that are intended to be adjusted freely. +3. Encourages each assembly to own and tune its own timeout set without cross-assembly coupling. +4. Reduces the risk that production code (outside test projects) references test timing constants. + +Disallowing `static` (while permitting `sealed`) helps future flexibility (e.g., partial conditional compilation patterns) and keeps the shape consistent with existing guidance in related timeout analyzers. A `sealed` internal class with only `const` members satisfies the same usage needs without requiring `static`. + +## What triggers the diagnostic + +The diagnostic is reported for any class whose identifier is exactly `TestTimeouts` if either: + +* It is declared `public` (with or without `sealed`). +* It is declared `internal static`. + +The class name comparison is exact and case-sensitive. + +## How to fix + +Change the declaration to be `internal` (optionally `sealed`) and remove `static` if present. The provided Code Fix will: + +* Remove `public`. +* Remove `static` (when paired with `internal`). +* Insert `internal` if missing. +* Preserve `sealed` if it exists. + +## Examples + +Code that triggers the diagnostic (public): +```cs +public class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Fix: +```cs +internal class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Code that triggers the diagnostic (public sealed): +```cs +public sealed class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Fix (Code Fix output): +```cs +internal sealed class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Code that triggers the diagnostic (internal static): +```cs +internal static class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Fix (Code Fix output): +```cs +internal class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Code that does NOT trigger the diagnostic: +```cs +internal class TestTimeouts +{ + public const int CiAppropriate = 1000; +} + +internal sealed class TestTimeouts +{ + public const int CiAppropriate = 1000; +} +``` + +Other class names are ignored: +```cs +public class SomeOtherClass +{ + public const int CiAppropriate = 1000; +} +``` + +## Code Fix behavior + +Selecting the Code Fix on a reported diagnostic rewrites the class modifiers as described above. It does not otherwise reorder members or alter whitespace beyond necessary modifier changes. + +## Similar analyzers + +| ID | Title | +|--|--| +| [PH2012](./PH2012.md) | Test has Timeout | +| [PH2015](./PH2015.md) | Test must have appropriate Test Category | + +These analyzers collectively encourage consistent timeout usage patterns. + +## Configuration + +This analyzer does not currently support custom configuration keys. Standard suppression mechanisms (pragma, GlobalSuppressions.cs, or EditorConfig severity adjustments) apply, but suppression is discouraged—prefer adopting the recommended accessibility. + +``` +# Example: adjust severity (not recommended) +dotnet_diagnostic.PH2162.severity = warning +``` + + diff --git a/Philips.CodeAnalysis.Common/DiagnosticId.cs b/Philips.CodeAnalysis.Common/DiagnosticId.cs index 890ae1fa7..0a06932b0 100644 --- a/Philips.CodeAnalysis.Common/DiagnosticId.cs +++ b/Philips.CodeAnalysis.Common/DiagnosticId.cs @@ -144,5 +144,6 @@ public enum DiagnosticId AvoidUnusedToString = 2153, AvoidUnlicensedPackages = 2155, AvoidPkcsPaddingWithRsaEncryption = 2158, + TestTimeoutsClassShouldBeInternal = 2162, } } diff --git a/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityAnalyzer.cs b/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityAnalyzer.cs new file mode 100644 index 000000000..411a1d860 --- /dev/null +++ b/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityAnalyzer.cs @@ -0,0 +1,61 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Philips.CodeAnalysis.Common; + +namespace Philips.CodeAnalysis.MsTestAnalyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class TestTimeoutsClassAccessibilityAnalyzer : SingleDiagnosticAnalyzer + { + private const string Title = @"TestTimeouts class should be internal"; + private const string MessageFormat = @"Class 'TestTimeouts' should be declared as internal"; + private const string Description = @"TestTimeouts classes should be internal to avoid accessibility issues"; + + public TestTimeoutsClassAccessibilityAnalyzer() + : base(DiagnosticId.TestTimeoutsClassShouldBeInternal, Title, MessageFormat, Description, Categories.MsTest, isEnabled: true) + { } + + protected override void InitializeCompilation(CompilationStartAnalysisContext context) + { + context.RegisterSyntaxNodeAction(AnalyzeClassDeclaration, SyntaxKind.ClassDeclaration); + } + + private void AnalyzeClassDeclaration(SyntaxNodeAnalysisContext context) + { + if (Helper.ForGeneratedCode.IsGeneratedCode(context)) + { + return; + } + + var classDeclaration = (ClassDeclarationSyntax)context.Node; + + // Check if the class is named "TestTimeouts" + if (classDeclaration.Identifier.ValueText != "TestTimeouts") + { + return; + } + + SyntaxTokenList modifiers = classDeclaration.Modifiers; + + // Check if the class has problematic modifiers: + // 1. public (with or without sealed) + // 2. internal static + var hasPublic = modifiers.Any(SyntaxKind.PublicKeyword); + var hasInternal = modifiers.Any(SyntaxKind.InternalKeyword); + var hasStatic = modifiers.Any(SyntaxKind.StaticKeyword); + + var needsFix = hasPublic || (hasInternal && hasStatic); + + if (needsFix) + { + Location location = classDeclaration.Identifier.GetLocation(); + var diagnostic = Diagnostic.Create(Rule, location); + context.ReportDiagnostic(diagnostic); + } + } + } +} diff --git a/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityCodeFixProvider.cs b/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityCodeFixProvider.cs new file mode 100644 index 000000000..093490e30 --- /dev/null +++ b/Philips.CodeAnalysis.MsTestAnalyzers/TestTimeoutsClassAccessibilityCodeFixProvider.cs @@ -0,0 +1,54 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Philips.CodeAnalysis.Common; + +namespace Philips.CodeAnalysis.MsTestAnalyzers +{ + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(TestTimeoutsClassAccessibilityCodeFixProvider)), Shared] + public class TestTimeoutsClassAccessibilityCodeFixProvider : SingleDiagnosticCodeFixProvider + { + protected override string Title => "Make TestTimeouts class internal"; + + protected override DiagnosticId DiagnosticId => DiagnosticId.TestTimeoutsClassShouldBeInternal; + + protected override async Task ApplyFix(Document document, ClassDeclarationSyntax node, ImmutableDictionary properties, CancellationToken cancellationToken) + { + SyntaxNode rootNode = await document.GetSyntaxRootAsync(cancellationToken); + + // Remove problematic modifiers and ensure internal is present + SyntaxTokenList modifiers = node.Modifiers; + + // Remove public, sealed (when public), and static (when internal) modifiers + var newModifiers = modifiers + .Where(m => m.Kind() is not SyntaxKind.PublicKeyword and not SyntaxKind.StaticKeyword) + .ToList(); + + // Check if we need to add internal + var hasInternal = newModifiers.Any(m => m.Kind() == SyntaxKind.InternalKeyword); + + if (!hasInternal) + { + // Add internal modifier at the beginning + newModifiers.Insert(0, SyntaxFactory.Token(SyntaxKind.InternalKeyword)); + } + + // Create new class declaration with updated modifiers + ClassDeclarationSyntax newClass = node.WithModifiers(SyntaxFactory.TokenList(newModifiers)); + + // Replace the node in the root + SyntaxNode root = rootNode.ReplaceNode(node, newClass); + Document newDocument = document.WithSyntaxRoot(root); + + return newDocument; + } + } +} diff --git a/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidProblematicUsingPatternsAnalyzerTest.cs b/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidProblematicUsingPatternsAnalyzerTest.cs index ead9d4480..9fd816016 100644 --- a/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidProblematicUsingPatternsAnalyzerTest.cs +++ b/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidProblematicUsingPatternsAnalyzerTest.cs @@ -1,4 +1,4 @@ -// © 2024 Koninklijke Philips N.V. See License.md in the project root for license information. +// © 2024 Koninklijke Philips N.V. See License.md in the project root for license information. using System.Threading.Tasks; using Microsoft.CodeAnalysis.Diagnostics; @@ -239,4 +239,4 @@ protected override DiagnosticAnalyzer GetDiagnosticAnalyzer() return new AvoidProblematicUsingPatternsAnalyzer(); } } -} \ No newline at end of file +} diff --git a/Philips.CodeAnalysis.Test/MsTest/TestTimeoutsClassAccessibilityAnalyzerTest.cs b/Philips.CodeAnalysis.Test/MsTest/TestTimeoutsClassAccessibilityAnalyzerTest.cs new file mode 100644 index 000000000..569243b06 --- /dev/null +++ b/Philips.CodeAnalysis.Test/MsTest/TestTimeoutsClassAccessibilityAnalyzerTest.cs @@ -0,0 +1,105 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Philips.CodeAnalysis.Common; +using Philips.CodeAnalysis.MsTestAnalyzers; +using Philips.CodeAnalysis.Test.Helpers; +using Philips.CodeAnalysis.Test.Verifiers; + +namespace Philips.CodeAnalysis.Test.MsTest +{ + [TestClass] + public class TestTimeoutsClassAccessibilityAnalyzerTest : CodeFixVerifier + { + protected override DiagnosticAnalyzer GetDiagnosticAnalyzer() + { + return new TestTimeoutsClassAccessibilityAnalyzer(); + } + + protected override CodeFixProvider GetCodeFixProvider() + { + return new TestTimeoutsClassAccessibilityCodeFixProvider(); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task TimeoutsClassPublicShouldTriggerDiagnostic() + { + const string code = @"public class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + const string expectedCode = @"internal class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + await VerifyDiagnostic(code, DiagnosticId.TestTimeoutsClassShouldBeInternal).ConfigureAwait(false); + await VerifyFix(code, expectedCode).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task TimeoutsClassInternalShouldNotTriggerDiagnostic() + { + const string code = @"internal class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + await VerifySuccessfulCompilation(code).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task TimeoutsClassPublicSealedShouldTriggerDiagnostic() + { + const string code = @"public sealed class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + const string expectedCode = @"internal sealed class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + await VerifyDiagnostic(code, DiagnosticId.TestTimeoutsClassShouldBeInternal).ConfigureAwait(false); + await VerifyFix(code, expectedCode).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task TimeoutsClassInternalStaticShouldTriggerDiagnostic() + { + const string code = @"internal static class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + const string expectedCode = @"internal class TestTimeouts +{ + public const int CiAppropriate = 1000; +}"; + + await VerifyDiagnostic(code, DiagnosticId.TestTimeoutsClassShouldBeInternal).ConfigureAwait(false); + await VerifyFix(code, expectedCode).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task OtherClassNameShouldNotTriggerDiagnostic() + { + const string code = @"public class SomeOtherClass +{ + public const int CiAppropriate = 1000; +}"; + + await VerifySuccessfulCompilation(code).ConfigureAwait(false); + } + } +}