SetArgDisplayNames for TestCaseData and TestFixtureData

[jnm2]

Edit: TestFixtureData.SetName is being made public again after we pulled it from 3.9 now that the behavior is consistent with TestCaseData

TestCaseData.SetName leaves you in an impossible situation explored by #1943 if you are forced to customize parameterized test names and the parameter names contain curlies. To summarize:

The thing that makes me uncomfortable here is that NUnit forces us to use SetName because otherwise the autogenerated test names collide, which in turn forces us to replace curlies with some other character. Feature holes are fine unless the feature is forced on you.

/cc @stewart-r as promised, who said:

Maybe I am guilty of seeing the world through the prism of my own recent experience but I have to say am really surprised that it doesn't seem to affect more people. People test edge cases, any strings with curly braces are edge cases for string manipulation code.... where is the tsunami!?? :-p

Also to say nothing of the escaping problems we ran into with curlies, SetName is just plain verbose. The style just about everyone wants to use, and I agree with them, is "{m}(" + string.Join(", ", customParameterNames) + ")".

We explored different alternatives and this is the only one that addresses all my concerns, @stewart-r's concerns, and @oznetmaster's concerns with the SetName being added to TestFixtureSource (#2529):

namespace NUnit.Framework
{
    public class TestCaseData
    {
+       public TestCaseData SetArgDisplay(params string[] names);
    }
    public class TestFixtureData
    {
+       public TestFixtureData SetName(string name);
+       public TestFixtureData SetName(params string[] names);
    }
}

Usage example for TestFixtureData: .SetArgDisplay("Issue-2464")

NUnit.Framework.Internal.Execution.ParallelExecutionTests(Issue-2464).AllTestsPassed

TestCaseData usage example:

// x = new Person { Name = "Stewart {C} Robertson" }, y = 42
new TestCaseData(x, y, z).SetArgDisplay(x.Name, y)

Resulting test name:

Namespace.FixtureName.MethodName(Stewart {C} Robertson, 42)

SetArgDisplay fills a need that no other solution fills. It's urgent for my real world use cases. If I implement it in time for 3.9, it also allows us to remove TestFixtureData.SetName until we have a proper discussion about expansions and escaping (and whether we'd even want it). Thus I'm marking this design discussion pri:high.

[CharliePoole]

I'd lean to changing the method name to SetArgDisplayNames or something similar so it doesn't give the impression that we are actually changing the arguments of the method. I understand "Names" is intended to say that, but it's not as obvious as I'd like it.

Otherwise, it seems like a good idea. SetName still remains as a more general way to define the name if you don't like the idea of args in parentheses. For example, I have used `SetName("{m}_SomeSuffix") in order to differentiate the case without spelling out args.

[jnm2]

@CharliePoole what do you think about SetArgNames(params string[] displayNames)? That's a little more appealing to me, though SetArgDisplayNames is also good.

[CharliePoole]

I could go that way as well. I wonder if there is a name to use if you don't actually want to put arg names in parentheses, but just something like "Issue-xxxx" or "edge case". In that case you are essentially replacing the display of args. DisplayArgsAs ?

The naming is important but doesn't have to hold you up if you intend to implement this. We can discuss it in the PR.

[jnm2]

It was taking way too long to figure out part of the testing, so I just put the code up for comments: #2538

[jnm2]

@nunit/framework-team I would like to get this in 3.10. I think it's been ready to go in for a while, and I am not seeing outstanding concerns from any of the participants. (If I'm wrong, please clarify so that we can get this work done.)

I updated #2538 to make TestFixtureData.SetName public now that it behaves consistently with TestCaseData.SetName. Would you like me to split the work in my PR into multiple PRs?

[ChrisMaddock]

@jnm2 - I've already opened the wine tonight, but will make sure I review in the next few days! 😄

Any chance you could do a quick few lines on old vs new behaviour, and why the change? I'm wary there's been a lot of discussion on this, and don't want to miss something!

[jnm2]

The change in behavior of TestFixtureData.SetName is that it now does test name templating like TestCaseData rather than setting the verbatim fixture name. It was an integral part of keeping the SetArgDisplay implementation simple, though it's separable should we decide to prevent TestFixtureData.SetName from doing templating.

The commit "Tell-don't-ask to apply test name" moves the test name templating logic into TestParameters so that the builder stops reaching into the parameters and pulling out pieces of data like TestName and ArgNames and deciding how to apply them to the test being built. This allows ArgNames in the next commit to stay protected. (If we allowed C# 7.2, this would have been private protected. 😎) This is all in the same spirit as the existing contents of the ApplyToTest method.

The commit "Added TestCaseData.SetArgNames" demonstrates implementation of the titular feature in combination with the existing templating TestCaseData.SetName.

The commit "Added TestFixtureData.SetArgNames" demonstrates trivially plugging the building of test fixtures into the same tell-don't-ask ApplyToTest and widens TestNameGenerator to work on Test instead of only TestMethod. This lights up both test name templating and respect for the TestParameters.ArgNames property for fixtures. Finally, TestFixtureData.SetArgDisplay is added to set the protected ArgNames property.

[jnm2]

Finally ran into this again while giving it a fresh start. I need some direction. Which should I shoot for?

1. TestName/SetTestName and SetArgDisplayNames cannot be used together

new TestCaseData(...).SetArgDisplayNames("a", "b") { TestName = "{m}{a:50}" }:
throws InvalidOperationException.

new TestFixtureData(...).SetArgDisplayNames("a", "b") { TestName = "{c}{a:50}" }:
throws InvalidOperationException.

➡ No way to do automatic argument length trimming or any name customization besides customizing the argument display names.

---

2. TestName/SetTestName and SetArgDisplayNames can be used together, and we enable name templating with TestFixtureData

new TestCaseData(...).SetArgDisplayNames("a", "b") { TestName = "{m}{a:50}" }:
formats as TestMethod(a, b)

new TestFixtureData(...).SetArgDisplayNames("a", "b") { TestName = "{c}{a:50}" }:
formats as TestFixture(a, b)

➡ We start treating the property TestParameters.TestName consistently, whether it's being used from the derived class TestCaseData or the derived class TestFixtureData.

---

3. TestName/SetTestName and SetArgDisplayNames can be used together, and we do not enable name templating with TestFixtureData

new TestCaseData(...).SetArgDisplayNames("a", "b") { TestName = "{m}{a:50}" }:
formats as TestMethod(a, b)

new TestFixtureData(...).SetArgDisplayNames("a", "b") { TestName = "{c}{a:50}" }:
formats as {c}{a:50}

---

Which do you prefer, or maybe a fourth option?

/cc @nunit/framework-team, @rprouse

[rprouse]

Personally, I prefer option 1 because it is the least complex and easiest to understand. You say that "No way to do automatic argument length trimming or any name customization besides customizing the argument display names." but that doesn't seem like a problem since you are already specifying the arguments. Am I missing something?

The other two options seem more correct, but they increase the complexity dramatically and I expect that different people will expect different things. Personally, I like living with constraints 😄

[jnm2]

Nope, good point! Overriding the runner's automatic trimming is probably rather a feature, anyway!

Thanks for that. I'll start on this as soon as I get some free time. :D

[jnm2]

PR is up: #2919