Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8c6eaa8
Initial plan
Copilot Jul 24, 2025
8ebe896
Add TestShouldUseDisplayNameAnalyzer implementation and new DiagnosticId
Copilot Jul 24, 2025
e87072b
Fix explicit types in analyzer and demonstrate expected functionality
Copilot Jul 24, 2025
1b0af01
Address PR feedback: Change diagnostic ID to 2146 and add documentation
Copilot Jul 26, 2025
809ae19
Fix merge conflict markers causing build failures
Copilot Jul 26, 2025
fac9da7
Fix diagnostic severity to match test expectations
Copilot Jul 26, 2025
025dae2
Merge branch 'main' into copilot/fix-207
bcollamore Jul 28, 2025
c8ace77
Fix build failures by improving comment filtering and disabling analy…
Copilot Jul 28, 2025
5ad28b3
Merge branch 'main' into copilot/fix-207
bcollamore Jul 30, 2025
2113104
Fix analyzer findings by disabling by default and fixing DataRow comment
Copilot Jul 30, 2025
d900363
Merge branch 'main' into copilot/fix-207
bcollamore Jul 30, 2025
a7cb058
Fix dogfood build pipeline errors by disabling PH2146 analyzer in edi…
Copilot Jul 30, 2025
e88eb83
Fix PH2000 test method naming violations and improve PH2146 analyzer …
Copilot Jul 31, 2025
47725f9
Complete PH2146 analyzer fixes - all tests passing, enhanced comment …
Copilot Jul 31, 2025
f3d313f
Fix build issues: revert .editorconfig, disable analyzer by default, …
Copilot Aug 4, 2025
4288bf9
Merge branch 'main' into copilot/fix-207
bcollamore Aug 4, 2025
eaed4cd
Merge branch 'main' into copilot/fix-207
bcollamore Aug 9, 2025
57956d8
refactor: Change diagnostic ID from PH2146 to PH2150
Copilot Aug 9, 2025
3fdc09d
Delete copilot-instructions.md
bcollamore Aug 11, 2025
cb7d7a6
Merge branch 'main' into copilot/fix-207
bcollamore Aug 11, 2025
7022935
Update EnforceRegionsRemoveEmptyRegionAnalyzerTest.cs
bcollamore Aug 11, 2025
c256631
fix: Correct syntax error in EnforceRegionsRemoveEmptyRegionAnalyzerT…
Copilot Aug 12, 2025
d818605
Merge branch 'main' into copilot/fix-207
bcollamore Aug 12, 2025
a6c4257
Merge branch 'main' into copilot/fix-207
bcollamore Aug 13, 2025
cbb1f61
refactor: Simplify loops using LINQ Where method and document comment…
Copilot Aug 13, 2025
b900f87
refactor: Address code review feedback for TestShouldUseDisplayNameAn…
Copilot Aug 15, 2025
0962cb6
Merge branch 'main' into copilot/fix-207
bcollamore Aug 25, 2025
bf46025
Merge branch 'main' into copilot/fix-207
bcollamore Aug 25, 2025
360bd05
Update comment formatting in test data rows
bcollamore Aug 26, 2025
799f0f7
Merge branch 'main' into copilot/fix-207
bcollamore Aug 26, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions Documentation/Diagnostics/PH2150.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# PH2150: Test should use DisplayName or Description attribute instead of comments

| Property | Value |
|--|--|
| Package | [Philips.CodeAnalysis.MsTestAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MsTestAnalyzers) |
| Diagnostic ID | PH2150 |
| Category | [MsTest](../MsTest.md) |
| Analyzer | [TestShouldUseDisplayNameAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MsTestAnalyzers/TestShouldUseDisplayNameAnalyzer.cs)
| CodeFix | No |
| Severity | Info |
| Enabled By Default | No |

## Introduction

Test methods and DataRow attributes often contain inline comments that describe their purpose, but these comments are not visible in test runners. MSTest provides better alternatives: `Description` attribute for test methods and `DisplayName` parameter for DataRow attributes.

## How to solve

For test methods with meaningful comments, add a `Description` attribute instead of using inline comments.

For DataRow attributes with meaningful comments, add a `DisplayName` parameter instead of using inline comments.

## Example

Code that triggers a diagnostic:
``` cs
[TestClass]
public class TestClass
{
// This test verifies that addition works correctly
[TestMethod]
public void TestAddition() { }

[DataTestMethod]
[DataRow(1, 2)] // Should add positive numbers
[DataRow(-1, 1)] // Should handle negative numbers
public void TestAdditionWithData(int a, int b) { }
}
```

And the replacement code:
``` cs
[TestClass]
public class TestClass
{
[TestMethod]
[Description("This test verifies that addition works correctly")]
public void TestAddition() { }

[DataTestMethod]
[DataRow(1, 2, DisplayName = "Should add positive numbers")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer DisplayName to be very short, almost steno. So in this case, I would go for Positive numbers and Negative numbers

[DataRow(-1, 1, DisplayName = "Should handle negative numbers")]
public void TestAdditionWithData(int a, int b) { }
}
```

## Configuration

This analyzer ignores certain boilerplate comment patterns to reduce noise:
- Comments starting with `TODO`
- Comments starting with `FIXME`
- Comments starting with `HACK`
- Comments starting with `NOTE`
- Comments starting with `BUG`

Additionally, comments with 5 or fewer characters are considered too short to be meaningful and are ignored.

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.
1 change: 1 addition & 0 deletions Philips.CodeAnalysis.Common/DiagnosticId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ public enum DiagnosticId
AvoidToStringOnString = 2146,
AvoidVariableNamedUnderscore = 2147,
AvoidProblematicUsingPatterns = 2149,
UseDisplayNameOrDescription = 2150,
AvoidTodoComments = 2151,
AvoidUnusedToString = 2153,
AvoidUnlicensedPackages = 2155,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information.
// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider reverting, as this is out of scope and in another PR already


