Real Testing Example, Part Two

What with upward of two people saying nice things about last week’s post, I’ve decided to keep going with part two of a look at some real testing code.

Most code-heavy tutorials show the code but not the tests — I’m doing the opposite here and showing the tests, but not much of the code. Also, although I’m presenting these tests in chunks, you should realize that there was a lot of back-and-forth from Cucumber to tests to code and some backtracking, most of which I’ll spare you from having to wade through.

At the end of last week, I had run through the tests for spam-prevention code which worked by limiting the rate at which a user could send messages to other users of a particular social networking site. Cucumber was involved, and I think I went off on a tangent about writing lots of tests.

Once the basic rate limiting code was in place, the client and I came up with a couple of special cases. The application allows a user to explicitly reply to a specific message sent to them. We agreed that these replies should not count towards the rate limit on the grounds that a reply was, pretty much by definition, not spam. Similarly, we decided that any message between users with a friendly relationship on the site also doesn’t count toward the rate limit. More subtly, we decided that if a user was inadvertently blocked, then reinstated by an administrator, that their rate count should drop back to zero, so the user doesn’t just get immediately re-blocked.

The Cucumber tests:

Scenario: Reply behavior
  Given I am a user who is not a new member
  When I send 4 messages in a day
  And I reply to a message
  And I send 1 message in a day
  Then 6 messages are sent
  And I am not blocked from sending further messages
  And the administrator does not get an email

Scenario: Friend Behavior
  Given I am a user who is not a new member
  When I send 4 messages in a day
  And I send a message to a friend
  And I send 1 message in a day
  Then 6 messages are sent
  And I am not blocked from sending further messages
  And the administrator does not get an email

Scenario: Unblocked behavior
  Given I am a user who is not a new member
  When I send 6 messages in a day
  Then I am blocked from sending further messages
  When I am unblocked by an administrator
  And I send 5 messages in a day
  Then 5 messages are sent
  And I am not blocked from sending further messages
  And the administrator does not get an email

These tests are quite similar in structure to the tests I started with last week. I might start thinking about combining some of the steps, but in Cucumber, I bias in favor of having the actual test be as clear and readable as I can get it. I don’t think these scenarios are so complicated that they require simplification.

Most of these steps have already been defined, here are a couple that aren’t. The first two steps are slight variants on the message sending step definition:

When /^I reply to a message$/ do
  original_message = Factory.create(:message, :sender => @recipient,
      :recipient => @sender)
  message = Factory.attributes_for(:message, :sender => @user)
  message[:in_reply_to] = original_message.id
  visit(messages_path,
      :post, {:recipient => @recipient.id, :message => message})
end

When /^I send a message to a friend$/ do
  @user.become_friends_with(@recipient)
  visit(messages_path,
      :post,
      {:recipient => @recipient.id,
          :message => Factory.attributes_for(:message, :sender => @user)})
end

One thing that you may have noticed about these step definitions. Despite all my jumping up and down about Cucumber being a black-box system, both of these steps bypass the UI to complete the setup. The first step creates a factory message and the second step creates a friend relationship. In both cases, the actual message send being tested passes through the UI.

There are basically two reasons for bypassing the UI in the setup. One is, of course, that it’s the setup and not the action being tested, the second is that being a purist all the time is a pain, and it’s much easier to get the background info out of the way directly.

The final missing step, however, is completely through the UI — the step logs the user out, logs in as an administrator, navigates to the page, unblocks the user, and then logs the administrator out and the original user back in.

When /^I am unblocked by an administrator$/ do
  @blocked = @user
  visit "/logout"
  Given "I am a logged in administrator"
  visit path_to("the admin messaging page")
  click_link("Unblock")
  @user = @blocked
  visit "/logout"
  Given "I am logged in"
  reset_mailer
end

A little on the Rube Goldberg side, perhaps, but it works.

With the Cucumber step definitions in place, the question becomes what tests and code need to be written to make these pass. For the first two scenarios that adjust the definition of what makes a message count toward the rate limit, the code change will be in the User and Message models. Since it’s a thin controller that defers to the models for data, there shouldn’t be any code change in the controller at all.

