NUnit3: change the way test name is created so that it doesn't change between discovery and execution

[frblondin]

There are some problems when using the test explorer for running tests that use the AutoData / InlineAutoData attributes. When the parameter values are converted as string for generating the test name, the name contains the value that will change each time AutoFixture is requested to produce a value. In Visual Studio there are two distinct steps: discovery and execution. Because of the test name that changes constantly the test framework will fail at running/debuggin the test. Here are the steps to reproduce:

1. Create an empty C# project and add the Autofixture.NUnit3 and NUnit3TestAdapter packages
2. Declare one simple method: [Test, AutoData] public void SomeMethod(string unfixedArgumentProducedByTheGreatAutofixture) { }
3. Go to the Test Explorer, it displays one test SomeMethod("<some value that constantly change from one discovery / execution to another>")
4. Try to Run/Debug -> doesn't work

This can be reproduced with any primitive type, string, or types/structs whose ToString() method shows the property values that have been created by AutoFixture (Version, Guid, StringBuider...).

Even if there was no such error, I don't think there is real value to display values that constantly change. The only benefit would be if a test randomly fails because of a particular value but that's very unlikely in general. This PR make it so that:

• AutoData: the test name and full test name will display respectively SomeMethod(auto) and NameSpace.SomeMethod(auto)
• InlineAutoData: the test name and full test name will display respectively SomeMethod("fixed value 1", "fixed value 2"..., auto, auto) and NameSpace.SomeMethod("fixed value 1", "fixed value 2"..., auto, auto)

[frblondin]

FYI I created a temporary NuGet package because I needed to share this.

Any chance you can have a look at this PR?

[moodmosaic]

As added to the README

AutoFixture is getting a new governance model. This most likely means that you can expect delays if you have questions, issues, or pull requests.

[zvirja]

@frblondin Could you clarify whether you are still interested and have time to work on this PR? 😉 That is to understand whether the review makes sense..

[frblondin]

@zvirja Sure I am, please let me know what I need to do so that this PR gets merged 😄

[zvirja]

@frblondin Finally I had a chance to review the suggested changes and think a bit about that. As far as I understand this issue is specific to VS runner only and other runners (like console, R#) works well. We also have a similar situation for xUnit and the priority seems to be the same as here.

I've checked your PR and have a few concerns regarding the implementation:

• It's not very good idea to enable this by default for all the consumers. There might be people who are fine how it works now (e.g. they don't use VS runner) and they don't need this feature. Also this feature makes it hard to later troubleshoot the issue as you don't see the values that failed test. On other hand, I don't see great ways how to make it configurable - each approaches has some drawbacks.
• The code which manually concatenates the test name scares me. It's very low-level and very specific, it's hard to be sure that we covered all the cases. If later NUnit3 will fix some bug or change the rendering approach, we will produce names that will differ from the standard ones. Ideally, we should be able to use some NUnit API to format test name, however change volatile argument values to static ones. If current NUnit doesn't expose any of such APIs, we should discuss that at NUnit side and implement such an API.

What are your thoughts regarding that?

[frblondin]

• About not displaying values and preventing from quickly know the values causing the error it's a very valid point. As you pointed out, there is no good alternative I can think of. In any case from my experience this is a rare case and the logging tools or good assertion messages seems acceptable to me.
• Test names are normally created by NUnit in NUnitTestCaseBuilder using the TestNameGenerator. As you said there are raw concatenations but I followed what used to be done in the existing implementation of AutoFixture.Nunit. I agree with you, we should change the implementation so that it uses it. I will do the changes and push a new commit.

What do you think?

[zvirja]

As you pointed out, there is no good alternative I can think of. In any case from my experience this is a rare case and the logging tools or good assertion messages seems acceptable to me.

My point is that we shouldn't change the existing behavior. The new behavior should be disabled by default and only those ones who wants it might enable it. I'm thinking about property injection to AutoDataAttribute and InlineAutoDataAttribute with default strategy that a doesn't modify the display name. However, it might happen that there is a better way..

Later, if you wish to have a static test names, you could create your own AutoData attributes and enable the appropriate strategy.

Only after some time if we see that a lot of people come to us and ask to make it default, we could probably make it enabled out of the box.

I agree with you, we should change the implementation so that it uses it. I will do the changes and push a new commit.

I don't know NUnit code base, so, in fact, I don't imagine how it will look like 😟 If you believe that it will address the concerns I added - feel free to push those changes so we can see them 😉

