Code emphasis for tests that teach

Posted in testing, TDD | Sunday, November 25th, 2007 | Trackback

In product code, not repeating yourself is almost always a good idea. In tests, it’s not so clear-cut. Repeating yourself has the same maintenance dangers as it does for code, but not repeating yourself has two additional downsides:

  • A common knock against well-factored object-oriented code is that no object does anything; they all just forward work on to other objects. That structure turns out to be useful once you’re immersed in the system, but it does make systems harder to learn.

    One purpose of tests is to explain the code to the novice. Remove duplication too aggressively, and the tests do a poor job of that.

  • Another purpose of tests is to make yourself think. One way to do that is to force yourself to enumerate possibilities and ask “What should happen in this case?” That’s one of the reasons that I, when acting as a tester, will turn a state diagram into a state table. A state diagram doesn’t make it easy to see whether you’ve considered the effect of each possible event in each possible state; a state table does. (It’s not as simple as that, though: it’s hard to stay focused as you work through a lot of identical cases looking for the one that’s really different. It’s like the old joke that ends “1 is a prime, 3 is a prime, 5 is a prime, 7 is a prime, 9 is a prime…”)

    If you factor three distinct assertions into a single master assertion, it’s easy to overlook that the second shouldn’t apply in some particular case. When you factor three distinct setup steps into one method, you can more easily fail to ask what should happen when the second setup step is left out.

So as I balance the different forces, I find myself writing test code like this:

  # Guard against manufactured URLs.
  def test_cannot_update_a_user_unless_logged_in
    new_profile = NewProfile.for(’quentin‘).is_entirely_different_than(users(:quentin))

    put :update,
        {:id => users(:quentin).login, :user => new_profile.contents}
        # Nothing in session
    assert_redirected_to(home_path)
    assert_hackery_notice_delivered
    new_profile.assert_not_installed
  end

  def test_cannot_update_a_user_other_than_self
    new_profile = NewProfile.for(’quentin‘).is_entirely_different_than(users(:quentin))

    put :update,
        {:id => users(:quentin).login, :user => new_profile.contents},
        {:user => users(:aaron).id}
    assert_redirected_to(home_path)
    assert_hackery_notice_delivered
    new_profile.assert_not_installed
  end

There’s duplication there. In an earlier version, I’d in fact reflexively factored it out, but then decided to put it back. I think the tests are better for that, and I’m willing to take the maintenance hit.

Nevertheless, there’s a problem. It’s not obvious enough what’s different about the two tests. What to do about that?

Consider explaining the evolution of a program over time in a book. Authors don’t usually show a minimal difference between before and after versions. Instead, they show both versions with a fair amount of context, somehow highlighting the differences. (When I write, I tend to bold changed words.) I wish I could highlight what’s special about each test in my IDE, so that it would look like this:

  # Guard against manufactured URLs.
  def test_cannot_update_a_user_unless_logged_in
    new_profile = NewProfile.for(’quentin‘).is_entirely_different_than(users(:quentin))

    put :update,
        {:id => users(:quentin).login, :user => new_profile.contents}
        # Nothing in session
    assert_redirected_to(home_path)
    assert_hackery_notice_delivered
    new_profile.assert_not_installed
  end

  def test_cannot_update_a_user_other_than_self
    new_profile = NewProfile.for(’quentin‘).is_entirely_different_than(users(:quentin))

    put :update,
        {:id => users(:quentin).login, :user => new_profile.contents},
        {:user => users(:aaron).id}
    assert_redirected_to(home_path)
    assert_hackery_notice_delivered
    new_profile.assert_not_installed
  end

Something for IDE writers to implement.

5 Responses to “Code emphasis for tests that teach”

  1. David Peterson Says:
    November 26th, 2007 at 6:49 am

    I’m not keen on this kind of duplication even in test code. How about defining some helper methods with names like these…

    def when_not_logged_in
    def when_logged_in_as_aaron
    def attempt_update_of_quentins_details
    def assert_hacking_attempt_is_handled

    David

  2. Jim Kingdon Says:
    November 26th, 2007 at 9:54 pm

    It is one of those balancing things, but my favorite tests look roughly like Brian Marick’s in terms of how much duplication they have. I just haven’t found that modest amounts of duplication in tests has the same costs as in code. For example, if there are two copies of a piece of code, even if it is just a line or two, bugs tend to lurk, because the two aren’t really quite identical, or because a future fix is made to only one, or because the duplication is a symptom of not really having thought out what two business processes have in common (for example, is your domain language missing a term?). I could probably make up a contrived example of test duplication making it harder to see what the functionality of the system is, but I don’t remember it happening (other than the obvious cases of tests getting too long and being unreadable for that reason).

    A good test will read something like: “do A, B, and C. assert that X and Y happened”. If everything is factored out in the name of removing duplication, there might be a lot of browsing of test code to find the A, B, C, X, and Y.

  3. Brian Marick Says:
    November 27th, 2007 at 8:43 am

    David: Normally, I’m inclined to hide things like put :update, {:id => blah blah blah} behind methods like when_not_logged_in. These particular tests are Rails “functional” tests of a RESTy interface, which means they’re about what kind of HTTP gets sent in and what happens in response. Being more specific seems appropriate.

    In my graphical business-facing tests, I do use more abstract language like “Sasha is still not logged in.”

    I originally had a method named something like assert_hacking_attempt_is_handled but split it into three parts because I wanted to be clear about what it meant for a hacking attempt to be handled and make myself think about whether each of the three parts applied to each situation. For example, you can do more when you notice one logged-in user is trying to edit someone else. Unlike a not-logged-in user, you have an email address. Thinking thoughts like that is harder when you bundle all the responses behind hacking_attempt_is_handled.

  4. Chris McMahon Says:
    November 27th, 2007 at 11:34 am

    I’ve just started some black-box tests for a REST interface and I find myself resisting the urge to refactor the HTTP calls to functions. Here is some real code, at least as of right now:

    def page_delete_json
    req = Net::HTTP::Delete.new(@@page_loc, initheader = {’Content-Type’ => ‘text/x.socialtext-wiki’})
    req.basic_auth @@user, @@pass
    response = Net::HTTP.new(@@host, @@port).start {|http| http.request(req) }
    assert_equal(’204′, response.code)
    assert_equal(’No Content’, response.message)
    end

    def page_put_json

    req = Net::HTTP::Put.new(@@page_loc, initheader = {’Content-Type’ => ‘text/x.socialtext-wiki’})
    req.basic_auth @@user, @@pass
    req.body = “test get page json”
    response = Net::HTTP.new(@@host, @@port).start {|http| http.request(req) }
    assert_equal(’201′, response.code)
    assert_equal(’Created’, response.message)
    end

    def test_page_get_json
    puts “starting test get page json”
    page_delete_json
    page_put_json

    req = Net::HTTP::Get.new(@@page_loc,initheader = {’Accept’ => ‘application/json’})
    req.basic_auth @@user, @@pass
    response = Net::HTTP.new(@@host, @@port).start {|http| http.request(req) }
    parsed = JSON.parse(response.body)
    assert_equal(parsed.keys, [”last_editor”, “name”, “uri”,”revision_id”, “last_edit_time”,”tags”,”modified_time”,”revision_count”,”page_id”,”page_uri”])
    assert_equal([’devnull1@socialtext.com’],parsed.values_at(”last_editor”))
    assert_equal([’TestGetPageJSON’],parsed.values_at(”name”))
    assert_equal([’testgetpagejson’],parsed.values_at(”uri”))
    assert_equal([’testgetpagejson’],parsed.values_at(”page_id”))
    end

    For one thing, if I move the HTTP calls out to their own functions, I’m only going to save one or two LOC per method. For another thing, my Accept and Content-Type headers can be any one of about four values, and calling the right function with the right headers looks from here like more trouble than it’s worth. If I keep all the guts exposed, it’s a heck of a lot easier to tinker with data types and HTTP functions as I go along.

    When I get more tests, I might change my mind, but for now I like having all the wiring exposed where everyone can see it.

  5. Agile Software Development Says:
    December 2nd, 2007 at 12:57 pm

    Weekly Agile Reading. Pack 7…

    Manually filtered top of the most interesting writing published since last Saturday. Of course, most interesting from my personal point of view….

Leave a Reply

You must be logged in to post a comment.