Extend PR #77 - Add Test Data Builders

[jondavis9898]

This is an extension of the concepts originally introduced in PR #77 by @up2go-rsoesemann. A big Thank You to him for getting this started!

Enhancements from #77:

1. Add ability to build existing or new SObjects in-memory
   ** build(Boolean) on Base / build() & buildNew() on derived
2. Persist single builder to DB or register multiple builders for persistence to DB
   ** Single - persistBuilder(fflib_ISObjectUnitOfWork) on base / persist(fflib_ISObjectUnitOfWork) on derived
   ** Multiple - persistRegistered(fflib_ISObjectUnitOfWork) on base
3. Test Coverage - fflib_DomainObjectBuilder
4. Add Object Mother pattern for simplified builder creation

Comments:

1. Per discussion in Added base class for TestDataBuilders #77:
   ** Rather than have builder create Unit Of Work, decided to have it be a method parameter. The reason for this is a couple of fold but ultimately, every test data situation is slightly different and if list of SObjectTypes is maintained in the builder itself, it limits what SObject can be built during persist operations. By passing in as a parameter, the test setup code can ensure the UOW has the correct SObjectTypes. Also, the derived class can choose to have a persist() and a persist(fflib_ISObjectUnitOfWork) convenience methods that both wrap persistBuilder(uow). The persist method can create a 'default' UOW internally and when necessary, persist(uow) can be called when the default of the builder does not satisfy the needs. In short, get the best of both worlds here I think.
   ** Persistance to the DB when operating on registered builders held in static is done via a static method
   ** Persistance to the DB on an individual builder instance (and it's related data) is done via instance method
   ** Decided not to add @istest to fflib_DomainObjectBuilder since this class can be used for non-test purposes (see future enhancements below as well) and has 100% test coverage.
   ** Regarding a single file per builder or one file with lots of inner builders, this is really the choice of the implementer and I don't think having to choose one way or the other as it relates to fflib itself is required.
2. Chose to be a little verbose on method names in base class (e.g. persistBuilder, registerBuilder, etc.). While these methods are marked public, the intention is that derived classes will always wrap these methods exposing a shorter name (e.g. persist, register). This allows the derived class to cast to its own type (or the specific SObject type) avoiding the caller from having to cast return values all over the place and also allowing for fluent configuration in certain situations (e.g. registerBuilder).
3. fflib_DomainObjectBuilderTest contains samples of how to leverage the library for all 4 "build" methods. Review any of the TestXXXBuilder inner classes for the standard methods that would/should be included in all derived classes.
4. Liberal use of protected & virtual allowing for derived classes to extend as needed

Future Enhancements:

1. Test Data Builder generator similar to Apex Mocks Generator
2. Ability to handle 'updates' of SObject data not just inserts making the pattern more useful in production situations
3. Updates to fflib-apex-samplecode demonstrating pattern (samples already exist in fflib_DomainObjectBuilderTest) - See Samples
4. MavensMate template - See Templates

Feedback encouraged and welcome. Look forward to everyone's thoughts!

[rsoesemann]

@jondavis9898 thanks for giving my idea a new chance of being merged by adressing some of the problems.

I get the rationale behind your changes. Your changes (actually a rewrite ;-) are way to massive to understand them without really digging in for hours ;-).

But..what I can say from looking at the tests for 5 minutes is that the resulting test don't look like I would like to have them. I don't care if I don't understand the implementation anymore (thats why I use a library). But...if the result doesn't make my test more readable and concise and tech free I won't use it. There are TestXYZBuilders and pattern-related names all over the place.

