n.times { code! }

Thursday, August 30, 2007

BDD, isolation, integration

I have been reading a lot of posts about skinny controllers, isolation and good behaviour-driven development. Let me explain from the beginning:

The typical approach to spec a 'create' action looks like this (Taken from rspec documentation):


describe PeopleController do
it "with a valid person should redirect to index on successful POST to create" do
@person.should_receive(:new_record?).and_return(false)
Person.should_receive(:create).with({"name" => 'Aslak'}).and_return(@person)

post 'create', {:person => {:name => 'Aslak'}}

response.should redirect_to(:action => 'index')
end
end


That is too tied to the implementation. What if I want to use the 'create' method in my controller? And what about assigning attributes in a block?
What you are spec-ing here, is how your controller should look, not how it should behave.

Then I saw this in one of the blogs that I linked above:

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


This is really testing behaviour. The main problem with this (although I really liked it the first time I saw it) is, of course, that interacts with the database and the models. I don't want to hit the database from my controller specs at all, I want full isolation.

Add a pair of helpers in spec_helper.rb:

def mock_valid_model(klass)
mock_model(klass, :save => true, :save! => true)
end

def mock_invalid_model(klass)
m = mock_model(klass, :save => false)
m.stub!(:save!).and_raise(ActiveRecord::RecordNotSaved)
m
end

Then stub Model.new to return the mock you want. Spec your controller 'create' action in order to redirect in case that the mock is valid, and to render a template if it is not valid (of course you can add specs for flash or whatever):


describe ThingsController, '"create" a valid model by POST' do
before do
@thing = mock_valid_model(Thing)
Thing.stub!(:new).and_return(@thing)
end

it 'should redirect to model show' do
post :create
response.should redirect_to(thing_url(@thing))
end
end

describe ThingsController, '"create" an invalid model by POST' do
before do
@thing = mock_invalid_model(Thing)
Thing.stub!(:new).and_return(@thing)
end

it 'should render "create" template' do
post :create
response.should render_template('things/create')
end
end

There are various interesting things here:
  1. You don't need to keep adding expectations to your mocks. Expectations couple your specs with the implementation. With my approach you can use create, new + save, new + block assignement, whatever. And the spec still passes.
  2. The database isn't touched at all. That's good stuff, I guess.
  3. You are really testing the behaviour. You know that the only way your controller will know if it should redirect or render, is to create a new instance of Thing, and call save or save!. That is actually an implied expectation, but without the need of should_receive(), just stubs.
Some of you may have noticed that I don't need to use the params() hash in the controller in order to make the specs pass. Right. But you know, that your controller is creating a new instance of Thing, and calling save, save!, create or whatever. That's when integration/acceptance tests come in

The only moment that you want your controller mess with params, is when you are not isolating. Because, if you are isolating, you can't validate parameters, since you stubbed save and save!.

With this approach, you keep your specs to a minimum amount of lines, you isolate, you test just behaviour.

Anyway, let me know of any issues you find with this.

Thanks for reading and sorry about the syntax non-highlighting.

20 comments:

Mike Vincent said...

'Ver nice Papipo. I've been doing a lot of expectation setup in my controller specs and have noticed more than a few times that it bites back. Particularly when changing things like changing includes on finds, etc..

When I do start to get nibbled by things like that I try to look at it as an opportunity to encapsulate the finder into a wrapper method instead, sometimes that seems reasonable.

Papipo said...

I agree with the custom methods in models. If you read the posts about skinny controllers, that is the advice they give.

In order to relax expectations and stubs in controller specs, you keep your controller lines to a minimum, usually moving a lot of logic to a custom method in the model.

Anonymous said...

Very reasonable approach that balances goals well. You probably want to also stub :create, :create! and valid? in both the valid and invalid versions - that would allow you to freely change among any of the Rails methods for making new objects.

I wonder if stubbing new on the class object would be good too. Then you can just create mock model (valid or invalid) and your off to the races. I'm just sure if that crosses the line of too much being implicit. WDYT?

Anonymous said...

ps - I left out the fact that some people prefer the create/create! approach. Stubbing those methods as well would give you a lot more flexibility.

Papipo said...

Not sure about stubbing Class' new(), maybe it's too intrusive. We could test it, anyway :).

As you pointed out, I can update the helpers:


def mock_valid_model(klass)
  mock_model(klass, :save => true, :save! => true, :valid? => true)
end

def mock_invalid_model(klass)
  m = mock_model(klass, :save => false, :valid? => false)
  m.stub!(:save!).and_raise(ActiveRecord::RecordNotSaved)
  m
end


But you don't need to stub create nor create!, since those methods call new() internally.

