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.
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.
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
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
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