thoughtbot / factory_bot

A library for setting up Ruby objects as test data.
https://thoughtbot.com
MIT License
7.92k stars 2.6k forks source link

Allow specification of default assocations depending on context #1485

Open hschne opened 3 years ago

hschne commented 3 years ago

Problem this feature will solve

FactoryBot allows us to easily define associations that should automatically be constructed when creating or building a model. These associations are statically defined in the Factory.

If I want to change which object is used in an association I can overwrite the association when constructing an object, but I have to do that each time I want to create something. To illustrate, consider these factories:

factory :post do 
  text { 'text' }
end 

factory :comment do
  text { 'comment' }
  association :post
end

It may be fine to automatically construct a new post for every comment most of the time, but there are contexts (e.g. a small set of tests) where I always want to use a specific post. I then have to specify the association manually in each test:

def setup 
 @post = create(:post)
end

test 'first test'  do 
  create(:comment, post: post)
  create(:comment, post: post) 
  ...
end

# Imagine tons of similar tests like that below...

I would like a solution that allows greater flexibility than what is currently there.

Desired solution

TestProf provides a Factory Default helper.

It should be possible to specify a set of associations to be used in a context, e.g. a block or a test class / Rspec context, using a helper method use_factory_defaults (or similar)

The tests above could then be rewritten as:

def setup 
 @post = create(:post)
 use_factory_defaults(post: @post)
end

test 'first test'  do 
  create(:comment) # These then use @post automatically
  create(:comment) 
  ...
end

or alternatively, for a single test:

test 'first test'  do 
  use_factory_defaults(post: create(:post)) do 
    create(:comment) # These then use @post automatically
    create(:comment) 
  end
end

Alternatives considered

It is of course possible to simply use FactoryProf. However, I feel that this specific functionality would work nicely in FactoryBot itself.

As outlined above, create(:comment, post: post) is verbose when many similar models are created.

I have considered that one may be able to hack something together with traits, but I'm not even sure how. I couldn't come up with anything :sweat_smile:

Additional context

TestProf provides additional helpers such as create_default, which are essentially shortcuts to the functionality outlined above.

There are parts of this feature that are strictly RSpec/Minitest specific (e.g. cleaning up defaults after a test suite is done using framework specific hooks). Including those in FactoryBot is not required.

I already have some helpers cobbled together for Minitest (since TestProf only comes with Rspec integration), and I'd be prepared to port the TestProf functionality to FactoryBot if desired :slightly_smiling_face:

aledustet commented 3 years ago

Thanks for your suggestion @hschne we have some suggestions that could help you achieve the same result without the need of a new feature. By reading through the example it seems like a similar result could be achieved by using the {strategy}_list in order to utilize the same post for different comments, so this:


post = create(:post)
create(:comment, post: post)
create(:comment, post: post)

# Could turn into:

create_list(:comment, 2, post: post)

A concern that bubbles up when considering adding something like a default factory value is that it has the potential to introduce mystery guests. This could encourage folk to write tests that are less readable in favor of reducing repetition in the test suite.

If implementing something like this, since factory_bot assigns the default post defined in a factory, how would factory_bot know when to assign your custom value vs the default one that's in the factory?

hschne commented 3 years ago

Thanks a lot for getting back to me!

Using list strategies works well when I want to create exactly the same records a couple of times - which definitely would help in some cases. However, there are other cases where it definitely isn't a satisfying fit.

For example, say I want to create several comments that share the same post, but do not contain the same content:

valid_comment = create(:comment, text: 'some valid text', post: post)
invalid_comment = create(:comment, text: '', post: post)

This is of course a made-up example, but it still illustrates my point. Keeping some associations constant while varying others is something that is currently not well supported.

Another thing that create_list will not be able to cover is, in a similar vein, creating multiple records in, for example, multiple tests. One test may need to create several comments with Post A, and certain contents, and another one may need to once again create several comments with Post A, and other contents.

This could encourage folk to write tests that are less readable in favor of reducing repetition in the test suite.

I wholeheartedly agree with that! This feature may absolutely result in tests becoming less readable if not used with care. But one may argue that this is true for the whole of FactoryBot. It certainly is possible to create unreadable, unmaintainable tests if not using that carefully (e.g. by abusing traits, or creating factories with deeply nested associations that end up creating tons of records you don't know about etc.).

My feeling is that as a whole this would do more good than harm - but I can absolutely accept if you don't share that sentiment and thus would not want to include this in FactoryBot.

how would factory_bot know when to assign your custom value

I'm not an expert on that - I have not reviewed the code in TestProf in detail :sweat_smile:

It seems to me that FactoryProf modifies the existing strategies to either use the associations defined in {strategy}_default or to fall back to the association defined in the factory. See FactoryDefault and StrategyExt.

Extending strategies in such a way would no longer be necessary when modifying the existing Strategies to allow for this feature, I believe?

composerinteralia commented 3 years ago

If you're using Rails, you could probably get some of this by using with_options. For example:

Code example ```rb # frozen_string_literal: true require "bundler/inline" gemfile(true) do source "https://rubygems.org" git_source(:github) { |repo| "https://github.com/#{repo}.git" } gem "activerecord" gem "sqlite3" gem "factory_bot" end require "active_support/core_ext/object/with_options" require "active_record" require "minitest/autorun" require "logger" ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") ActiveRecord::Base.logger = Logger.new(STDOUT) ActiveRecord::Schema.define do create_table :posts, force: true do |t| end create_table :comments, force: true do |t| t.integer :post_id end end class Post < ActiveRecord::Base has_many :comments end class Comment < ActiveRecord::Base belongs_to :post end FactoryBot.define do factory :post factory :comment do post end end class BugTest < Minitest::Test include FactoryBot::Syntax::Methods def test_association_stuff post = create(:post) with_options(post: post) do comment1 = create(:comment) comment2 = create(:comment) assert_equal post, comment1.post assert_equal post, comment2.post end end end ```
hschne commented 3 years ago

@composerinteralia Excellent idea, I did not think of that. Thanks a bunch, this should work nicely for me.

I'll leave this open still (as this issue persists for users without Rails), but feel free to close.