[WIP] SetArgDisplayNames for TestCase/FixtureData and templating for TestFixtureData.SetName #2538

jnm2 · 2017-11-07T04:38:20Z

Closes #2536 and also brings TestFixtureData.SetName into consistency with TestCaseSource.SetName.

See the TestNameGeneratorTests for a quick intro to the approach I took. Walking through the commits one by one could help because they are separable.

I also added it to TestFixtureData which made it super easy to make TestFixtureData.SetName consistent with TestCaseData with name generation. Expansions will work. This is something we should discuss and agree on before TestFixtureData.SetName makes its debut in 3.9. /cc @rprouse @CharliePoole

API changes:

namespace NUnit.Framework
{
    public class TestCaseData : NUnit.Framework.Internal.TestCaseParameters
    {
+       public TestCaseData SetArgDisplay(params string[] displayNames);
    }

    public class TestFixtureData : NUnit.Framework.Internal.TestFixtureParameters
    {
+       public TestFixtureData SetArgDisplay(params string[] displayNames);
    }
}
namespace NUnit.Framework.Internal
{
-   public abstract class TestParameters : NUnit.Framework.Interfaces.ITestData, NUnit.Framework.Interfaces.IApplyToTest
+   public abstract class TestParameters : NUnit.Framework.Interfaces.ITestData
    {
-       public void ApplyToTest(Test test)
+       public void ApplyToTest(Test test, TestNameGenerator defaultTestNameGenerator)
    }
}

/fyi @stewart-r

jnm2 · 2017-11-07T04:42:18Z

Hmm, perhaps I could avoid the IApplyToTest changes by making the TestCaseParameters and TestFixtureParameters be responsible for the default name generator instance rather than the fixture builder and test case builder?

CharliePoole · 2017-11-07T05:56:34Z

What does "tell-dont-ask" to apply test names mean?

Also, why drop the IApplyToTest interface, which is used in other places to modify a test once it has been constructed?

jnm2 · 2017-11-07T11:48:12Z

It means to let the test parameters decide how to apply the Name rather than querying TestName and now ArgNames and possibly future properties, then making decisions based on those properties before applying the result to test.Name.

It allowed me to make the ArgNames property protected instead of public and to not duplicate the whole chunk of logic when applying the test fixture name.

I dropped the interface because it was in the way and I can't see any reason for TestParameters to use it. There is no code which needs an IApplyToTest to which we pass a TestParameters.

It might make sense to let TestParameters be in control of the default test name generators though, in which case I could put the interface back.

jnm2 · 2017-11-07T12:18:32Z

I just checked to make sure I wasn't neglecting a cast to object and then to IApplyToTest. This is the only client code of IApplyToTest in the entire framework, and it's all about attributes: https://github.com/nunit/nunit/blob/master/src/NUnitFramework/framework/Internal/Tests/Test.cs#L336-L378

CharliePoole · 2017-11-07T16:29:57Z

That's what I thought it probably meant. I was confused, however, because NUnit already works that way pretty much everywhere. The IApplyToTest interface is implemented by an object (usually an attribute) that wants to be able to change a test after it is created.

If the same object also creates the test, then of course it can either implement the separate interface or just set up the test in the first place with the desired changes. In general, I used the separate interface (see TestAttribute, for example) because I wanted the changes to go in at the same stage of construction as changes made by other attributes. So, these two work the same:

[Test(Description="X")]
[Test][Description("X")]

What about non-attributes. In that case, it can go either way. My feeling was that use of the interface signaled to developers the intention that this is supposed to work the same way as for attributes.

Anyway, thanks for the explanation. I don't like to review code unless I know the intent behind it first. 😊

CharliePoole · 2017-11-07T16:53:08Z

I found a few places to comment on details, but I'm finding a more general problem with this PR, so I'll just make it as a comment.

In addition to what the title of the PR says, you are making several other changes, which appear to me to be orthogonal to what the issue was about. IF you want to entirely change how we go about naming tests, that's valid. But I think you need to create an issue that explains the changes, have it discussed and approved and then implement it. Just putting the code out there forces me to read the code merely to deduce your intention.

Here's what I see in this issue

Implements SetArgNames for both fixtures and cases (as the title says)
Implements template-based naming of fixtures, a much bigger change and one we haven't decided we even want. Personally, I thought we might want it one day, which is why {c} etc. exist, but over time I came to feel it was overkill. The entire template thing initially got pushed because of problems with name duplication in VS. Fixtures don't enter into that at all.
Restructures the underlying implementation rather more than is needed for the first point, but probably needed if we wanted to do templates for fixtures.

