Small Revelation – FactoryGirl, build_stubbed, associations, and let

Not really a problem, but something I didn’t really look out for because all my tests were passing… and hey, if tests are passing, I sleep well.

TL;DR Dumped let{}, removed FactoryGirl associations, back to before(:all) and instance variable/FG.build pattern = super speedy tests.

TL;DR.1 And read the docs.

Background

I need to test a fairly complex permission matrix at the model level. To properly test it, I need to build out several complete company structures, with a multitude of users at various authorization levels. And 200+ actual tests.

Being a fan of rspec’s let{}, I decide to use it copiously. 40 lines worth.

In these tests, I don’t really need to have persisted records, so using FactoryGirl.build_stubbed fits the bill here.

let(:company) { FactoryGirl.build_stubbed(:company)}

… an so on, 39 times.

What I Found

I thought one particular spec was running a little slow. So I converted the FactoryGirl.build to the shiny new FactoryGirl.build_stubbed. No difference in time. So I decided to watch the test.log. Well, I quickly saw what was happening… it was persisting thousands of records.

But wait. I was using FactoryGirl.build_stubbed. Where were all these records coming from?

Problem #1

In most of my FactoryGirl factories, I was using ‘association :area‘, ‘association :store‘, and so on. Didn’t think much about these until yesterday.

Turns out that FactoryGirl will build/create and save those associated factories, even if you are using FactoryGirl.build or FactoryGirl.build_stubbed. Learned something new there. Honestly, I didn’t expect this behavior, but I understand why. Shoulda read the docs.

Now, the easy way around this is to pass into a FactoryGirl.create/build/build_stubbed a nil for the association, if it is not needed. Ala:

FactoryGirl.build_stubbed(:store, company: fg_company, area: nil)

Now it won’t build out the associated area. Alas, I had forgotten just one of these nil associations. And at the worst possible relationship level, the bottom. So every time one factory was built, it create the entire supporting structure above. Thus, every hit to an let(:invoice) builds an entire company structure from one single FactoryGirl.build_stubbed(:invoice) call.

But it get’s worse.

Problem #2

I love the let{}. But to be honest, I never read the docs on it. Well, I did read them yesterday. Relavent line being (emphasis added):

The value will be cached across multiple calls in the same example but not across examples.

Uh-oh. Let{} is like a before(:each). Which is what most specs need. But I don’t, not for this spec. I’m never modifying the AR records, just testing some methods within models, which don’t modify AR instances.

Resulting Big Problem

Ok, not really a problem. But certainly very, very inefficient.

By forgetting to nil an association in a FactoryGirld.build_stubbed, and with let{} recreating an entire company structure, to the database, for every 200+ test. Well, you get the picture. It’s slooooow. 22 seconds worth of slow.

Solution

You know I wouldn’t drag you along this far without a solution.

Just remove all the ‘association :model‘ statements from all FactoryGirl definitions. I know they are handy, but I want CONTROL over my factories. And just one small mistake can make a spec run many X-times longer.
Remove the let{} and replace with the good ol’ instance variable/build pattern.
Move all the instance variables into a before(:all).

before(:all) do

@company = FactoryGirl.build_stubbed(:company)

@store = FactoryGirl.build_stubbed(:store, company: company)

… and so on, 38 times.

end

Note for step #1. It caused me to refactor some other specs as well. This turned out to be a good thing, as I was able to speed up several other specs, and add some clarity to those specs that required building out a company structure.

Results

2.5 seconds. Not too shabby.

After the refactoring for all tests, I dropped ~30 more seconds off the entire suite.

Hope this helps someone else out there improve the speed of their specs too.

6 Responses

Ryan Graham October 1, 2012 at 3:56 pm

I was running into the same problem, but my models have a lot more associations. It seemed wrong to have to create all the dependencies manually, so I dug further.

