Battle Royale - Testing

Posted by Tammer Saleh

Mar 17

Overmocking makes tests brittle

Let’s look at the controller action from the last post:

1
2
3

def search
  @users = User.find :all, :conditions => ['name like ?', "%#{params[:q]}%"]
end

And the proposed test:

1
2
3
4
5
6
7
8
9
10

def test_should_find_all_users_whos_name_matches_the_given_search_phrase_on_GET_to_search
  users = []
  User.expects(:find).with(:all, :conditions => ['name like ?', '%a%']).returns users

  get :search, :q => 'a'

  assert_equal users, assigns(:users)
  assert_response :success
  assert_template 'search'
end

So what happens if we decide that the call to find should really be a search_by_name method on the User model? We refactor, the controller action becomes:

1
2
3

def search
  @users = User.search_by_name(params[:q])
end

And our tests break. Note that the functionality is the same, and normal tests would have passed to prove it. This means that every time we refactor, we have to change this and every other functional test on search, easily five times the work. This would also happen if we added a call to User.count (for whatever reason). That is seriously unagile.

By now you should have noticed that the test name above is misleading…

Overmocking gives a false sense of security

The test above is named test_should_find_all_users_whos_name_matches_the_given_search_phrase_on_GET_to_search, but that’s not actually what it’s testing. A more accurate name would be test_should_call_User_find_with_all_and_conditions_set_to_name_like_params_q_on_GET_to_search. This isn’t just about accurate naming, but about your expectations on what a test means. If I refactor the search method again:

1
2
3

def search
  @users = User.find :all, :conditions => ['name == ?', "%#{params[:q]}%"]
end

The test would fail, and the fixed test would be:

1
2
3
4
5

def test_should_call_User_find_with_all_and_conditions_set_to_name_equal_params_q_on_GET_to_search
  ...
  User.expects(:find).with(:all, :conditions => ['name == ?', '%a%']).returns users
  ...
end

Now the tests pass, the test does what it espouses to do, and your application is broken (did you catch the bug? ‘==’ isn’t valid SQL). This is they key: Effective tests only care about the user-visible behavior of an action, regardless of how it goes about doing it. This is the essence of modern computer science, and what all that talk in college about “black boxes”, “objects”, “APIs” and “contracts” was trying to show you. As a coworker put it: our tests shouldn’t care whether our method calls find, or produces a fairy princess to do the work for us. Our tests should only care that the work gets done.

Interaction testing is TDD, but backwards

The above tests are examples of what the kids in the know call Interaction Testing.

The problem with overdoing this method of testing is that it encourages you to make assumptions about how your method will work before you write it. The whole point of TDD is to force you to take a step back, pretend you’re a client of your own API, and write only the code necessary for that client. Writing a test that asserts that find is called before you write the method encourages you to use find instead of find_by_id, which is not something the client cares about. This is not the purpose of TDD.

So you’re hatin’ on Interaction Testing?

Absolutely not. Interaction testing is necessary if you don’t have full control of or access to a module your code works with. If you work in a large shop, it’s completely legit to test the interactions between your team’s module and the modules of the team down the hall. It’s the only practical way to keep coding without waiting on them.

Likewise, if you’re application works against Amazon’s API, or does some intense filesystem access, you had better mock that stuff out for your tests. Nobody’s arguing against these points.

So are there problems with rails testing?

Rails testing is worlds easier than in any other framework I’ve dealt with, but of course there are problems.

Speed

rake can be dog slow on a large application. Using in memory sqlite3 databases roughly halves your test times. Also, the rspec folk have a really good idea with their spec server, which loads the rails environment and serves access to tests through DRB. These are much better solutions than sacrificing developer time through brittle and ineffective testing.

Sugar

Rails tests tend to be very wet. We’ve been working on some testing helpers that are addressing that. I’m not quite ready to release it (soon, soon), but here’s a quick peek:

1
2
3
4
5
6
7
8
9
10

class UserTest < Test::Unit::TestCase
  should_require_attributes :username, :hashed_password, :name
  should_require_unique_attributes :username
  should_not_allow_values_for :username, 'b.ad', 'b/ad', 'b ad', 'b#ad', 'b&d', 'b,ad'
  should_ensure_length_in_range :username, 3..10
  should_have_many :ratings
  should_have_many :rated_books, :through => :ratings
  should_belong_to :company
  should_have_have_one :friend
end

We’re hoping to do the same thing to controller tests fairly soon, and we promise to let you (our loyal fans) know when we do.