Now, that’s a much easier determination to make in hindsight than up front, and as it happens I did write controller tests for both of those scenarios, largely because they were so similar to the controller context chunks in last week’s post that it only took about five minutes to set them both up.

The real test action was in the user and message models. I added a database column for messages called counts_toward_spam. The idea is that this would be true for most messages, but false for replies, friend messages, or messages otherwise cleared by the admins. This requires some unit tests in the message class to support it. Again, I’m using Matchy here, and testing the boolean spam_message?, which is basically an alias for counts_toward_spam that I added for readability. In retrospect, adding the method was probably unnecessary.

context "spam count" do

  setup do
    @sender = Factory.create(:user)
    @recipient = Factory.create(:user)
  end

  should "give an ordinary message a spam count" do
    @message = Message.new_from_params(
        {:subject => "fred", :body => "body"}, @sender, @recipient)
    @message.sender_id.should == @sender.id
    @message.recipient_id.should == @recipient.id
    @message.should be_spam_message
  end

  should "not give an ordinary message a spam count if it is a reply" do
    @message = @message = Message.new_from_params(
        {:subject => "fred", :body => "body", :in_reply_to => 11},
        @sender, @recipient)
    @message.should be_reply
    @message.should_not be_spam_message
  end

  should "not make a message between mutual friends a spam count" do
    @sender.become_friends_with(@recipient)
    @message = Message.new_from_params(
        {:subject => "fred", :body => "body"}, @sender, @recipient)
    @message.should be_friend_message
    @message.should_not be_spam_message
  end
end

The message tests simply set up the different classes of message and verify that they are classified appropriately, they are pretty straightforward.

I also added a test to user to verify that the spam count was being used appropriately — this is part of the same context as last week’s user tests. The user has already sent four messages in the setup.

should "not count a reply message" do
  message = Factory.create(:message, :sender => @user,
      :created_at => 10.minutes.ago)
  message.in_reply_to = 3
  message.update_count_toward_spam
  message.save!
  @user.spam_message_count.should == 4
  @user.update_message_block_status.should be_nil
  @user.should be_able_to_send_messages
end

It’s interesting what you see when you go back over a chunk of code to explain it to other people. In this case what I notice is the update_count_toward_spam method, which is part of Message that unsets the count_toward_spam_ field if the message is a reply and is automatically called as part of the controller create message code. However, strictly speaking, it should be tested in the message class rather than here (although, the message controller tests would also exercise it).

That’s the bulk of the testing for those features — at the time, I didn’t think there were any other special cases.

The part about unblocking a user requires controller tests, because the unblock action needs to be written in the controller. I’m making it part of the message controller, although I know that RESTfully speaking it probably should be a separate resource — the legacy app is such a REST tangle that it’s not worth it.

The controller test context looks like this — the setup creates a blocked user and calls the unblock method.

context "GET unblock" do

  setup do
    ActionMailer::Base.deliveries.clear
    @recipient = Factory.create(:user)
    @user = Factory.create(:user,
      :message_sending_status => User::BLOCKED_STATUS)
    @user.should_not be_able_to_send_messages
    admin!
    get :unblock, :user_id => @user.id.to_s
    @user.reload
  end

  expect { @user.should be_able_to_send_messages }
  expect { assert_redirected_to :action => :admin }

  should "send email to unblocked user" do
    assert_sent_email do |email|
      email.to.first == @user.email &&
      email.from.first == "[email protected]"
    end
  end
end

The assertion tests verify that the user can send messages again, and that the user gets an email from the system to that effect. The associated user model test validates that the user’s spam message count is reset back to zero, which was the specific point of this round of tests (in the actual application, there were other Cucumber scenarios supporting basic administrative behavior).

context "unblocking a user" do

  setup do
    Message.new_from_params(Factory.attributes_for(:message,
        :created_at => 10.minutes.ago), @user,
         Factory.create(:user)).save!
    @user.update_message_block_status.should == "blocked"
    @user.should_not be_able_to_send_messages
    @user.unblock_message_status
  end

  expect { @user.should be_able_to_send_messages }
  expect { @user.spam_message_count.should == 0 }

  should "reblock" do
    @user.block_message_status
    @user.should_not be_able_to_send_messages
  end

