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

Rough draft of Namespaced Factories #1453

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

zspencer
Copy link

See: #199

I have a number of clients who use FactoryBot and are working through
the difficult challene of decomposing a large monolith into either
domain-driven namespaces; or small gems/engines that are "plugged into"
a larger monolith.

In most cases, we've been able to get away with keeping the factories
bundled with their extracted modules and using distinct names.

However there's also been a bit of copy-pasting of factory code back into the monolith in cases where the names really do need to be shared.

This is an attempt to bring @joshuaclayton's example of namespacing into
the core FactoryBot DSL.

If you'd prefer I packaged this as it's own gem, I'd be happy to do so;
but I'd be stoked to see this in the core FactoryBot package!

See: thoughtbot#199

I have a number of clients who use FactoryBot and are working through
the difficult challene of decomposing a large monolith into either
domain-driven namespaces; or small gems/engines that are "plugged into"
a larger monolith.

In most cases, we've been able to get away with keeping the factories
bundled with their extracted modules and using distinct names.

However there's also been a bit of copy-pasting of factory code back into the monolith in cases where the names really _do_ need to be shared.

This is an attempt to bring @joshuaclayton's example of namespacing into
the core FactoryBot DSL.

If you'd prefer I packaged this as it's own gem, I'd be happy to do so;
but I'd be stoked to see this in the core FactoryBot package!

instance = FactoryBot.build(:category)

expect(instance).to be_kind_of(Category)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you think this is the correct behavior? As opposed to clobbering the existing factory, or raising a DuplicateDefinitionError?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the use case I am currently fiddling with is decomposing a monolith into domain-specific namespaces (and some rails engines); and there's some integration tests that have cross-cutting concerns and also some name collisions.

The integration test suite is currently loading definitions for the following packages:

  1. "Core" (the main monolith being decomposed)
  2. "ProductA"
  3. "ProductB"

Each of these have a User class; with factory definitions named :user; as requiring the developers to use :product_a_user within the ProductA domain was not particularly desired.

If we clobbered the existing :user factory, then existing tests which loaded the :user definition from the ProductA module began to fail due to different default values, as well as the reduced surface area of attributes and traits in the ProductA namespace.

Similarly, if we raised the DuplicateDefinitionError, our integration tests would stop loading definitions once it attempted to load the ProductA :user factory.

As a result, discarding the unprefixed duplicate seemed like the safest way to allow folks working within the ProductA/ProductB namespaces the convenience of referencing create(:user) while still allowing downstream integration test consumers to leverage the :product_a_user and :product_b_user factories.

context "when prefixes are not required" do
it "uses the namespaced factory without the prefix" do
FactoryBot.define do
with_namespace(:Movies, require_prefix: false) do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say more about why having the prefix by default is more helpful than the inverse?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I could go either way, with a slight preference to not include a default so that we don't introduce breaking changes by swapping the default later.

Comment on lines +14 to +18
with_namespace(:Movies) do
factory :category do
name { "Emmy Winners" }
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with_namespace(:Movies) do
factory :category do
name { "Emmy Winners" }
end
end
factory :movies_category, class: "Movies::Category" do
name { "Emmy Winners" }
end

@zspencer Can you expand on what your suggestion gives us that this existing factory_bot syntax does not?

@composerinteralia and I are working to understand what problem this is solving for your use case that's not covered by existing functionality. Thanks!

Copy link
Author

@zspencer zspencer Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, given the following application structure:

core - A Rails Application that serves as the primary monolith
product-a - A Rails Engine that holds the Product A teams code
product-b A Rails Engine that holds the Product B teams code

We use FactoryBot across all three codebases; with the core code-base having a mix of end-to-end integration tests that want to use the Factories defined in product-a and product-b

In core, product-a and product-b all of them have User classes. In core it's simply User, but in product-a it is ProductA::User and in product-b it is ProductB::User.