using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
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 TestShouldUseDisplayNameAnalyzer : TestMethodDiagnosticAnalyzer
{
private const string Title = @"Test should use DisplayName or Description attribute instead of comments";
public const string MessageFormat = @"Consider using DisplayName parameter for DataRow or Description attribute for test method instead of inline comments";
private const string Description = @"Using DisplayName parameter for DataRow attributes or Description attribute for test methods makes test purpose more visible in test runners and provides better documentation.";
private const string Category = Categories.MsTest;

private static readonly DiagnosticDescriptor Rule = new(DiagnosticId.UseDisplayNameOrDescription.ToId(), Title, MessageFormat, Category, DiagnosticSeverity.Error, isEnabledByDefault: false, description: Description);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

protected override TestMethodImplementation OnInitializeTestMethodAnalyzer(AnalyzerOptions options, Compilation compilation, MsTestAttributeDefinitions definitions)
{
return new TestShouldUseDisplayName(definitions, Helper);
}

public class TestShouldUseDisplayName : TestMethodImplementation
{
public TestShouldUseDisplayName(MsTestAttributeDefinitions definitions, Helper helper) : base(definitions, helper)
{ }

protected override void OnTestMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, bool isDataTestMethod)
{
if (isDataTestMethod)
{
CheckDataTestMethodForDisplayName(context, methodDeclaration);
}
else
{
CheckTestMethodForDescription(context, methodDeclaration);
}
}

private void CheckDataTestMethodForDisplayName(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration)
{
// Check for DataRow attributes with comments but no DisplayName
IEnumerable<AttributeSyntax> dataRowAttributes = methodDeclaration.AttributeLists
.SelectMany(list => list.Attributes)
.Where(attr => Helper.ForAttributes.IsDataRowAttribute(attr, context));

foreach (AttributeSyntax attribute in dataRowAttributes)
{
// Check if this DataRow has a comment but no DisplayName
var hasDisplayName = attribute.ArgumentList?.Arguments.Any(arg =>
arg.NameEquals?.Name.Identifier.ValueText == "DisplayName") ?? false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the ?? false right, that is default dotnet behavior.


if (!hasDisplayName)
{
// Look for trailing comment on the same line
var comment = GetMeaningfulTrailingComment(attribute);
if (!string.IsNullOrWhiteSpace(comment))
{
var diagnostic = Diagnostic.Create(Rule, attribute.GetLocation());
context.ReportDiagnostic(diagnostic);
}
}
}
}

private void CheckTestMethodForDescription(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration)
{
// Check if test method has Description attribute
var hasDescription = Helper.ForAttributes.HasAttribute(methodDeclaration.AttributeLists, context, MsTestFrameworkDefinitions.DescriptionAttribute, out _, out _);

if (!hasDescription)
{
// Look for leading comment before the method
var comment = GetMeaningfulLeadingComment(methodDeclaration);
if (!string.IsNullOrWhiteSpace(comment))
{
var diagnostic = Diagnostic.Create(Rule, methodDeclaration.Identifier.GetLocation());
context.ReportDiagnostic(diagnostic);
}
}
}

private string GetMeaningfulTrailingComment(AttributeSyntax attribute)
{
// Look for the parent AttributeListSyntax to get the closing bracket
if (attribute.Parent is AttributeListSyntax attributeList)
{
SyntaxToken closeBracketToken = attributeList.CloseBracketToken;
SyntaxTrivia trivia = closeBracketToken.TrailingTrivia.FirstOrDefault(t => t.IsKind(SyntaxKind.SingleLineCommentTrivia));

if (trivia.IsKind(SyntaxKind.SingleLineCommentTrivia))
{
return ExtractCommentText(trivia.ToString());
}
}

return string.Empty;
}

private string GetMeaningfulLeadingComment(MethodDeclarationSyntax methodDeclaration)
{
SyntaxTriviaList leadingTrivia = methodDeclaration.GetLeadingTrivia();

// Look for single-line comment immediately before the method
SyntaxTrivia comment = leadingTrivia.LastOrDefault(t => t.IsKind(SyntaxKind.SingleLineCommentTrivia));
if (comment.IsKind(SyntaxKind.SingleLineCommentTrivia))
{
return ExtractCommentText(comment.ToString());
}

return string.Empty;
}

private string ExtractCommentText(string commentTrivia)
{
if (string.IsNullOrWhiteSpace(commentTrivia))
{
return string.Empty;
}

// Remove // and trim whitespace
var text = commentTrivia.Trim();
if (text.StartsWith("//"))
{
text = text.Substring(2).Trim();
}

// Only consider meaningful comments (more than just a few characters)
if (text.Length <= 5)
{
return string.Empty;
}

// Filter out boilerplate comment patterns
var upperText = text.ToUpperInvariant();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add these boilerplate exceptions to the documentation in PH2150.md

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added documentation for the boilerplate comment patterns that are filtered out (TODO, FIXME, HACK, NOTE, BUG) in the Configuration section of PH2150.md. Also documented the 5-character minimum length requirement. Commit cbb1f61.

if (upperText.StartsWith("TODO") ||
upperText.StartsWith("FIXME") ||
upperText.StartsWith("HACK") ||
upperText.StartsWith("NOTE") ||
upperText.StartsWith("BUG"))
{
return string.Empty;
}

return text;
}
}
}
}
Loading
Loading