When mocks do more harm than good

Your boss comes to you one day, and gives you the following task:

Hey Bob, we need a feature that sends an email to all employees that have their birthday on the day the function is called, HR thinks this is a good way to increase retention.

You start working on an implementation, and you get something working along the lines of

class GratulateOnBirthdaysService { execute() { employees = DatabaseService().findEmployees(birthDate: Date.today) employees.each { employee email = builBirthdayEmail(employee) EmailDeliveryService().send(email) } } buildBirthdayEmail(employee) { // some nice text wishing only good for that employee, because we wish them good only on their birthdays :) } }
Code language: JavaScript (javascript)

Something’s not right, though, buildBirthdayEmail() doesn’t fit very well into the new service, you can’t easily reuse, and let’s face it, it breaks SRP. So you drop in a new class, BirthdayEmailBuilder

class GratulateOnBirthdaysService { execute() { employees = DatabaseService().findEmployees(birthDate: Date.today) employees.each { employee email = BirthdayEmailBuilder().build(employee) EmailDeliveryService().send(email) } } }
Code language: JavaScript (javascript)

Things look pretty good at this point, you have a clean service that aggregates other services, and its implementation looks clean enough.

However, you’re a diligent developer, and know this code will need to be unit tested. But there’s a problem, the implementation has hidden dependencies. Don’t worry, DI to the rescue!

So, you rewrite the service as this

class GratulateOnBirthdaysService { construct(databaseService = DatabaseService(), birthdayEmailBuilder = BirthdayEmailBuilder(), emailDeliveryService = EmailDeliveryService()) { } execute() { employees = databaseService.findEmployees(birthDate: Date.today) employees.each { employee email = birthdayEmailBuilder.buildBirthdayEmail(employee) emailDeliveryService.send(email) } } }
Code language: JavaScript (javascript)

You maybe throw some interfaces/protocols , if the programming language has support for such this, add default values for the injected params, so the callers won’t have duplicate code, and voila! You finished the feature, with spare time to also write unit tests. The code reviewer will certainly be happy that you wrote clear and testable code.

Or have you?

Well, the code is clear enough, it transmits its intent quite well. It’s definitively testable, since you can inject mocks/fakes from unit tests. So, where’s the problem?

The first red flag is the fact that this approach leaks too much implementation details into the unit tests.

The fact is the unit tests will have to know that the implementation will call a certain database service method to get the employees, and it will call a certain method on the email builder, and it will call a certain method on the email delivery service.

This makes your code rigid and decrease the value of the unit tests, as changes to the service implementation will cascade to changes within the unit tests, and if changing the implementation results in changes of the unit tests, then what’s the point of the unit tests in the first place? Are those unit tests present just to guard that the code you wrote is the code you wrote?

This is the major problem with mocks, they strongly couple the tests with the implementation. This makes the code more rigid, and the tests more fragile.

Unit tests should validate specifications, not implementation, so unless your unit specifications say that the unit should call a certain method on a certain class/interface, then you should not mock.

But in that case should we simple not test the GratulateOnBirthdaysService unit? No, we should still test, and we’ll get back to that in a minute.

The second problem with the above “neat” implementation is the fact that is has multiple responsibilities. Yes, it violates SRP, given the implementation effectively has 4 lines of code:

  • it has to locate all employees born today
  • it has to compute today, yes, as silly as it might sound, this is a responsibility, and people go to great extents like creating DateProvider classes/interfaces just to be able to inject the date from tests
  • it has to call the email builder service
  • it has to call the email delivery service, passing the exact email received from the email builder service

4 lines with 4 different responsibilities. This translates to various aspects that need to be tested:

  1. the appropriate database service method needs to be called
  2. today’s date needs to be passed to the database service method
  3. the email builder method needs to be called
  4. the appropriate email delivery service needs to be called
  5. the email returned by the email builder needs to be passed to the email delivery service
  6. steps 3-5 need to be executed as many times as employees there are
  7. don’t even get me started about the possible exceptions, basically every method that can raise an exception will add more and more tests

The heck, how can 4 lines of code generate so many problems? I don’t want say it’s DI’s fault, but partially it is. We are so used in advocating DI that we forget about other important aspects of the code. Mocks/Fakes are a convenient way to control everything that is injected, that we don’t realise how fragile are the tests we write.

OOP-style DI also leads to lots of interfaces/protocols being created so we can obey the ISP principle. We end up with lots of declarations that have only one method, since we don’t want to burden classes with knowledge about other methods that they will never use.

But we can do better here, let’s not give up DI, as this is a powerful tool. Let’s just convert constructor injection to parameter injection, and objects to closures/functions:

class GratulateOnBirthdaysService { execute(date, employeesLocator, birthdayEmailBuilder, emailDeliveryService) { employeesLocator(birthday: date).each { employee email = birthdayEmailBuilder(employee) emailDeliveryService(email) } } }
Code language: JavaScript (javascript)

Not a big change, though, we got rid of an extra line but maybe we made the code less readable to people less initiated in Functional Programming.

However, this code is more reusable, since it no longer depends on the current date, thus we could theoretically generate all emails for tomorrow, and enqueue them to be sent only after 23:59, with just a change to the passed arguments.

More, we could pass references to methods of existing objects, so we don’t need to do much refactor in case we already have a DatabaseService and an EmailDeliveryService with matching method signatures. Adding new interfaces/protocol means retroactive modelling over existing types, which might not always be desirable, and by passing functions/methods/closures, we avoid this.

And not last, this enables easier testing, as we won’t have to mock the production classes, we can inject hardcoded arrays of employees, we can inject any date we need, we can have the email delivery closure just set a boolean which is inspected by the unit test.

Another important aspect is the fact that this code is stateless, thus we can reason about its behaviour just by looking at those 3 lines of code. There is no other code (like the constructor) that can change how this code behaves. Which makes the code review process easier than ever, as the reviewer will not have to scroll up and down through the class to see which methods can change the injected dependencies, and in which circumstances, or if you actually implemented the methods on the injected objects.

Conclusion? Use mocks only for code you don’t have control over, like networking, or some database. Don’t write mocks for your own code, it will make refactoring harder.

You own your code, write your functions/methods as input/output as much as possible, this not only for easier unit testing, but because it’s also a simpler mental model.

Leave a Reply

Your email address will not be published.