This post is a little long, but ultimately what I would love to hear opinions on could be summarized as:
When refactoring leads to moving logic out of the system under test to another object, what do you do with the “old” tests?
I recently ran into a situation where a refactoring moved a block of code from the system under test to another object. This code contained a little bit of logic that, at the time the class was written, seemed specific to the class, but after another feature required very similar logic, it was moved in order to be shared. The refactor meant creating the new object with the existing collaborator using dependency injection, and all tests were green and everything seemed great.
Here is an attempt at a contrived example of what I’m describing - please disregard that it’s a bit silly and unrealistic. Pretend there are two third party APIs that is not under our control and we will use doubles for. A messages API that only has a method to get all messages and does not provide any filtering. Our first feature asks that send notifications of unread messages. Which we implement like so:
Original Test
it 'notifies of messages that are currently unread' do
unread_message = Message.new(status: :unread)
read_message = Message.new(status: :read)
message_api = stub('message_api', messages: [unread_message, read_message])
notifier = mock('notifier')
expect(notifier).to receive(:notify).with([unread_message])
ClassUnderTest.new(message_api, notifier).notify_of_unread_messages
end
Original Class
def initialize(message_api, notifier)
@message_api = message_api
@notifier = notifier
end
def notify_of_unread_messages
@notifier.notify(unread_messages)
end
private
def unread_messages
@message_api.messages.select { |m| m.status == :unread }
end
A feature later in the project also needed to use the concept of what was previously a private method. For this example, let’s say we now want a dashboard that shows unread messages. This, in addition to some other things, meant that introducing a facade on top of the API would be helpful. So the class was modified as such (ignore names - still just an example):
Class after refactoring
def initialize(message_api, notifier)
@message_api_facade = MessageApiFacade.new(message_api)
@notifier = notifier
end
def notify_of_unread_messages
@notifier.notify(@message_api_facade.unread_messages)
end
Everything is still green. The private method is gone, and now this class is only concerned with the act of getting the right messages and sending the notification. I would count this as a good refactoring, but I have a few problems now.
- Since we wrote tests for the newly introduced class (MessageApiFacade in this case), tests for the logic that it encapsulates are duplicated (the original class’ tests and the new ones)
- When writing future tests we would want to stub the facade, but cannot do that at the moment since we create it in the initializer.
- New developers may be confused when coming into the test class because of setup of objects/properties that we don’t directly care about
What I ended up doing in this instance is: committing and having the CI server build it to show that everything was green after the refactor. Then doing the following modification in a separate commit in an attempt to address the above three points.
New Test
it 'notifies of messages that are currently unread' do
unread_messages = stub('unread_messages')
message_api_facade = stub('facade', unread_messages: unread_messages)
notifier = mock('notifier')
expect(notifier).to receive(:notify).with(unread_messages)
ClassUnderTest.new(message_api_facade, notifier).notify_of_unread_messages
end
New Class
def initialize(message_api_facade, notifier)
@message_api_facade = message_api_facade
@notifier = notifier
end
def notify_of_unread_messages
@notifier.notify(@message_api_facade.unread_messages)
end
Now this particular test only tests that the interactions work as expected, it does not have any knowledge of what happens to determine whether a message is “unread”.
I did not feel good about this change, though, because I am simultaneously altering test and production code, and I’m not sure that I can still “trust my tests” since the new tests did not come from TDD, but was rather altered after the production code was written (or, in this case, refactored).
Obviously this type of refactoring is very common. What would be the preferred way of handling it from a testing perspective?