As a smaller issue, it looks like you are trying to make both SetName and SetArgNames work when they are both used. My suggestion would be to just tell users they can't do that and either use the last one we find or give an error.

As a matter of simple change control, I wouldn't want to do all this a few days before a release anyway, even if I agreed with the changes. If SetArgNames is something you really need for your own work, can't you implement it simply and in a way that will allow later doing more, if we decide we want more?

CharliePoole · 2017-11-07T16:53:53Z

BTW, this is the sort of thing that makes me wish for a develop branch!

CharliePoole · 2017-11-07T17:03:03Z

Currently, NUnitTestCaseBuilder creates the single default TestNameGenerator but doesn't maintain the default value. It might make sense to move that default value there and eliminate the default constructor of TestNameGenerator.

I'm not sure I understand what you mean by having TestParameters maintain the default instances. That wouldn't make them available to non-parameterized tests, would it?

jnm2 · 2017-11-07T18:16:59Z

None of the changes are orthogonal from the perspective of illustrating how painless and unifying it is to have TestFixtureData.SetName template just like TestCaseData, which I needed to reference in our conversation on #2529. But like I said, waiting for the outcome of that design discussion, it's no problem to peel off that part and leave the templating for SetArgNames.

If SetArgNames is something you really need for your own work, can't you implement it simply and in a way that will allow later doing more, if we decide we want more?

So yes, I very much can. Further, I'm even asking for TestFixtureData.SetName to be pulled if a longer discussion makes you more comfortable because of the issue I have with the status quo. Of course Rob has the final say and is doing the release; if he or any third @nunit/framework-team member think I should let it go, I will. Once SetName goes in and people start using it, we can't allow for doing more later.

The entire template thing initially got pushed because of problems with name duplication in VS. Fixtures don't enter into that at all.

I don't understand this. They control a segment in the test name. If the full test name is duplicated, both VS and NCrunch have problems.

jnm2 · 2017-11-07T18:35:01Z

I'm not sure I understand what you mean by having TestParameters maintain the default instances. That wouldn't make them available to non-parameterized tests, would it?

True. I believe there is a TestParameters that gets created for non-parameterized tests but I'd have to look. Creating a default TestParameters if none exists would tie up a couple loose ends in NUnitTest*Builder. Alternately, I think making static TestNameGenerator.TestCaseDefault and .TestFixtureDefault properties is a perfectly suitable way to handle this too.

CharliePoole · 2017-11-07T18:40:39Z

@jnm2 Answering your last comment first, It merely seemed to me that since `NUnitTestCaseBuilder` is in charge of how tests are built, it should (and does) create a default instance of the `TestNameGenerator`. If we generate names for fixtures, then I'd say that the Fixture builder should be in charge of that one.

…

On Tue, Nov 7, 2017 at 10:35 AM, Joseph Musser ***@***.***> wrote: I'm not sure I understand what you mean by having TestParameters maintain the default instances. That wouldn't make them available to non-parameterized tests, would it? True. I believe there is a TestParameters that gets created for non-parameterized tests but I'd have to look. Creating a default TestParameters if none exists would tie up a couple loose ends in Test*Parameters. Alternately, I think making static DefaultTestNameGenerator instances is a perfectly suitable way to handle this too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2538 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACjfhShux8oQbmorSEviBawEau_EERzcks5s0KLWgaJpZM4QUQQN> .

jnm2 · 2017-11-07T18:41:41Z

And I'm fine leaving it that way, as it is in the PR. 👍

CharliePoole · 2017-11-07T18:43:25Z

Thanks for clarifying your 👍 I was just about to ask what you were plus-one-ing. 😄

CharliePoole · 2017-11-07T19:16:13Z

Issue for this?

CharliePoole

See inline and general comments... I think we need to deal with the big questions first, however.

CharliePoole · 2017-11-07T19:21:00Z

src/NUnitFramework/framework/Internal/TestCaseParameters.cs

    /// a parameterized test case.
    /// </summary>
-    public class TestCaseParameters : TestParameters, ITestCaseData, IApplyToTest
+    public class TestCaseParameters : TestParameters, ITestCaseData


If the ApplyToTest method does the same thing that Attributes implementing the IApplyToTest interface do, then I like the idea that we signal this to the developers, even though an interface is not strictly needed.