[frblondin]

For the activation of the "static" test names, I cannot think of an easy way to switch to this mode:

• Additional attribute to be applied on methods: really heavy solution. If you want to switch to this mode I don't see why you'd want it to be applied for some tests but not for others...
• Additional entry in the assembly app.config file: same as above, not really convenient

Since these tests can only work if static names are used in VS I think we could change them without having to use an option, only in the context of VS. It may sound like hacky but we a simple way to detect which names should be generated can be checking the process name hosting the running code. Something like System.Diagnostics.Process.GetCurrentProcess().MainModule.FileName.StartsWith("vstest", StringComparison.OrdinalIgnoreCase).

Crazy?

[zvirja]

Crazy?

Yep! 😉 I think that it will be very tricky approach and it might be non-stable. It's better to have consistency, so it works the same in all the runners.

Additional attribute to be applied on methods: really heavy solution.

It seems that you misunderstood my idea with property injection and strategy. Let me show how it looks:

public class AutoDataAttribute
{
    public ITestNameStrategy NameStrategy { get; set; } = new DefaultNameStrategy();
    ....
}

later you can create your own data attribute in solution:

public class AutoDataStableNameAttribute : AutoDataAttribute
{
    public AutoDataStableNameAttribute()
    {
        this.NameStrategy = new StableNameStrategy();
    }
}

and later you simply use your own AutoDataStableNameAttribute everywhere instead of default AutoData:

[Test, AutoDataStableName]
public void SomeTest(...) { ... }

This approach is not so bad, as:

• It's explicit and you have total control over the look.
• Usually you use AF together with AutoMoq or AutoNSubstitute libraries, so you already have your own attribute.

This way allows users to opt-in only if they want and out of the box the name will not be changed.
Later we can review the approach further and probably use some other default strategy (e.g. controlled by AppContext switch).

[zvirja]

I've recently learned this question deeply for xUnit and found that actually it supports auto-data out of the box. It expects that theory data might be dynamic and adds a special support for that.

What we are doing here for NUnit looks more like a workaround, rather than a real solution. We are not the guy who is doing something wrong. Rather, the issue is with NUnit that doesn't have support for that.

Probably, we should go one step ahead and raise the discussion on their side, so they support this feature and we simply consume it.

[frblondin]

I think I explorer this path already but I'll to it once more: NUnit provides the ability to generate values that change over time - see Random as an example. They implemented a way so that the value can be passed between the vstest discovery process to the vstest runner.

There is one big difference though: supported value types for RandomAttribute are simple serializable values (see the source file, only integers, doubles, longs...). Other than for simple types (scalar types, strings, int...) it is impossible to create test values during discovery and pass them to the runner. These are two different processes.

That could mean that for simple types we could try to reuse the same behavior as for the NUnit RandomAttribute but for more complex type we should display autodata... but you are reluctant to do so. For most cases the test name for a complex type parameter will always produces a non-meaningfull value - NameSpace.Type<SomeOtherNameSpace.OtherType> as an example. This is why I'm still not getting why changing the behavior is not acceptable.

Before considering how to implement it, what do we want to do?

[zvirja]

Notice, as I mentioned before, I'm with xUnit background and personally I don't use NUnit. Therefore, I apologize if there are some areas in which I have wrong vision 😟

I've just played more and got some vision how NUnit decorate names. I used the following code for testing:

public class Data
{
    public int IntValue { get; set; }
}

public class DataWithToString
{
    public int IntValue { get; set; }
    public override string ToString() => $"MyData: {IntValue}";
}

public interface ISomeData
{
    int IntProp { get; }
}

public class AutoNData : AutoDataAttribute
{
    public AutoNData():base(new Fixture().Customize(new AutoConfiguredNSubstituteCustomization()))
    {
    }
}

public class TestNameTester
{
    [Test, AutoNData]
    public void Sample(int a, Data d1, DataWithToString d2, ISomeData d3)
    {
        Assert.IsTrue(true);
    }
}

and this is the result I got:

NUnit3RunnerTest709.TestNameTester.Sample(105,NUnit3RunnerTest709.Data,MyData: 236,Castle.Proxies.ISomeDataProxy)

As we can see from this (and here) potentially any argument (primitive or complex) could have dynamic name.

Our goal (if I see that clearly) is to ensure that we have a stable test name. From this perspective it doesn't make any sense to distinguish between primitive and non-primitive types as both kinds could lead to variable test name (see a sample above).

That could mean that for simple types we could try to reuse the same behavior as for the NUnit RandomAttribute but for more complex type we should display autodata... but you are reluctant to do so. For most cases the test name for a complex type parameter will always produces a non-meaningfull value - NameSpace.Type<SomeOtherNameSpace.OtherType> as an example.

I think that we are a bit unaligned, so let me explain that for one more time. Our library is quite popular and we have a lot of people using it to test their enterprise or opensource code. We don't know about all the scenarios, therefore it's unsafe to make decision that could affect them. Most of those people might use different runners (e.g. R#, Console) and are not affected by this issue. If you change test name, this will make them harder to troubleshoot if they sporadically failed on CI. Consider the sample I used above:
Currently:

NUnit3RunnerTest709.TestNameTester.Sample(105,NUnit3RunnerTest709.Data,MyData: 236,Castle.Proxies.ISomeDataProxy)

After:

NUnit3RunnerTest709.TestNameTester.Sample(auto<System.Int32>,auto<NUnit3RunnerTest709.Data>,auto<NUnit3RunnerTest709.Data>,auto<Castle.Proxies.ISomeDataProxy>)

If test sporadically failed on CI, currently you can troubleshoot that likely either 105 or 236 caused the issue. After we accept your fix and enable it, users will not have that information.

I do understand that we need this change to fix VS runner, but that should be something you opt-in, something that you must explicitly enable understanding the trade-off and benefits. That's why I suggest to implement this feature, however keep it in the disabled state by default. It will be very easy to enable it in your project if you need it, however you will not affect people that prefer diagnostics information over working VS runner they don't use.

---

As further steps I suggest the following:

1. Rework current approach, so that we don't decorate string manually, but rather use NUnit API for that (e.g. new TestNameGenerator("{m}{a:40}").GetDisplayName(test, ....)).
2. Rework implementation so that feature is disabled by default and you need to enable it manually. I'd prefer the Strategy pattern I already described here. Don't use some static switchers for now (like static variable, AppConfig, etc), we could introduce them later if strongly needed.
3. Don't distinguish between primitive and non-primitive types as both them could produce dynamic names. It will be complex and benefit is little.
4. Register an issue (or find an existing) for NUnit to support tests with dynamic data out of the box, so that we don't need to invent these kludges to simply work.

Hope that makes sense. If not - describe your concerns.

[zvirja]

It appeared that not only VS runner becomes crazy - NCrunch is affected as well. Therefore, we definitely should provide a way for users to support those runnres.

@frblondin Looking forward to your follow up so we can proceed! 😉

[frblondin]

Thanks for this deep analysis. I did some more tests and things are getting more and more confusing (for xUnit)... but I think I have an acceptable plan in the end for NUnit.

I did some more tests in the attached solution:

• I wanted to see how it works with xUnit... VS 2017 15.3.2 + xUnit + AutoFixture seems to be also failing. Use this simple procedure: 1/ Open VS, 2/ Add breakpoint for each unit test in XUnit.cs, 3/ Go to VS Test Explorer, 4/ Click on debug -> the tests decorated with AutoDataAttribute or InlineAutoDataAttribute fails at being debugged. May be I missed something and this is not the primary topic for this PR - but I thought you might be interested. Also I'm a bit confused why this would not be worrying anyone since VS is used by 99% of .Net developers and xUnit is a mainstream unit test framework.
• I created a simple CustomDataAttribute attribute that mimics the AutoDataAttribute attribute from AutoFixture.NUnit. It turns out that what is the identifier for a test case between the test discovery and the test execution is the FullName property, not the Name property. FullName gets usually automatically computed from the Name. In my CustomDataAttribute I only modified the FullName and things are working.
  Of course I don't know of all test execution platforms (dev platforms, CI tools, coverage...) but it seems acceptable to only change the fullname, given that the Name would remain unchanged and would contain the actual values (105 or 236 caused the issue...).
  Please see the attached solution: you should be able to debug NUnit_HomeMade(...), not NUnit_AutoData(...).

AutoFixtureCases.zip

---

As next steps I propose:

1. Do not override the test Name property any longer. From what I can see there is no need for AutoFixture.NUnit to override the way the test name is created. NUnit will do that for you and will use the customized naming formatting if any.
2. Override the FullName property. Note that for the proper implementation we will need to only provide inlined argument values, not auto data argument values. They reason if that if the same test method has multiple test cases (with different inline values) the full name must be able to identify them uniquely.

Hope this sounds good. If that's not the case we will need to reach out to NUnit folks.

[zvirja]

I wanted to see how it works with xUnit... VS 2017 15.3.2 + xUnit + AutoFixture seems to be also failing.

That is because you have installed AutoFixture.xunit glue library instead of AutoFixture.xunit2. If you install the valid library - everything will be fine 😉

Do not override the test Name property any longer.
Override the FullName property.

I don't like this idea and vote to override both the properties. Internally, nUnit uses the Name property to generate the FullName one. I've checked source code and found about 75 usages of the Name property. I'm not sure that we will not break something if we keep those 2 ones inconsistent.

Therefore, let's override both the properties - it will be safer.

[frblondin]

Therefore, let's override both the properties - it will be safer.

Are you saying that you are ok with generating a name that doesn't the values anymore (auto( instead)? I'd be suprised that you changed your mind about this. I probably misunderstood...

[zvirja]

Are you saying that you are ok with generating a name that doesn't the values anymore (auto( instead)?

Yes, let's override stable name for both Name and later FullName properties as otherwise it could lead to unpredictable results. I don't know nUnit enough to assert that it will work fine in all the cases.

I'd be suprised that you changed your mind about this. I probably misunderstood...

I'm not sure why you see that I changed my mind.. I still expect that this feature will be disabled by default. What we are currently discussing - is just a detail of implementation of the strategy that changes the test name.

[frblondin]

Thanks for the clarification.

Ok, I'll implement the plan you suggested here and will keep you posted.

[frblondin]

During the last days I had limited availability to complete the PR. I have one last remark/question before I start.

The purpose of this PR is to circumvent a limit that affects the runtime (NUnit3 combined with VS test runner or other runners). I believe the source code should not be affected when we need one naming convention or another. We shouldn't have to change the attribute because we want/need another naming generation.

There are two choices: we can add an option using some test file (ie. if there is a file named XXX.config then read it) or we could also decide to use one NuGet package or another.

Today there is the AutoFixture.NUnit3 and AutoFxiture.Nunit3.PatchedName. Finally I believe that this is an acceptable solution.

I propose to add one more nuget package in this github repository so that any update made to AutoFixture.NUnit3 would benefit to the fixed name nuget package. We would need to find a nicer name that the nuget package I created - something like AutoFixture.NUnit3.FixedName or AutoFixture.NUnit3.ImmutableName.

What do you think?

[zvirja]

@frblondin Thanks for sharing some new details and your vision!

I propose to add one more nuget package in this github repository so that any update made to AutoFixture.NUnit3 would benefit to the fixed name nuget package.

The approach with a standalone NuGet package looks attractive at the first glance, however I don't like it too much. That is simply a copy of the AutoFixture.NUnit3 project with a slightly tuned AutoData and InlineAutoData attributes. We already have a plenty of almost identical glue libraries in our code base (like NUnit2, xUnit, xUnit2) and it would cost a lot to maintain one more without any strong reason. Another issue is that it would be hard for people to depend on us and create their own public libs based on our AutoFixture.NUnit3 glue library - they will need to explicitly support both AutoFixture.NUnit3 and AutoFixture.NUnit3.PatchedName as classes from these two assemblies will have different identities.

we can add an option using some test file (ie. if there is a file named XXX.config then read it)

This option looks probably reasonable, however we should do that using iterations. We could finally come up with that solution in the future if that's indeed strongly demanded. What I'm actually suggesting now is to do steps one by one - not all at once! The first step would be to implement that as a configurable strategy. Later we could improve that further so that default strategy will be a switcher and will look at config first.

We shouldn't have to change the attribute because we want/need another naming generation.

I don't see actually too much issues with that, but it's probably my experience. I never used AutoFixture.xUnit2 without AutoFixture.AutoNSubstitute (or AutoFixture.AutoMoq). Also I always needed to tune the default behavior of AutoFixture and add project-specific customizations. As result, I always ended up in creation of my own AutoData and InlineAutoData attributes and using them all over the project instead of the default ones.

With such experience behind I think that a lot of people are doing the same. That's why I expect that it will not be a big issue to change the default naming strategy as you already have your own data attributes. However, if that appears to not be so - we are free to improve that in future by adding the configuration support.

---

The bottom line is that I'd suggest to proceed with the strategy approach described above and improve that later if needed. It will simplify both review and testing the changes.

[frblondin]

I finally found a much better way to modify the test name. The TestParameters has two properties: OriginalArguments and Arguments. The latter is used for the execution whereas the first is only used for the display. Instead of overriding the test name generation the code is now simply changing the name used by the test generation. Note that I had to set values on the existing array using indexed setters as these properties are readonly. Both arrays are two distinct instances, see here.

Also, instead of setting a string value in the OriginalArguments array, I had to find an object which is not wrapped in double quotes ("auto" vs. auto) by the TestNameGenerator (see here). This is why I'm storing StringBuilder instances in the OriginalArguments as the TestNameGenerator simply calls ToString().

I decided to implement the logic in AutoDataAttribute instead of AutoDataFixedNameAttribute: as discussed if we later find a better way to decide whether fixed test names should be generated, the internal boolean (false by default) will just need to be set differently.

Lastly I had to update NUnit because of a bug in the TestNameGenerator - see last commit.

[zvirja]

Thank you for the follow up! 👍

I finally found a much better way to modify the test name.

I don't like this approach. While the documentation for the OriginalArguments property says that it's being used for the display, the name of the property doesn't imply that. Therefore, such approach might be non-stable and looks a bit cumbersome. In future versions NUnit could add some additional responsibility to this property and our integration will fail or will break something inside the NUnit as it will not expect a trash there.

The previous approach when we format the name manually looks much better and I'd vote to stick to that.

This is why I'm storing StringBuilder instances

For me usage of StringBuilder looks like an overhead as this class is a bit complicated under the hood. I'd suggest to create a custom type instead with overridden ToString() method. It will be much quicker and safer as NUnit could start to support StringBuilder in the future versions.

Also ensure to use full type name in the placeholder to avoid collisions when multiple types have the same name, but reside in different namespaces.

I decided to implement the logic in AutoDataAttribute instead of AutoDataFixedNameAttribute

Previously we agreed to implement the logic via the strategy, however you changed the design. Could you please stick to that decision and follow it? The current approach has drawbacks:

• It adds additional responsibility to the AutoData attribute and from design perspective it's better to keep that logic in a separate class. It will look much simpler and cleaner.
• It adds unnecessary dependency between InlineAutoData and AutoData attributes as now you call the AutoDataAttribute.UpdatedFixedTestArguments static method. It's again a good indicator that strategy will work better.
• You have introduced AutoDataFixedNameAttribute attribute while we haven't agreed on that before. Please don't introduce that attribute for now as it could be easily created later in users solution. We could add it later via a separate PR if needed, however that is a whole new story.
• The GeneratesFixedUnitTestName property has protected setter - any reason for that? I believe that setter should be public (of course, in our case that will be a strategy property rather than a bool flag). Ensure you also have guards, so null value cannot be set.

Lastly I had to update NUnit because of a bug in the TestNameGenerator - see last commit.

You haven't provided any information in the commit message what is the bug. Could you share this info? Notice, we cannot accept these updated NUnit dependency in v3 as this is a HUGE breaking change. We are following the SemVer in this project and we provide a guarantee to customers that no breaking changes will appear withing the same major version. If you update the dependency of the package, it will be impossible for part of users to longer use this library.

Unless there is very strong reason, I suggest to keep the dependency version the same as it is. Otherwise, the chance we will be fine to accept these changes is lower 😟

Merge remote-tracking branch 'upstream/master'

Please don't use back-merge from master into your feature branch and use the rebase instead. The history will look much simpler and it will be easier to later track what is going on. Please rebase your branch on top of the latest master and ensure that merge commit is not present. If that is present, use the interactive rebase to get rid of it.

Also don't forget to squash the obsoleted commits to make the commits history look cleaner. Of course, the important commits should be preserved, but the obsolete or non-significant ones should be squashed.

---

Thank you for the time you spend on this - highly appreciated. Sorry that we cannot the changes in the current form - we should think about the project maintability and the suggested approach has some undesired implications.

Notice, previously we have already agreed on approach, but you implemented it in a completely different way 😕 It's a bit hard to interact in such a way as it confuses and requires a lot of efforts from both my and your sides. I'd suggest to try to stick to the discussed approach as much as possible so we can proceed quicker.

Looking forward to your follow-up! 😉

[frblondin]

You must think I'm crazy after reading your comment lol.

I don't think I really changed the plan:

1. Rework current approach, so that we don't decorate string manually, but rather use NUnit API for that (e.g. new TestNameGenerator("{m}{a:40}").GetDisplayName(test, ....)).

This is exactly what I intended to do. When you use xUnit the test names are including the namespace but test name under NUnit do not. This is something that could evolve and NUnit could provide a general option to amend the way test names are constructed. So I wanted to rely on NUnit API to generate the names just as you suggested, and not directly provide to the TestNameGenerator the pattern of test names that NUnit may change. Moreover if you change the test name you also need to change the FullTestName.

My primary implementation was relying on a direct use of TestNameGenerator but had the limitation I just mentioned... and I ended up seeing that the OriginalArguments is used to generate test names. I agree that the property name is crappy but the description says it's used for this purpose only.
Finally there is no perfect solution but I vote for the use of OriginalArguments.

After hearing these arguments let me know if you agree now.

This is why I'm storing StringBuilder instances

I'd suggest to create a custom type instead with overridden ToString() method.

Agreed

Also ensure to use full type name in the placeholder to avoid collisions when multiple types have the same name, but reside in different namespaces.

You mean that we should display auto<System.String> instead of auto<String>, right? This will make the test names longer, reduce the readability, and could lead to other issues. I remember that Visual Studio Test Explorer has limitations with loooong test names (couldn't find any reference on the web but remember this was causing problems with xUnit 2). Moreoever the risk of having in the same test method two test cases that are having a reference to two different types with same name but different namespaces seems very unlikely to me.

I decided to implement the logic in AutoDataAttribute instead of AutoDataFixedNameAttribute

Previously we agreed to implement the logic via the strategy, however you changed the design. Could you please stick to that decision and follow it?

Ok

Lastly I had to update NUnit because of a bug in the TestNameGenerator - see last commit.

You haven't provided any information in the commit message what is the bug. Could you share this info?

Because I tried different attempts and because TestNameGenerator is declaring additional constructors in later versions of NUnit I did my developments locally on a more recent version of NUnit. Since I finally didn't use TestNameGenerator I primarily didn't push the NUnit update. The tests were then failing because appveyor used NUnit 3.0.0. I then pushed the NUnit 3.5.0 update without clearly identifying which commit resolves the issue.
I was not aware of the SemVer so I'll see if I can find a workaround in 3.0.0.

Please use the rebase instead

Sure I'll do so for the next changes.

[zvirja]

So I wanted to rely on NUnit API to generate the names just as you suggested, and not directly provide to the TestNameGenerator the pattern of test names that NUnit may change.
I agree that the property name is crappy but the description says it's used for this purpose only.

I've checked the implementation more carefully and must admit that the suggested approach with OriginalArguments looks better. I was about to reject this approach, but just noticed this line for FullName formatting. It would be too much to keep this logic in our integration. While I don't like usage of OriginalArguments property as it isn't descriptive, it looks like less pain. Also I noticed that NUnit allows to set default test name pattern via console, so it's indeed better to support this feature by not hardcoding the pattern on our side.

So please go ahead with this approach, but please extract it to a standalone strategy. Probably, you should extract the whole TestMethod creation to the strategy, so later strategy can decide whether it wants to patch OriginalArguments or the Name & FullName properties. That's in case we find some issues in future and cannot bring the breaking changes.

I remember that Visual Studio Test Explorer has limitations with loooong test names
Moreoever the risk of having in the same test method two test cases that are having a reference to two different types with same name but different namespaces seems very unlikely to me.

It might be right as I noticed that previous version of NUnit had argument values limit set to 40 chars, while later the limit was removed..

OK, let's proceed with the suggested approach and put the type name only. We could change that later if users report about name collision issues.

---

So please go ahead and

• rebase your branch on top of fresh master;
• clean-up the commit history (e.g. revert the NUnit upgrade and remove that ugly merge commit);
• extract name management to the strategy;
• remove the introduced AutoDataFixedNameAttribute like attributes.

Will be looking forward to your updates and thanks for your efforts 😉

P.S. Note, I'll be in vacation for one week starting the upcoming Saturday, so you might observe the delay in that period.

[zvirja]

Thank you very much for your passion and time! I've reviewed the changes (it took a half of a day) and the whole approach looks fine. I've left some comments, please take a look :)

P. S. Don't be scared by number of comments - most of them are really tiny 😉 We need to keep our code base clean, so that's why I left them.

[zvirja]

Please make the setter public and add a guard for null value. That would be a typical Property Injection pattern in action.

[zvirja]

Please extract the whole TestMethod creation to a strategy, as current approach looks cumbersome. Creation of TestMethod is a single logical responsibility and it shouldn't be spread among multiple different classes.

Another point is that current design is tightly bound to a way how you fix the name. For instance, if later we want to fix name by setting the Name and FullName properties, it would be impossible with current extensibility design.

[zvirja]

Remove the "Default" prefix - name shouldn't reflect how this type is used. We might change the default builder later, so name will automatically become obsoleted.

Also rename class to reflect a fact that it uses values in method name. It would be hard to choose between "TestCaseParameterBuilder" and "FixedNameTestCaseParameterBuilder" as you can only indirectly guess what is the particularity of the first strategy.

[zvirja]

Remove this static cache as it's a pure premature optimization. Use the strategy constructor instead where needed. Later we could optimize that if required.

[zvirja]

Should be public.

[zvirja]

How is this test different from the test above? Likely, the one above should be removed in favor of this test.. In this case don't forget to align name with similar InlineAutoDataFixedNameUsesFixedValuesForTestFullName Test.

[zvirja]

Please add a test for InlineAutoData attribute to asset this default value.

[zvirja]

Please add a test for AutoData attribute to assert this default value.

[zvirja]

Please rewrite doc to not mention who uses this type and how. Instead simply document the behavior of this strategy as if it would be one among many other similar strategies.

[zvirja]

Rephrase doc to emphasize that this strategy generate static test name and omit the implementation details.

[frblondin]

Thanks a lot for your review. I'm really impressed with the quality of your comments.

I just pushed the changes and I think I took into account all your comments. Note that I'm a but confused with the github interface...

[frblondin]

May be I should have pushed new commits instead of squashing them...

[zvirja]

@frblondin Thank you a lot for those changes! Now code looks significantly cleaner! 😎

I've noticed that some of the comments were not addresses. Notice, it's always better to add some marks to the comments when you addressed them (e.g. 👍) and comments when you won't fix.

I'll take a look again and will keep the leftover comments.

[zvirja]

Huge thanks for the applied changes - now code looks like a charm!

I've left a few minor leftovers - it shouldn't be an issue for you to fix them. Currently I'm on vacation and don't have a laptop, so cannot apply those changes by myself..

P. S. Feel free to use squash and don't worry about GitHub heavior. Also don't forget to add marks on processed comments 😉

[zvirja]

Specify the strategy explicitly here. Don't rely on a fact which one is default - it might be changed in future. Don't forget to update the name and related test names 😉

[zvirja]

The purpose of this test was to test what it the default strategy for non-regression testing - eg. make sure that the name won't be altered after people update their NuGet package.

Currently you have two kind of attributes - AutoDataFixedNameAttribute and AutoDataDefaultNameAttribute. In this scenario context it's better to have AutoDataFixedNameAttribute and AutoDataVolatileNameAttribute - as you seem to cover all the strategies individually.

If you wish add tests for regression, you should:

• Assert the default strategy - already done (AutoDataAttributeUsesRealValueByDefault() and InlineAutoDataAttributeUsesRealValueByDefault()).
• Add yet another set of tests where you test the AutoDataAttribute and InlineAutoData attributes directly and assert that name is mutating by default.

I'd say that the last point is not obligatory, but if you wish - feel free to add it.

P.S. Please add reply comments in place, rather than in global scope - it's easier to track them 😉

[zvirja]

The main issue with current approach is that when we change the default strategy, all those tests will become failing and no tests will cover the VolatileTestBuilder anymore.

[zvirja]

Specify the strategy explicitly here. Don't rely on a fact which one is default - it might be changed in future. Don't forget to update the name and related test names 😉

[zvirja]

Move this helper method to the class with tests - it's related to verification and is not a test data.

[zvirja]

Don't say "by default" in test name. Rather, say like "when volatile name strategy is selected". When we change the defaults, this name will not become obsoleted.

Please see around for the similar tests and fix them as well.

[zvirja]

Better name it "VolatileName..." - looks nicer IMO

[zvirja]

I'd vote to hard-code int and string values and don't use the concatenation. Currently test are read bad because of this concatenation..

[zvirja]

Embed injected value and avoid concatenation - readability is poor :(

[zvirja]

Embed injected value and avoid concatenation - readability is poor :(

[zvirja]

Fix to "OriginalArguments has a..", as you will change this property.

[zvirja]

We are changing the Arguments property, not OriginalArguments.

That's the main point concern - we should modify the OriginalArguments array instead. It's a bit strange that we patch the OriginalArguments array, but make a copy of the Arguments one. See my comment below - it's exactly about that.

[zvirja]

I've just checked the source code and found that actually the setter is present, but it's private. You can use the following code snippet to set the value:

// We should use the declaring type, otherwise private setter is not available.
var property = typeof(TestParameters)
    .GetProperty(nameof(TestCaseParameters.OriginalArguments));
property.SetValue(parametersObject, newValue);

It doesn't matter whether you access internal or private setter - code is ugly in any way 😅 However, it will be more correct to modify the OriginalArguments array.

[zvirja]

Please add a test for exception case. Ensure to add tests for both the strategies.

[zvirja]

@frblondin Do you know whether you will have time in the nearby future to fix my comments? We are on the final way and I wish to publish it soon 😅 Also I'm actively working on v4 release and it would be good to finish this one, so I could apply safely apply the breaking changes to v4 and don't worry about merge conflicts.

[zvirja]

@frblondin Replied in appropriate thread. Please add replies in the related threads for better readability.

[zvirja]

@frblondin Could you please reply in threads as I asked you here?

[zvirja]

I've replied to all your questions - please take a look.

[zvirja]

@frblondin Pinging just in case you missed my reply 😉

[frblondin]

@zvirja I was in a long flight... I just pushed the round of updates - let me know!

[zvirja]

I've pushed some minor changes to prettify the code and comments and now PR looks good to me! 👍

[zvirja]

@moodmosaic I've worked closely with Thomas on this one and finally we made a shape when PR is fine to me.

A short story - NUnit doesn't support tests with volatile names as it's unable to identify them between discovery and execution (opposed to xUnit which support this feature). Therefore, some runners like VS and NCrunch simply cannot run tests with AutoFixture. In this PR we add strategies to produce different kind of test names. One of the strategy is to use the real argument values for test name (the default NUnit behavior), while another one is to replace real argument values with placeholders (auto<ArgTypeName> in our case). That allows user to create their own data attributes using the latter strategy to correctly work with NCrunch and xUnit. Of course, it's a bit clumsy behavior and this feature should be supported by NUnit rather than by us, however currently NUnit doesn't provide such API and our users suffer.

In this PR we don't change the default behavior and users should enable it manually. Therefore, this PR is a perfect candidate for v3. I have a plan to make this fixed name strategy the default one in v4, so we support all the test runners out of the box. Later if some people with to disable this feature, they could override the strategy.

Nikos, do you want to review the PR by yourself? If not - let me know and I'll proceed with a merge - it looks OK to me.

@frblondin Thank you a lot for all the time you spent on this. One last consideration - we should probably create a wiki article and describe all this stuff - so we can provide users with a link.. Or simply update ReadMe - I'm unsure..

[moodmosaic]

@zvirja, go ahead 👍

[zvirja]

@frblondin Merged! One more time thank you for the contribution. 🥇

I'll try to allocate some time later and create a page on wiki describing how to use this feature. Later I'll release a new minor version of v3, so you can consume the contribution.

[frblondin]

Thanks a lot @zvirja for your time & support!

[zvirja]

@frblondin The feature is released as v3.51.0. It's the first minor update since the ownership change 🎉
Mentioned you on twitter: https://twitter.com/AutoFixture/status/915953470578024449.

[frblondin]

Awesome... and thanks for the credit :-)

BTW hope you'll have a chance to share your opinion about this potential (much smaller) pull request that I could quickly create.

[zvirja]

@frblondin I'm currently looking into that issue and will update you soon.

Do you have any plans to unlist the AutoFixture.NUnit3.PatchedName package from NuGet? It doesn't seem to be required anymore, while might lead to people using it because they are unaware of this feature being supported by AutoFixture. The existing clients will be still able to download it, while new clients will use the AutoFixture only.

What do you think?

[frblondin]

Sure I was planning to hide this NuGet package. The remaining bit is the lazy IFixture load which we are relying on for speeding up test discovery - purpose of this issue.