Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: Add Clone Method to IFixture #1200

Closed
ajorians opened this issue Sep 24, 2020 · 9 comments
Closed

Request: Add Clone Method to IFixture #1200

ajorians opened this issue Sep 24, 2020 · 9 comments
Labels
feature request An idea for a change, aimed at impoving quality of life

Comments

@ajorians
Copy link

Introduction

I would like if IFixture had a Clone method or something similar to create a copy of the fixture.

At the moment I can create an extension method to do this:

public static IFixture Clone( this IFixture fixture)
{
   var cloneFixture = new Fixture();

   cloneFixture.Behaviors.Clear();
   foreach ( var behavior in fixture.Behaviors)
   {
      cloneFixture.Behaviors.Add( behavior );
   }

   cloneFixture.Customizations.Clear();
   foreach ( var customization in fixture.Customizations )
   {
      cloneFixture.Customizations.Add( customization );
   }

   cloneFixture.OmitAutoProperties = fixture.OmitAutoProperties;
   cloneFixture.RepeatCount = fixture.RepeatCount;

   cloneFixture.ResidueCollectors.Clear();
   foreach ( var residueCollector in fixture.ResidueCollectors )
   {
      cloneFixture.ResidueCollectors.Add( residueCollector );
   }

   return cloneFixture;
}

But I feel such an ability to make a copy ought to be in the project itself.

Details

My scenario is I construct a couple instances of a class with a lot of parameters in a test. In addition I want to have a constructor parameter be a certain value for one instance and a different constructor value for a different instance. There are several ways I can do this; the way I have been doing it is:

fixture.Customizations.Add( new FilteringSpecimenBuilder(
            new FixedBuilder( mediaId ),
            new ParameterSpecification(	
                typeof( ObjectId ),	
                "mediaId" ) ) );
var mediaVM = fixture.Build<MediaViewModel>()	
                     .With( mvm => mvm.ParentMixerId, mixerId )
                     .With( mvm => mvm.TrackId, trackId )
                     .Create();

_ = fixture.Customizations.Remove( fixture.Customizations.Last() );

//...

The reason for the removal of the customization is I tried without it thinking maybe the last customization added will have a higher precedence and be used; but it wasn't the case.

If I try to simplify this with some sort of extension method like so:

public static IFixture AddCustomization<T>( this IFixture fixture, T value, string name )
{
   fixture.Customizations.Add( new FilteringSpecimenBuilder(
         new FixedBuilder( value ),
         new ParameterSpecification(
            typeof( T ),
            name ) ) );
   return fixture;
}

It might work in some places but won't in others because after the object is built the fixture contains a customization I no longer desire and I need to remove.

I would prefer not to have a line to remove the added customization(s). So if instead my extension method made a copy of my fixture to which I add modifications to the copy then my creations works as expected and the original fixture is untouched.

Also would work is the ability to add a customization that is automatically removed after the object is created.

Hope this all makes sense.

Thanks for considering my issue 😃

@zvirja
Copy link
Member

zvirja commented Sep 24, 2020

@ajorians Good day! From how I understood your example, it sounds like we already have what you need. The Build() method of fixture is actually creating an immutable copy of internal graph, so you can apply "one time" customizations on top of the AutoFixture without persisting them.

Review this playground demo:

public class Model
{
    public int Id { get; set; }
    public string Name { get; set; }
}

[Fact]
public void Demo()
{
    var fixture = new Fixture();
    fixture.Customize<Model>(c => c.With(m => m.Id, 42));

    var value1 = fixture.Create<Model>();
    Assert.Equal(42, value1.Id);
    Assert.NotNull(value1.Name);

    var value2 = fixture.Build<Model>()
        .With(m => m.Id, (int genInt) => genInt * 2 /* make it always even */)
        .Without(x => x.Name)
        .Create();
    Assert.NotEqual(42, value2.Id);
    Assert.Equal(value2.Id % 2, 0);
    Assert.Null(value2.Name);

    var value3 = fixture.Create<Model>();
    Assert.Equal(42, value3.Id);
    Assert.NotNull(value3.Name);
}

Let us know whether it solves your needs. If not, please describe more specifically why it doesn't work for you, so we could try helping.

Thanks!

@ajorians
Copy link
Author

Hi @zvirja ,

Good day!

If I may; our Model class would not have a set and would only be settable through the constructor. So if you were to substitute it with this:

public class Model
{
   public Model( int id, string name )
   {
      Id = id;
      Name = name;
   }

   public int Id { get; }
   public string Name { get; }
}

Sorry for not specifying that originally.

So as I understand things I have to add customizations in order to do constructor parameters. And these customizations are before the Build().

If interested some of the reasons we do a lot in the constructor and without setters is so we can use the readonly keyword and be sure that the member variables are set and can't be unset by the time a method is called.

Hope that makes better sense. Thanks for helping with this! 😃

@zvirja
Copy link
Member

zvirja commented Sep 25, 2020

@ajorians Do I understand your problem correctly, that you have a model with large amount input arguments, so you would like to customize just one of it, without having to explicitly worry about it. Right?

Otherwise, you can always write something like

