diff --git a/documentation/NUnit1033.md b/documentation/NUnit1033.md new file mode 100644 index 00000000..c174d0b8 --- /dev/null +++ b/documentation/NUnit1033.md @@ -0,0 +1,65 @@ +# NUnit1033 + +## The Write methods on TestContext are Obsolete + +| Topic | Value +| :-- | :-- +| Id | NUnit1033 +| Severity | Error +| Enabled | True +| Category | Structure +| Code | [TestContextWriteIsObsoleteAnalyzer](https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzer.cs) + +## Description + +Direct Write calls should be replaced with Out.Write. + +## Motivation + +The `Write` methods are simple wrappers calling `Out.Write`. There is no wrapper for `Error` which always required to use `TestContext.Error.Write`. +Besides this being inconsistent, later versions of .NET added new overloads, e.g. for `ReadOnlySpan` and `async` methods like `WriteAsync`. +Instead of adding more and more dummy wrappers, it was decided that user code should use the `Out` property and then can use any `Write` overload available on `TextWriter`. + +## How to fix violations + +Simple insert `.Out` between `TestContext` and `.Write`. + + +## Configure severity + +### Via ruleset file + +Configure the severity per project, for more info see +[MSDN](https://learn.microsoft.com/en-us/visualstudio/code-quality/using-rule-sets-to-group-code-analysis-rules?view=vs-2022). + +### Via .editorconfig file + +```ini +# NUnit1033: The Write methods on TestContext are Obsolete +dotnet_diagnostic.NUnit1033.severity = chosenSeverity +``` + +where `chosenSeverity` can be one of `none`, `silent`, `suggestion`, `warning`, or `error`. + +### Via #pragma directive + +```csharp +#pragma warning disable NUnit1033 // The Write methods on TestContext are Obsolete +Code violating the rule here +#pragma warning restore NUnit1033 // The Write methods on TestContext are Obsolete +``` + +Or put this at the top of the file to disable all instances. + +```csharp +#pragma warning disable NUnit1033 // The Write methods on TestContext are Obsolete +``` + +### Via attribute `[SuppressMessage]` + +```csharp +[System.Diagnostics.CodeAnalysis.SuppressMessage("Structure", + "NUnit1033:The Write methods on TestContext are Obsolete", + Justification = "Reason...")] +``` + diff --git a/documentation/index.md b/documentation/index.md index 44ba0c2e..2984e88d 100644 --- a/documentation/index.md +++ b/documentation/index.md @@ -51,6 +51,7 @@ Rules which enforce structural requirements on the test code. | [NUnit1030](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1030.md) | The type of parameter provided by the TestCaseSource does not match the type of the parameter in the Test method | :white_check_mark: | :exclamation: | :x: | | [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: | | [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: | +| [NUnit1033](https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit1033.md) | The Write methods on TestContext are Obsolete | :white_check_mark: | :exclamation: | :white_check_mark: | ## Assertion Rules (NUnit2001 - ) diff --git a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs index c0e907ae..4ea50688 100644 --- a/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs +++ b/src/nunit.analyzers.tests/Constants/NUnitFrameworkConstantsTests.cs @@ -76,6 +76,10 @@ public sealed class NUnitFrameworkConstantsTests (nameof(NUnitFrameworkConstants.NameOfMultipleAsync), "MultipleAsync"), #endif + (nameof(NUnitFrameworkConstants.NameOfOut), nameof(TestContext.Out)), + (nameof(NUnitFrameworkConstants.NameOfWrite), nameof(TestContext.Out.Write)), + (nameof(NUnitFrameworkConstants.NameOfWriteLine), nameof(TestContext.Out.WriteLine)), + (nameof(NUnitFrameworkConstants.NameOfThrows), nameof(Throws)), (nameof(NUnitFrameworkConstants.NameOfThrowsArgumentException), nameof(Throws.ArgumentException)), (nameof(NUnitFrameworkConstants.NameOfThrowsArgumentNullException), nameof(Throws.ArgumentNullException)), @@ -208,6 +212,8 @@ public sealed class NUnitFrameworkConstantsTests (nameof(NUnitFrameworkConstants.FullNameOfCancelAfterAttribute), typeof(CancelAfterAttribute)), (nameof(NUnitFrameworkConstants.FullNameOfCancellationToken), typeof(CancellationToken)), + (nameof(NUnitFrameworkConstants.FullNameOfTypeTestContext), typeof(TestContext)), + (nameof(NUnitFrameworkConstants.FullNameOfSameAsConstraint), typeof(SameAsConstraint)), (nameof(NUnitFrameworkConstants.FullNameOfSomeItemsConstraint), typeof(SomeItemsConstraint)), (nameof(NUnitFrameworkConstants.FullNameOfEqualToConstraint), typeof(EqualConstraint)), diff --git a/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzerTests.cs b/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzerTests.cs new file mode 100644 index 00000000..2ca84ae0 --- /dev/null +++ b/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzerTests.cs @@ -0,0 +1,39 @@ +using Gu.Roslyn.Asserts; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Analyzers.Constants; +using NUnit.Analyzers.TestContextWriteIsObsolete; +using NUnit.Framework; + +namespace NUnit.Analyzers.Tests.TestContextWriteIsObsolete +{ + public class TestContextWriteIsObsoleteAnalyzerTests + { + private static readonly DiagnosticAnalyzer analyzer = new TestContextWriteIsObsoleteAnalyzer(); + private static readonly ExpectedDiagnostic expectedDiagnostic = + ExpectedDiagnostic.Create(AnalyzerIdentifiers.TestContextWriteIsObsolete); + + [TestCaseSource(typeof(TestContextWriteIsObsoleteTestCases), nameof(TestContextWriteIsObsoleteTestCases.WriteInvocations))] + public void AnyDirectWriteMethod(string writeMethodAndParameters) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + public void Test() + {{ + TestContext.{writeMethodAndParameters}; + }}"); + + RoslynAssert.Diagnostics(analyzer, expectedDiagnostic, testCode); + } + + [TestCaseSource(typeof(TestContextWriteIsObsoleteTestCases), nameof(TestContextWriteIsObsoleteTestCases.WriteInvocations))] + public void AnyIndirectWriteMethod(string writeMethodAndParameters) + { + var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + public void Test() + {{ + TestContext.Out.{writeMethodAndParameters}; + }}"); + + RoslynAssert.Valid(analyzer, testCode); + } + } +} diff --git a/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteCodeFixTests.cs b/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteCodeFixTests.cs new file mode 100644 index 00000000..c58194c5 --- /dev/null +++ b/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteCodeFixTests.cs @@ -0,0 +1,36 @@ +using Gu.Roslyn.Asserts; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Analyzers.Constants; +using NUnit.Analyzers.TestContextWriteIsObsolete; +using NUnit.Framework; + +namespace NUnit.Analyzers.Tests.TestContextWriteIsObsolete +{ + public class TestContextWriteIsObsoleteCodeFixTests + { + private static readonly DiagnosticAnalyzer analyzer = new TestContextWriteIsObsoleteAnalyzer(); + private static readonly CodeFixProvider fix = new TestContextWriteIsObsoleteCodeFix(); + private static readonly ExpectedDiagnostic expectedDiagnostic = + ExpectedDiagnostic.Create(AnalyzerIdentifiers.TestContextWriteIsObsolete); + + [TestCaseSource(typeof(TestContextWriteIsObsoleteTestCases), nameof(TestContextWriteIsObsoleteTestCases.WriteInvocations))] + public void AnyWriteMethod(string writeMethodAndParameters) + { + var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + public void Test() + {{ + TestContext.{writeMethodAndParameters}; + }}"); + + var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings($@" + public void Test() + {{ + TestContext.Out.{writeMethodAndParameters}; + }}"); + + RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode, + fixTitle: TestContextWriteIsObsoleteCodeFix.InsertOutDescription); + } + } +} diff --git a/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteTestCases.cs b/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteTestCases.cs new file mode 100644 index 00000000..818040bf --- /dev/null +++ b/src/nunit.analyzers.tests/TestContextWriteIsObsolete/TestContextWriteIsObsoleteTestCases.cs @@ -0,0 +1,46 @@ +namespace NUnit.Analyzers.Tests.TestContextWriteIsObsolete +{ + internal static class TestContextWriteIsObsoleteTestCases + { + public static readonly string[] WriteInvocations = + { + "Write(true)", + "Write('!')", + "Write(new char[] { '!', '!' })", + "Write(default(char[]))", + "Write(1D)", + "Write(1)", + "Write(1L)", + "Write(1M)", + "Write(default(object))", + "Write(1F)", + "Write(\"NUnit\")", + "Write(default(string))", + "Write(1U)", + "Write(1UL)", + "Write(\"{0}\", 1)", + "Write(\"{0} + {1}\", 1, 2)", + "Write(\"{0} + {1} = {2}\", 1, 2, 3)", + "Write(\"{0} + {1} = {2} + {3}\", 1, 2, 2, 1)", + "WriteLine()", + "WriteLine(true)", + "WriteLine('!')", + "WriteLine(new char[] { '!', '!' })", + "WriteLine(default(char[]))", + "WriteLine(1D)", + "WriteLine(1)", + "WriteLine(1L)", + "WriteLine(1M)", + "WriteLine(default(object))", + "WriteLine(1F)", + "WriteLine(\"NUnit\")", + "Write(default(string))", + "WriteLine(1U)", + "WriteLine(1UL)", + "WriteLine(\"{0}\", 1)", + "WriteLine(\"{0} + {1}\", 1, 2)", + "WriteLine(\"{0} + {1} = {2}\", 1, 2, 3)", + "WriteLine(\"{0} + {1} = {2} + {3}\", 1, 2, 2, 1)", + }; + } +} diff --git a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs index 8abaaa78..79c92c74 100644 --- a/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs +++ b/src/nunit.analyzers/Constants/AnalyzerIdentifiers.cs @@ -36,6 +36,7 @@ internal static class AnalyzerIdentifiers internal const string TestCaseSourceMismatchWithTestMethodParameterType = "NUnit1030"; internal const string ValuesParameterTypeMismatchUsage = "NUnit1031"; internal const string FieldIsNotDisposedInTearDown = "NUnit1032"; + internal const string TestContextWriteIsObsolete = "NUnit1033"; #endregion Structure diff --git a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs index 300763e9..7f5f7b2c 100644 --- a/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs +++ b/src/nunit.analyzers/Constants/NUnitFrameworkConstants.cs @@ -52,6 +52,10 @@ public static class NUnitFrameworkConstants public const string NameOfMultiple = "Multiple"; public const string NameOfMultipleAsync = "MultipleAsync"; + public const string NameOfOut = "Out"; + public const string NameOfWrite = "Write"; + public const string NameOfWriteLine = "WriteLine"; + public const string NameOfThrows = "Throws"; public const string NameOfThrowsArgumentException = "ArgumentException"; public const string NameOfThrowsArgumentNullException = "ArgumentNullException"; @@ -153,6 +157,8 @@ public static class NUnitFrameworkConstants public const string FullNameOfCancelAfterAttribute = "NUnit.Framework.CancelAfterAttribute"; public const string FullNameOfCancellationToken = "System.Threading.CancellationToken"; + public const string FullNameOfTypeTestContext = "NUnit.Framework.TestContext"; + public const string NameOfConstraint = "Constraint"; public const string FullNameOfSameAsConstraint = "NUnit.Framework.Constraints.SameAsConstraint"; diff --git a/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzer.cs b/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzer.cs new file mode 100644 index 00000000..d87eaf08 --- /dev/null +++ b/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzer.cs @@ -0,0 +1,68 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using NUnit.Analyzers.Constants; + +namespace NUnit.Analyzers.TestContextWriteIsObsolete +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class TestContextWriteIsObsoleteAnalyzer : DiagnosticAnalyzer + { + private static readonly DiagnosticDescriptor descriptor = DiagnosticDescriptorCreator.Create( + id: AnalyzerIdentifiers.TestContextWriteIsObsolete, + title: TestContextWriteIsObsoleteAnalyzerConstants.Title, + messageFormat: TestContextWriteIsObsoleteAnalyzerConstants.Message, + category: Categories.Structure, + defaultSeverity: DiagnosticSeverity.Error, + description: TestContextWriteIsObsoleteAnalyzerConstants.Description); + + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(descriptor); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(AnalyzeCompilationStart); + } + + private static void AnalyzeCompilationStart(CompilationStartAnalysisContext context) + { + INamedTypeSymbol? testContextType = context.Compilation.GetTypeByMetadataName(NUnitFrameworkConstants.FullNameOfTypeTestContext); + if (testContextType is null) + { + return; + } + + context.RegisterOperationAction(context => AnalyzeInvocation(testContextType, context), OperationKind.Invocation); + } + + private static void AnalyzeInvocation(INamedTypeSymbol testContextType, OperationAnalysisContext context) + { + if (context.Operation is not IInvocationOperation invocationOperation) + return; + + // TestContext.Write methods are static methods + if (invocationOperation.Instance is not null) + return; + + IMethodSymbol targetMethod = invocationOperation.TargetMethod; + + if (!targetMethod.ReturnsVoid) + return; + + context.CancellationToken.ThrowIfCancellationRequested(); + + if (!SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType, testContextType)) + return; + + if (targetMethod.Name is NUnitFrameworkConstants.NameOfWrite or + NUnitFrameworkConstants.NameOfWriteLine) + { + context.ReportDiagnostic(Diagnostic.Create( + descriptor, + invocationOperation.Syntax.GetLocation())); + } + } + } +} diff --git a/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzerConstants.cs b/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzerConstants.cs new file mode 100644 index 00000000..b3549685 --- /dev/null +++ b/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteAnalyzerConstants.cs @@ -0,0 +1,9 @@ +namespace NUnit.Analyzers.TestContextWriteIsObsolete +{ + internal static class TestContextWriteIsObsoleteAnalyzerConstants + { + public const string Title = "The Write methods on TestContext are Obsolete"; + public const string Message = "The Write methods are wrappers on TestContext.Out"; + public const string Description = "Direct Write calls should be replaced with Out.Write."; + } +} diff --git a/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteCodeFix.cs b/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteCodeFix.cs new file mode 100644 index 00000000..0c98a020 --- /dev/null +++ b/src/nunit.analyzers/TestContextWriteIsObsolete/TestContextWriteIsObsoleteCodeFix.cs @@ -0,0 +1,66 @@ +using System.Collections.Immutable; +using System.Composition; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using NUnit.Analyzers.Constants; + +namespace NUnit.Analyzers.TestContextWriteIsObsolete +{ + [ExportCodeFixProvider(LanguageNames.CSharp)] + [Shared] + public class TestContextWriteIsObsoleteCodeFix : CodeFixProvider + { + internal const string InsertOutDescription = "Replace TestContext.Write with TestContext.Out.Write"; + + public override ImmutableArray FixableDiagnosticIds { get; } + = ImmutableArray.Create(AnalyzerIdentifiers.TestContextWriteIsObsolete); + + public sealed override FixAllProvider GetFixAllProvider() + { + return WellKnownFixAllProviders.BatchFixer; + } + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + + if (root is null) + { + return; + } + + context.CancellationToken.ThrowIfCancellationRequested(); + + var node = root.FindNode(context.Span); + var invocationNode = node as InvocationExpressionSyntax; + if (invocationNode is null) + return; + + var memberAccessExpression = invocationNode.Expression as MemberAccessExpressionSyntax; + if (memberAccessExpression is null) + return; + + var updatedMemberAccessExpression = + SyntaxFactory.MemberAccessExpression( + memberAccessExpression.Kind(), + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + memberAccessExpression.Expression, + SyntaxFactory.IdentifierName(NUnitFrameworkConstants.NameOfOut)), + memberAccessExpression.Name); + + var newRoot = root.ReplaceNode(memberAccessExpression, updatedMemberAccessExpression); + + var codeAction = CodeAction.Create( + InsertOutDescription, + _ => Task.FromResult(context.Document.WithSyntaxRoot(newRoot)), + InsertOutDescription); + + context.RegisterCodeFix(codeAction, context.Diagnostics); + } + } +}