GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS

Written by thoughtbot

This should_change your mind

There's been a bunch of hullabaloo about our recent deprecation of the Shoulda macros should_change and should_not_change. A lot of people have asked, "why remove it?" Short answer: we don't use it anymore. Long answer: it's confusing and may encourage bad programming practices.

Consider this example of should_change:

context "updating a post" do
  setup do
    @post = Post.create(:title => "old")
    put :update, :post => {:title => "new"}, :id => @post.to_param
  end

  should_change("the post title", :from => "old", :to => "new") { @post.title }
end

This reads well and seems to be fairly straightforward. Sadly, this doesn't work because of how should_change works internally (hence the confusing part). It would actually need to be written like this:

context "given a post" do
  setup do
    @post = Post.create(:title => "old")
  end

  context "updating" do
    setup do
      put :update, :post => {:title => "new"}, :id => @post.to_param
    end

    should_change("the post title", :from => "old", :to => "new") { @post.title }
  end
end

The @post instance variable needs to be assigned a context above the context containing should_change. I told you it was confusing.

Now, onto the bad practice aspect. This tests that the post was saved! Come on, we don't want to do that here. That's why we write Cucumber scenarios! We're testing the controller, right? If that's the case, then we shouldn't care whatsoever if update_attributes worked with specific data passed to it, just that it was called and that we handle the result accordingly.

context "updating a post" do
  setup do
    @post = Factory :post
    Post.stubs(:find => @post)
  end

  context "when the update is successful" do
    setup do
      @post.stubs(:update_attributes => true)
      put :update, :post => {:title => "new"}, :id => @post.to_param
    end

    should set_the_flash.to(/updated/)
    should redirect_to(posts_path)
  end

  context "when the update is unsuccessful" do
    setup do
      @post.stubs(:update_attributes => false)
      put :update, :post => {:title => "new"}, :id => @post.to_param
    end

    should render_template(:edit)
    should assign_to(:post).with(@post)
  end
end

This is more inline with what we really want to test; that the controller handled update_attributes correctly when the update worked and when it didn't. Leave the model logic and whether it should save or not in the model (and its tests), and test the controller to ensure it handles whether the model was updated.

I mentioned before that Cucumber scenarios should cover overall integration. Something like these would work:

Scenario: Update a post successfully
  Given I am signed in
  And I authored a post titled "Great Scott"
  When I go to edit the "Great Scott" post
  And I fill in "Title" with "Willard Scott"
  And I press "Update"
  Then I should see the flash message "Your post was updated successfully"
  And I should have authored "Willard Scott"

Scenario Example: Update a post unsuccessfully
  Given I am signed in
  And I authored a post titled "Great Scott"
  When I go to edit the "Great Scott" post
  And I fill in "<required field>" with ""
  And I press "Update"
  Then I should see error messages
  When I fill in "<required field>" with "<valid value>"
  And I press "Update"
  Then I should see "Your post was updated successfully"
  Examples:
    | required field | valid value       |
    | Title          | Awesome Post Name |
    | Body           | Awesome story     |

I'm sure this isn't the only way people were using should_change, but hopefully this clarifies how deprecating the methods will encourage better tests.

Do you use should_change and think it's by far the best way to test certain pieces of code? I'd like to see some people who'll sorely miss it post some real code (Pastebin or Gist).