TestCaseSourceAttribute to support optional arguments

[ChrisMaddock]

From #1279. TestCaseAttribute supports a number of 'special-cases' that TestCaseSourceAttribute does not - principally optional arguments and the params keyword.

This is managed within TestCaseAttribute.GetParametersForTestCase(IMethodInfo method). This functionality could probably be shared by both attributes, however, there's some additional complexity in how TestCaseSourceAttribute.GetTestCasesFor(IMethodInfo method) derives the type of it's parameters which will need modification.

[CharliePoole]

Params arguments are deliberately absent from TestCaseSource because it never seemed useful to me. If you are returning args from a method, you simply return the final set of values as an array. The syntactic icing of params isn't really needed.

Optional arguments are missing because they were never implemented.

It may be useful to know that the code for these two attributes (and some others) was once combined in the class that created the tests. The attributes were just "dumb" argument-holders at that time. When we went to NUnit 3.0, I made a tradeoff of having independent attributes at the price of some duplication.

If you want to work on this, I'll be glad to add your name but you should probably come up with a plan for review first, including a few examples of user code that does not currently work but that you want to work. I don't want you to write code that we might turn down for non-code reasons.

[CharliePoole]

Another take on this: I consider TestCase as being a starting point. Peole using TestCaseSource should be more knowledgable and have less need of syntactic niceties.

[ChrisMaddock]

@CharliePoole That's a valid point about params not being so necessary. My main thought behind this is that I was expecting that the two attributes to interpret arguments in the same way. (While implementing optional arguments in TestCase, I expected to just put similar in to TestCaseSource - but didn't realise the special-cases implemented separately, per attribute.)

Can you clarify what you mean by plan? As in, how the code would be structured? I'd like to look at it - although it may be a few days before I can look into it properly.

[CharliePoole]

Strictly speaking, the attributes don't have the same "arguments" Those of TestCase are the actual arguments of the test method. Those of TestCaseSource are info about where to find those arguments. Additionally, TestCase has special considerations because some test arguments can't be specified as constructor argumetns on an attribute. For example, you can't use a decimal so we have special conversion in TestCase.

By "plan" I meant (1) what do you propose that users can do in their test code as a result of the change and (2) where do you propose to share code between the two attributes. The second only comes up because you might not be aware of our intention to keep attributes a bit more independent of one another as compared to other code. It may not turn out to be an issue at all.

[CharliePoole]

Furthermore... and this is just me talking... I like creating duplication before removing it. Preach that to the teams I coach all the time. :-)

What I mean is don't imagine duplication in your head and then remove it. The act of actually making the duplicate code often lets you see better ways to do things. There may not be as much duplication as you had imagined or it may be different than you imagined. If you can actually see the duplication, then you can intelligently remove it. It also helps avoid the notorious developer anti-pattern "Generalizing from a single case." :-)

[ChrisMaddock]

Great, thanks Charlie. I wonder if the best plan is to repurpose this issue as being to implement optional arguments in TestCaseSource - and then see if the functionality required for that allows the other special cases to be dropped in naturally, or not.

[CharliePoole]

I think it's always good if an issue aims at some user-visible feature.

[CharliePoole]

I classified this as an "idea" which we use to indicate that we don't know whether we want to do it. I'm not crazy about it, frankly, since it seems it would lead to ambiguity when the same TestCaseSource was used for different tests. OTOH, I suppose some might consider that a feature. :-)

[ChrisMaddock]

@CharliePoole Thanks, sorry - was keen to look into this, but it has fell down my to-do list recently! Will hopefully revisit in future.

[Stan-RED]

I believe that this is a bug, not idea)) May be not so obvious and users rarely come across, but a serious design issue when you have two pieces of code responsible for the same function. For instance I have this code that is working perfectly:

[TestCase(1)]
[TestCase(2, 2)]
public void Member_Case_Expectation(int a, int b = 1)
{
    Assert.That(a - b, Is.EqualTo(0));
}

I'm gonna to replace it with TestCaseSource because want to use shared data or load it from.. Excel for example.

internal static IEnumerable TestCases
{
    get
    {
        yield return new TestCaseData(1);
        yield return new TestCaseData(2, 2);
    }
}

[TestCaseSource(nameof(TestCases))]
public void Member_Case_Expectation(int a, int b = 1)
{
    Assert.That(a - b, Is.EqualTo(0));
}

And expecting that this behavior should work. Both logically and from documentation: "TestCaseSourceAttribute is used on a parameterized test method to identify the source from which the required arguments will be provided."

But it doesn't. Looks like DRY principle is violated and you have two pieces of code with the same responsibility, but different implementation. Not sure that the label for this should be an "idea"))

[ChrisMaddock]

