Tip #24 - Being clever in specs is for dummies

June 25th, 2008

This tip is coming to you from a frustrated developer having to read someone else’s specs….

RSpec is a wonderful tool.

In the right hands, it makes behavior driven development almost natural. It definitely makes producing readable specifications possible and allows you to stop focusing and worrying about writing your specifications, and just get on with life and livingness.

Another wonderful tool is the DRY principle.

The idea of the DRY (Don’t Repeat Yourself) principle is that every datum in your system that you are programming, and every procedure, should be defined and delimited to one location in your code / system structure. This is a wonderful principle that allows you later to make changes to your code with simplicity and aplomb, as you only have to look in location A to find all the code relating to A. You don’t have to look in A, then repeat your change 15 times in different locations in A all to fix one small part of the code.

You know unDRY code, you have all seen it. It’s like someone crossed a big 1980’s BASIC for loop with a code generator and let it loose in your app folder. It isn’t pretty and it is a pain to maintain.

However…

Being crispy DRY is the WRONG tool to apply to writing SPECS.

What do I mean?

Well, consider this (simple) controller example:

1
2
3
4

it "should redirect if we are not logged in" do
  get :index
  response.should redirect_to login_path
end

Simple, this just ensures that the response from the server when we are requesting the index action in the current controller will be a redirect to the login page.

Now, we keep coding and we get:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

it "should redirect if we are not logged in" do
  get :index
  response.should redirect_to login_path
end

it "should let us see the index if we are logged in" do
  session[:logged_in] = true
  session[:user_id] = 99
  get :index
  response.should be_success
end

it "should render the index template if we are logged in" do
  session[:logged_in] = true
  session[:user_id] = 99
  get :index
  response.should render_template('people/index')
end

Now, this is where the DRY-o-meter in your head starts going, “Err… Mikel, are you on drugs or what, why not put that logged in thing into a before call?!”

Why? Well, I think it is the wrong place for it, but to humour you, lets try that:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

describe "when not logged in" do
  it "should redirect if we are not logged in" do
    get :index
    response.should redirect_to login_path
  end
end

describe "when logged in" do
  before(:each) do
    session[:logged_in] = true
    session[:user_id] = 99
  end

  it "should be a success" do
    get :index
    response.should be_success
  end

  it "should show the index template" do
    get :index
    response.should render_template('people/index')
  end
end

OK, now you DRY-gurus can cast an evaluative eye over that and grant it a stamp of approval.. except (you all say) that get :index is a bit of a give away, can’t we put that in the before call as well?

Well….err… yeah… I guess so… lets try that:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22

describe "when not logged in" do
  it "should redirect if we are not logged in" do
    get :index
    response.should redirect_to login_path
  end
end

describe "when logged in" do
  before(:each) do
    session[:logged_in] = true
    session[:user_id] = 99
    get :index
  end

  it "should be a success" do
    response.should be_success
  end

  it "should render the index template" do
    response.should render_template('people/index')
  end
end

THERE! you shout, THAT IS DRY!

And you are right, it is DRY, nothing is repeated.

But you know what? It is also unmaintainable and will be a nightmare in a few months when you look back on it.

Right now, it isn’t a problem, after all, it is only 22 lines of code and you can see everything, but sooner or later, that controller spec could get up to a few hundred lines line, and then you, my dry friend, are up the DRY creek without so much as a paddle. Because when you go to fix the line ‘it should redirect if we are not logged in’, you are going to have to go hunting up through the before call chain (which could be several levels deep) keeping track of instance variables, session state and any other cobweb that has been inserted in there, and find what in blazes is going on.

Of course the above example is simplistic. But I am sure you can agree that looking at a spec that tells you NOTHING about the context or scope it is running in is useful for a computer, and saves some hard drive space, but basically useless for you, and really, that is who matters, the programmer.

Here is a tip:

Before calls are to set up and clean up your testing environment, they are not there to setup up your testing expectations

Valid use of a before call? Making sure some directories that you are about to create in a test suite are not there. Valid use of an after call? Ensuring that the directories you made in the test suite are no longer there.

That is, anything that your program is not designed to handle or is not part of the specification, can go into the before or after calls….

N-O-T-H-I-N-G   E-L-S-E

Get the drift?

It might seem harsh, but believe me, you will thank me when you are 6 months down the track and come back to it…

So, if you can’t use before and after, what are you to do? Well, the simple solution is to explicitly call inline helper methods… lets refactor our code above:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

describe "when not logged in" do
  it "should redirect if we are not logged in" do
    get :index
    response.should redirect_to login_path
  end
end

describe "when logged in" do
  def given_a_logged_in_user
    session[:logged_in] = true
    session[:user_id] = 99
  end

  it "should let be a success" do
    given_a_logged_in_user
    get :index
    response.should be_success
  end

  it "should render the index template" do
    given_a_logged_in_user
    get :index
    response.should render_template('people/index')
  end
end

There.

Now, we have added 3 lines of code and made it much clearer. Why? Because now you don’t have to make the mental note of ‘oh, before this spec runs, I set the session params to log in the user’ instead, it is right there in the spec.

