When FactoryGirl leads to bad habits

FactoryGirl is a solution for cleaning up the repetitive task of
setting up test data, used in many Rails projects.

I think FactoryGirl encourages bad design by hiding design problems.

The need for repetitive or complex test-data is an indication that your code and its API need improvement. Instead of improving the application, FactoryGirl enables you to only improve this in your test suite: that is not fixing at all, it is hiding the problem.

Building up a state before we can test

We have an app that measures fuel-usage. The feature, or integration test for inserting a
"measurement" could be something like this, when not using FactoryGirl, we can immediately see a problem: (the testing framework is irrelevant here, so I am using RSpec to minimize the noise because it allows me to be concise)

scenario:'secretary on a project with cars adds new fuel-usage entries and sees them'doproject=Project.create(name:'new bridge in sahara',starts_on:Date.today.beginning_of_year,ends_on:Date.today.end_of_year)user=User.create(email:'speedy-joe@example.com',password:'secret',password_confirmation:'secret')user.roles.create(name:'secretary',on:project)Resource.new(type::car,slug:'13-37-42',name:'Joes Truck',project:project)sign_inuser

If you consider that only the last four lines are relevant for this
test,the first block is just noisy set-up. Now, we could refactor this
into a before-block in RSpec. Or into a helper method
create_project_with_secretary_and_car. But that is what FactoryGirl
does for us, with a much nicer interface then such ad-hoc helpers. With FactoryGirl
the test could look like:

scenario:'secretary on a project with cars adds new fuel-usage entries and sees them'douser=create(:user_on_project,role_name:'secretary')create(:car,project:user.project)sign_inuser

The first benefit becomes clear because we see far fewer lines of irrelevant objects being created.
The second benefit of FactoryGirl immediately becomes clear too: we no longer need to provide irrelevant attributes such as the project name.

We have, however not fixed the our application, the implementation is still clumsy; our applications API is still unfriendly. A rake task, an "import"-job, a REST-client still needs to walk through (or consider the state of) all the other records.

If you consider your tests to be one (important) user of your API, a client, the problem is clear: we need to fix the application, not the client using that API.

scenario:'secretary on a project with cars adds new fuel-usage entries and sees them'doresource=Car.new('Joes Truck')project=Project.create(name:'New Bridge',resources:[resource])user=Secretary.create(email:'speedy-joe@example.com',password:'secret',project:project)sign_inuser

We have added code to our application that is used by one of the
clients: the tests. We have abstracted some of the details away: a
client does not need to know how we have implemented roles. For
example: we can use Car to build a resource, which takes care of
creating a slug, and setting other defaults. If, instead we created some
"car" trait in FactoryGirl, that effort was "wasted" on our app. Worse:
we still maintain knowledge about how "resource" deals with cars in our
test suite. That knowledge is merely centralised in a FactoryGirl file,
but still lives in the client: the tests-suite. Again worse: if your
trait happens to be "helpful" and create a Project too, the factories
become unpredictable. I've spent many hours debugging tests only to find
out that some factory "helpfully" created its own relations: leaving me
with "why the hell are there three Projects, all in a different state, when I thought I created only one?"

As with any good API-design, you want your clients to remain as dumb as possible. You certainly don't want a client having to know how projects, roles, users, resources and so on are related to one another.

In the last example, we have improved our application. Introduced useful defaults, additional, useful models all of which are immediately used by at least one
user of the code: the tests.

Other users can start using these Secretary or Car models and the automatic defaults on Projects too, now. They allow us to clean up code that used the clumsy interface: controllers, rake-tasks, async-workers and so on.

Quite often, I've found that by removing the usage of a
FactoryGirl.create in a test, I could improve not only the code under
test, but a lot of other places in the application too: Suddenly you
realize that the "case param[:resource_type]" in your controller can be
completely removed and replaced with a few smart calls to Car.new and
Boat.new and so on. It even, quite often, resulted in improvements of
the UX: by setting sane defaults, instead of presenting the user with a
"starts on is a required field" we save it and set a default. All this
because we learned from our tests that the code had some issues. All
this we would not have learned if instead we cleaned up the tests with
FactoryGirl.

Unit testing

I'd argue that with Unit tests there is even more reason not to use
FactoryGirl. I often go as far as to forbid the usage of FactoryGirl
in unit-tests.

First of all, there is the "unit" part of the unit test: A test for
Role, applies to Role alone. If you, somehow need to store a User with
every Role, in order to test that Role, you have tightly coupled code.
When your Role cannot live without a User (or Vice Versa), you don't have
a Role and a User, but a UserWithRole unit. Since we want
decoupled code (we want decoupled code, don't we?) getting into this
mess is something to avoid. FactoryGirl does not help you to avoid this,
it even encourages this design somewhat:

describeUserdodescribe'#function'doit'includes the name of the role and its project'douser=create(:user,:secretary_on_project)expect(user.name).tobe'Secretary for New Bridge'endendend

classUserdeffunctionroles.map{|r|"#{r.name} for #{r.project.name}"}.join(',')endend

This looks like a nice, clean test. But only because we have hidden away
all the complexity of coming to a state in which the user has a role and
a project and both have a name. And because the test looks nice and clean, we might think we
are done. We are not: One line of code with four issues (and
probably actual bugs):

It cannot deal with a user that has no roles.

It cannot deal with a user with roles which have no project.

It will render strange texts when a role or a project has no name.

The User is dealing with attributes on roles, and even attributes on
that roles' attributes (Law of Demeter).

Sidenote: a method like function should, IMO not be part of the Model,
but is presentation and should either live in the views, or in some
decorator or presentation double UserPrester. For the sake of the
example, let's leave it here, just note that our tests are telling us
this too: we are creating a presentable name in an object that deals
wit details wit database-state: we are violating the Single
Responsibility Principle.

When I recently encountered several of these exact issues in one of my projects' codebase, the developers argued that the
solution was to add more "validation". "Yea, but our models
require a role and each role requires a project. And we are only
rendering stored users, so this should never happen".

It will happen, when you render the unstored role back on the screen when there validation-issues on storing the
user, or its role. Or when deleting a project and not dealing correctly with deleting the associated roles and users.

When you test with a clean, predefined set of test-data, you won't notice all such edge cases. But when you test with the simplest and most extreme edge case: an unstored, empty User, most, if not all, these problems will be avoided; in the design, rather then in validations, delete-hooks, checking-for-nil in views and so on.

If we had tested without FactoryGirl, it would have looked something
like this:

describe'#function'dobefore{@user=User.new}# In RSpec, this is returned when using the method 'subject'context'when user has roles'dobefore{@user.roles=[Struct.new(name:'Secretary')]}it'includes the name of the role'doexpect(@user.function).tobe'Secretary'end

We explicitly have to deal with the situation when a user has no roles.

We see clearly that the "function" is reaching too deep, through roles
into the project.

Because it is made clear, we can now fix that:

describe'#function'docontext'when user has roles'dobefore{subject.roles=[Struct.new(as_function:'Secretary on New Bridge')]}it'includes role as a function'doexpect(subject.function).tobe'Secretary on New Bridge'endendcontext'when user has no roles'doit{expect(subject.function).tobe'no function'}endend

The tests are clean, and explain very well how we deal with edge cases.
Because we have minimized the issues appearing in edge cases and because
we have delegated the methods to the right objects.

We have now clearly improved our code, not just the tests. Bonus: since we no
longer have to store stuff in the database, our unit-test is much faster now.

Sidenote: subject.roles = [Struct.new] or subject.roles =
[double(:role)] is a way of mocking the Role: a User, nor its
unit-test, needs to know how 'Role' exactly works, making that
explicitly clear in a test is often considered a good practice. Yet
mocking and stubbing itself has a lot of downsides too. That is beyond
this post. One important thing, though: this code, when using
ActiveRecord Relations, will not work, since AR does not allow roles
to be assigned anything other then a list of Roles: passing in a
double or Struct will cause an exception. I use mock_model() to fix
this.

Whether you consider a controller a unit and test it as such, or whether
you test it in a more "integration" makes little difference for the
usage of FactoryGirl. A controller tested as
unit, has no
interaction with the database at all: so it really needs no FactoryGirl.
When you test a controller and let it go through the database, that
database needs state. But the above "feature test" examples apply here
even more: if a certain controller action needs a lot of records
in the database before it "can work", the problem lies there, not in
your tests which create all these records.

Don't hide problems, fix them.

FactoryGirl is a solution for hiding problems with your application and
its API. When a test tells you that the API is clumsy, or dirty, instead
of fixing that API, FactoryGirl encourages you to hide this problem.

I'd argue that when some test needs 20+ records created before it can
run, it is far better to leave these 20+ lines of code creating these
records in your tests. At least you then admit and document the problem
you have. Refactoring them with FactoryGirl makes it appear as if there
is no problem with your application code. This is even more true for
unit-tests that use FactoryGirl.

When to use FactoryGirl?

Is it completely useless and should you never use it then? No, it has a
very good usecase: when your API does demand complex state, or lots of
repetitive attributes. When your Order requires a ShippingAddress, it is poor
practice to hide this away in factories. Even more so in the unit-tests
of Order. Instead consider introducing a ShippedOrder model a
'OrderBuilderservice-object or anOrder.create_with_address()` helper
even. Improve the API of your application.

I also want to give a shout-out to the developers of FactoryGirl: I've
used it with great pleasure to replace Rails core -even more clumsy-
fixtures. Please don't read this as an attack on FactoryGirl and its
developers, it are personal observations on using a "test data framework".

But when Address requires you to type your address over and over in your
tests, then FactoryGirl is a nice DSL to put the "generating a street,
city and postal code" in a central place. After all: generating this, is
part of the task of the client, not the API. FactoryGirl is a fancy
solution for creating such data. But since factorygirl is very tightly
coupled to the database and the models that represent that database, it
is way more.
When you need an "address data" in your tests, you could just as well
add a small helper:

About the author: Bèr Kessels is an experienced webdeveloper with a great passion for
technology and Open Source. A golden combination to implement that technology in a good and efficient
way. Follow @berkes on Twitter. Or read more about Bèr.