This approach takes control away from the attribute, and gives it back to internal nunit processing. What's mirror does it in a way that affects any method called by nunit through Reflect, whether it's a test method or not.

It also implies that all new data attributes, including those written by users, will have the same logic applied and breaks Reflect in the sense that it no longer uses exactly the arguments given to it.

As discussed, we need a utility class that transforms arguments in various ways. However, the policy decisions about which transforms to use has to rest with the attribute itself. It should be possible to write a data attribute that doesn't allow optional arts, for example.

FYI this is similar to the NUnit V2 approach we have moved away from.

I should add.. thanks for taking this on. I think you have the right idea but need to massage it a bit to be consistent with our design goals. Separation of policy (deciding what to so) from implementation is part of that.

I'm not sure what exact problem you’re focusing on so have decomposed into separate concerns.

1. InvokeMethod should be more flexible and have a setting to configure if alignment should be applied or not?

I don’t see a big reason why InvokeMethod should work differently compared to how methods are invoked in real word (you won't receive TargetParameterCountException when calling method with optional argument missing). But I think that such additional flexibility may be useful and have updated the idea in code (separate alignment method, used by default in invocation and easy way to change the default behavior).

2. Reflect class is not a good place for this and attributes should be responsible for this?

Obviously that this is a reflection specific details, because in natural way we don't call someMethod(1, Type.Missing) we do it as someMethod(1). And only because method is executed in denatured way (through reflection) the call had to be adjusted with Type.Missing values instead of non-existing args.

Let's back up and take a broader view of how test case arguments come to be and how they are used.

Data attributes provide arguments in an instance derived from the TestParameters class. That class has an Arguments property and an OriginalArguments property. The Arguments property starts out equal to OriginalArguments but gets transformed in various ways, depending on the attribute. For example, TestCaseAttribute knows to transform a double or string to decimal when the test parameter requires a decimal - which cannot be specified as an Attribute argument.

The two attributes are used as follows:

1. OriginalArguments is used as part of the name of the test.
2. Arguments is used as the corresponding Arguments property of the test, consequently...
   2.1 It is used to call the method
   2.2 It ends up in the XML output
   2.3 It is available to user test code through TestContext (not sure if this merged yet)

Putting the transformation of arguments in Reflect.Invoke satisfies 2.1, but not 2.2 or 2.3. For that to happen, TestParameters.Arguments has to be changed.

There may be some changes made to the arguments that don't need to be reflected in the XML or through TestContext. Such changes could be placed directly in the Reflect.Invoke method. I don't think this is one of them, however.

Your original idea was to put this code in TestCaseParameters. We may want to think about putting it in TestParameters, the base class, but not as something that happens automatically. Attribute that transforms the arguments is necessarily using the parameters object already. Rather than calculating the change directly and setting TestParameters.Arguments directly, it could just tell the object to do it. How does that sound?

Thank you for the detailed explanation. I think that the only slight disagreement is here:

There may be some changes made to the arguments that don't need to be reflected in the XML or through TestContext. Such changes could be placed directly in the Reflect.Invoke method. I don't think this is one of them, however.

Maybe I'm wrong, but right now feel a bit more arguments that this is an internal specifics of the Reflection's invocation process. Just imagine such virtual conversation between user and NUnit:

Me: Call the TestMethod with two arguments (1,2).
NUnit: No problem. It was called with 4 arguments (1,2,Type.Missing,Type.Missing).
Me: Why? I never called this method in a such way? If I write TestMethod(1,2,Type.Missing,Type.Missing) I get my default values overwritten and a wrong result.
NUnit: Sorry, this is an internal behavior of MethodInfo.Invoke that you should know about 😄

Note the big difference between this situation and arguments conversion. After conversion you receive the proper list of arguments that meets the method signature. E.g. for

[TestCase(1, 2)]
public void TestMethod(byte a, byte b) => Assert.That(a + b, Is.EqualTo(3));

You get the list of real arguments with matching types used to call this method. But for this method:

[TestCase(1)]
public void TestMethod(byte a, byte b = 2) => Assert.That(a + b, Is.EqualTo(3));

You'll get arguments (1, Type.Missing). And, of course, the call TestMethod(1, Type.Missing) wouldn't be even compiled. In case of "object" type there will be even worse behavior, because it overwrites the default value. So there is a good question, what arguments should be in XML output and TestContext info:

1. Used to call method in a natural way. I.e. arguments that we can put in the call "TestMethod(...here...)".
2. Used to call via reflection with all current and future workarounds and also understanding that some of such arguments can't be put inside a natural call.

Probably it becomes a little better if replace Type.Missing with default value of missing argument. At least, from the point of reporting.

Looking at how the TestCaseAttribute implements optional parameters, I see your point clearly. The use of Type.Missing satisfies 2.1 above, but not 2.2 or 2.3. And it makes no sense to have these arguments visible in either the XML or through the TestContext.

@ChrisMaddock Is there a particular reason you did it that way rather using ParameterInfo.DefaultValue?

Maybe I'm asking for too much, but what I would really like to see in @Yarmonov 's example is (1, 2) for both cases!!!

I"m embarrassed to say that I thought we already did that, which has probably made this discussion a little less coherent than it should have been!

@ChrisMaddock Is there a particular reason you did it that way rather using ParameterInfo.DefaultValue?

Not off the top of my head, no! 😄 I've never considered 2.2 - and 2.3 didn't exist at the time I wrote it. 2.3 alone probably means that the attribute should be responsible for filling in ParameterInfo.DefaultValue - so TestContext.Arguments will always be all the parameters the method actually took. (Whether they have been passed in, or take the default values.)

So, from all this, I guess my conclusion is

• Move the method supplying optional parameters to TestParameters
• Make it an instance method so it can take from and output to the arguments property
• Ensure that any error messages are generic enough - i.e. don't mention a particular attribute.
• Call it from TestCase and TestCaseSource

As further steps, in subsequent issues we could

• Add other methods that do other things... e.g. params arguments
• After we have a few different operations, add a master method that does them all by default or uses a Flags enum to determine which ones to do.

Does this approach seem sensible to both of you?

I'm not sure where you meant by TestArguments (TestCaseParameters, perhaps?) - but other than that, yes, sounds like a great plan!

I'd also like to see 'further steps' include all similar attributes such as TestFixtureAttribute/TestFixtureSourceAttribute, which also have similar code. Maybe that was implied!

@ChrisMaddock I meant TestParameters, which is the base class of both TestCaseParameters and TestFixtureParameters. The implication was we should fix those other attributes you mention as well, although if it's done by a contributor, I'd prefer to get it a bit at a time so we can check it as it goes.

1. Finish this PR.
2. Create some issues for expanding this to other attributes
3. Create some issues for expanding it to other "transformations" of the attributes

@Yarmonov are you moving ahead with this based on the recommended changes?

@CharliePoole not yet, have a short vacation right now. But if you're not in hurry I'll try to deal with it during the next week.

@Yarmonov I don't think there's a hurry on this one... just wanted to be sure you were still on it after our various comments. 😄

@CharliePoole Yes, I hate unfinished things 😄

Hey @Yarmonov - is this something you're interested in coming back to?

The issue came up again recently - I raised #2268 to cover progress so far.

Hey @ChrisMaddock, I've commented it in #2268.

Things are not 100% completed yet. Right now working on TestFixture/TestFixtureSource and also going to improve tests. Will try to continue one the week after the next one.

Great, thanks for the update, @StanEgo.

When this is ready for review, will you let us know, and also write a little overview of the structure your going for? There's a lot of change here, and I think it would help review if we had your idea of what you're working towards, rather than working it out from the changes! I'm hopeful this change will make this bit of the code much more maintainable. 😄

Also, could you give a rough outline of any known changes to functionality? From a quick skim, this is going to introduce some new parameter-parsing options to some attributes (which is good, in that it will standardise them) - just to clarify, there should be no breaking changes from this?

When this is ready for review, will you let us know, and also write a little overview of the structure your going for?

Sure. I intentionally split all changes into specific commits, with some portion of updated focused on some area.

Sorry to pick up on the little things

Feel free to pick up such things. I like the consistency of code style and have read the developer's FAQ. Just have many projects on the table with different conventions, so sometimes mix them 😃

Also, could you give a rough outline of any known changes to functionality?

I tried to avoid breaking changes so almost all existing tests stay unchanged. There is some room for discussion, improvements and unlocking some new benefits, but right now for compatibility purposes I even left some things that I'm not completely agreed with 😄

Sounds great, thanks. 😄

Yes - in general NUnit is pretty serious about backwards-compatibility. Ideally, this change would have no breaking changes - we'll see what it looks like!

@jnm2, thank you. Unfortunately I'm having business trip right now and strongly overloaded. Will be back in the early beginning of September and will definitely answer all your questions. And hopefully will finish the last tricky thing with TestFixture migration to complete this PR.

[edited to fix mention]
@StanEgo I sure know what that's like! There's no rush here. Feel free take your time!

Sorry, it's slightly delayed again, but just for the purpose of another PR #2439 😃

@StanEgo - @jnm2 raised the point that this PR is getting quite large - from our point of view, this can make things difficult to review to ensure no unexpected changes slip through.

Once you're happy with where you've got to, do you think there's any way you might be able to chunk the work here up into multiple PRs, to review as logical changsets? e.g. standardise one attribute at a time or something? That could be really helpful for us

@StanEgo - @jnm2 raised the point over in #2638 that this PR is getting pretty large. From our point of view, this can make things very difficult to review, and ensure no unintended changes are slipping through.

Once you're happy with where you're at on this one, do you think you might be able to chunk the work up here into multiple PRs, each changing one logical area? I'm not sure what will work best - but perhaps attribute by attribute, so you could explain the changes required in each case?

@ChrisMaddock this is a good idea. I've intentionally did this PR as a group of relatively independent commits, so it will be easy to implement sub-PRs. Especially it will be good because the next commits I'm preparing right now, related to TestFixture/TestFixtureSource, are really huge due to design issues in reflection wrappers. So I also afraid of how you'll review them.

May be use #2268 as a kind of roadmap to discuss all PR's involved?

Closing because nobody has touched this PR in so long.