Inside the factories, I used the after(:stub) callback to add the associations via build_stubbed and after(:build) to add the associations via create. It makes the factories twice as ugly, but it simplifies the tests a, which there are probably more of. It also allows for easier switching between create and build_stubbed, depending on the tests and how far along the migration you are.

Kurt Werle December 5, 2012 at 12:49 pm

That sounds like a nice workaround, @Ryan. Thanks for the idea.

Tamas Herman December 12, 2012 at 11:07 pm

We are facing the same problem: tests run for minutes.
Thanks @Ryan for sharing this idea; it was not obvious to find though.

We have also noticed, it’s quite easy to instantiate an otherwise non-realistic data relationship scenario.

I tried adding code to the after(:build) hook to make the inverse of the associations consistent, but it broke a lot of tests which were already doing it in before hooks + doing “magic” object.reloads here and there.
(think about doing
my_comment = build :comment, article: my_article
this wont ensure
my_article.comments to contain my_comment)

After all this Im curious why isn’t there any definitive examples WITH EXPLANATIONs about when certain FactoryGirl’s strategies should be used.

Q1, Why most examples are heavily relying on #let instead of instance variables?
A1, To make it possible to tweak a complete setup in sub-contexts, so it’s easy to create failure cases for example. ???

Q2, Why “no examples” are using before(:all) for example data setup?
(or after(:all) in case of method call expectations)
A2, The it { should … } syntax seems to be recommended, since it’s quite concise and still reads naturally, but it re-evaluates the subject unconditionally, which is very inefficient in these cases. ???

Q3, Why dynamic attribute definitions are counter advised for associations?

Q4, Why the #association attribute definition function is not inheriting the build_stubbed strategy?

Q5, Why rails-rspec generate scaffold controller spec examples are against stubbing?

# Compared to earlier versions of this generator, there is very limited use of
# stubs and message expectations in this spec.

eg:
describe “PUT update” do
describe “with valid params” do
it “updates the requested xample” do
xample = Xample.create! valid_attributes

Ryan Graham December 13, 2012 at 8:34 am

I haven’t found much use for the build strategy. I’ve used the build_stubbed for views so that they don’t need to use the DB at all. Sadly, I’ve found myself using create most often.

Since people seem to be finding this post when looking for info, here’s a more concrete example of my use of after(:build)/after(:stub)

