Add support for cancellationtoken, params, and optional arguments to TestCaseSource by stevenaw · Pull Request #5057 · nunit/nunit

stevenaw · 2025-11-17T15:50:32Z

Relates to #2268

This PR primarily moves existing functionality in TestCaseAttribute into a common place where it can also be used by TestCaseSourceAttribute.

Copilot

Pull Request Overview

This PR adds support for cancellation tokens, params arrays, and optional arguments to TestCaseSource attributes, bringing it to feature parity with TestCaseAttribute. The implementation extracts common argument adjustment logic into a new AdjustArgumentsForMethod method in TestCaseParameters, which is used by both TestCaseAttribute and TestCaseSourceAttribute.

Refactored argument handling logic from TestCaseAttribute into a reusable AdjustArgumentsForMethod method
Added support for params arrays, optional parameters, and implicit CancellationToken injection in TestCaseSource
Removed .NET 6.0+ preprocessor directives to make generic TestCaseData classes available on all target frameworks
Added comprehensive test coverage for the new functionality

Reviewed Changes

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

Show a summary per file

File	Description
src/NUnitFramework/tests/Attributes/TestCaseSourceTests.cs	Added comprehensive tests for params arrays, optional arguments, and cancellation token support in TestCaseSource; removed .NET 6.0+ conditional compilation for generic TestCaseData tests
src/NUnitFramework/tests/Attributes/TestCaseAttributeTests.cs	Added test case using named parameter syntax to verify params array handling
src/NUnitFramework/framework/TestCaseData.cs	Added new constructors for generic TestCaseData to support 2 and 3 arguments; removed .NET 6.0+ preprocessor directives to enable usage on all frameworks
src/NUnitFramework/framework/Internal/TestCaseParameters.cs	Introduced `AdjustArgumentsForMethod` method that centralizes logic for handling params arrays, optional parameters, CancellationToken injection, and type conversions
src/NUnitFramework/framework/Internal/Extensions/ArgumentsExtensions.cs	Added `LastParameterIsParamsArray` extension method to detect params array parameters
src/NUnitFramework/framework/Attributes/TestCaseSourceAttribute.cs	Updated to use `AdjustArgumentsForMethod` and modified array unpacking logic to support params arrays; added error handling
src/NUnitFramework/framework/Attributes/TestCaseAttribute.cs	Refactored to use the new `AdjustArgumentsForMethod` method, removing duplicated argument handling logic

Comments suppressed due to low confidence (1)

src/NUnitFramework/framework/Attributes/TestCaseSourceAttribute.cs:118

This assignment to methodName is useless, since its value is never read.

            var methodName = method.Name;

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

Copilot · 2025-11-17T16:00:04Z

+                        catch (Exception ex)
+                        {
+                            parms = new TestCaseParameters(ex);
                        }


Generic catch clause.

Correct. We want to catch all exceptions here so that we can report them and associate them with the testcase instead of losing a record of it

This method is also rather horrible. And no tests are verifying that any exception ever is raised

(Again, outside scope of this issue, see comment at the bottom, but we should refactor this one too later)

This error checking was added by me as the method previously had none. That meant that an error with processing a single testcase would cause the entire collection of them to not be discovered or run.

I primarily added it to make it easier to debug my own other bugs while trying to complete this. Agreed this would be good to add coverage over later.

OsirisTerje · 2025-11-17T16:39:13Z

Nice @stevenaw

stevenaw · 2025-11-18T17:19:42Z

Phew. CI is happy now. This is ready for review

Copilot

Pull Request Overview

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

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

OsirisTerje · 2025-11-18T19:44:01Z

@stevenaw

Line 167 is never touched by any of the tests

And I agree that the the condition on lin 166 is always false.

The Roslyn and in my case Resharper should have flagged this as dead code. It doesn't and the reason is probably that the whole method is simply too complex. The dead code warning is not very advanced.

The whole method is rather complex with a bunch of if then else and for loops. (and has probably been like this for a very long time), 107 lines long . That's outside the scope of this issue, but we should do a refactoring of this piece of code.

OsirisTerje

@stevenaw I am actually VERY impressed you have managed to work this out with this code base. A lot of this code is 16 years old (at least), and with way too high cyclomatic complexity, and large methods. To me it is too hard to figure out what the code is actually doing, I am drowning in the details, and then I loose the context.

I mark this just as comments, and not sure if doing anything more here within the scope of the issues you're fixing should be done. I think it is better to mark this code to be refactored and at that time fix the lacking tests too.

Awesome work!

