Refactor PH2159 to target specific use case: single-parameter methods with multiple DataRows

Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com>

Author: Copilot

Documentation/Diagnostics/PH2159.md

@@ -12,56 +12,49 @@
 
 ## Introduction
 
-Test methods with multiple DataRow attributes may benefit from using combinatorial testing instead. Combinatorial testing allows you to test multiple combinations of parameter values more efficiently using the `Combinatorial.MSTest` package.
+Test methods with a single parameter and multiple DataRow attributes can be simplified using CombinatorialValues from the `Combinatorial.MSTest` package. This makes the test more maintainable and concise when you have many values to test for a single parameter.
 
 ## How to solve
 
-Consider refactoring your test method to use combinatorial testing with the `Combinatorial.MSTest` package. This can make your tests more maintainable when you have many parameter combinations.
+Consider refactoring your test method to use CombinatorialValues with the `Combinatorial.MSTest` package. This is particularly beneficial for single-parameter methods with many different test values.
 
 First, install the Combinatorial.MSTest package:
 ```xml
 <PackageReference Include="Combinatorial.MSTest" Version="1.0.11" />
 ```
 
-Then refactor your test to use `CombinatorialValues` attributes.
+Then refactor your test to use `CombinatorialValues` attribute on the parameter.
 
 ## Example
 
 Code that triggers a diagnostic:
 ``` cs
 [TestMethod]
-[DataRow("private static readonly", false)]
-[DataRow("private static", false)]
-[DataRow("private const", false)]
-[DataRow("private", false)]
-[DataRow("public static readonly", true)]
-[DataRow("public const", true)]
-[DataRow("public static", true)]
-[DataRow("public readonly", true)]
-[DataRow("public", true)]
+[DataRow("private")]
+[DataRow("public")]
+[DataRow("internal")]
+[DataRow("protected")]
+[DataRow("static")]
+[DataRow("readonly")]
+[DataRow("const")]
 [TestCategory(TestDefinitions.UnitTests)]
-public void TestMethodWithManyDataRows(string modifiers, bool expected)
+public void TestMethodWithManyDataRows(string modifier)
 {
     // Test implementation
 }
 ```
 
-Potential refactored code using combinatorial testing:
+Refactored code using CombinatorialValues:
 ``` cs
 [TestMethod]
 [TestCategory(TestDefinitions.UnitTests)]
 public void TestMethodWithCombinatorialValues(
-    [CombinatorialValues("private", "public", "internal", "protected")] string visibility,
-    [CombinatorialValues("", "static", "readonly", "const")] string modifier,
-    [CombinatorialValues(true, false)] bool expected)
+    [CombinatorialValues("private", "public", "internal", "protected", "static", "readonly", "const")] string modifier)
 {
-    var modifiers = $"{visibility} {modifier}".Trim();
     // Test implementation
 }
 ```
 
-**Note**: The `Combinatorial.MSTest` package may not work with all parameter combinations and has limitations when mixing combinatorial and non-combinatorial parameters. Evaluate whether the refactoring is appropriate for your specific test case.
-
 ## Configuration
 
 This analyzer is disabled by default. To enable it, add the following to your .editorconfig:
