search

nickdodd79 / AutoBogus

A C# library complementing the Bogus generator by adding auto creation and population capabilities.
MIT License
442 stars 50 forks source link

Merge into Bogus #1

Closed bchavez closed 5 years ago

bchavez commented 7 years ago

Hi @nickdodd79 ,

I took a browse through your AutoBogus code. I wanted to reach out to you because I might be interested in merging your code into Bogus as a starting point for https://github.com/bchavez/Bogus/issues/8. Your code looks really clean. Great job btw! :+1:

I don't know how much time you have, but ...

Let me know and enjoy your day.

Thanks, Brian

nickdodd79 commented 7 years ago

Hey Brian,

Thanks for the message and feedback. It would be great to see AutoBogus merged into the Bogus codebase, but I do have some more changes to push that allow for a mocking framework to create interfaces and abstract classes (they would be null in the current changeset). My implementations support Moq, FakeItEasy and NSubstitute, and will be pushed soon.

I would certainly be interested in contributing to the Bogus implementation. I have used AutoFixture for many years, but having moved to .NET Core recently, discovered it wasn't compatible (I believe they are working on it). However, I can see the potential Bogus has over AutoFixture to generate 'real' values as part of the auto generation. I hadn't seen that feature thread but can there are some good ideas on how a strategy could be formed. Let me know what you would want me to do with regards to a contribution.

In terms of the Bogus API, the only work around I needed to put in place was to determine whether a CreateAction needed to be setup. The CustomInitiator method was not virtual, so I needed to make the default null and check whether anything had been assigned. If the CustomInitiator could be virtual, that code would be cleaner.

Thanks, Nick.

bchavez commented 7 years ago

Hey Nick,

That's really great news. I think we should give it a shot. I really look forward to getting AutoBogus merged in. I just now opened up CustomInstantiator to be virtual for you. One of the benefits having AutoBogus merged will be access to the internals and ability to change things like that as much as you need.

A couple of philosophies I've tried to maintain with Bogus are:

So, keeping in line with those two ideas, I think one way we can move forward merging is:

  1. I'll give you commit access to Bogus.
  2. You can then clone Bogus' repo.
  3. Create a feature branch /autobogus off the clone's /master.
    1. Run build restore on the CMD prompt. (This is to fix up Newtonsoft dependency for consistency).
    2. Open Bogus' SNL file and add AutoBogus sources to Bogus whenever you're ready. Could be now or later, whichever you think is best.
    3. Continue committing/pushing on /autobogus until you're ready to merge with /master.