I agree - and think we roughly decided as such in https://github.com/nunit/nunit/pull/1850. I'd say enhancement rather than bug though - nothing was ever written, to be broken. 😄

@nunit/framework-team - happy to add to backlog?

[CharliePoole]

I categorized this as an "idea" and explained in my comment what we mean by that. Since it doesn't seem to be clear, I'll try again, both for @Yarmonov 's benefit and for anyone else who reads this. I guess I also want to address with the @nunit/framework-team and @nunit/core-team the problem of "ideas" staying as ideas for such a long time.

Pretty much any software change that we actually make is either a bug, an enhancement or a feature. The boundaries are unclear and of little importance. First, however, we have to decide whether we want to accept the issue as something to work on. It's impossible to find a label that makes everyone happy, but we have settled on "idea". So, it's @Yarmonov 's idea that this is a bug, but it might not be - it might instead be "something we don't want to implement."

When we have something that needs discussion among the committers in order to decide what to do, we do two things: (1) we put it into the "Discussion" pipeline, which is only visible if you have ZenHub installed and (2) we label it as "is:idea" so that people without ZenHub will be able to see something.

I characterized this as an idea a year ago. I also gave some opinions about the issue, which may have led @Yarmonov to imagine that "idea" had some negative connotation, which it doesn't. Since then, the only committer who has commented on this is @ChrisMaddock who is also the originator of the issue. To my mind, that's us falling down on the job. Aside from resolving this issue, I think we need to figure out some way whereby things that need discussion really get discussed. Maybe that will entail a single person responsible for making that happen, either per-issue or per-project, or maybe we can come up with another approach.

OK... back to this issue...

@Yarmonov The reason this is not an example of DRY, at least IME, is that we have a principle in the design of NUnit that attributes should be independent. In fact, under NUnit V2, the code to generate arguments for attributes was kept in one place, with lots of if statements. The attributes themselves generally didn't implement anything - they were only markers. This made it much harder for users to add attributes of their own because the NUnit core needed to know the name of every attribute it dealt with. In NUnit 3, I wanted to try the idea of "active" attributes - attributes that actually do something to or with the test.

Now we can and should remove duplication. To the extent that we find duplication between TestCase and TestCaseSource, we need to determine the best way to remove it. I'm a big advocate of removing duplication as a refactoring step, while people who quote DRY usually mean that you should never create it in the first place. It's the difference between someone with an XP orientation and someone with a more top-down approach. Whatever school you are in, I am sure you can accept that the work has to be done in the style of the people who are doing it!

Right now, TestCaseSource doesn't have the feature you want. If we decide to do it, then there may be duplication - actual or potential. But right now, that duplication doesn't exist. As a user, if you can have the feature you want, I assume you could care less about internal design. 😄

More practically...

I think it's a good idea for TestCase and TestCaseSource to have similar features, so users are not confused. I think that's the principle @ChrisMaddock is referring to in reference to #1850. But I have raised an issue above (a year ago) about the potential for ambiguity when the same TestCaseSource is used for multiple methods, perhaps with different signatures. Nobody has replied to that yet.

In addition, I want to see a few more @nunit/core-team or @nunit/framework-team members comment on this either positively or negatively. @ChrisMaddock you may not even have been a committer when you first filed this issue, it's so old. 😄

Let's either reject this issue cleanly or decide to implement it soon! @ChrisMaddock are you willing to remind folks if nobody responds in the next few days?

[Stan-RED]

Sorry, look like my smileys reduced to just brackets, were not a good sign of how seriously I'm focused on the "bug" definition 😄 Of course this is just my idea that it is closer to bug, because:

• It doesn't work as it is stated in documentation.
• It doesn't work as user expected it should work.

I was going to ignore this problem as I think it is a very rare case. But because I'm using NUnit every day I was be happy to invest some time into its quality. Especially because I have an idea that there is a small design flaw.

Just take a look at TestCaseSourceAttribute.GetTestCasesFor method starting from

var parameters = method.GetParameters();
var argsNeeded = parameters.Length;
var argsProvided = args.Length;
…

and TestCaseAttribute.GetParametersForTestCase method starting from

IParameterInfo[] parameters = method.GetParameters();
int argsNeeded = parameters.Length;
int argsProvided = Arguments.Length;
…

I feel that this not DRY. And when in TestCaseSourceAttribute.GetTestCasesFor I replace manual TestCaseParameters generation with call to TestCaseAttribute.GetParametersForTestCase, the tests above works like a charm, because the last method knows about optional arguments with Type.Missing.

My though was that you'd probably like to have a single method that has signature like "TestCaseParameters (object[] args, MethodInfo method)" reusable from both TestCaseAttribute and TestCaseSourceAttribute instead of managing two implementations 😄