This is not possible unless we stop passing the TestNameGenerator to the method which requires either the Test*Parameters classes to know the default test name generator, or for us to go back to the builder asking for all name-related arguments rather than telling the parameters to apply the name (plus duplicating the name application logic between the two builders).

I agree that the way the code is written currently is wrong. It's not how any other attribute or object works. The TestCaseParameters object should decide what to do in it's ApplyToTest method if anything and the method should always be called. But ApplyToTest should not be responsible for the default name. That should just be there before it is ever called.

It's true that doing it that way causes the name to be set twice, but that doesn't seem to have caused any problems up to now.

Let me clarify: I'm not passing the default name generator in order to generate the default name. I'm passing the default name generator in order to use the default name pattern with non-default overridden argument display names. It just happens to set the default name when ArgNames is null. The alternative is to make ArgNames public and the logic external.

Ah! That makes a bit more sense. My assumption was that SetArgNames would be just a wrapper around SetName("{m}(whatever,you,tell,it)").

Can't do that in case an argument contains curlies... there's no escape method =D
To be honest I do think it's cleaner this way, plus {m}{a:40} works.

I do actually believe I did the simplest thing possible. =D Could be wrong, but so far I think so.

CharliePoole · 2017-11-07T19:22:17Z

src/NUnitFramework/framework/Internal/TestParameters.cs

-
        /// <summary>
-        /// Applies ParameterSet values to the test itself.
+        /// Applies the encapsulated parameters to the test method.


TestMethod

👍 This change gets reversed in a later commit.

CharliePoole · 2017-11-07T19:27:43Z

src/NUnitFramework/framework/Internal/TestParameters.cs