(Also, if you have other ideas too, I'm always open to learning new ways of doing things).

I recently migrated from NUnit to Xunit (something I've wanted to do or a while now), so you should feel right at home.

If at any point you need help with anything, feel free to tag me on GitHub here in your repo or on an issue I created for this merge.

Thanks, Brian

(Note: Invite sent, let me know if you didn't get it.)

nickdodd79 commented 7 years ago

Hi @bchavez

Invite all received and working, and thanks for the update.

I am just in the stages of putting my lib in a NuGet package so I can reference it in some projects I have going - needed for build servers and the likes. Once I have done that I shall start moving stuff into a new branch. I'll give you a shout when I have done the initial migration.

Nick.

bchavez commented 7 years ago

Hey @nickdodd79 ,

Just wanted to let you know I made some progress on this.

I created a branch Bogus/autobogus, that contains the FakeItEasy, Moq, and NSubstitute projects. All are integrated with the CI build and F# FAKE scripts now. All are building fine now. Additionally, the AutoFaker Unit Tests are all passing.

I had to make some minor changes to the code to make it run on .NET 4.0; but they were easy enough.

Also, found a StackOverFlowException bug when using something like this:

public class Foo{
   public int N {get;set;}
   public Foo RecrusiveFoo { get;set; }
}

Eventually, I'll try to get around to fixing this (lil pressed for time atm) but in the meantime feel free to have full range on the source. :+1:

nickdodd79 commented 7 years ago

Hey @bchavez,

Glad to hear you managed to get everything in and working. The recursive generation issue has been on my list of things to do, I just haven't got to it yet (a work around is to add a rule rather than rely on the auto process).

However, having used AutoBogus in my projects I have identified some changes and improvements.

  1. I use models to both return dummy data from my API and provide fake data for my unit tests. In the first instance it is well structured and meaningful (a Faker is used), but the second is just random (AutoBogus is used). What I discovered is I needed to maintain 2 data sets for the same model (one Faker and one AutoFaker) as opposed to a single definition for both.

  2. Having read the details of Feature: Auto Populate by Convention? #8 I thought the concept of conventions to drive the generation process would work well.

Based on those, I have been putting together a rework of AutoBogus. It is now convention based with the ability to provide custom conventions from your unit tests. This also negates the need for specialized Moq, FakeItEasy, etc. libraries because an 'on the fly' convention can be registered by the developer.

I have also opted for Faker extension methods over an AutoFaker class. Point 1 showed that the auto generation was an extension to the faker definitions rather than something separate.

Additionally, the recursive StackOverflowException has been handled by a specialized convention invoked during the generation process. An exception is thrown at the moment, but may be a flag can be used to configure this by the developer.

I have completed the development, I am just putting some tests in place to ensure it works as expected. I will commit an initial version for you to check out in the next day or so. So I don't clash with the work you have done for the integration, I will create a new branch called autobogus_conventions.

I hope that all made sense. Shoot over any comments or questions.

Nick.

bchavez commented 7 years ago

Hey @nickdodd79 ,

No worries. :+1: It was a good exercise for me to get a feel of how your code would merge into /master. I'm really encouraged things should be pretty smooth.

W.R.T. Point 1, that's a wonderful insight. Thank you for sharing. I can most definitely see how maintaining two AutoFaker and Faker can be a maintenance issue.

Overall, I think the idea of 'on the fly' registration is really great for stub generation since it eliminates the need for the satellite assemblies. I felt a small hesitation about the new assemblies when I was merging, but told myself to get over it since the benefit of having AutoFaker was greater than the maintenance of the satellite assemblies. So, it sounds like your rework is going to be even better!

Keep up the great work! And as always, feel free to change any internals you might need to make life easier. :)

Brian

nickdodd79 commented 7 years ago

Hey @bchavez,

I have committed an initial version of the AutoBogus rework so it includes conventions. It is in a branch called autobogus_conventions so as not to clash with the migration work you did.

I am in the progress of writing the necessary unit tests and sanity checking, so there could be some changes to come. The convention implementations are all covered by tests and I have added a quick/dirty test to generate an object end to end and show how it would be pieced together by a developer.

Take a look and fire over any feedback.

Nick.

bchavez commented 7 years ago

Hi @nickdodd79 ,

I had a chance to review your branch today. You've done a really amazing job on everything. :+1: Honestly, you have a really elegant approach to solving the issue.

I still haven't totally understood and digested the overall architecture feel yet; so it might take me a few more days. My gut feeling so far is the core implementation looks very solid.

Here are just some of my random observations, I totally get everything still in progress and fluid so only take them with a grain of salt :smiley:. These are just my random thinkings right now:

AutoGenerate Instance Generation

I looked at AutoTests.cs and wanted to get your opinion on the public API. Specifically, this part here:

var company1 = faker.AutoGenerate(options =>
{
    options.Add("FirstName", b => b.Name == "FirstName", c => c.Binding.Value = "TestFirstName");
});

From my understanding, a call to AutoGenerate( opts => ... ) creates an instance (similar to Faker.Generate()) using a ConventionsBuilder builder Action callback.

var company1 = faker.Generate(); //and generate like normal on Faker var manyCompany = faker.Generate(5); //if you want many


   * Perhaps separating the two actions would be good. The second structural thing I would mentally chew on is the concept of a `ConventionsBuilder` and its role.

Again, all these comments are mostly unfiltered ramblings in my mind. It's 11 PM and pretty tired so my brain is barely functioning. I'm not sure if any of this makes sense, but the fog should clear once I study your code more. :+1: 

Regardless, you have done an amazing and wonderful job so far. A++++++ work! Rock star yo! :+1: 

Thanks,
Brian
nickdodd79 commented 7 years ago

Hey @bchavez,

Thanks for the feedback and glad you like what's there so far.

Totally understand what you are saying and you make a very valid point. I think I may have focused primarily on the conventions side of things and gone a bit tunnel visioned on the actually API to invoke them. The current implementation would mean having to potentially configure the conventions for an AutoGenerate multiple times - part of which I was trying to mitigate against in the first place.

I therefore think a slight deviation is needed where a hybrid of an AutoFaker class and extension methods could work. My original idea was that a central repo of Faker models would be available that could then be used for both setting up fake runtime data (i.e. from a database) as well as populating fake instances for unit tests. The AutoGenerate methods would then allow each unit test instance to be configured for a particular scenario. This is how I am current using it anyway, so might be a bit bias towards my use case!

I'm wondering whether we could still use Faker instances as an entry point, but then they can be passed to an AutoFaker instance where the auto-magicals happen. The following illustrates the various ways this could be implemented (some or all). This includes leveraging a Faker as a constructor argument, the use of an extension method on a Faker instance to create an AutoFaker (like a proxy creator) and going straight to an AutoFaker instance.


var faker = new Faker<Person>();

var person = new AutoFaker<Person>(faker)
    .UsingConvention(new MoqConvention())
    .Generate();

var people = faker
    .AutoPopulate()
    .UsingConvention(p => p.Age, f => f.Random.Number(100))
    .Generate(10);

var user = new AutoFaker<Person>()
    .UsingConvention(c => c.Binding.Value = new Person(c.FakerHub.Name.FirstName()))
    .Generate();

How does that work for you? This approach I think still uses the concepts of my original AutoBogus implementation, but adds the ability to setup a conventions pipeline like the new one. I would still also include the mechanism to register 'global' conventions that are applied in the the pipeline for ALL AutoFaker generations. I envisage the global conventions as a place to register the likes of a mocking framework used to auto generate interfaces, abstract classes, etc. (a bit like the filters used in an MVC application). This is also taken from AutoFixture where customizations are registered on a Fixture instance, which is then used for all create requests.

I think the guts of my new implementation is still relevant so I shall continue with my unit testing efforts around that.

Hope that all made sense.

Nick.

nickdodd79 commented 7 years ago

Hey @bchavez,

Upon further reflection, I am wonder whether mimicking the behaviour of AutoFixture would be an elegant way of designing the public API. The follow shows how this would work, and how a single AutoGenerator instance governs the creation of multiple objects with a single pipeline of conventions:


public AutoTests()
{
    Generator = new AutoGenerator()
        .UsingConvention(new NSubstituteConvention())
        .UsingConvention<IDictionary<Product, int>>(c => new Dictionary<Product, int>
        {
            {c.Generate<Product>(), c.FakerHub.Random.Int()},
            {c.Generate<Product>(), c.FakerHub.Random.Int()}
        });
}

private AutoGenerator Generator { get; }

[Fact]
public void Should_Generate_Order()
{
    var orderFaker = new Faker<Order>();
    var orderItems = Generator.Generate<OrderItem>(3);

    Generator.UsingConvention(b => b.Items, orderItems);

    var order = Generator.Generate(orderFaker);
}

[Fact]
public void Should_Generate_OrderItems()
{
    var orderItems = Generator.Generate<IEnumerable<OrderItem>>();
}

[Fact]
public void Should_Generate_ShoppingCart()
{
    var shoppingCart = Generator.Generate<ShoppingCart>();
}

It shows how a series of conventions can be defined for the generator (as well as the internal ones), that are then used by all Generate() calls. Further more, additional conventions are applied for an individual test, which are only scoped to that test (the AutoGenerator is refreshed for every test).

An object can be created using the generic (ShoppingCart) or by passing a predefined Faker instance. For the latter, any CustomInstantiator or RuleXXX definitions are also applied to the resulting instance.

How does that seem?

Nick.

bchavez commented 7 years ago

Hi @nickdodd79 ,

Thanks for all the ideas. I'll try to give you my thoughts as best I can. I think it's great we're approaching this from the public API side now. I think once we have the public API, the details for meeting up with the core/guts of the implementation should be okay.

:white_check_mark: I think the concept of a Global Registry for "conventions" for registering MoqConvetion makes a lot of sense. Let's keep that idea. :)

W.r.t your first sample:

var faker = new Faker<Person>();

var person = new AutoFaker<Person>(faker)
    .UsingConvention(new MoqConvention())
    .Generate();

var people = faker
    .AutoPopulate()
    .UsingConvention(p => p.Age, f => f.Random.Number(100))
    .Generate(10);

var user = new AutoFaker<Person>()
    .UsingConvention(c => c.Binding.Value = new Person(c.FakerHub.Name.FirstName()))
    .Generate();

I have a feeling the hybrid AutoFaker<T> approach might have legs. Perhaps we could explore this a bit more and see where the approach can fall short. More specifically:

var person = new AutoFaker<Person>(faker)
    .UsingConvention(new MoqConvention())
    .Generate();
var faker = new Faker<Person>();

var people = faker
    .AutoPopulate()
    .UsingConvention(p => p.Age, f => f.Random.Number(100))
    .Generate(10);
var faker = new Faker<Person>();

var people = faker
    .AutoPopulate()
    .RuleFor(p => p.Age, f => f.Random.Number(100))
    .Generate(10);

Semantically, I think it's a bit more clear because we get help from VS for the LINQ expression p => p.Age. And p is typed with Person. I think the idea of a "convention" is something that acts as a cross-cutting concern over all types. To me, a convention detrmines if some type+property/field meet some condition. If a condition is met, the "convention" generates a rule for that particular type+property combo.

As an example (ignoring the existence of Faker.RuleForType), if the user wanted, they could create a "PositiveIntegerOnlyConvention" that, when added to the pipeline (either added globally or to a Faker<T>/AutoFaker<T> instance) would make a rule to generate only positive numbers when encountering an int property or field).

Or perhaps a developer wanted to create an Age convention to ensure that whenever an age is encountered on a Manager type, Employee type or Customer type, all which have Age properties, an Age rule is created with a value between 20 and 40.

MyAgeRangeConvention could look like:

public class MyAgeRangeConvention : Convention{
   public bool CanGenerate(MemberInfo, Type declaringType, context){
   public bool CanMakeRule(MemberInfo, Type declaringType, context){
      return declaringType == typeof(Manager) ||
                  declaringType == typeof(Employee) ||
                  declaringType == typeof(Customer);
  }
   //OPTION 1: Just get a value, downside is, we don't
   //have access to Faker<T> at this point and we'd 
   //potentially miss out on calculated properties/fields.
   //See option 2.
  public object Generate(context){ //called at Faker.Generate() time that returns a value
      return context.Random.Int32(20,40);
  }
  //OPTION 2: Make an actual rule. Could be useful
  //in situations where you'd like to get the Faker<T>
  //object and compose properties like 
  //merging a FirstName+LastName for a FullName or
  //to make specific decisions on a condition.
  // Downside, it could be
  //a bit hacky to get to a Faker<T> object via casting;
  //but the code smell might be worth it?
  public void MakeRule(context){
      //bit ugly, if we wanted strongly typed Faker<T>
      if( context.Faker is Faker<Manager> fakeUser ){
               fakeUser.RuleFor( u => u.Manager, f => f.Random.Int32(30,40);
      }
      if( context.Faker is Faker<Customer> fakeCustomer ){
             //Our customer base is 20,30.
              fakeCustomer.RuleFor( c => c.Age, f => f.Random.Int32(20,29));
     }
     //Otherwise
     var fakeManager = context.Faker as Faker<Manager>;
     fakeManager.RuleFor( m => m.Age, f => f.Random.Int32(35,40);
   }
}

That's kinda how I see the idea of a "convention" going or some variation thereof. Please, let me know what you think. Also, please feel free to critique/criticise the idea.

Also, one benefit of going with #2 is, since it's a simple as applying a regular Faker<T>.RuleFor, as the convention pipeline is executed, RuleFors are overwritten, preserving order, of the pipeline (last one wins) and and it kinda makes sense because the developer would expect the same behavior (i think) to how RuleFor works on Faker<T>.

class Company{
   Manager[] Managers;
   Customer[] Customers;
   Employee[] Employees;
}
var companyFaker = new Faker<Company>();

var company = companyFaker
    .AutoPopulate()
    .UsingConvention(new MyAgeRangeConvention())
    .Generate(10);

Conventions kind of work like custom Newtonsoft.Json Converters that know how to serialize/deserialize a type. Except, in this case, a converter can apply a Faker rule when specifics are met.

var user = new AutoFaker<Person>()
    .UsingConvention(c => c.Binding.Value = new Person(c.FakerHub.Name.FirstName()))
    .Generate();
var company = companyFaker
    .AutoPopulate()
    .UsingConvention(new MyAgeRangeConvention())
    .UsingConvention<Customer>(c => c.Age, f => f.Random.Int32(80,90) );
    .Generate(10);

// All customers in the 10 companies are 80-90 in this test.

W.r.t AutoFixture:

[Fact]
public void Should_Generate_Order()
{
    var orderFaker = new Faker<Order>();
    var orderItems = Generator.Generate<OrderItem>(3);

    Generator.UsingConvention(b => b.Items, orderItems);

    var order = Generator.Generate(orderFaker);
}

I'm not sure about Generator.UsingConvention(b => b.Items, orderItems); because it looks like a rule more than a convention?

I think I'd have to understand more of the AutoFixture idea. I think we have to explore the idea a bit more.

Also, perhaps we should start posting in https://github.com/bchavez/Bogus/issues/74 or https://github.com/bchavez/Bogus/issues/8. If we did that, we could solicit comments from the community. There are quite a few subscribed the repository and would probably like to comment with how they're using Bogus (and how they'd like to use AutoBogus). I think we could cherry pick the best ideas there too.

Many thanks! It's hard work, but I think we're getting there! I hope some of my thoughts help.

Brian

nickdodd79 commented 7 years ago

Hi @bchavez,

Thanks for the feedback. It seems there are several ideas and avenues to consider, and I think opening it up to the community is definitely the right way to go to ensure the framework is of use to people. I shall there look to get the dialogue started in bchavez/Bogus/issues/74.

During my experimentation with the AutoGenerator class I did realize the property conventions would be better aligned to the Faker<T>.RuleFor design and so changed them.....great minds 👍

The angle I have been looking at for the auto populate functionality (which I think has been biased by what I did for AutoBogus) has been to keep the Faker<T> instances intact. This is so multiple instances can be created from a single definition each with slight variations on how the population is applied. Adding RuleFor calls under the hood will mutate the base model definitions and affect subsequent generation of a Faker<T>.

As an example, if I have Faker<Person> and want to generate 2 instances for my unit test I would call Faker<T>.Generate() twice. However, I then want the first to set the manager to be null, but keep the second to have an auto generated manager, so I apply .RuleFor(p => p.Manager, f => null) before calling Generate() on the first. If we use a method of applying these rules through conventions and mutate Faker<T> then the second instance would also have a manager of null. Having said that, a workaround for this is to use the ruleset string parameter of Generate(). Is that an approach a developer would use for this scenario?

I have also been keeping the auto classes separate to the Bogus implementation. Do you think it would benefit from having the auto logic baked into the main implementation you put together (i.e. add something like AutoRuleFor() to fFaker<T>)?

Hope all that made sense.

Nick.

bchavez commented 5 years ago

Just closing to keep your issue tracker clean. https://github.com/bchavez/Bogus/issues/74#issuecomment-502246410 The TLDR: merging can always be an option if or when the time is right.