factory :post do
title # sequence
author # sequence, so the example is shorter
# stuff
after(:build) {|p| p.comments ||= create_list(:comment, 5, post: p) }
after(:stub) {|p| p.comments ||= build_stubbed_list(:comment, 5, post: p}
end

create(:post) # with auto comments
create(:post, comments: some_other_comments) # with comments for a specific test
build_stubbed(:post) # with auto comments and no DB
build_stubbed(:post, comments: []) # a post with no comments, still not touching the DB

Since the DB is used for these, before(:all) totally bones the use of transactional fixtures. Or at least makes you have to tweak the way you write your tests. If you’re creating model instances in a before(:all), you’re basically just creating seed data and defining it closer to your tests. The problem with that is you’re making your test world state dependant on your test execution order. That’s a situation I try hard to avoid. Of course, you could add data cleaning to your before/after :all blocks. I think it comes down to personal preference at that point.

For the re-evaluating subject point, you could solve that with your other question: subject { @some_instance_set_earlier }

Some people like the its() form:
its(:attr1) { should read_like_this }
its(:attr2) { should_not cause_the_instance_to_reeval }

Which I think makes a more compelling argument for subjects and multiple tests on them.

Karl December 13, 2012 at 9:24 am

@ryan
Excellent points.

One caveat about ‘its’… I recall reading somewhere that it will be deprecated and moved to a separate gem?

Ryan Graham December 13, 2012 at 11:30 am

@Karl You’re right, seems I missed the memo!

In fact, here’s the gem alluded to: https://github.com/dnagir/its

Ryan Graham October 1, 2012 at 3:56 pm
I was running into the same problem, but my models have a lot more associations. It seemed wrong to have to create all the dependencies manually, so I dug further.

Inside the factories, I used the after(:stub) callback to add the associations via build_stubbed and after(:build) to add the associations via create. It makes the factories twice as ugly, but it simplifies the tests a, which there are probably more of. It also allows for easier switching between create and build_stubbed, depending on the tests and how far along the migration you are.
Kurt Werle December 5, 2012 at 12:49 pm
That sounds like a nice workaround, @Ryan. Thanks for the idea.
Tamas Herman December 12, 2012 at 11:07 pm
We are facing the same problem: tests run for minutes.
Thanks @Ryan for sharing this idea; it was not obvious to find though.

We have also noticed, it’s quite easy to instantiate an otherwise non-realistic data relationship scenario.

I tried adding code to the after(:build) hook to make the inverse of the associations consistent, but it broke a lot of tests which were already doing it in before hooks + doing “magic” object.reloads here and there.
(think about doing
my_comment = build :comment, article: my_article
this wont ensure
my_article.comments to contain my_comment)

After all this Im curious why isn’t there any definitive examples WITH EXPLANATIONs about when certain FactoryGirl’s strategies should be used.

Q1, Why most examples are heavily relying on #let instead of instance variables?
A1, To make it possible to tweak a complete setup in sub-contexts, so it’s easy to create failure cases for example. ???

Q2, Why “no examples” are using before(:all) for example data setup?
(or after(:all) in case of method call expectations)
A2, The it { should … } syntax seems to be recommended, since it’s quite concise and still reads naturally, but it re-evaluates the subject unconditionally, which is very inefficient in these cases. ???

Q3, Why dynamic attribute definitions are counter advised for associations?

Q4, Why the #association attribute definition function is not inheriting the build_stubbed strategy?

Q5, Why rails-rspec generate scaffold controller spec examples are against stubbing?

# Compared to earlier versions of this generator, there is very limited use of
# stubs and message expectations in this spec.

eg:
describe “PUT update” do
describe “with valid params” do
it “updates the requested xample” do
xample = Xample.create! valid_attributes
Ryan Graham December 13, 2012 at 8:34 am
I haven’t found much use for the build strategy. I’ve used the build_stubbed for views so that they don’t need to use the DB at all. Sadly, I’ve found myself using create most often.

Since people seem to be finding this post when looking for info, here’s a more concrete example of my use of after(:build)/after(:stub)

factory :post do
title # sequence
author # sequence, so the example is shorter
# stuff
after(:build) {|p| p.comments ||= create_list(:comment, 5, post: p) }
after(:stub) {|p| p.comments ||= build_stubbed_list(:comment, 5, post: p}
end

create(:post) # with auto comments
create(:post, comments: some_other_comments) # with comments for a specific test
build_stubbed(:post) # with auto comments and no DB
build_stubbed(:post, comments: []) # a post with no comments, still not touching the DB

Since the DB is used for these, before(:all) totally bones the use of transactional fixtures. Or at least makes you have to tweak the way you write your tests. If you’re creating model instances in a before(:all), you’re basically just creating seed data and defining it closer to your tests. The problem with that is you’re making your test world state dependant on your test execution order. That’s a situation I try hard to avoid. Of course, you could add data cleaning to your before/after :all blocks. I think it comes down to personal preference at that point.

For the re-evaluating subject point, you could solve that with your other question: subject { @some_instance_set_earlier }

Some people like the its() form:
its(:attr1) { should read_like_this }
its(:attr2) { should_not cause_the_instance_to_reeval }

Which I think makes a more compelling argument for subjects and multiple tests on them.
Karl December 13, 2012 at 9:24 am
@ryan
Excellent points.

One caveat about ‘its’… I recall reading somewhere that it will be deprecated and moved to a separate gem?
Ryan Graham December 13, 2012 at 11:30 am
@Karl You’re right, seems I missed the memo!

In fact, here’s the gem alluded to: https://github.com/dnagir/its