@@ -70,6 +63,9 @@ This analyzer is disabled by default. To enable it, add the following to your .e
 dotnet_diagnostic.PH2159.severity = suggestion
 ```
 
-The analyzer triggers when a test method has 4 or more DataRow attributes.
+The analyzer triggers when:
+- A test method has exactly 1 parameter
+- The method has 4 or more DataRow attributes  
+- All DataRow attributes have exactly 1 argument each
 
 The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply.

Philips.CodeAnalysis.MsTestAnalyzers/PreferCombinatorialTestingAnalyzer.cs

@@ -12,8 +12,8 @@ namespace Philips.CodeAnalysis.MsTestAnalyzers
 	public class PreferCombinatorialTestingAnalyzer : TestMethodDiagnosticAnalyzer
 	{
 		private const string Title = @"Consider using combinatorial testing";
-		public const string MessageFormat = @"Consider using combinatorial testing instead of multiple DataRow attributes. This method has {0} DataRow attributes.";
-		private const string Description = @"Methods with multiple DataRow attributes may benefit from combinatorial testing using the Combinatorial.MSTest package for better maintainability.";
+		public const string MessageFormat = @"Consider using CombinatorialValues instead of {0} DataRow attributes for this single-parameter method.";
+		private const string Description = @"Methods with a single parameter and multiple DataRow attributes can be simplified using CombinatorialValues from the Combinatorial.MSTest package.";
 		private const string Category = Categories.MsTest;
 
 		private static readonly DiagnosticDescriptor Rule = new(DiagnosticId.PreferCombinatorialTestingOverDataRows.ToId(), Title, MessageFormat, Category, DiagnosticSeverity.Info, isEnabledByDefault: false, description: Description);
@@ -36,9 +36,16 @@ public PreferCombinatorialTestingImplementation(Helper helper, MsTestAttributeDe
 
 			protected override void OnTestMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, bool isDataTestMethod)
 			{
-				// Count DataRow attributes
+				// Only analyze methods with exactly 1 parameter
+				if (methodDeclaration.ParameterList.Parameters.Count != 1)
+				{
+					return;
+				}
+
+				// Count DataRow attributes and verify they all have exactly 1 argument
 				SyntaxList<AttributeListSyntax> attributeLists = methodDeclaration.AttributeLists;
 				var dataRowCount = 0;
+				var allDataRowsHaveSingleArgument = true;
 
 				foreach (AttributeListSyntax attributeList in attributeLists)
 				{
@@ -47,12 +54,18 @@ protected override void OnTestMethod(SyntaxNodeAnalysisContext context, MethodDe
 						if (Helper.ForAttributes.IsDataRowAttribute(attribute, context))
 						{
 							dataRowCount++;
+
+							// Check if this DataRow has exactly 1 argument
+							if (attribute.ArgumentList?.Arguments.Count != 1)
+							{
+								allDataRowsHaveSingleArgument = false;
+							}
 						}
 					}
 				}
 
-				// Only suggest if there are enough DataRows
-				if (dataRowCount >= MinimumDataRowsForSuggestion)
+				// Only suggest if there are enough DataRows with single arguments
+				if (dataRowCount >= MinimumDataRowsForSuggestion && allDataRowsHaveSingleArgument)
 				{
 					Location location = methodDeclaration.Identifier.GetLocation();
 					var diagnostic = Diagnostic.Create(Rule, location, dataRowCount);

Philips.CodeAnalysis.Test/MsTest/PreferCombinatorialTestingAnalyzerTest.cs

@@ -17,7 +17,7 @@ public class PreferCombinatorialTestingAnalyzerTest : DiagnosticVerifier
 	{
 		[TestMethod]
 		[TestCategory(TestDefinitions.UnitTests)]
-		public async Task PreferCombinatorialTestingTriggersDiagnosticWithMultipleDataRowsAsync()
+		public async Task PreferCombinatorialTestingTriggersDiagnosticWithSingleParameterMethodAsync()
 		{
 			var testCode = @"
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -26,12 +26,12 @@ public async Task PreferCombinatorialTestingTriggersDiagnosticWithMultipleDataRo
 public class TestClass
 {
 	[TestMethod]
-	[DataRow(""value1"", 1)]
-	[DataRow(""value2"", 2)]
-	[DataRow(""value3"", 3)]
-	[DataRow(""value4"", 4)]
+	[DataRow(""value1"")]
+	[DataRow(""value2"")]
+	[DataRow(""value3"")]
+	[DataRow(""value4"")]
 	[TestCategory(TestDefinitions.UnitTests)]
-	public void TestMethodWithManyDataRows(string text, int number)
+	public void TestMethodWithManyDataRows(string text)
 	{
 		// Test implementation
 	}
@@ -41,7 +41,7 @@ public void TestMethodWithManyDataRows(string text, int number)
 			DiagnosticResult expected = new()
 			{
 				Id = DiagnosticId.PreferCombinatorialTestingOverDataRows.ToId(),
-				Message = new Regex(@"Consider using combinatorial testing instead of multiple DataRow attributes. This method has 4 DataRow attributes."),
+				Message = new Regex(@"Consider using CombinatorialValues instead of 4 DataRow attributes for this single-parameter method."),
 				Severity = DiagnosticSeverity.Info,
 				Locations = new[] { new DiagnosticResultLocation("Test0.cs", 13, 14) }
 			};
@@ -51,7 +51,7 @@ public void TestMethodWithManyDataRows(string text, int number)
 
 		[TestMethod]
 		[TestCategory(TestDefinitions.UnitTests)]
-		public async Task PreferCombinatorialTestingNoDiagnosticWithFewDataRowsAsync()
+		public async Task PreferCombinatorialTestingNoDiagnosticWithMultipleParametersAsync()
 		{
 			var testCode = @"
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -63,8 +63,9 @@ public class TestClass
 	[DataRow(""value1"", 1)]
 	[DataRow(""value2"", 2)]
 	[DataRow(""value3"", 3)]
+	[DataRow(""value4"", 4)]
 	[TestCategory(TestDefinitions.UnitTests)]
-	public void TestMethodWithFewDataRows(string text, int number)
+	public void TestMethodWithMultipleParameters(string text, int number)
 	{
 		// Test implementation
 	}
@@ -98,7 +99,7 @@ public void TestMethodWithoutDataRows()
 
 		[TestMethod]
 		[TestCategory(TestDefinitions.UnitTests)]
-		public async Task PreferCombinatorialTestingTriggersDiagnosticWithManyDataRowsAsync()
+		public async Task PreferCombinatorialTestingTriggersDiagnosticWithManySingleParameterDataRowsAsync()
 		{
 			var testCode = @"
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -107,34 +108,83 @@ public async Task PreferCombinatorialTestingTriggersDiagnosticWithManyDataRowsAs
 public class TestClass
 {
 	[TestMethod]
-	[DataRow(""private static readonly"", false)]
-	[DataRow(""private static"", false)]
-	[DataRow(""private const"", false)]
-	[DataRow(""private"", false)]
-	[DataRow(""public static readonly"", true)]
-	[DataRow(""public const"", true)]
-	[DataRow(""public static"", true)]
-	[DataRow(""public readonly"", true)]
-	[DataRow(""public"", true)]
+	[DataRow(""private"")]
+	[DataRow(""public"")]
+	[DataRow(""internal"")]
+	[DataRow(""protected"")]
+	[DataRow(""static"")]
+	[DataRow(""readonly"")]
+	[DataRow(""const"")]
 	[TestCategory(TestDefinitions.UnitTests)]
-	public void TestMethodWithManyDataRows(string modifiers, bool expected)
+	public void TestMethodWithManyDataRows(string modifier)
 	{
-		// Test implementation similar to HelperTest example
+		// Test implementation 
 	}
 }
 ";
 
 			DiagnosticResult expected = new()
 			{
 				Id = DiagnosticId.PreferCombinatorialTestingOverDataRows.ToId(),
-				Message = new Regex(@"Consider using combinatorial testing instead of multiple DataRow attributes. This method has 9 DataRow attributes."),
+				Message = new Regex(@"Consider using CombinatorialValues instead of 7 DataRow attributes for this single-parameter method."),
 				Severity = DiagnosticSeverity.Info,
-				Locations = new[] { new DiagnosticResultLocation("Test0.cs", 18, 14) }
+				Locations = new[] { new DiagnosticResultLocation("Test0.cs", 16, 14) }
 			};
 
 			await VerifyDiagnostic(testCode, expected).ConfigureAwait(false);
 		}
 
+		[TestMethod]
+		[TestCategory(TestDefinitions.UnitTests)]
+		public async Task PreferCombinatorialTestingNoDiagnosticWithFewSingleParameterDataRowsAsync()
+		{
+			var testCode = @"
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+[TestClass]
+public class TestClass
+{
+	[TestMethod]
+	[DataRow(""value1"")]
+	[DataRow(""value2"")]
+	[DataRow(""value3"")]
+	[TestCategory(TestDefinitions.UnitTests)]
+	public void TestMethodWithFewDataRows(string text)
+	{
+		// Test implementation
+	}
+}
+";
+
+			await VerifySuccessfulCompilation(testCode).ConfigureAwait(false);
+		}
+
+		[TestMethod]
+		[TestCategory(TestDefinitions.UnitTests)]
+		public async Task PreferCombinatorialTestingNoDiagnosticWithDataRowsHavingMultipleArgumentsAsync()
+		{
+			var testCode = @"
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+[TestClass]
+public class TestClass
+{
+	[TestMethod]
+	[DataRow(""value1"", 1)] // These DataRows have multiple arguments
+	[DataRow(""value2"", 2)] // but method has single parameter  
+	[DataRow(""value3"", 3)]
+	[DataRow(""value4"", 4)]
+	[TestCategory(TestDefinitions.UnitTests)]
+	public void TestMethodWithMismatchedDataRows(string text)
+	{
+		// Test implementation
+	}
+}
+";
+
+			await VerifySuccessfulCompilation(testCode).ConfigureAwait(false);
+		}
+
 		protected override DiagnosticAnalyzer GetDiagnosticAnalyzer()
 		{
 			return new PreferCombinatorialTestingAnalyzer();