If you'd like I'd try to find some time to address this "enhancement".

[CharliePoole]

I don't think it's terribly important whether it's a bug, feature or enhancement. What is important is whether it's something we have agreed to fix or not. I"m working hard here, trying to tell you the kind of input we need in order to make that decision. It has nothing to do with how to implement it - we're programmers too! It has to do with whether it's a good idea.

I suggest we drop discussion of how to implement it - which isn't the problem - and of DRY, etc. - which has nothing to do with whether the feature is good. Let's stick with the info we need to make a decision.

@ChrisMaddock You seem to favor this enhancement without reservation.

I have said I favor it but raised (twice now) a question which nobody has tried to answer: How will this work when multiple tests with different signatures use the same source. That's one of the key differences of why TestCaseSource and TestCase has always needed to work differently. I haven't looked at it in detail but on the surface it seems like a potential problem.

I'm of two minds about whether we need @rprouse to chime in here before moving it to the backlog. I think we need to get serious about the role of project leads but OTOH some things are obvious or at least become obvious in the course of discussion.

[rprouse]

I am in favor of this. I agree we need to step back from the discussion on how we should do this. I don't see much disagreement on whether or not this is a good idea, so let's move this to the backlog and we can discuss implementation when we see some code.

[ChrisMaddock]

I'm very much in favour of this, but I don't think that's ever been in question. ;-) I agree with @rprouse - would be good to see this in code, and see how the approach looks after that. My personal thoughts are that this belongs in a utility class referenced by both attributes, rather than an one attribute referencing the other, as was done in #1850.

@Yarmonov - I've wanted to do this for forever. Would be great if you could do a PR! (providing @CharliePoole is convinced of the feature, of course!)

How will this work when multiple tests with different signatures use the same source. That's one of the key differences of why TestCaseSource and TestCase has always needed to work differently. I haven't looked at it in detail but on the surface it seems like a potential problem.

@CharliePoole - can you run through the problem you envisage? It's been a while since I've looked at this code, but as I remember, a TestCaseParameters is build against a MethodInfo, for each test method. If the parameters are compatible with each MethodInfo, would it matter that the TestCaseSource is used twice? What am I missing? 😄

[Stan-RED]

@ChrisMaddock - you're absolutely right. Each test method will have its own instance of TestCaseParameters and specifically tuned optional params as a result.

[Stan-RED]

My personal thoughts are that this belongs in a utility class referenced by both attributes, rather than an one attribute referencing the other, as was done in #1850.

TestCaseParameters may be a good candidate to be responsible for this.

[CharliePoole]

@ChrisMaddock If we keep the current structure, that's true. However, I was taking @Yarmonov 's suggestion to mean that we would avoid repetition at runtime by computing the values once, and reusing the same set of values for each method. That wouldn't work - and it may not be what he meant.

I agree with you that factoring this into some utility class is a good idea. I don't think TestCaseParameters is a good choice because it's a class designed to hold data and also because we may need some of the same utility methods in generating arguments for fixture constructors and other things in addition to test cases.

If I were doing this (which I'm not) I'd go ahead and duplicate the code and then refactor to remove it.

[Stan-RED]

@CharliePoole No. I was talking about two repeated pieces of code (samples above) those mostly duplicates each other. And just suggested an idea to have some reusable method that accepts (object[] testArgs, MethodInfo testMethod) and returns TestCaseParameters. Without suggestion to use any memoization techniques to return the same instance for different methods.

I don't think TestCaseParameters is a good choice because it's a class designed to hold data and also because we may need some of the same utility methods in generating arguments for fixture constructors and other things in addition to test cases.

It is a good practice to have static factory methods inside data class (int.Parse, string.Format, XDocument.Load).

But you're absolutely right. If this routine will be used somewhere else and will require to return a list of arguments aligned with the test method signature (e.g. Type.Missing added for optional args) instead of TestCaseParameters. Then better to put it somewhere else. Maybe already existing Reflect class.

[Stan-RED]

Probably it's easier to put this responsibility on the shoulders of Reflect.InvokeMethod. It should care about aligning of optional values before execution because this is reflection implementation specific. I have added a PR to show the idea.

[CharliePoole]

As explained in the PR comments, I would see this as separate. A utility class of some kind could include a bunch of methods to transform a provided argument list on request. Adding optional parameters would be one. Params args might be another. A particular attribute would decide which to use.

Making it all happen automatically as a part of invoke seems initially to be easier for the user but makes it less flexible.

[jnm2]

I'd be in favor of this too.

[ChrisMaddock]

See https://github.com/nunit/nunit/issues/2268 for a more recent update on this issue.