"Pretty" Display Name Implementation for Test Cases

[commonsensesoftware]

Summary

This PR contains the implementation for "pretty" test case display names as described in issue #759.

The default behavior for test case display names is unchanged. Developers must still opt into changing the display name settings via the xunit.methodDisplay configuration setting, but now with additional options.

The following new TestMethodDisplay options can be used in conjunction with the existing options to control test case display names:

• ReplaceUnderscoreWithSpace - replaces each '_' with a ' '
• AllowOperatorMonikers - recognizes and replaces supported operator monikers (ex: 'eq' to "=")
• AllowEscapeSequences - recognizes and replaces supported escape sequences (ex: 'X2C' to ',')

Developers can choose which, if any, of these options to turn on. I realize not everyone may agree with using all of these options, but for those that do, I believe it's significantly more productive than re-entering special text via the DisplayName of facts or theories for scenarios that only involve a few characters. The DisplayName continues to trump all of these settings and should be used for complex test case names to prevent the corresponding method names from becoming unintelligible.

Notes

I chose to change the TestMethodDisplay to be a bit vector rather than introduce a new enumeration and configuration option. I feel these new settings are too close in meaning with the existing options to warrant creating completely new configuration options. Since the values are backward-compatible, this seemed more than reasonable.

Changing the TestMethodDisplay to support flags means that it should technically be renamed to indicate this behavior (ex: "TestMethodDisplayOptions"). Since this would introduce a breaking change, I decided against it.

For reasons I cannot explain, the value specified for xunit.methodDisplay will not parse when the new options are specified unless the value is supplied as an integer. For example, the value "Method,AllFormatExtensions" should work, but it doesn't; however, using "30" does. I've gone through all of the parsing code, unit tests, etc. and I can't explain this behavior. Parsing bit vector enumerations from strings this way has been supported since .NET 1.0. The exact cause and solution escapes me. For now, I'm accepting this limitation. If you know why this is happening, I'm happy to make the appropriate update.

I haven't signed the contribution agreement yet, but I've read it, agree with it, and I'm happy to return a signed copy to you once it's provided. I've reviewed all of the contribution guidelines and believe that I have followed them accordingly.

Screenshots

Here are some example screenshots from a demo application using a local build:

[bradwilson]