I have just test it, and specs pass:

def create
   @thing = Thing.create
   if @thing.valid?
     redirect_to thing_url(@thing)
   end
end


and this way too:

def create
   @thing = Thing.new
   if @thing.save
     redirect_to thing_url(@thing)
   end
end



Thanks for your input.

Anonymous said...

Actually, reviewing your original post, there is something missing. In the second example, if the action never creates a new Thing, there is no failure. You have can simply render 'things/create' and never interact with the model and the spec will pass. So I think that we need one of two things:

Either the mock framework should support an 'or' expectation like:

mock.should_receive(:save).
and_return(false).
or_receive(:save!).
and_raise(ActiveRecord::RecordNotSaved)


or at the very least the specs should use Thing.should_receive(:new) rather than Thing.stub!(:new).

WDYT?

Papipo said...

That's right, but it's the "interaction" between the two examples (the valid model and the invalid one), that makes you sure that the controller WILL call both new and save/save!, since it's the only way that both specs will pass (it's the only way for the controller to know if it must render or redirect). And this is the main point of my post, I guess.

Apart from that, I like the alternative expectation, it expresses my intention better than stubbing both save and save! (Although stubbing is enough here).

As you say, I could add an expectation on new(), but as I have just said, the "interaction" between opposite examples assures that new() will be called anyway. It's an implied expectation.

Anonymous said...

Well, actually, neither would fail if the action never called new (not just the second one). I appreciate what you're describing - that the combination of the individual examples tells the whole story - but that part of the story is never specified here.

If it were me, I'd add either a specific expectation in the first example that new is called or possibly a post-action state-based expectation in both descriptions that @thing gets assigned to the view. The only way those can pass is if the implementation actually interacts with the model.

WDYT?

Anonymous said...

Oh - I see what you're saying now about the call to new being the only way for the two examples to pass/fail differently. Personally, I think that's too much work to have to do that analysis. I'd rather make it more explicit. I think that what I proposed in my last comment would be a good compromise. You?

Papipo said...

You got it ;)

I understand that being in the habit of using expectations makes my approach too "light". It seems that I'm specifying not very much, and giving then too much freedom to the implementation, but in reality that was my intention.

If you still don't check the assignation of the model instance to a variable, you are still able to expect the interaction due to what you said in your last comment.

The problem here is that explicitness forces you to spec implementation instead of behaviour (at least, partially). In my example, you can use whatever variable name you like. I think that which variable name will be used is still something that should be checked on integration/acceptance, since it's about the interaction with the view. Being more explicit assures you that the things are going fine, but forces the implementation in some ways.

Anyway, I'm starting to test this approach, and I know that it assumes, maybe, too many things.

About the analysis part, I can't agree with you more :). I was like 3 days thinking about a way to test in real isolation, and this was the result.

Thanks for your input.

Anonymous said...

This is a nice approach. How is it working out for you papipo? How do you go about testing the corresponding views (if you test them in isolation)?

Saludos desde Sevilla

Papipo said...

You should spec the assignment of the model instance to a variable in the controller side.

The views should make intensive use of custom helpers. That way you can spec the helpers in isolation, and keep views to a minimum of ruby code.

You should avoid using more than one instance variable from the controller in the views.

It's preferable that you create models that don't use the database, since those are as spec-able in isolation as helpers.

Papipo said...

I forgot to write about the most important part :).

Once your views are skinny, you can spec what you want: there should be a form tag, there should be a given user name, etc

Views are not easily unit tested, at least not as much as models or helpers are.

The real way to test views is in acceptance tests.

Anonymous said...

Il semble que vous soyez un expert dans ce domaine, vos remarques sont tres interessantes, merci.

- Daniel

Anonymous said...

I THINK i was one of the test subjects, and boy did i like the probing!!!

Anonymous said...

I have to state, you chose your words well. The ideas you wrote on your encounters are well placed. This is an incredible blog!

Anonymous said...

Hi, I really like the theme. I want to add a banner to the top right-hand space for an advertiser… how would I do that?

Anonymous said...

It was a really nice thought! Just wanna say thank you for the selective information you have diffused. Just continue writing this kind of post. I will be your loyal reader. Gives Thanks over again.

Anonymous said...

Truly inspiring, touching and moving true stories. Every human being is a bunch of different life stories. Let us keep knowing and learning.

Anonymous said...

Good luck getting people behind this one. Though you make some VERY fascinating points, youre going to have to do more than bring up a few things that may be different than what weve already heard. What are trying to say here? What do you want us to think? It seems like you cant really get behind a unique thought. Anyway, thats just my opinion.