var value2 = fixture.Build<Model>()
    .FromFactory((int id, string name) => new Model(id, name))
    .Create();

Unfortunately, it does not scale pretty good with growing number of parameters 😟

@ajorians
Copy link
Author

@ajorians Do I understand your problem correctly, that you have a model with large amount input arguments, so you would like to customize just one of it, without having to explicitly worry about it. Right?

Yes, that is exactly right.

Unfortunately, it does not scale pretty good with growing number of parameters 😟

Luckily Freeze works for many of our classes with growing number of parameters. But yes the class I had in mind where I am using the customizations to do constructor parameters currently has more than 30 parameters:
image

And will probably grow an additional 4 or so before the end of this year.

I get this may be unusual.

But yes I have a large amount of growing input constructor arguments and I would like to customize just one (or a few) of them without thinking about the rest of them. Without having that type registered or frozen for the whole test. And in a concise way as adding a customization is about 5 lines of code.

I luckily am able to do all of that; but the way I did it involved making a clone extension method in my client-side code.

Hope this all makes sense. Thanks for looking at this with me 😃

@aivascu
Copy link
Member

aivascu commented Dec 8, 2020

@ajorians I was about to close the issue due to inactivity but I could not walk by that gigantic constructor.
I got to ask, have you considered changing your design?

By looking at your constructor I can see several issues and potential solutions to your problem.

First of all you are mixing the concepts of newable and injectable entities.
The most straightforward solution is to move your injectable dependencies (aka Service Interfaces) to properties and inject them using property injection.
This will solve your immediate problem of having to copy the Fixture and will make the constructor more manageable.

Second is addressing the over-injection code smell.
This means moving away the most commonly used together services into separate abstractions.
This should help your design respect the SRP and by extention it should reduce the huge amount of setup you have to do at the moment, in your tests.

Since the class is called ViewModel I assume you have a MVVM application. It is likely that you could further decouple your application by introducing a messaging model. Perhaps an event aggregator. Most MVVM frameworks have them built in so you don't have to do a lot of setup work, to benefit from them.

Let me know if this helps.

@ajorians
Copy link
Author

ajorians commented Dec 8, 2020

Hi @aivascu ,

@ajorians I was about to close the issue due to inactivity

If there is anything I can do just let me know. Being a feature request I can see until implemented it might have quite a fair amount of inactivity.

I got to ask, have you considered changing your design?

At the moment we are considering becoming a prism application. That would help with some of the items you mentioned. But sadly I don't see us changing the philosophy of just put everything in the constructor such that it can be checked for null once and be readonly and always be non-null through the lifetime of the class. We need to do something. But I don't think this is going to improve in the next year or so.

Let me know if this helps.

All of this does help. I'll let me team know that even people in the world think we need to consider making changes.

But if I may ask is it reasonable for IFixture to have a Clone or similar method? Or somehow allow people to add a customization and then pop the last added customization off or somehow return to the state before adding a customization? I do agree that understanding the scenario that prompts this desired behavior is important and certainly the scenario has many problems in the first place. And I do see you don't want to add additional complexity unless there is a good use case. Well you can choose what happens next but if I can help just let me know.

Thanks for reading and considering 😃

@aivascu
Copy link
Member

aivascu commented Dec 9, 2020

@ajorians my opinion is that we should not add a .Clone() method to the Fixture class. As I see it there should be only one test fixture at any given time for any single test.

There have been a few requests to implement an unfreeze/eject/freeze-once feature in the past but no one got to implement it.
I think the reason why is that there is no trivial way to reliably identify which builder is going to return the frozen value or what was the last injected value.

What you can do is implement a relay that ignores the requests after a certain count of successful resolutions, like below. You can also update the implementation to skip a certain amount of requests. The rest should be covered by the customization you used in the original post.

public class CountingRelay : ISpecimenBuilder
{
    public CountingRelay(ISpecimenBuilder builder, int maxCount)
    {
        this.MaxCount = maxCount;
        this.Builder = builder;
    }

    public int Count { get; private set; }
    public int MaxCount { get; }
    public ISpecimenBuilder Builder { get; }

    public object Create(object request, ISpecimenContext context)
    {
        if (this.Count == this.MaxCount) return new NoSpecimen();
        var result = this.Builder.Create(request, context);
        if (!(result is NoSpecimen)) this.Count++;
        return result;
    }
}

@aivascu aivascu added the feature request An idea for a change, aimed at impoving quality of life label Dec 9, 2020
@ajorians
Copy link
Author

ajorians commented Dec 9, 2020

Hi @aivascu ,

@ajorians my opinion is that we should not add a .Clone() method to the Fixture class.

OK. Well thank you for considering.

What you can do is implement a relay that ignores the requests after a certain count of successful resolutions

I'll take a look.

Thanks again! 😃

@ajorians ajorians closed this as completed Dec 9, 2020
@aivascu
Copy link
Member

aivascu commented Dec 19, 2020

@ajorians I've created a more formal feature request #1214 for explicit and automatic customization reset. You can track the progress of this feature there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request An idea for a change, aimed at impoving quality of life
Projects
None yet
Development

No branches or pull requests

3 participants