end

Anyway, the code that I wrote along side these tests to make them pass seemed to work fine, and we deployed to production.

Shortly thereafter, we got a bug report. I haven’t stressed it here, but new users were limited to a single message in their earliest time on the system. We heard from a new user who sent a message, received a reply, and then replied back to that message, only to find that he was blocked, even though the reply message shouldn’t count toward the rate limit.

Interesting. It’s one of those bugs that almost turns philosophical — exactly when in this process does a user become blocked? In the code that I originally wrote, a user was blocked as soon as he or she hit the rate limit, and before sending another message. In fact, the code should wait until the user tries to send that next message to consider that user blocked, because a user who is at the rate limit border should still be able to send replies and friend messages without being blocked.

So, bug. Since the bug involves multiple interactions, I started in Cucumber:

Scenario: New User Behavior on reply
  Given I am a new user
  When I send 1 message in a day
  And I reply to a message
  Then 2 messages are sent
  And I am not blocked from sending further messages

All those step definitions exist, but the last two steps fail without further work. I wasn’t initially sure whether the code change for this would wind up in the controller or not — as it happened, it turned out to be a minor controller change and minor model change but it took me a few tries to get it right.

Since I already had a decent controller harness for similar scenarios, it took almost no time to adapt to the new condition:

context "with a borderline new user and a reply" do

  setup do
    reply = Factory.create(:message, :recipient => @user,
        :sender => Factory.create(:user))
    flexmock(User).should_receive(:new_user?).and_return(true)
    flexmock(Message).should_receive(
        :toward_spam_in_last_24_hours).and_return(1)
    post :create, :recipient => @recipient.id,
        :message => Factory.attributes_for(:message, :sender => @user,
            :in_reply_to => reply.id.to_s)
    @user.reload
  end

  expect { @user.messages_sent.size.should == 1 }
  expect { @user.spam_message_count.should == 1 }
  expect { assigns(:message).should_not be_new_record }
  expect { @user.should_not be_message_sending_blocked }
  expect { assert_no_email_to_administrator }
end

I have to say, in retrospect, that I’m not 100% sold on the use of the mock package here — it’s a clear user of mocking to limit the test’s exposure to the model layer, but I’m not convinced it makes the test more clear or readable. This setup and tests is very similar to the other bundles of controller test and makes most of the same assertions.

The main change was in the user method, triggered by the following tests — one of which tests the positive sequence, one the negative — and yes, the setups probably should have been combined.

should "move a new user to blocked mode after first message" do
  Timecop.freeze(Date.today)
  new_user = Factory.create(:user, :created_at => 1.day.ago)
  m = Message.new_from_params(Factory.attributes_for(:message),
      new_user, Factory.create(:user))
  m.created_at = 2.hours.ago
  m.save!
  assert_equal(2.hours.ago, new_user.last_message_sent_time)
  assert_equal("blocked", new_user.update_message_block_status(true))
  assert new_user.message_sending_blocked?
end

should "allow a new user a second message that is a reply" do
  Timecop.freeze(Date.today)
  new_user = Factory.create(:user, :created_at => 1.day.ago)
  m = Message.new_from_params(Factory.attributes_for(:message),
      new_user, Factory.create(:user))
  m.created_at = 2.hours.ago
  m.save!
  assert_equal(2.hours.ago, new_user.last_message_sent_time)
  assert_nil new_user.update_message_block_status(false)
  assert !new_user.message_sending_blocked?
end

The key here — which would be easier to see if the setups were combined — is the next to last line where update_message_block_status is called. That’s the method called by the controller when a new message is sent, and the false argument means that the new message does not count toward the rate limit. So in the second test, the user can still send messages, while in the first test, the user does move to a blocked status.

Having the existing suite of tests around the controller and model behaviors was a big relief when fixing this bug — it’d be easy to make a fix for this issue that affected one of the other scenarios (I know because I made fixes that broke a lot of tests…). This was a case where the tests clearly made me more confident in the fix that I made.

Related Services: Full Life Cycle Software Development