Comments on this post

ted theodore logan

Mar 17

ted theodore logan said,

Yes, in the previous article it did say interaction based testing is brittle and prevents refactoring. All you did was restate that.

How about this, say we write User#search_by_name. And we make no assumptions about its implementation. In our unit tests, we call User#search_by_name and then run a bunch of assertions on our results (basically regexps against each User’s name). Then in our functional test, for say UsersController#search, that calls User#search_by_name, we run those same assertions on the results. You’ve just tested the same thing twice.

All of a sudden User#search_by_name changes to search only by last name, so you update your unit test, watch it fail, then modify User#search_by_name to search by last_name, run the test, it passes. Yea! Then you run your functional test, uh-oh it breaks because it did the same thing as the unit test. Nice how that User model change rippled out and broke your functional test. That is brittle. This would not have happened if you used interaction based testing and mocked out your User model in your functional test.

I’ve had this same thing happen when I changed 1 attribute in a fixture that’s used in some unit test. All of a sudden the unit test fails and so does the functional because its doing the same thing. Or worse some totally unrelated unit or functional test fails that uses the fixtures. This is a real pain to track down.

The bottom line is that state-based testing and interaction-based testing result in tests that are brittle. You have to make your choice, do you want that ripple effect of state-based or the nice break only one test effect of interaction-based testing?

Dan Kubb

Mar 18

Dan Kubb said,

This is a great post. I tend to mock too far in depth, and I think I should just be testing the surface of my API and try to treat it more like a black box so that my tests aren’t brittle.

I know your testing helpers aren’t finished, but I have a question about the syntax. In your tests I see the following:

should_have_many :rated_books, :through => :ratings

For the purposes of testing does it really matter if rated_books is found through ratings? It might only be better to make sure that User#rated_books can be responded to, returns zero, one or more RatedBook models, and whatever other behaviour the dependent code relies upon.

The fact that the models are found through a join with another model isn’t really important (IMHO) to test. If you were ever to change this, but leave the interface the same, all dependent code shouldn’t break, but the tests will.

I think when tests break but dependent code doesn’t that means the test cases are too brittle.

Tammer Saleh

Mar 18

Tammer Saleh said,

Dan: That’s a great point, and something I’ve been thinking about with those helpers. They are actually more guilty than you know, since they use @user.reflect_on_association to do their work. This means that they already assume RoR associations (which is perhaps forgivable).

Unfortunately, I can’t think of a better way of testing the associations without needing to actually instantiate one (or more) of the records. Once you go there, you have all kinds of validations to worry about, and it becomes very difficult.

Jon Yurek

Mar 18

Jon Yurek said,

Then you run your functional test, uh-oh it breaks because it did the same thing as the unit test.

I’d argue this is a feature, not a bug. If the search doesn’t return what the client (that is, the test) expects it to return, that’s broken. If the search calls #new and #save instead of #create, that’s not broken.

As for the point about having multiple tests break, I would much rather have my tests break when the functions are not doing what they’re supposed to be doing (that is, giving me the correct output for any particular input) rather than having them break when I change how they’re doing what they’re doing.

Dan Kubb

Mar 19

Dan Kubb said,

Tammer: Actually I don’t think that using reflection is such a bad way of testing the associations, as long as the logic is wrapped up nicely and can be updated in one spot should the reflection API in ActiveRecord change at some point. ActiveRecord is pretty well tested, so as long as its API says the association exists I think its reasonable to believe it.

I’ve been thinking about the problem of over-mocking, and I just came across a presentation by Chad Fowler called ‘Quick and Clean: Well-factored Rails’ that presents a few approaches that I think would simplify controller testing. Here’s a summary of his talk as well.

For example, he asserts that you don’t want to have finder methods with conditions/orders in your controller actions such as:


def search
  @users = User.find :all, :conditions => ['name like ?', "%#{params[:q]}%"]
end

Instead he thinks the controller should have no knowledge of how the User.find method works, and that you should wrap it into a finder method like User.search_by_name, as in your example above: 


def search
  @users = User.search_by_name(params[:q])
end

This is also known as the Skinny Controller, Fat Model approach.

If this was used then to test the search controller action, all you’d have to do is mock User.search_by_name in your functional test. You’d still want to test User.search_by_name directly in the unit test, which is sort of just like moving the complexity to a different spot, although I think its moving it to the more correct location.

Sorry, comments are closed for this article.

© 2000 - 2009 by thoughtbot, inc.
written by a bushel of tiny robots