Would you use stubs in this test?

My peers and I are trying to take our testing and object design to the next level, where we’re more concerned with messages than specific objects. For the sake of this question, let’s say we have users and comments models. User has many comments.

class User < ActiveRecord::Base
  def update_related_statuses
    comments.each do |comment|
       comment.update_status
    end
  end
end

And the corresponding test

describe User do
  describe '#update_related_statuses' do
    it 'calls update_status on all related comments' do
      # A number of FactoryGirl.create()s omitted for brevity
      expect_any_instance_of(Comment).to receive(:update_status).once
      user.update_related_statuses
    end
  end
end

And that started a little debate among my group because there was a faction that argued for adding lines like this to the spec

describe User do
  describe '#update_related_statuses' do
    it 'calls update_status on all related posts' do
      comment = double 'comment'
      user.stub(:comments).and_return [comment]
      expect(comment).to receive(:update_status).once
      user.update_related_statuses
    end
  end
end

And one of my peers expressed discomfort with that approach because he argued that the test knew too much about the implementation, which… I can’t necessarily disagree with in principle. So, putting aside the obvious rebuttals of “well, how else would you implement this method,” what do you all think about these alternative test approaches? And am I really just opening a debate about white- vs black-box testing?

I find the second version better to the first for one reason: any_instance is a naughty band-aid.

I don’t have any major complaints with the second version. These aren’t the tests I tend to write these days, but writing tests like that sent me the way I do things now, so I think it is a good step in the right direction.

Next you would pull these domain constants away from persistence and test them as simple collections without involving rails.

Simple answer: No.

The first test has the obvious flaw that you could make it pass by Comment.first.update_status and the second one is basically restating the implementation. There’s also a rule to not stub methods of the object under test.

So, I’d just use the database for this test.

Hi @Dreamr, I see this in my peer’s code all the time and know it’s wrong, but could you elaborate a bit more as to why it’s a test smell?

As @timhabermaas said, it is not a good idea to stub the methods of the object which you are testing.

I think I would do this kind of test:

describe User do
  describe '#update_related_statuses' do
    it 'calls update_status on all related posts' do
      comment = Comment.new
      comment.stub(:update_status)

      user = User.new(comments: [comment])
      expect(comment).to receive(:update_status).once
      user.update_related_statuses
    end
  end
end

What do you think?

I like @ronaldoteruia’s approach. Here a few suggestions based on that:

  • Use a plain double rather than a stubbed Comment. The actual type isn’t important to the test. This allows you to have a more isolated test, if you’re into that approach.
  • Use more than one comment in the tests, since we want to verify that it updates multiple comments
  • Use a spy approach for better readability
describe User do
  describe '#update_related_statuses' do
    it 'calls update_status on all related posts' do
      comment_1, comment_2 = double, double
      user = User.new(comments: [comment_1, comment_2])

      user.update_related_statuses

      expect(comment_1).to have_received(:update_status)
      expect(comment_2).to have_received(:update_status)
    end
  end
end
1 Like