I don't think reusing the methodDisplay value is actually appropriate because it's not a flags enum (it's one or the other). I think using separate formatting flags makes more sense. I think what I'd want to see is something like:

methodDisplay = choose one of Method, ClassAndMethod, Pretty
prettyMethodFlags = flag enum of pretty print options, including a value All which is the default

[commonsensesoftware]

I can definitely see your viewpoint. I thought about it for a quite a while before deciding to switch the enumeration to support flags. Luck would have it that changing the enumeration to support flags had no change to the existing values or their meaning.

Splitting out the pretty display options makes sense, but it still poses a problem unless we were to add two options because ClassAndMethod | Pretty and Method | Pretty produce two distinctly different combinations. The enumeration could be changed to:

public enum TestMethodDisplay
{
    ClassAndMethod = 1,
    Method,
    PrettyClassAndMethod,
    PrettyMethod
}

The pretty method display options can be then be broken out into a separate enumeration. Selecting one of the new pretty options enables the feature, including the existing support the full class name or just method name.

I also had the goal of trying to avoid introducing a new configuration value. If an xUnit user opts into pretty display options, is it even worth having configuration options? I wanted users to have some level of control on what could be used, but it may honestly be over-configuration. In using the fork I created, my usage pattern has been to always turn on all the pretty display features and then only use the ones I want.

What's your option? Do you think it would be better to just ditch the pretty display enumeration, enable all the features, and users just decide when or if to use particular subfeatures after they opt-in with one of the PrettyXXX TestDisplayMethod options?

[bradwilson]

Hmm, I'd assumed that pretty meant you only used the method name, but if it's the case that pretty applies to both Method and ClassAndMethod, then you don't really need the flag at all. We just set up the transforms as flags (on a separate config value), with the default being None instead of All. That way, whether you're using ClassAndMethod or Method, you enable the name transformations by picking the flags.

Thoughts?

[commonsensesoftware]

I think that's the right behavior. To avoid surprises to users, the current formatting behavior should be the same as it is today until users choose to enable them.

The name Pretty is a misnomer. I didn't have better name when I was originally describing the feature. Since the feature is really just additional display formatting options, I've added a new enumeration with the Options suffix that is a bit vector. I thought this name would have better symmetry with the existing TestMethodDisplay. What do you think?

[Flags]
public enum TestMethodDisplayOptions
{
    None = 0x00,
    ReplaceUnderscoreWithSpace = 0x01,
    UseOperatorMonikers = 0x02,
    UseEscapeSequences = 0x04,
    ReplacePeriodWithComma = 0x08,
    All = ReplaceUnderscoreWithSpace | UseOperatorMonikers |
          UseEscapeSequences | ReplacePeriodWithComma
}

The ReplacePeriodWithComma is a new option from the last iteration to support the period replace in test class names. This option will only be honored when combined with TestMethodDisplay.ClassAndMethod because there is nothing to format with TestMethodDisplay.Method since method names cannot have periods in them.

This will enable a user to write:

namespace given_a_placed_order
{
    public class when_the_order_is_fulfilled
    {
        [Fact]
        public void then_the_ordered_items_should_be_delivered() { }
    }
}

Which will output:

given a placed_order, when the order is fulfilled, then the ordered items should be delivered

In terms of the configuration settings, I've added the following:

<configuration>
 <appSettings>
   <add key="xunit.methodDisplay" value="method" />
   <add key="xunit.methodDisplayOptions" value="all" />
 </appSettings>
</configuration

{
 "methodDisplay": "method",
 "methodDisplayOptions": "all"
}

I am a little concerned about the number of changes caused by introducing a new configuration setting. Effectively, all the places where TestMethodDisplay was used, parsed, or passed around, I needed to update it with the companion support for TestMethodDisplayOptions. I supplied default values to follow the current implementation for TestMethodDisplay, but it still changes the signature of several constructors and methods. Isn't that a breaking change for users that write their own SDK extensions?

I've taken all the points we've discussed thus far, incorporated them, updated the corresponding tests, and verified all the tests still pass. Once you're satisfied with the names, behaviors, and direction, I can send an updated PR.

[bradwilson]

Closing & re-opening for CLA bot.

[dnfclas]

Hi @commonsensesoftware, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@commonsensesoftware, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[commonsensesoftware]

I've updated the PR based on previous feedback. I assume the runner would have to also be updated to take advantage of the new configuration setting. I'm sure you're up to your eyeballs with other stuff. Let me know any additional thoughts you have when you eventually have time to review and reply.

[commonsensesoftware]

Any additional thoughts? I'm sure you've been very busy with far more pressing features and I can only imagine that there has been a lot of pressure to get 2.2.0 out. Let's not forget that you still have day job too! Congrats!

I did eventually figure out why the enumeration only parses correctly using the original approach. The runner doesn't have the updated reference to the utility library. So while the configuration setting exists and can be read, it will only be processed when the value is numeric because the string names aren't present in the version of TestMethodDisplay referenced in the utility library.

That said, I was really hoping to make the cut for 2.2.0 since the direction was to add a configuration setting (TestMethodDisplayOptions) and the runner has to have a reference to the version of the utility library that understands how to read the new setting.

I didn't think there was anything else you were waiting on from me. If there is, please let me know and I'm happy to address those items straight away.

[damianh]

Would be nice to see this land.

[commonsensesoftware]

I thought I had largely followed the checklist:

• Ensured it was an accepted 'help wanted' issue
• Read and followed the project governance
• Signed the CLA agreement
• Followed the existing coding styles
• Wrote unit tests

The original issue never had any traction. Rather than wait indefinitely, I created this PR given how simple and small the code was. That eventually got some traction, but it's still not an accepted feature. On the positive side though, @bradwilson could have flat out said "No" a long time ago. I suspect he's just been too busy with other aspects of the framework and hasn't gotten around to fully evaluating whether he wants this to be formally part of xUnit; I can appreciate that.

In the meantime, I've reluctantly been maintaining a fork which enables this feature. You can find the packages at:

• more.xunit
• more.xunit.runner.visualstudio

The only difference in those packages is the support for the new configuration setting and test case display formatting. If the feature is ever formally added to xUnit, I'll shut down the fork.

@bradwilson, there's been about 1K downloads of this variant package over the past 6 weeks alone (I can send you the NuGet stats if necessary). It seems the community does want it, should you still be up in the air about it. Until then, I'll keep this PR up-to-date in hopes that it will one day be accepted.

[bradwilson]

Sorry, this never raised high enough up on my radar for 2.3. I'll make sure to take another look for 2.4.

[bradwilson]

I had to make a few changes, and this PR pre-dates the Github feature that lets me push directly to your branch, so I did them locally and pushed up the result. Everything has been merged.

Thanks again! This is an excellent job.