I have taken the given syntax from Story running, I like it. And further, you can just read that code.

When you come back to it in 6 months, you can look at it and instantly see what is going on, with NOTHING left out.

Anyway, end of rant… I’m off to refactor some 4 deep nested before calls….

blogLater

Mikel

Posted by mikel Filed in RSpec, Ruby on Rails, Ruby on Rails Tips

11 Responses to “Tip #24 - Being clever in specs is for dummies”

  1. Brandon Wright Says:
    June 25th, 2008 at 06:14 AM

    I love it! The given_a_blank concept is fantastic and adds a tremendous amount of clarity. Also, your point about the things in the before and after blocks not being part of the spec is something that I’ve never seen in all my Rspec research (I just started using it a little while ago). I’m integrating this into my specs starting immediately.

  2. Zach Dennis Says:
    June 27th, 2008 at 08:21 AM

    Good post. One qualm though. Having a describe that reads “when logged in” and then removing the “before” in favor of a inline helper method named “given_a_logged_in_user” doesn’t really add any value to the test.

    I already know the context I’m testing is “when a user is logged in”—the describe told me that. By setting up the session data to reflect a logged in user I am setting up the context (environment) I’m going to be running the following examples in.

    I agree with you that you should not have test expectations setup in your before blocks, but setting up the environment to act appropriately for what you are testing is not setting test expectations, it’s setting up the test environment.

    I don’t think using inline helper methods is wrong (in fact I think they are great) and I use them myself, but the example and argument you gave in the post doesn’t exemplify when to use them, and when not go. Perhaps a better example is needed. This example seems to just be an even swap of the “before” in favor of “given_a_logged_in_user”. If the describe didn’t already tell me the context I expected to be in then I’d buy it, but now you’ve introduced two things to tell me what context I’m in.

    However, I completely agree with moving the “get :index” stuff out of the before. It makes it difficult to update/change the object under test later if you need to introduce mocking. I’d rather extract a inline helper method “get_index” and have each example call that.

    Thanks for posting!

  3. Mikel Says:
    June 27th, 2008 at 10:18 AM

    @Zach – thanks for your comments. I guess for me, I still prefer to have that mental nudge in the spec itself. Even if it means I am just replacing a before block, I don’t need to check if there is a before block or not.

    It is a totally moot point on an example that is only 20 lines long… But when you get to specs several hundred lines long, it is easy to loose sight of what describe block you are in as you are drilling down.

    Anyway, YMMV. I might do another post showing different examples.

    Mikel

  4. Bryan Ray Says:
    June 29th, 2008 at 04:45 AM

    Excellent post.

    I’m a big fan of not being overly DRY in specs and inline helper methods. This is the first post I’d seen that described it so eloquently.

    Much appreciated.

  5. Alex C Says:
    June 30th, 2008 at 07:14 AM

    Love the advice, and it nicely articulates something I’ve been teaching for years.

    But dude. Please. Lighten up on the caps.

    “Before calls are to set up and clean up your testing environment, they are not there to setup up your testing expectations,” sounds much more convincing.

  6. mikel Says:
    June 30th, 2008 at 09:59 AM

    @alex: Heh… I was pretty frustrated when I wrote it, I guess it came through :) I lightened it up some :)

  7. Aaron Jensen Says:
    July 2nd, 2008 at 02:22 AM

    I’m afraid I disagree: http://codebetter.com/blogs/aaron.jensen/archive/2008/07/01/don-t-take-ry-repeat-yourself-in-specs-too-far.aspx

  8. Dan Manges Says:
    July 5th, 2008 at 01:57 PM

    +1. If you’re doing the same thing a few times, it’s not a problem to duplicate it. If you’re doing the same thing more than a few times, it’s likely a smell about the design and removing the duplication in the tests doesn’t help.

  9. Zach Dennis Says:
    July 9th, 2008 at 06:32 AM

    Mikel,

    Ever since I posted my long winded comment I’ve been much more proactive in using inline helpers over before blocks when my set of examples for a given context isn’t absolutely tiny and readability is increased. I must admit that the added readability is very nice and contagious. I am enjoying updating some examples to increase readability where it helps. Thanks again for sharing,

  10. Mikel Says:
    July 9th, 2008 at 02:38 PM

    Heya Zach,

    That’s great to hear!

    Thanks for updating the comments too! Much appreciated that you are letting others know how you have found it to help.

    Mikel

  11. Ichsan Says:
    August 25th, 2008 at 02:56 AM

    Mikel, nice post. But I think, we can use ‘before(:each)’ to setup the context. So I totally disagree with one of your snippets:

    describe “when logged in” do before(:each) do session[:logged_in] = true session[:user_id] = 99 get :index end ...

    get :index here is misplaced. The context was clearly “when logged in”. So I prefer to take out ‘get :index’ because it is not related to the context.

    Personally, I like don’t like to use ‘describe’ inside ‘describe’ because this often makes me confused and makes my specs unmaintainable.

Leave a Reply