Adjust NUnit1029 to account for TestCaseSource support for params and optional arguments #958

stevenaw · 2025-11-19T00:55:33Z

Fixes #957

stevenaw · 2025-11-19T01:00:12Z

@manfred-brands @mikkelbu I've made this as a draft already as it's most of the way there, however I have very little experience with the analyzers and am encountering an issue.

There's a failing test, NoWarningWhenNumberOfParametersOfTestWillFitIntoParamsArrayForGenericMethod (which I've added) to account for a params argument of type T in a generic method. There is some validation in place already to check for type assignments to validate NUnit1018 however I don't know if that's relevant for the generic case. My complication is that I'm unsure how to determine if the type of the params array is a generic type in order to opt out of the type checking there. I was hoping I could ask for a hint in the direction to go to work past that

stevenaw · 2025-11-19T02:33:42Z

Ok I think I'm making a bit more headway here. I think I've moved past my issue and I'll continue chipping away at this.

manfred-brands · 2025-11-19T03:15:37Z

Ok I think I'm making a bit more headway here. I think I've moved past my issue and I'll continue chipping away at this.

Just when I was going to have a look, now I can just sit and wait.

No testcasesource warnings for optional or params are present

stevenaw · 2025-11-23T19:27:35Z

I think this is ready for review. I ended up avoiding generic method scenarios since it looks like the existing code also option to avoid explicit type inference + validation for those cases

Copilot

Pull request overview

This PR adjusts the NUnit1029 analyzer to properly handle TestCaseSource usage with params arrays and optional parameters. The analyzer now correctly accounts for methods that can accept a variable number of arguments through params or optional parameters, preventing false positive warnings.

Key Changes:

Modified parameter count validation to include params parameters in the analysis
Restructured the validation logic to allow methods with optional or params parameters to accept TestCaseSource data
Added comprehensive test coverage for params arrays, optional parameters, and generic methods

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
TestCaseSourceUsesStringAnalyzer.cs	Updated parameter counting logic to include params parameters and restructured validation to handle optional/params scenarios
TestCaseSourceUsesStringAnalyzerTests.cs	Added four new test cases covering params arrays, generic methods, and optional parameters

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-23T23:59:06Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));
+        }
+
+        [TestCaseSource(↓nameof(TestData))]
+        public void ShortName(int a = 1, int b = 2)
+        {
+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));


Variable 'x' does not exist in this scope. The parameters are named 'a' and 'b', not 'x'.

Suggested change

Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));

}

[TestCaseSource(↓nameof(TestData))]

public void ShortName(int a = 1, int b = 2)

{

Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));

Assert.That(a, Is.GreaterThanOrEqualTo(0));

Assert.That(b, Is.GreaterThanOrEqualTo(0));

}

[TestCaseSource(↓nameof(TestData))]

public void ShortName(int a = 1, int b = 2)

{

Assert.That(a, Is.GreaterThanOrEqualTo(0));

Assert.That(b, Is.GreaterThanOrEqualTo(0));

Copilot · 2025-11-23T23:59:06Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));
+        }
+
+        [TestCaseSource(↓nameof(TestData))]
+        public void ShortName(int a = 1, int b = 2)
+        {
+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));


Variable 'x' does not exist in this scope. The parameters are named 'a' and 'b', not 'x'.

Suggested change

Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));

}

[TestCaseSource(↓nameof(TestData))]

public void ShortName(int a = 1, int b = 2)

{

Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));

Assert.Pass();

}

[TestCaseSource(↓nameof(TestData))]

public void ShortName(int a = 1, int b = 2)

{

Assert.Pass();

manfred-brands

I had some small remarks on the use of RoslynAssert.
Your example test code didn't get to the analyzer as there were compile errors.

I pushed the necessary fixes up, but the underlying test code doesn't work when run in NUnit 4.4.0.
I'm trying it now on master and see if it is related to the bug you raised

manfred-brands · 2025-11-24T00:03:10Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+        [TestCaseSource(↓nameof(TestData))]
+        public void ShortName(int a, int b = 2)
+        {
+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));


I'm surprised this compiles during testing as there is no x variable.

manfred-brands · 2025-11-24T00:05:48Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+        [TestCaseSource(↓nameof(TestData))]
+        public void ShortName(int a = 1, int b = 2)
+        {
+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));
+        }


Duplicate function with same name and parameters. should also not compile.

manfred-brands · 2025-11-24T00:09:49Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+        }
+    }", additionalUsings: "using System.Collections.Generic;");
+
+            RoslynAssert.NoAnalyzerDiagnostics(analyzer, testCode);


This might explain it.
I thought we always used RoslynAssert.Valid to ensure the code is correct.

manfred-brands · 2025-11-24T00:12:18Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+    [TestFixture]
+    public class NoWarningWhenNumberOfParametersOfTestWillFitIntoOptionalParams
+    {
+        [TestCaseSource(↓nameof(TestData))]


If the code is valid, there should be no ↓ character which otherwise indicates the expect error location.

manfred-brands · 2025-11-24T00:23:05Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+    [TestFixture]
+    public class NoWarningWhenNumberOfParametersOfTestWillFitIntoParamsArrayForGenericMethod
+    {
+        [TestCaseSource(↓nameof(TestData))]
+        public void ShortName<T>(params T[] x)
+        {
+            Assert.That(x.Length, Is.GreaterThanOrEqualTo(0));
+        }
+
+        static IEnumerable<int> TestData()
+        {
+            for (int i = 1; i <= 3; i++)
+            {
+                yield return i;
+            }
+        }
+    }", additionalUsings: "using System.Collections.Generic;");


NUnit itself fails to run this test with Unable to determine type arguments for method.

I always extract the code from the string to see if NUnit will actually run it.

manfred-brands · 2025-11-24T00:29:34Z

src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

+        [TestCaseSource(↓nameof(TestData))]
+        public static void TestLotsOfNonRequiredParams(params int[] z)
+        {
+            Assert.That(z, Is.Not.Default);


Default does not exist in NUnit 3.14. The analyzer test run against both 3.14 and 4.4
Also Default for an array is the same as Null, but a params array should never be null.

stevenaw · 2025-11-24T01:32:12Z

Thanks @manfred-brands ! Appreciate your fixes here. I mentioned that I am new on the analyzer side.

I thought we always used RoslynAssert.Valid to ensure the code is correct.

I hadn't realized this was the case but I can see now why we would want to. I suppose my using NoAnalyzerDiagnostics would also mask when no diagnostics could be raised due to compilation issues 🤦‍♂️
I'll pull your changes and push another quick update. The original inclusion of the ↓ character was copied from an existing test. I'll quickly remove the remaining places the file uses it to avoid future copy/paste issues.

manfred-brands · 2025-11-24T02:01:05Z

I'll quickly remove the remaining places the file uses it to avoid future copy/paste issues.

No. Only the tests that have RoslynAssert.Valid.
The ones that expect a diagnostic use the location of the marker to match line numbers.

Please revert your last commit!

stevenaw · 2025-11-24T02:46:00Z

Done. Sorry about that, thanks for the clarification

stevenaw · 2025-12-07T18:21:45Z

@manfred-brands Thanks for the review here.
I understand we're probably waiting to merge and release this until after the framework side goes out in v4.5 of the framework. While we are waiting is there anything else you'd like to see here? Alternately, were you waiting to approve until after the framework side goes stable?

manfred-brands · 2026-01-10T02:50:08Z

src/nunit.analyzers/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzer.cs

+                                {
+                                    // more than one required parameter is always a mismatch
+                                    // zero parameters of any kind is also a mismatch
+                                    context.ReportDiagnostic(Diagnostic.Create(


I don't see a new test case that validates this.

Thanks @manfred-brands
Do you mean this comment?

// more than one required parameter is always a mismatch
// zero parameters of any kind is also a mismatch

That comment was added by me to clarify the existing logic for myself prior to and after refactoring the logic a bit. I had thought they're already covered through existing tests here:

nunit.analyzers/src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

Lines 658 to 659 in 3028d64

[TestCaseSource(↓nameof(TestData), new object[] { 3 })]

public void ShortName()

nunit.analyzers/src/nunit.analyzers.tests/TestCaseSourceUsage/TestCaseSourceUsesStringAnalyzerTests.cs

Lines 686 to 687 in 3028d64

[TestCaseSource(↓nameof(TestData))]

public void ShortName(int x, int y)

Unless, would you prefer additional cases with 0 and two required args which also include optional args in the signature like this?

[TestCaseSource(↓nameof(TestData))] public void ShortName(int x, int y, int z = 2)

Add a test which passes now but will fail once we make our change

60a55b9

stevenaw force-pushed the testcasesource-params branch 2 times, most recently from a2f6b2d to 4955308 Compare November 23, 2025 19:05

testcase params and optional support

eb23a58

No testcasesource warnings for optional or params are present

stevenaw force-pushed the testcasesource-params branch from 4955308 to eb23a58 Compare November 23, 2025 19:08

stevenaw marked this pull request as ready for review November 23, 2025 19:26

manfred-brands requested a review from Copilot November 23, 2025 23:58

Copilot AI reviewed Nov 23, 2025

View reviewed changes

manfred-brands added 2 commits November 24, 2025 08:18

Review fixes.

8da80c2

NUnit 3.14 doesn't support Is.Default

3028d64

manfred-brands reviewed Nov 24, 2025

View reviewed changes

stevenaw force-pushed the testcasesource-params branch from 23522b6 to 3028d64 Compare November 24, 2025 02:45

manfred-brands reviewed Jan 10, 2026

View reviewed changes

	[TestCaseSource(↓nameof(TestData), new object[] { 3 })]
	public void ShortName()

	[TestCaseSource(↓nameof(TestData))]
	public void ShortName(int x, int y)

Adjust NUnit1029 to account for TestCaseSource support for params and optional arguments #958

Are you sure you want to change the base?

Adjust NUnit1029 to account for TestCaseSource support for params and optional arguments #958

Uh oh!

Conversation

stevenaw commented Nov 19, 2025

Uh oh!

stevenaw commented Nov 19, 2025

Uh oh!

stevenaw commented Nov 19, 2025

Uh oh!

manfred-brands commented Nov 19, 2025

Uh oh!

stevenaw commented Nov 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

manfred-brands left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenaw commented Nov 24, 2025

Uh oh!

manfred-brands commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenaw commented Nov 24, 2025

Uh oh!

stevenaw commented Dec 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

manfred-brands commented Nov 24, 2025 •

edited

Loading