OsirisTerje · 2025-11-18T19:53:32Z

-                    }
-                    parms.Arguments = newArgList;
-                }
+                parms.AdjustArgumentsForMethod(method);


Love you're moving all that code off.

Which I just now understood was moved......

That's a sign that I found the right abstraction 😀 It took 3-4 tries until I found one that seemed right.

You're right, that entire method is just moved. There are improvements to be had, but I tried to minimize changes to it in order to minimize risk of breaking anything.

OsirisTerje · 2025-11-18T19:58:54Z

+        {
+            if (parameters.Length == 0)
+            {
+                return false;


No tests checks this

OsirisTerje · 2025-11-18T20:03:25Z

+        /// <summary>
+        /// Construct a TestCaseData with a list of arguments.
+        /// </summary>
+        public TestCaseData(T argument1, T argument2, T argument3)


This method is not tested

I can quickly add a test over this. This was added for API parity with the pre-existing TestCaseData<T1, T2, T3> since we have a TestCaseData<T> which accepts 1 arg and I was found myself adding support for a TestCaseData<T> which accepts 2 args of the same type.

Co-authored-by: Copilot <[email protected]>

…e.cs Co-authored-by: Copilot <[email protected]>

Co-authored-by: Copilot <[email protected]>

manfred-brands

Nice work @stevenaw

I added a few small clean-up commits.
We had an inconsistency between uses of collection initializers for arrays.
Your new code used them, the old code not.
I have aligned them for consistency and readability.

manfred-brands · 2025-11-19T07:20:34Z

+            {
+                IParameterInfo lastParameter = parameters[argsNeeded - 1];
+
+                if (parameters.LastParameterIsParamsArray())


This re-determines lastParameter.
I have split the method and call it here with lastParameter.

manfred-brands · 2025-11-19T07:24:34Z

        }
+
+        /// <summary>
+        /// Construct a TestCaseData with a list of arguments.


This is also not a list. I update the comments.

manfred-brands · 2025-11-19T07:47:10Z

            return n / d;
        }

+        [Test, TestCaseSource(nameof(SingleDimensionArray))]


We don't need a Test attribute if we have another TestXXX attribute.

stevenaw · 2025-11-19T17:00:10Z

Thanks @manfred-brands !
Appreciate your catching and fixing some of those things

manfred-brands · 2025-11-23T00:49:41Z

@stevenaw This PR would have been a good candidate for squashing to not pollute the repository with commit messages like: "tmp commit".

@OsirisTerje Do we have a rule/guideline for merging vs squash vs rebase?

stevenaw · 2025-11-23T01:10:39Z

Good point. I had thought I had kept much of the early experimentation out of the history but I see there was some still there. Agreed this should've been squashed.

@OsirisTerje I'd also be curious to hear thoughts on if you'd prefer we always or just prefer a given strategy

OsirisTerje · 2025-11-23T20:04:41Z

@manfred-brands @stevenaw No guidelines for branching/merging (afaik).

The @nunit/core-team and the @nunit/framework-team are the only one who can merge a PR, so this only applies to us.

Merging has the benefit of being visible afterwards, since Git will know of the task branch. I find that useful.

However, if there are many commits (like >3) and they are very much small commits with no new real info in their messages (like your experimentation), I would prefer a squash. I myself also often do a lot of small commits, I find that kind of safer.

But, a squash is not directly visible afterwards, so I would say, use it when you should, and not when you should not.

There are however no way for a github branch ruleset to have such a rule, so I would then prefer to have no rules - and that "we" do as well as we can, and try to ensure that we don't merge in a multitude of small commits, but squash instead.

I see in our docs about Team Practices, we have no branch guidelines. We could add some, but not sure if anybody really would read them.

I am not a fan of policing, and strict rules. So common sense, and do as well as one can, is fine with me.

PS:

One thought; We could add a CoPilot instructions.md expressing such a concern, and then say we should try to always use CoPilot as a reviewer, then we could be warned to merge or squash.

PS2:

I didn't mention Rebasing. I rarely do that myself, but anyone who like to do that, feel free.

CharliePoole · 2025-11-23T20:42:07Z

I use rebasing a lot for long-running feature branches but I would not suggest a "rule" about it. If you were documenting guidelines, I'd suggest that as a "Consider" item.

I've occasionally merged a series of commits that included a wip or temp item. That's to be avoided but I don't think it's worth worrying about if it doesn't happen often.

IMO, talking about this kind of thing on a case by case basis, rather than establishing "rules" is good for a team.

stevenaw requested a review from Copilot November 17, 2025 15:50

Copilot started reviewing on behalf of stevenaw November 17, 2025 15:51 View session

Copilot finished reviewing on behalf of stevenaw November 17, 2025 15:58

Copilot AI reviewed Nov 17, 2025

View reviewed changes

stevenaw marked this pull request as ready for review November 18, 2025 17:19

stevenaw requested a review from Copilot November 18, 2025 17:20

Copilot started reviewing on behalf of stevenaw November 18, 2025 17:20 View session

Copilot finished reviewing on behalf of stevenaw November 18, 2025 17:21

Copilot AI reviewed Nov 18, 2025

View reviewed changes

Comment thread src/NUnitFramework/framework/Internal/TestCaseParameters.cs Outdated

Comment thread src/NUnitFramework/framework/Internal/TestCaseParameters.cs

Comment thread src/NUnitFramework/framework/Attributes/TestCaseSourceAttribute.cs

This was referenced Nov 18, 2025

Document support for cancellationtoken, params, and optional arguments with TestCaseSource nunit/docs#1095

Open

Relax NUnit1029 for TestCaseSource where method accepts a single "params" array nunit/nunit.analyzers#957

Closed

OsirisTerje approved these changes Nov 18, 2025

View reviewed changes

stevenaw and others added 15 commits November 19, 2025 14:56

Commit what I have

56dfa99

Only adjust the parameters if it's runnable

9c989e0

It works for the initial tests I've ported

2b1dabc

Improve test coverage for params

bcb8d02

Remove a few things which aren't needed

c98212f

Bring it back to where I was before

171146d

tmp commit

f98cbaf

Everything seems to work

4f4fc0f

Params is working. Now for optional/cancellationtoken tests

c837fb3

Add test cases for optional

8de0425

Add test for cancellationtoken

d8a8a45

Fix the outstanding tests and add a few more

b138f59

Update src/NUnitFramework/framework/Internal/TestCaseParameters.cs

17966cb

Co-authored-by: Copilot <[email protected]>

Update src/NUnitFramework/framework/Internal/TestCaseParameters.cs

c66b53a

Co-authored-by: Copilot <[email protected]>

Update src/NUnitFramework/framework/Internal/TestCaseParameters.cs

31edd35

Co-authored-by: Copilot <[email protected]>

stevenaw and others added 9 commits November 19, 2025 14:56

Update src/NUnitFramework/tests/Attributes/TestCaseSourceTests.cs

66be562

Co-authored-by: Copilot <[email protected]>

Update src/NUnitFramework/framework/Attributes/TestCaseSourceAttribut…

b9b0223

…e.cs Co-authored-by: Copilot <[email protected]>

Remove debugging code.

f2be4bd

Update src/NUnitFramework/framework/Internal/TestCaseParameters.cs

1e3b57b

Co-authored-by: Copilot <[email protected]>

Add some missing test coverage

4c30e86

Fix build

0dfc0e0

Use collection initializers for arrays consistently

e137fc7

Prevent obtaining lastParameter twice

f25acad

We don't need Test when we have TestCase

33b975d

manfred-brands force-pushed the testcase-params branch from 713a964 to 33b975d Compare November 19, 2025 08:13

manfred-brands approved these changes Nov 19, 2025

View reviewed changes

stevenaw merged commit e11ec35 into main Nov 19, 2025
5 checks passed

stevenaw deleted the testcase-params branch November 19, 2025 17:00

stevenaw mentioned this pull request Dec 5, 2025

Add support for generic types for params parameter. #5066

Merged

manfred-brands mentioned this pull request Dec 8, 2025

Fixes Issue #3125 #5079

Merged

Conversation

stevenaw commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

stevenaw Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenaw Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OsirisTerje commented Nov 17, 2025

Uh oh!

stevenaw commented Nov 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OsirisTerje commented Nov 18, 2025

Uh oh!

OsirisTerje left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

stevenaw commented Nov 19, 2025

Uh oh!

Uh oh!

manfred-brands commented Nov 23, 2025

Uh oh!

stevenaw commented Nov 23, 2025

Uh oh!

OsirisTerje commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharliePoole commented Nov 23, 2025

Uh oh!

Reviewers

stevenaw commented Nov 17, 2025 •

edited

Loading

stevenaw Nov 18, 2025 •

edited

Loading

stevenaw Nov 19, 2025 •

edited

Loading

OsirisTerje left a comment •

edited

Loading

OsirisTerje commented Nov 23, 2025 •

edited

Loading