4

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')
  • 2
    As a rule of thumb, take care when someone says "always do X" or "never do Y" when talking about development or other software-related stuff. There is a use case for everything, and blanket statements like those don't really help you understand why you should or shouldn't do something. – T. Sar Oct 6 '16 at 20:24
  • Cool, yes I ignored and I'm using these methods. Nevertheless, I'm curious why some people consider this a design smell as I've heard/read this warn in other languages/contexts. – Rodrigo Oct 6 '16 at 20:33
  • The designers of RSpec must have found at least one valid use-case for expect_any_instance_of, or it would not have existed at all. – Bart van Ingen Schenau Oct 7 '16 at 10:21
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.

  • Adding application code to support a test is not a good practice IMO. Also although you did say this is pseudo code, for Rubists out there to be tempted to copy this; the var @sender will never get assigned make_default_sender() because = operator as a higher priority than or, you need to use || which is evaluated before = – Mathieu J. Jul 26 '18 at 4:49
  • @shigazaru Thank you for your comment – feel free to edit the code (I don't actually know Ruby). However, I do think that testability is a legitimate and important design constraint/requirement. In my experience the benefits of less fragile tests significantly outweigh the costs of making the application code more flexible, in most cases. I've worked on legacy projects where the design completely ignored testability. That just made life more difficult for everyone involved. – amon Jul 26 '18 at 6:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.