-        #endregion
+            if (TestName == null)
+            {
+                test.Name = defaultTestNameGenerator.GetDisplayName(test, OriginalArguments);


I'll have to look elsewhere to understand this but in the original implementation, the test already has a name when it is created. The name is changed by ApplyToTest after the fact. In that way, the method doesn't need to have the default name generator passed to it.

If we do end up deciding to keep this, however, then let's change the name of the method so it doesn't cause confusion with the IApplyToTest method. Objects that implement IApplyToTest are supposed to only work with a small, local context and not have extra information available like whether the user specified a different default on the command line.

We still need the default name generator because we pass the argument display names to it (seen in a later commit). It's either this, or make ArgNames public and keep the logic external- hopefully in a single place and not in each builder.

I'm happy with changing the method name. I almost did so.

CharliePoole · 2017-11-07T19:31:35Z

src/NUnitFramework/framework/Internal/TestParameters.cs

            else
            {
-                test.Name = TestName.Contains("{")
+                test.Name = TestName.IndexOf('{') != -1


I'm not in love with using a less clear code structure for hypothetical efficiency. If there were a String.Contains(char) method, I'd use that. It's quite possible that the internal implementation of Contains deals with the special case of a one-character string, but even if it doesn't, we don't really know that we need efficiency here.

Fair. I actually agree with you rather strongly so there's that. 😆

CharliePoole · 2017-11-07T19:37:27Z

Do you have a particular use case in mind for wanting SetArgNames to interoperate with SetName, rather than making them mutually exclusive?

jnm2 · 2017-11-08T02:55:52Z

Do you have a particular use case in mind for wanting SetArgNames to interoperate with SetName, rather than making them mutually exclusive?

I deliberated over this and first tried the mutually exclusive approach but found it more arbitrary and more complex not to allow the symbiosis. I already knew I wanted to use TestNameGenerator for this to allow for simple implementation (like character limits; the default pattern used to have a limit) and to keep similar things in close proximity to each other for consistency. Once there, the interoperation with SetName was already present. I'm of course open to other ideas. Each decision I made seemed the simplest at the time.

jnm2 · 2017-11-08T03:03:32Z

I removed the IndexOf commit. Did my replies give you all the information you needed? Is this direction acceptable to you or would you like me to try a different approach first?

jnm2 · 2017-11-08T03:27:55Z

I'm actually interested in making TestParameters.ArgNames internal so that it doesn't affect our public API at all. I'd make it private protected if this was C# 7.2 because that's exactly what it should be. We can always loosen this later on request.

ChrisMaddock · 2017-11-08T13:34:59Z

Sorry - I'm struggling for time this week and have had to skim this and the associated threads - but I agree Joseph's concerns about the consistency between setting fixture names and test case names, and agree we should temporarily pull the Fixture.SetName API for 3.9, if we can't get this resolved in time.

jnm2 · 2017-11-08T14:39:45Z

I am going to add the remaining tests and make a couple small changes and ask for reviews again soon.

jnm2 · 2017-12-13T13:31:47Z

Alright, ready now! @nunit/framework-team One more please?

jnm2 · 2017-12-30T21:55:59Z

🕸 This PR got a bit stale and had to be rebased on master.

The walkthrough is still accurate though some API names have slight changes: #2536 (comment)

Anyone willing to do that second review @ChrisMaddock asked for? 🤓

jnm2 · 2018-01-05T14:46:32Z

Ran into another use for this today. 😃

jnm2 · 2018-01-15T19:20:06Z

Hey, quick question for everyone just crossed my mind! Would you expect this to mean "set the argument display names back to the defaults" or "set the argument display names to new string[] { null }"?

testCaseData.SetArgDisplayNames(null);

I lean towards thinking that you'd do testCaseData.ArgDisplayNames = null; in the rare event that you actually wanted to reverse a possible earlier customization. You'd more likely expect testCaseData.SetArgDisplayNames(null); to behave consistently with testCaseData.SetArgDisplayNames(null, null); and with testCaseData.SetArgDisplayNames("");.

This requires a new commit if you agree.

I'm starting to wonder if it would make sense to have a static analysis test that makes sure we spec the null behavior for every params array we accept in the entirety of the framework. This stuff is too easy to miss and risk a NRE (or worse, a misinterpretation).

rprouse · 2018-01-15T19:37:38Z

What use case would there be for resetting the display names at a later time?

jnm2 · 2018-01-15T19:43:48Z

@rprouse I can't think of one, other than an over-engineered multiple-pass process inside a TestCaseSource.

…treated as string-typed

jnm2 · 2018-01-16T02:10:39Z

I went ahead and made sure we interpret .SetArgDisplayNames(null) as string-typed and not as resetting ArgDisplayNames. This is in line with our decisions on other params APIs. Please review!

jnm2 · 2018-01-20T02:41:15Z

@nunit/framework-team, is there anything I can do to make it easier to review this PR?

It's worth it to me to break it into smaller PRs if that means it'll make it in 3.10. What do you think?

CharliePoole · 2018-01-23T01:01:26Z

I can't review it because i disagree with the sub-objective of making naming of fixtures use a template approach like we do for test cases. This highlights one of the problems of mixing multiple objectives in a single PR, even if the implementations are related. If you do three things in one PR, I have to agree with all three of them to approve it.

rprouse

You've bitten off a bit too much in this PR and is outside of the scope of the original issue. Could you back up a bit, tackle just what was proposed in #2536 in a new PR?

rprouse · 2018-01-23T01:23:12Z

src/NUnitFramework/framework/Internal/TestNameGenerator.cs

            {
+                var method = test.Method;
+                if (method == null) return null;
+


Should we return an empty string instead of null? The only usage I see currently is a StringBuilder.Append() which is safe, but it might change in the future.

rprouse · 2018-01-23T01:24:04Z

src/NUnitFramework/framework/Internal/TestNameGenerator.cs

            {
+                var method = test.Method;
+                if (method == null) return null;
+


As above 😄

jnm2 · 2018-01-23T02:21:37Z

You've bitten off a bit too much in this PR and is outside of the scope of the original issue. Could you back up a bit, tackle just what was proposed in #2536 in a new PR?

😄 With pleasure!

I've looked at it a bit. What I do here will look quite different depending on the decision at #2675, so I plan to wait. Either way this PR will be cut way down to scope.

Brar · 2018-06-12T16:24:26Z

Hi folks,
It seems I'm a bit late to the party and I most certainly lost track of the discussion in #1943, #2536 and #2538 so please let me ask you:

Is this still the intended fix for Escaping test method name patterns #1943?
Is there some progress or hope for people who are being bitten by the lack of escaping support?

jnm2 · 2018-06-14T22:05:15Z

@Brar If I remember right, we hesitated to implement escaping because that would change the meaning of folks' existing code. In most cases, SetArgDisplayNames would provide a more semantically useful workaround anyway. I can certainly work on this next if that unblocks you.

Brar · 2018-06-15T04:39:47Z

SetArgDisplayNames would provide a more semantically useful workaround anyway.

I'd be happy with that.

I can certainly work on this next if that unblocks you.

Yes I think it would but what I'm doing is not a high priority project, so there's no need to hurry.

jnm2 · 2018-06-21T23:32:21Z

This PR has a lot of comments and the branch name is outdated. Going to reboot as a new PR.

jnm2 · 2018-07-06T02:35:09Z

PR is up: #2919

jnm2 added 3 commits November 6, 2017 20:50

Tell-don't-ask to apply test name

d725b59

Added TestCaseData.SetArgNames

69863c7

Fixed out-of-date type names in XML docs

d7fd41e

jnm2 changed the title ~~Set arg names~~ TestCaseData.SetArgNames (and TestFixtureData) Nov 7, 2017

jnm2 mentioned this pull request Nov 7, 2017

SetArgDisplayNames for TestCaseData and TestFixtureData #2536

Closed

jnm2 mentioned this pull request Nov 7, 2017

TestFixtureSource attribute inhibits discovery when exploring, but not when running tests #2529

Closed

CharliePoole previously requested changes Nov 7, 2017

View reviewed changes

jnm2 force-pushed the SetArgNames branch from 2b1a2c0 to 220e00a Compare November 8, 2017 03:00

jnm2 changed the title ~~TestCaseData.SetArgNames (and TestFixtureData)~~ [NOMERGE] TestCaseData.SetArgNames (and TestFixtureData) Nov 8, 2017

jnm2 force-pushed the SetArgNames branch from 4406c7a to 1325f57 Compare November 8, 2017 15:01

jnm2 changed the title ~~SetArgDisplay for TestCase/FixtureData and templating for TestFixtureData.SetName~~ SetArgDisplayNames for TestCase/FixtureData and templating for TestFixtureData.SetName Dec 13, 2017

jnm2 force-pushed the SetArgNames branch from 0de099b to 3d15ab8 Compare December 13, 2017 13:51

jnm2 force-pushed the SetArgNames branch from 3d15ab8 to a2a91f7 Compare December 30, 2017 21:50

jnm2 force-pushed the SetArgNames branch from a2a91f7 to 8685f41 Compare December 30, 2017 21:56

rprouse added the awaiting:review label Jan 16, 2018

jnm2 added 2 commits January 15, 2018 21:07

Add failing tests ensuring SetArgDisplayNames null params literal is …

9fbab31

…treated as string-typed

Treat SetArgDisplayNames null params literal as string-typed

7c5a4e9

rprouse reviewed Jan 23, 2018

View reviewed changes

jnm2 mentioned this pull request Jan 23, 2018

Fixture TestName for fixtures: to template or not to template? #2675

Open

jnm2 added awaiting:discussion and removed awaiting:review labels Jan 23, 2018

jnm2 changed the title ~~SetArgDisplayNames for TestCase/FixtureData and templating for TestFixtureData.SetName~~ [WIP] SetArgDisplayNames for TestCase/FixtureData and templating for TestFixtureData.SetName Jan 23, 2018

jnm2 closed this Jun 21, 2018

jnm2 removed the awaiting:discussion label Jun 21, 2018

jnm2 deleted the SetArgNames branch June 22, 2018 00:37

[WIP] SetArgDisplayNames for TestCase/FixtureData and templating for TestFixtureData.SetName #2538

[WIP] SetArgDisplayNames for TestCase/FixtureData and templating for TestFixtureData.SetName #2538

Uh oh!

Conversation

jnm2 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Nov 7, 2017

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

jnm2 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Nov 7, 2017

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

jnm2 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharliePoole commented Nov 7, 2017 via email

Uh oh!

jnm2 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

CharliePoole 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!

jnm2 Nov 8, 2017 • 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!

jnm2 Nov 8, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

CharliePoole commented Nov 7, 2017

Uh oh!

jnm2 commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Nov 7, 2017 •

edited

Loading

jnm2 commented Nov 7, 2017 •

edited

Loading

jnm2 commented Nov 7, 2017 •

edited

Loading

jnm2 commented Nov 7, 2017 •

edited

Loading

jnm2 commented Nov 7, 2017 •

edited

Loading

jnm2 Nov 8, 2017 •

edited

Loading

jnm2 Nov 8, 2017 •

edited

Loading

jnm2 commented Nov 8, 2017 •

edited

Loading

jnm2 commented Nov 8, 2017 •

edited

Loading

jnm2 commented Nov 8, 2017 •

edited

Loading

jnm2 commented Jan 15, 2018 •

edited

Loading

jnm2 commented Jan 15, 2018 •

edited

Loading