When writing tests in the product-a/spec/ suite, we want to use FactoryBot.build(:user) instead of FactoryBot.build(:product_a_user). Same in the product-b/spec/ suite.

However, in core we sometimes want to leverage traits defined in product-a/spec/factories/product-a/users.rb. To do so, we have shared the factories with the core code-base. However because most of these factories were defined as factory :user, class: "ProductA::User" this results in a duplicate factory name definition.

To avoid having to go through and rename all the FactoryBot.create(:user) in product-a/spec to FactoryBot.create(:product_a_user) we sprouted the with_namespace method to wrap factories that are shared between product-a/product-b and core.

Now, when in product-b/spec/ we can call FactoryBot.create(:user) and it will use the factories in product-b/spec/factories, and in core we can call FactoryBot.create(:user) and it will use the top-level User class, or FactoryBot.create(:product_b_user) and it will use the factory defined in product-b/spec/factories/users.rb and create a ProductB::User class.

This is really only a useful feature in cases where a group has started to decompose a large monolith; so it may not be worth including in FactoryBot core.

petergoldstein and others added 8 commits January 14, 2022 15:08
This PR adds the Ruby 3.1 and Rails 7 elements to the CI matrix.  To support those changes a number of other changes were required:

- Adding a basic Rails 7 Gemfile and updating `Appraisals`
- Quoting 3.0 in the `.github/workflows/build.yml` file so that this entry pulls Ruby 3.0.x and not Ruby 3.1
- Adding the "--disable-error_highlight" value for RUBYOPT in the build environment to disable Ruby 3.1 error highlighting
- Bumping the version of `standard` and configuring it to run against our oldest supported version
…houghtbot#1522)

Fixes thoughtbot#1444 and thoughtbot#1479

Add a short note in GETTING_STARTED.md about the need to save the record
after it has been modified
What?
=====

This applies standard's formatting fixes across the codebase.
What?
=====

This adjusts the GitHub Actions setup so running `standard`
occurs separately from running the specs via Appraisal.
What?
=====

Within FactoryBot::Evaluator, the build strategy was reported either as
a class OR symbol.

A side-effect of this is misreporting within
ActiveSupport::Notifications hooks, since the instrumentation payload
may have strategies listed as e.g. `:create` or
`FactoryBot::Strategy::Create`, even though they semantically represent
the same information.

This introduces a new instance method for all strategies, `to_sym`,
to be called rather than `class`.
@elliotcm
Copy link

@entcheva I'm an unrelated third party, but is there any progress on this? I looks like this PR is waiting on ThoughtBot's response.

@zspencer
Copy link
Author

@elliotcm - I've been using this in production for about a year and a half now. It's not perfect; but I'd be down to publish it as a standalone gem sometime this week if you'd like.

@elliotcm
Copy link

@elliotcm - I've been using this in production for about a year and a half now. It's not perfect; but I'd be down to publish it as a standalone gem sometime this week if you'd like.

@zspencer That'd be great! Thanks.

@zspencer
Copy link
Author

@elliotcm - https://github.com/zinc-collective/factory_bot_namespaced_factories Here ya go! I just pulled it out of the production application I've been using it in since 2020. We mostly relied on our relatively comprehensive test suite to actually test the gem; but if you want to add better docs / specs / etc. I'd be happy to take patches!

ydah and others added 2 commits June 8, 2022 07:01
* Fix typo "delcaration" -> "declaration"

* Fix typo "Ususually" -> "Usually"

* Fix typo "assocition" -> "association"

* Fix typo "vaue" -> "value"
Fixed CI link. Updated Copyright year
@gap777
Copy link

gap777 commented Sep 14, 2022

@zspencer AGPL? Wouldn't that license prevent me from using this in a closed-source business application?

@zspencer
Copy link
Author

@gap777 - My understanding is that AGPL is safe to use in proprietary applications; so long as you're not distributing it as part of your production deployment. That said, I believe Sidekiq uses the LGPL so maybe I am mixed up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet