RSpec is getting too intimate with my code

rails | rspec | ruby | testing June 20 2007

The theory is that tests are supposed to be agnostic of the implementation. This leads to less brittle tests and actually tests the outcome (or behavior).

With RSpec, I feel like the common approach of completely mocking your models to test your controllers ends up forcing you to look too much into the implementation of your controller.

Here is an example of a spec that is generated by the rspec_scaffold generator:

describe ThingsController, "handling POST /things" do

  before do
    @params = {}
    @thing = mock_model(Thing, :to_param => "1", :save => true)
    Thing.stub!(:new).and_return(@thing)
  end

  def do_post
    post :create, :thing => @params
  end

  it "should create a new thing" do
    Thing.should_receive(:new).with(@params).and_return(@thing)
    do_post
  end

  it "should redirect to the new thing" do
    do_post
    response.should redirect_to(@thing)
  end
end

This by itself is not too bad, but the problem is that it peers too much into the controller to dictate how the model is used. Why does it matter if my controller calls Thing.new? What if my controller decides to take the Thing.create! and rescue route? What if my model has a special initializer method, like Thing.build_with_foo? My spec for behavior should not fail if I change the implementation.

This problem gets even worse when you have nested resources and are creating multiple models per controller. Some of my setup methods end up being 15 or more lines long and VERY fragile.

RSpec’s intention is to completely isolate your controller logic from your models, which sounds good in theory, but almost runs against the grain for an integrated stack like Rails. Especially if you practice the skinny controller/fat model discipline, the amount of logic in the controller becomes very small, and the setup becomes huge.

So what’s a BDD-wannabe to do? Taking a step back, the behavior that I really want to test is not that my controller calls Thing.new, but that given parameters X, it creates a new thing and redirects to it.

Here’s the approach I’ve been taking:

# spec_helper.rb
def valid_thing_attrs(attrs = {})
  {:name => 'Foo'}.merge(attrs)
end

# things_controller_spec.rb
describe ThingsController, "handling POST /things" do

  def do_post
    post :create, :thing => valid_thing_attrs
  end

  it "should create a new thing" do
    lambda { do_post }.should change { Thing.count }.by(1)
  end

  it "should redirect to the new thing" do
    do_post
    response.should redirect_to(@thing)
  end
end

My spec is now testing that posting certain attributes creates a new thing (in the database) and redirects to it. You’ll notice that I have a method called valid_thing_attrs. I don’t remember where I picked up this pattern, but it is something that has allowed me to minimize all of my test’s dependencies on the model.

My spec is now somewhat dependent on my model–and thus will break if my model gets hosed–but that is a small price to pay, in my opinion, for an implementation-agnostic spec with significantly less overhead.

What do you think?

posted by brandon

comments feed

20 comments

Thanks for sharing your idea. I find it very interesting!

Does it only apply to create/remove actions? It’s hard to test that we call the correct find_* method without depending on db fixtures.

I think that one of the reasons that RSpec suggests mocking the Model part is to avoid unnecessary calls to the database. Your solution is great when you often change the way the controllers interact with the Model (without changing the result), but if I don’t do it too often then mocking is also a good option (and results in faster tests).

What kind of setup you need to prepare, that it takes 15 or more lines?

I like the valid_thing_attrs trick :)

Andrzej
June 20, 2007 at 06:16 AM
It’s all about trade-offs.

The fact that AR chooses inheritance rather than delegation puts us in a testing bind – we have to be coupled to the database OR we have to be more intimate with the implementation. We accept this design choice because we reap benefits in expressiveness and DRY-ness.

In grappling with the dilemma, I chose faster tests at the cost of slightly more brittle. You’re choosing less brittle tests at the cost of them running slightly slower. It’s a trade-off either way.

In practice, I run the tests hundreds, if not thousands, of times a day (I use autotest and take very granular steps) and I change whether I use “new” or “create” almost never. Also due to granular steps, new models that appear are quite volatile at first. The valid_thing_attrs approach minimizes the pain from this a bit, but it still means that every new required field means that I have to change valid_thing_attrs.

But if your approach is working for you in practice, then its good! In fact, I’d strongly recommend that you publish a plugin with generators that produce the examples the way you like them. I’m sure that a lot of people would benefit from that.

David Chelimsky
June 20, 2007 at 07:07 AM
Andrzej,

Does it only apply to create/remove actions? It’s hard to test that we call the correct find_* method without depending on db fixtures.

Unfortunately, this approach does mean that I’m back to depending on fixtures. But aren’t mocks just another form of fixtures in most situations? Some good strategies for handling fixtures go a long way toward making your tests more manageable.

I think that one of the reasons that RSpec suggests mocking the Model part is to avoid unnecessary calls to the database.

A lofty goal, indeed. In practice, I’ve never had a rails test suite that was too slow.

Your solution is great when you often change the way the controllers interact with the Model (without changing the result), but if I don’t do it too often then mocking is also a good option (and results in faster tests).

In my experience, I’ve spent a lot more time writing and maintaining (and especially debugging) specs with mocks than I have with fixtures. Any performance hit is worth the benefit of being able to develop faster and have more maintainable tests.

What kind of setup you need to prepare, that it takes 15 or more lines?

Take this line from a controller for example:
```
photo = Product.find(params[:product_id]).photos.build(params[:photo])
```
In that line alone, I need to stub Product.find, mock the product that is returned, mock the photos association, stub photos.build, and mock photo that is returned. That by itself is about 5 lines, and this is one of the most common actions in any app with nested resources. As controllers do more (create multiple photos per controller, or update multiple objects), it only gets more complicated.

In an ideal world, every method would be tested independent of the rest of the application. But there is a point of diminishing returns, where I just can’t justify to my clients why I’m spending so much time on testing. Fixtures, for all their flaws, allow me to work faster, and I still have reasonable confidence that my code does what I say it does.
Brandon
June 20, 2007 at 07:10 AM
I would change this
```
photo = Product.find(params[:product_id]).photos.build(params[:photo])
```
to this
```
photo = Product.build_photo(params[:product_id),params[:photo])
```
Now you have only one Message Expectation (mock) to set and you’ve happily decoupled the controller from the details of how to build photos.
David Chelimsky
June 20, 2007 at 07:46 AM
I’m not sure if I understand correctly, but maybe moving the logic to the Product class (and test it there) would solve the problem of 15-line setup? I’d refactor this:
```
photo = Product.find(params[:product_id]).photos.build(params[:photo])
```
into this:
```
Product.build_photo(params[:product_id], params[:photo])
```
Then, you just mock one method in your controller’s test. That would also fit well with the idea of skinny controller, fat model
Andrzej
June 20, 2007 at 09:12 AM
I feel those pains a lot too Brandon, and I agree with Andrzej. Having a lot of setup code in the tests give the “smell” that the code is trying to do too much.

Unfortunately moving the code into an abstracted version like the Product.build_photo solution doesn’t always work, or you end up having tons of 1 liner methods.

My problems with fixtures isn’t the performance, but the headaches of managing lots of them. If you have a very specific corner case and you need 5 records spread across 3 models, adding additional corner cases can get spidery really quick. Or your tests become overly dependent on the relationships of the fixtures, which for me is harder to maintain than a setup method instantiating the models.

However to alleviate that problem, someone mentioned to me fixture_scenarios . Unfortunately I haven’t had a chance to play with it, but it looks very promising.

Zach Moazeni
June 20, 2007 at 11:25 AM
”...called valid_thing_attrs. I don’t remember where I picked up this pattern…”

Here’s where I picked it up: http://www.lukeredpath.co.uk/2006/8/29/developing-a-rails-model-using-bdd-and-rspec-part-1

Insightful post by the way.

meekish
June 20, 2007 at 11:37 AM
I have come to very similar conclusions. I posted about this on the railsforum.

While I hardly ever use mocks anymore, I still use stubs here and there. One case I use stubbing is to change the behavior of the validations. Instead of having a “valid_thing_attrs” method as you do, I stub out the “valid?” method to return either true/false. All create/save/update methods rely on this “valid?” method so it works really well.

As for fixtures, I went back to relying on them as well. However, the tests don’t truly rely on them, they just use them. I don’t use fixtures to test edge cases – in fact there’s only a couple records in each fixture. If I need to test an edge case then I make the models manually so the fixtures are also very flexible.

Ryan Bates
June 20, 2007 at 11:41 AM
Like Ryan I also stub out the “valid?” method. Another benefit of this trick is that it expresses the behavior more concisely then fixtures do, i.e. it “should create a new thing given valid attributes” (and all you have to do is stub “valid?” and verify a change in count).

Currently I try to avoid fixtures like the plague. Especially with skinny controller design, the behavior at the controller level is very independent of the model layer, and therefore need not know about model specific attributes. This makes all your controller very similar and easily maintainable.

Carl Porth
June 20, 2007 at 05:20 PM
I’ve noticed lately that in my red-green-refactor process, the reds are coming more from mock and stub errors than from code errors, which may speak to my mock/stub abilities as much as the approach, but which is disheartening nonetheless.

zilkey
June 21, 2007 at 04:06 AM
I agree with Andrzej’s comments.

ThingsController isn’t (or shouldn’t be) responsible for creating a Thing. It’s responsibility is to tell Thing to create itself and respond with Thing’s result. Thing’s spec should then thoroughly test Thing’s ability to create itself.

Instead of “should create a new thing”, I would say something like “should tell thing to create a saved instance”, then re-write the examples accordingly.

Andrzej’s Product.build_photo example would make that situation much easier to test, as well as avoid pesky law of demeter violations.

The Product.find(params[:product_id]).photos.build(params[:photo]) line is not only difficult to test ( as was pointed out already ), but makes too many assumptions—product_id must be valid, a product has an array of photos called ‘photos’, that array has a method called ‘build’. A change in any one of those facts breaks that controller unnecessarily.

brasten
June 21, 2007 at 08:35 PM
ThingsController isn’t (or shouldn’t be) responsible for creating a Thing. It’s responsibility is to tell Thing to create itself and respond with Thing’s result.

I agree, which is why it’s important that I don’t dictate in my spec how the controller tells Thing to create itself. If there was only one way to create Thing, this wouldn’t be an issue.

The Product.find(params[:product_id]).photos.build(params[:photo]) line is not only difficult to test ( as was pointed out already ), but makes too many assumptions—product_id must be valid, a product has an array of photos called ‘photos’, that array has a method called ‘build’. A change in any one of those facts breaks that controller unnecessarily.

I would buy that if my controllers were intended to stand alone from my object model, but they’re not! Their sole purpose is to control model objects. If they are using my model objects in a way that is inconsistent with the model, I want to know about it.

The bottom line (as I mentioned in a comment above) is, while it would be a wonderful thing if I could test/spec every method apart from all the other code, there comes a point where the price I pay in time outweights the gains that I get from doing it.

Brandon
June 22, 2007 at 02:15 AM
I would buy that if my controllers were intended to stand alone from my object model, but they’re not! Their sole purpose is to control model objects.

I actually look at it in a different light. The controller isn’t meant to control the model, but in fact to control the HTTP request and response.

Now how much of a hand it has in actually manipulating the model is up for debate (Whether the model should know how to create itself). In the past (mainly as a Java developer) I would have said no, but working with Rails more, it seems to be common design to allow the model to have that control. The Controller does dictate the call, but doesn’t care how it’s handled. Essentially going down the route of Law of Demeter

Recently I’ve been preferring that design more, especially thinking of different entry points into the application. Whether it be from a Controller, the Console, a Rake task, etc.

I do agree that moving the domain knowledge into either the Model or some other class seems to be easier to test. (You just mock the method from the controller), but it is easy to get swept away to the point where every method is a one liner to “pass the buck”.

Zach Moazeni
June 22, 2007 at 02:09 PM
I would buy that if my controllers were intended to stand alone from my object model, but they’re not! Their sole purpose is to control model objects.

Like Moazeni said, controller’s aren’t intended to control your ActiveRecord models. Their job is to handle incoming requests with outgoing responses.

Drew Colthorp, myself and Mark VanHolstyn have been working on a Model Conductor Pattern which is working to alleviate spreading logic around in controllers and creating a clean separation between controllers and domain logic.

In the end you end up with writing better testable code, less fragile tests and cleaner code throughout the app.

Watch the gr-ruby mailing list for when it’s going to be presented out. It should be coming up soon…. http://gr-ruby.org/2006/11/6/mailing-list

Zach Dennis
July 11, 2007 at 12:37 PM
Zach & Zach,

I actually look at it in a different light. The controller isn’t meant to control the model, but in fact to control the HTTP request and response.

Count the number of controller actions in the latest Rails app that you’ve worked on that doesn’t touch the model. (0 for the one I’m working on right now)

I understand that controllers “control” the request and response, but they’re worthless without the ways that they “control” (or call, or manipulate, or tell the model to manipulate…whatever you want to call it) the model. That’s why they exist.

My point is that the dependence on the model is worth the advantages of spending significantly less time testing and still having reasonable confidence that my code does what it is supposed to.

Brandon Keepers
July 11, 2007 at 01:22 PM
It’s like your saying that because a controller is a gateway into the kingdom that it should be in control of the kingdom!

For simple CRUD based (or similar) actions it is easy to throw model calls into your controller and test it quickly. As models need to interact and become more complex you shouldn’t leave the coordination of that complexity in your controller. By doing that you violate Single Responsibility and you couple business decisions with a controller (which should be a gatekeeper, not the staff manager).

The controller needs to know what “domain model” object to talk to, and then in return if things were successful or not. The domain model object may or may not be an ActiveRecord model, that depends on your application’s domain and the complexity of it. But that’s really about it.

Here’s oversimplified example. I have a form where the user can fill out his information and upload a picture. In a happy day scenario you’d typically see the controller saving the user object and then if it saved, saving the picture. The decisions that have to be made our is the user object valid, does it save, is the attachment valid, does it save? You may want to render different information, error messages, etc based on if the user object was valid and saved or if the user object wan’t valid, or if the user saved but the attachment wasn’t valid, or it was valid, but just didn’t save.

Why would you want that in my controller? The controller should take in the parameters and say, “UserModel.create_user_with_picture(params[:user], params[:picture])”. Now depending how far how you like to abstract things perhaps the UserModel returns a errors array, or a hash of invalid objects, or perhaps it just returns a boolean.

You have put all of the business decisions of how to handle the cases in your “UserModel”, hidden away from any other object. If you had left this in the controller then you’d end of sprinkling around business logic between controllers and models and again you violate Single Responsbility.

A real life example from a fellow developer revealed that a project he was consulting on had put the responsibility of controlling model interactions in there controllers. When the customer requested that they wanted to allow some things to be done via the command line it became a large rewrite because everything was coupled to a controller and web requests. This doesn’t happen everyday but making better decisions daily allow these types of requests to be minimal work/effort to implement rather then having to do rewrites of large sections of code.

It doesn’t really take any additional time upfront to keep a clean decoupled design, but it does take discipline. Creating reusable code is an art form.

Zach Dennis
July 18, 2007 at 09:33 PM
Count the number of controller actions in the latest Rails app that you’ve worked on that doesn’t touch the model. (0 for the one I’m working on right now)

I didn’t said that the controller shouldn’t interact with the models. In fact, a Resource-Oriented design would be a little silly if it didn’t touch the database or the filesystem.

There is a big difference between telling the model to do something, and taking the responsibility from the model and doing the action yourself.

In regards to testing, I’ve found that it’s actually easier to test software that passes responsibility. But then again, “easier testing” and “quicker development” aren’t always synonymous.

Zach Moazeni
July 19, 2007 at 08:06 PM
I’m not sure, but I think that I have found a way to test just behaviour in a controller, in isolation and without touching the database. Take a look at this post http://papipo.blogspot.com/2007/08/bdd-isolation-integration.html

Thanks.

Rodrigo Alvarez
August 30, 2007 at 07:56 AM
Back to the Fixtures versus mocking part of this debate. . . Adam Williams and John Long recently released this plugin for Rspec which implements the data “Scenarios” pattern nicely. We’re using it as we re-write all the tests in Radiant in Rspec and it’s making live much better than writing fixtures.

http://agilewebdevelopment.com/plugins/scenarios

Hope that helps someone,

Loren

Loren Johnson
December 24, 2007 at 12:24 AM
What a small world Google presents! Turned this up while looking for a solution to my “invalid options” to #render problem I was having (use RSpec’s trunk btw).

Anyway, I’ll add my two cents regarding mocks. I’m starting to think that any method I mock with `should_receive` specifies the contract that the method I’m testing has with its collaborators.

At least to me this makes sense, especially given duck typing. So for example, the spec has something like ”@car.engine.should_receive(:start)” states that whatever object ”@car.engine” is, it has to have a “start” method.

Now when I release libcar and some fool passes a Duck instance as an engine, they have no argument when they complain the car won’t start. That’s because Ducks quack, while Engines start. I can tell them to look at the spec to find out if their new “engine” will work with Car.

And now when I change my implementation to use something like #hotwire instead of #start, my spec will complain and rightly so. I just broke every client to my library that expects the engine to #start!

Nolan Eakins
May 26, 2008 at 09:33 PM

Speak your mind:

About

I'm Brandon Keepers, a web application developer that likes beautiful code, valid markup and adherence to standards. As a part of Collective Idea in Holland, Michigan, I practice Agile software development primarily using Ruby on Rails.

-86.103171 42.785037

Contact: brandon@opensoul.org

Latest Photos

opensoul.org: