diff --git a/Documentation/Diagnostics/PH2161.md b/Documentation/Diagnostics/PH2161.md new file mode 100644 index 000000000..c61fccf8f --- /dev/null +++ b/Documentation/Diagnostics/PH2161.md @@ -0,0 +1,64 @@ +# PH2161: Avoid Retry attribute + +| Property | Value | +|--|--| +| Package | [Philips.CodeAnalysis.MsTestAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MsTestAnalyzers) | +| Diagnostic ID | PH2161 | +| Category | [MsTest](../MsTest.md) | +| Analyzer | [AvoidAttributeAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MsTestAnalyzers/AvoidAttributeAnalyzer.cs) +| CodeFix | No | +| Severity | Error | +| Enabled By Default | Yes | + +## Introduction + +MSTest has a [Retry] attribute to attempt to overcome flaky tests. It would be preferable to avoid the flaky test. + +More thoroughly, one should avoid any attribute that derives from RetryBaseAttribute. + +## How to solve + +Remove the flaky test or fix the underlying cause of the flakiness. Rely on making your tests deterministic rather than retrying them. + +## Example + +Code that triggers a diagnostic: +``` cs + [TestMethod] + [Retry(3)] + public void BadTestMethod() + { + Assert.AreEqual(1, 1); + } +``` + +Code that also triggers a diagnostic (custom attribute deriving from RetryBaseAttribute): +``` cs + public class CustomRetryAttribute : RetryBaseAttribute + { + public CustomRetryAttribute(int count) : base(count) { } + } + + [TestMethod] + [CustomRetry(5)] + public void BadTestMethod() + { + Assert.AreEqual(1, 1); + } +``` + +## Similar Analyzers + +The following analyzers detect other situations that prevent tests from executing (either unwittingly or nefariously): + +| ID | Title | +|--|--| +| [PH2013](.\PH2013.md) | Avoid Ignore attribute | +| [PH2034](.\PH2034.md) | Test methods must be in TestClass | +| [PH2036](.\PH2036.md) | Test methods must be public | +| [PH2038](.\PH2038.md) | Test classes must be public | +| [PH2059](.\PH2059.md) | Public methods must be TestMethods | + +## Configuration + +This analyzer does not offer any special configuration. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply. \ No newline at end of file diff --git a/Philips.CodeAnalysis.Common/DiagnosticId.cs b/Philips.CodeAnalysis.Common/DiagnosticId.cs index 890ae1fa7..5883ad59c 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, + AvoidRetryAttribute = 2161, } } diff --git a/Philips.CodeAnalysis.MsTestAnalyzers/AvoidAttributeAnalyzer.cs b/Philips.CodeAnalysis.MsTestAnalyzers/AvoidAttributeAnalyzer.cs index 4dde2ee9a..c43968651 100644 --- a/Philips.CodeAnalysis.MsTestAnalyzers/AvoidAttributeAnalyzer.cs +++ b/Philips.CodeAnalysis.MsTestAnalyzers/AvoidAttributeAnalyzer.cs @@ -19,6 +19,15 @@ public class AvoidAttributeAnalyzer : DiagnosticAnalyzerBase private static readonly ImmutableDictionary> Attributes = GetAttributeModels(); + private static readonly DiagnosticDescriptor RetryBaseAttributeRule = new( + "PH2161", + "Retry attribute not allowed", + "Tests may not use attributes that derive from RetryBaseAttribute.", + Categories.Maintainability, + DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "Attributes that derive from RetryBaseAttribute attempt to overcome flaky tests. It would be preferable to avoid the flaky test."); + public static readonly ImmutableArray Rules = GetRules(Attributes); public override ImmutableArray SupportedDiagnostics => Rules; @@ -92,6 +101,49 @@ private void Analyze(ImmutableArray attributes, SyntaxNodeAnalys var diagnostic = Diagnostic.Create(attribute.Rule, descriptionLocation, id); context.ReportDiagnostic(diagnostic); } + + // Additional check for attributes that derive from RetryBaseAttribute + CheckForRetryBaseAttributeDerivatives(attributesNode, context); + } + + private void CheckForRetryBaseAttributeDerivatives(AttributeListSyntax attributesNode, SyntaxNodeAnalysisContext context) + { + foreach (AttributeSyntax attributeSyntax in attributesNode.Attributes) + { + SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(attributeSyntax); + if (symbolInfo.Symbol is IMethodSymbol constructorSymbol) + { + INamedTypeSymbol attributeClass = constructorSymbol.ContainingType; + + // Skip if this is the direct Retry attribute (already handled by the main check) + if (attributeClass.Name == "RetryAttribute" && + attributeClass.ContainingNamespace.ToDisplayString() == "Microsoft.VisualStudio.TestTools.UnitTesting") + { + continue; + } + + if (DerivesFromRetryBaseAttribute(attributeClass)) + { + var diagnostic = Diagnostic.Create(RetryBaseAttributeRule, attributeSyntax.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + } + } + } + + private bool DerivesFromRetryBaseAttribute(INamedTypeSymbol attributeType) + { + INamedTypeSymbol baseType = attributeType.BaseType; + while (baseType != null) + { + if (baseType.Name == "RetryBaseAttribute" && + baseType.ContainingNamespace.ToDisplayString() == "Microsoft.VisualStudio.TestTools.UnitTesting") + { + return true; + } + baseType = baseType.BaseType; + } + return false; } private bool IsWhitelisted(ImmutableHashSet whitelist, SemanticModel semanticModel, SyntaxNode node, out string id) @@ -117,6 +169,9 @@ private static ImmutableArray GetRules(ImmutableDictionary .Select(x => x.Rule); builder.AddRange(items); + // Add the RetryBaseAttribute rule + builder.Add(RetryBaseAttributeRule); + return builder.ToImmutable(); } @@ -172,9 +227,18 @@ private static ImmutableDictionary> GetAt isSuppressible: true, isEnabledByDefault: true); + var retryAttribute = new AttributeModel(@"Retry", + @"Microsoft.VisualStudio.TestTools.UnitTesting.RetryAttribute", + @"Retry attribute not allowed", + @"Tests may not use the Retry attribute.", + @"The Retry attribute attempts to overcome flaky tests. It would be preferable to avoid the flaky test.", + DiagnosticId.AvoidRetryAttribute, + isSuppressible: false, + isEnabledByDefault: true); + ImmutableDictionary>.Builder builder = ImmutableDictionary.CreateBuilder>(); - builder[StringConstants.AssertFullyQualifiedName] = ImmutableArray.Create(ownerAttribute, removedAttribute, testInitializeAttribute, testCleanupAttribute, classCleanupAttribute, classInitializeAttribute); + builder[StringConstants.AssertFullyQualifiedName] = ImmutableArray.Create(ownerAttribute, removedAttribute, testInitializeAttribute, testCleanupAttribute, classCleanupAttribute, classInitializeAttribute, retryAttribute); return builder.ToImmutable(); } diff --git a/Philips.CodeAnalysis.Test/MsTest/AvoidRetryAttributeAnalyzerTest.cs b/Philips.CodeAnalysis.Test/MsTest/AvoidRetryAttributeAnalyzerTest.cs new file mode 100644 index 000000000..798de8d88 --- /dev/null +++ b/Philips.CodeAnalysis.Test/MsTest/AvoidRetryAttributeAnalyzerTest.cs @@ -0,0 +1,132 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System.Text.RegularExpressions; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +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 AvoidRetryAttributeAnalyzerTest : DiagnosticVerifier + { + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task AvoidRetryAttributeTestAsync() + { + var givenText = @" +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.VisualStudio.TestTools.UnitTesting +{ + public class RetryAttribute : System.Attribute + { + public RetryAttribute(int count) { } + } +} + +[TestClass] +class Foo +{ + [TestMethod] + [Retry(3)] + public void TestMethod() + { + Assert.IsTrue(true); + } +} +"; + + DiagnosticResult expected = new() + { + Id = DiagnosticId.AvoidRetryAttribute.ToId(), + Message = new Regex(".*"), + Severity = DiagnosticSeverity.Error, + Locations = new[] + { + new DiagnosticResultLocation("Test0.cs", 16, 4) + } + }; + + await VerifyDiagnostic(givenText, expected).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task AvoidCustomRetryAttributeTestAsync() + { + var givenText = @" +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.VisualStudio.TestTools.UnitTesting +{ + public class RetryBaseAttribute : System.Attribute + { + public RetryBaseAttribute(int count) { } + } +} + +public class CustomRetryAttribute : Microsoft.VisualStudio.TestTools.UnitTesting.RetryBaseAttribute +{ + public CustomRetryAttribute(int count) : base(count) { } +} + +[TestClass] +class Foo +{ + [TestMethod] + [CustomRetry(5)] + public void TestMethod() + { + Assert.IsTrue(true); + } +} +"; + + DiagnosticResult expected = new() + { + Id = DiagnosticId.AvoidRetryAttribute.ToId(), + Message = new Regex(".*"), + Severity = DiagnosticSeverity.Error, + Locations = new[] + { + new DiagnosticResultLocation("Test0.cs", 21, 4) + } + }; + + await VerifyDiagnostic(givenText, expected).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task NonRetryAttributesAreIgnoredAsync() + { + var givenText = @" +using Microsoft.VisualStudio.TestTools.UnitTesting; + +[TestClass] +class Foo +{ + [TestMethod] + [TestCategory(""Unit"")] + public void TestMethod() + { + Assert.IsTrue(true); + } +} +"; + + await VerifySuccessfulCompilation(givenText).ConfigureAwait(false); + } + + protected override DiagnosticAnalyzer GetDiagnosticAnalyzer() + { + return new AvoidAttributeAnalyzer(); + } + } +} \ No newline at end of file