Testing: why expect_any_instance_of is considered a design smell?

Question

In this question, I'll use a ruby example, but I think it is a general question.

According to the most popular Ruby's test framework (RSpec), mocking any instance of a class (allow_any_instance_of) is a design smell.

Actually, I don't agree with this statement.

So, I would like to know how would be the "best way/correct" (or something like that) to implement/test a class like this below, and test if the format_phone method is formatting the phone numbers correctly.

class SmsSender
  def initialize(message)
    @message = message
    @client = Twilio::REST::Client.new(TWILIO_CREDENTIALS)
  end    

  def send_to(phone)
    return false unless validate_phone(phone)
    @client.send({
      from: '123',
      to: format_phone(phone),
      content: @message
    })
  end
end

This is how my test would looks like:

expect_any_instance_of(Twilio::REST::Client).to receive(:send).with({ from: '123', to: '+10002225555', content: 'hi' })

SmsSender.new('hi').send_to('0002225555')

Answer 1

By using this mocking mechanism, you don't just affect your @client. You affect all matching objects within that test. Here, this isn't much of a problem since only one such object happens to exist, but consider what would happen if you were to affect a more fundamental type like strings or numbers that is used throughout your program.

The reliance on this mocking mechanism is a strong indicator that your design isn't testable in itself – in particular, the fixed dependency on Twilio::REST::Client is problematic. By using a dependency injection mechanism such as constructor injection, we can write the same test but with much less magic.

Here's pseudocode to illustrate the concept:

class SmsSender
  def initialise(message, sender)
    @message = message
    @sender = sender || make_default_sender()
  end

  def send_to(phone)
    return false unless validate(phone)
    @sender({
      from: '123',
      to: format_phone(phone),
      message: @message,
    })
  end
end

def make_default_sender():
  client = Twilio::REST::Client.new(SECRET)
  return do |message|
    client.send(message)
  end
end

In the test, we can now easily check that we're sending the correct message:

expected_message = ...

checker_was_called = false

def checking_sender(message)
  checker_was_called = true
  assert message == expected_message
  # you could still send the message here if you want to.
end

SmsSender.new('...', checking_sender).send_to('...')

assert checker_was_called

Of course this is also much more code than your current test, and dependency injection always introduces some fragility into the system – you really need integration tests to make sure all dependencies are wired up correctly for production/deployment. It can therefore make a lot of sense to keep your current approach, as long as you're aware of the trade-offs.