I think some renaming in your classes public interface could probably do the job. Would you mind rewriting my sample tests (https://github.com/up2go-rsoesemann/fflib-apex-common-samplecode/commit/adab15414782cff1ffb914f89d211c5e7b283d2e) with your implementation but sticking to my naming style (hiding all implementation, naming the builders after the result objects,...)?

[jondavis9898]

@up2go-rsoesemann Thanks for reviewing the proposed changes and providing your feedback!

Regarding the tests for fflib_DomainObjectBuilder itself and the TestXYZBuilder naming convention, these are intended to be tests for the Builder itself and not meant to represent what a naming convention in actual use might be. That said, it does demonstrate that the name of the actual "builders" can be anything that the consumer of the builder desires as it doesn't influence the functionality. For example, your example used Opportunity_t while someone else could use OpportunityBuilder, etc. In short, apologize if the naming convention in the test class itself made things more challenging to understand.

I have updated the sample code repo with examples using the naming convention that you chose. I wrote the same test that you did in there as well as several others to demonstrate all the functionality of fflib_DomainObjectBuilder. Please have a look and let me know if this helps clarify usage.

I agree that the naming convention on the public interface of the fflib_DomainObjectBuilder is a little cumbersome. When writing a test, other than the persistBuilders(uow) method, you would never call a method on fflib_DomainObjectBuilder directly. Instead, you would call methods on the derived classes which, as you'll see in the sample, are shorter and easier to follow.

The reason I chose to do it this way is two-fold:

1. As I mentioned in my previous post above, it avoids having to cast the result of the method call whenever you build/persist something within a test. By having the derived classes expose "build()" and call fflib_DomainObjectBuilder's build(Boolean), the derived class build() can cast to the specific SObjectType it needs. If the test code called a build method on fflib_DomainObjectBuilder directly, each call would need to be cast in the test code itself making things much more difficult to follow/read within the test itself.
2. I've added 3 new ways of creating SObjects using the builders but wanted to keep the naming similar - this caused method names to be a little more verbose in the base class itself. As an example, with the ability to create in-memory SObjects and not just DB based, you can use the builder to write a true unit test that doesn't require the database, something that wasn't possible in the original version of fflib_DomainObjectBuilder (see testService_UsingBuildersWithoutDML).

Keeping with trying to make tests as easy to read/follow as possible, I've also implemented an Object Mother pattern to avoid having "new" keywords all over the tests.

In addition to adding the sample code, I've added MavensMate templates at joeferraro/MavensMate-Templates#18. By creating a new DomainObjectBuilderBase and then specific DomainObjectBuilder classes, the boiler plate code really leaves you only to have to add "withXXX" methods to the build classes themselves and then write your tests.

Here's the approach:

1. Create DomainObjectBuilderBase (e.g. Domain_t) deriving from fflib_DomainObjectBuilder
2. Modify createUnitOfWork() on DomainObjectBuilderBase as appropriate (see comments in method from template)
3. Create DomainObjectBuilder (e.g. Account_t, Opportunity_t) deriving from DomainObjectBuilderBase (e.g. Domaint_t) for each SObject you want to create a builder for
4. Add "withXXX" methods to the builders as appropriate
5. Optional: Add additional Object Mother static methods to the builders as appropriate
6. Write your tests and use the following:
   Instance Methods
   build() - Builds an in-memory Existing SObject (Id is NOT null) and returns the SObject cast to the derived SObjectType
   buildNew() - Builds an in-memory New SObject (Id is null) and returns the SObject cast to the derived SObjectType
   persist() - Commits the builder and any of its related data through the default Unit Of Work (obtained via createUnitOfWork) and returns an instance of the SObject cast to the derived SObjectType
   persist(uow) - Commits the builder and any of its related data through the Unit Of Work specified and return an instance of the SObject cast to the derived SObjectType
   register() - Registers the builder for commit when persistBuilders(uow) is called
   Record - Returns the SObject cast to the derived SObjectType
   but() - Creates a clone of the builder instance

Static Methods on Derived Builders
Object Mother Helpers

Static Methods on Base
persistRegistered(uow) - Commits all registered builders and any of their related data through the Unit of Work specified

I'm hopeful that once you review the sample code, you'll see how all this plays out and how clean the actual test itself is. Hope this helps, let me know what you think and any questions you might have. Thanks!

[rsoesemann]

Thanks for clarifiying you concepts.

1. Looks like there are dependancies to ApexMocks and its related classes (e.g. Application). I never bought into applying those concepts in Apex and hope that dicussions like Enterprise Patterns 'Light' #36 will lead to a more lightweight direction.

2. How to a child objects example :: Could you show in one of your examples how to add a child object to a parent? Currently I just see you connecting child to there parent in a method of the ChildBuilder. I need something like this

anAccount().add(Opportunity.new())

[jondavis9898]

@up2go-rsoesemann Thanks for reviewing the samples. To answer your questions:

1. There are no dependencies to Apex Mocks or the Application class within this PR. Apex Mocks is used in the samplecode to demonstrate how you could use the Builder when using Apex Mocks. The "Application" class (which is not part of Apex Mocks) is used in one place within the sample code, however it is completely optional. Since the sample code base itself contains an Application class, createUnitOfWork() uses it. It could very easily not use it and just create an fflib_SObjectUnitOfWork(List) directly. In the MavensMate template you'll see a comment that indicates there are multiple options on how to write createUnitOfWork. In fact, you don't even need to have a createUnitOfWork at all, it's simply a helper method. The consumer could choose not to implement persist() and only expose persist(fflib_ISObjectUnitOfWork) instead in which case createUnitOfWork() wouldn't even be needed.

2. If you wanted to add a child object to a parent, you could add a method to your Account_t called "add(OpportunityBuilder)" and then handle the relationship setting in that method. I've added an example of this to the samplecode. You'll see that the new test testService_UsingBuildersAddingChildToParentWithDML and the original test testService_UsingBuildersWIthDML are virtually the same with the only difference being using "add" on Opportunity_t instead of "withAccount" on OpportunityLineItem__t. In the case of adding an opportunity to an account, it would be something like:

public Account_t add(OpportunityBuilder value) {
    value.withAccount(this);
    return this;
}

Please keep in mind that fflib_DomainObjectBase is simply an abstract base class. How the consumer of the library chooses to use it is up to them. They could name their builder Opportunity_t or OpportunityBuilder, they could use a derived class like Domain_t or choose not to, they could use Application class or choose not to. The samplecode is just one example of how the library could be used, it doesn't require that everyone use it that way. This is one of the reasons why there is test coverage for fflib_DomainObjectBuilder, because there is no right or wrong way to use it. It's up to the consumer of the base class to choose how to use it and up to the base class to provide the functionality to satisfy most (if not all) use cases.

[rsoesemann]

@afawcett How is decided wether a pull request is going to be merged in, rejected or just ignored?

I am just asking as I think all of your and @daveespo's concerns have been addressed by the rewrite of @jondavis9898 The last concern I heard of is the question if Test Data Builders are a broad enough use case in the dev community to make them part of such a widely used library. I would argue no but yes ;-)

No, because the core of the library (SOC and trigger handling logic) is something basically everybody needs in the Salesforce world. But only a few percent will ever need patterns for getting their test code clean.

Yes, because Apex Commons has started to introduce advanced concepts into ist core that are definitely not used by the majority of its user. There are even attempts and discussions to remove it as a dependency from the library.

I'd go with a yes, because users should not care about the size of a library at all. If the library allows me to ignore features like Mocks or Builder why not just have them in there. (If I care about footprint I can even delete them in my org).

The beauty of this library is that it combines concepts that have been around before (even in the Apex space) but now have been compiles in a concise fashion. And by being so powerful they have attracted critical mass of developers and attention to be safe not to be discontinued in the next month.

[afawcett]

Do we need this kind of thing? Ok so i've been reviewing this finally, so sorry for the delay. Firstly i want to progress this for sure! I agree for certain developers that expect this type of facility on the platforms they work on its a great addition. I'm happy and honoured to support the great community around this library to lead the charge as you guys are doing.

ApexMocks parallel? For me this feels much like the time Paul Hardaker and I first discussed introducing ApexMocks. We worried about the bloat and who would really understand it and i'm pleased to see its really taken off. I think the same can be true here for sure!

Some more specific thoughts...

• In or Out this Repo? Given the thoughts already shared about the library getting large, which i also support, i'm torn by potentially compounding this and building debt for when we also get round to splitting up this library vs the fact that this is really an additional two classes.... Robert i hear what your saying, and tbh felt the same way. However as per the discussion on the Enterprise Patterns 'Light' #36 its also about barrier to entry for new devs that might intimidated and ignore the whole library all together and also adoption in none ISV/packaged scenarios john-m and others are talking about.
• Builder Classes There is a lot of boiler plate needed for the builder classes, i think this will inhibit adoption of it tbh. Again we had the same discussions around ApexMocks, hence we ended up with a generator tool, perhaps the same approach here is needed? I also think we need to provide the option to scope the builder classes or at least in the hand written samples, show them as inner classes and not pollute the top level namespace with essentially supporting classes.

Proposal: I think on balance i'm going to recommend we add this fine work into a fflib-apex-testbuilder repo. As far a test builder classes go perhaps contain these in the sample app in a TestBuilders class, hand maintained for now, but then perhaps think about a generator much like ApexMocks.

[jondavis9898]

Thanks for reviewing Andrew, glad to here you feel a builder concept like this has merit.

I'm in full support of a separate repo. I think using the builder pattern is completely optional and doesn't need to be in apex common core. Personally, I'd like to have it in the core library but in my use cases, I'm using everything in fflib so it's nice to just have one place to manage everything from. That said, I think there is very good merit to having thinks like Mocks, Builder and a few others in separate repos since they aren't "core".

Regarding inner classes vs. separate classes, I feel that decision is completely up to the consumer of the pattern/library and the library itself doesn't need to dictate. Demonstrating both options makes sense and provides flexibility. In the test cases for the builder I used inner classes, however in apex sample code, I used separate classes. Along these same lines, I feel that naming convention should also be at the consumers discretion (e.g. Account_t vs. AccountBuilder, etc.).

Regarding a generator, I actually started writing one when I updated this PR but unfortunately, I didn't get very far and finishing it has been put on the backburner do to other priorities. My design calls for a flag that would emit the classes as inner classes or separate so I think that is in-line with your suggestion. The design also called for options for naming pattern, what the "Object Mother" default method should be named, etc. If someone wants to jump in and write the generator, I'd be all for that!

In the meantime, I did update mavensmate templates with templates for the builders (also updated Domain template since it was a little out of date). For those using MM at least, creating a builder is very simple. I agree that there is a lot of boiler plate code (but there's a lot of goodies in there :)). If we can work to complete a generator, I think that will reduce concerns folks would have on the amount of code required to use the builder. In the meantime, the MM templates should hopefully help.

[jondavis9898]

Per discussion in #77, a new repo was created for this. Submitted apex-enterprise-patterns/fflib-apex-common-builder#2 to the new repo.

This issue can be closed.