Python’s patch decorator is a code smell

I’m a big fan of using mocks as a testing/design tool. But if I find myself reaching for patch instead of Mock in python, I usually stop and rethink my design.

I consider the use of patch in tests to be a code smell. It means the test code is not using my internal API. It’s reaching in to the private implementation details of my object.

For example, I recently needed a helper function for creating users on a third-party service with a set of default values. I could have written it like this:

from services import UserService

from settings import SERVICE_CONF


def create_user_with_defaults(**attributes):
  defaults = { "name": "test" }
  defaults.update(attributes)

  service = UserService(**SERVICE_CONF)
  return service.create_user(**defaults)

This would get the job done. And because this is python, I can test it without hitting real services using @patch:

@patch("users.helpers.UserService")
def test_creates_user_with_defaults_on_user_service(self, MockUserService):
  user_service = MockUserService.return_value
  
  # execution:
  user = create_user_with_defaults()
  
  # verification:
  user_service.create_user.assert_called_once_with(name="test")
  self.assertEqual(user, user_service.create_user.return_value)

But look at the verification step: there is nothing in the execution step about user_service, yet that’s what I’m asserting against. My tests have knowledge about private implementation details of the thing they’re testing. That’s bad news.

I prefer my tests to be normal consumers of my internal APIs. This forces me to keep my APIs easy to use and flexible. @patch lets me get around issues like tight coupling by hijacking my hard-coded dependencies.

Here is how I actually implemented the helper function:

def create_user_with_defaults(service, **attributes):
  defaults = { "name": "test" }
  defaults.update(attributes)
  return service.create_user(**defaults)

I didn’t even need to import anything! This is how I would test it:

def test_creates_user_with_defaults_on_user_service(self):
  user_service = Mock()
  
  # execution:
  user = create_user_with_defaults(user_service)
  
  # verification:
  user_service.create_user.assert_called_once_with(name="test")
  self.assertEqual(user, user_service.create_user.return_value)

Now compare the verification to the execution. Instead of patching the internal workings of the module, I’m explicitly passing in a mock object. I can do this because the function no longer depends on the concrete implementation of the user service, it depends on an abstraction*: some object that must be passed in that conforms to a certain interface. So it makes sense that my test verifies the interaction with that interface.

This means my test is now a normal consumer of my function, and my desire to avoid patch led me to a design that is more flexible. This became clear as soon as I wanted to create some test users in the repl. I happily created an instance of the UserService that uses the settings for our sandbox, and passed that in to my function.

*See The Dependency Inversion Principle (the D from SOLID).

Do you need DI in dynamic languages?

I’m often told dependency injection isn’t needed in dynamic languages. DHH wrote an article a couple years ago that sums the argument up nicely: Dependency injection is not a virtue. Read that and come back here for my 3-years-later response.

He uses this as an example:

def publish!
  self.update published_at: Time.now
end

He says DI folks would shiver at hard-coding a call to Time.now, and I’m assuming he thinks they would inject a clock or something to more easily test it. He argues (accurately) that it’s silly to make that more complex just for testing, and you should do something like this:

Time.stub(:now) { Time.new(2012, 12, 24) }
article.publish!
assert_equal 24, article.published_at.day

I do shiver at hard-coding Time in the publish! method, but I also shiver at stubbing global, built-in classes like that…

I prefer to think about Dependency Inversion first, and then Dependency Injection if that’s a way to accomplish it.

So when I see this:

def publish!
  self.update published_at: Time.now
end

I do shiver, because hard-coding like that is well established to be a code smell. Now, a smell does not mean it’s wrong in every case, but it means I should examine this. I’m not even thinking about tests right now, I’m thinking about inverting this dependency. Here’s how I’d do it:

def publish!(time)
  self.update published_at: time
end

I’ve inverted the dependency. No dependency injection framework needed. And no stubbing globals or bringing in complex gems (like Timecop) to test it:

article.publish! Time.new(2012, 12, 24)
assert_equal 24, article.published_at.day

The fact that it’s easier to test wasn’t the goal, it was a side effect of a better design. Now the system is ready to allow users to define publish dates in the future for example.

So just because a dynamic language lets us hijack objects to make testing easier, doesn’t mean we should completely ignore good practices. I’d argue that this is better code, regardless of what language I happened to write it in.

And if you miss the old API? Just give it a default:

def publish!(time = Time.now)
  self.update published_at: time
end