Not too long ago, a mistake I made caused a few thousand wrong emails to go out to customers. I edited the untested code and didn’t bother to add tests. Fortunately, the bug was discovered and fixed. After this happened, I wanted to add some tests to this code, but I soon realized the tests wouldn’t have found the original bug.

The problem? Mocks not reflecting the truth.

The script

We started with a piece of code that looked something like this:

It fetches all users with a specific state, sends them a mail, and gives them another state. This is by no means a perfect piece of code, but it has been working for many years and made the business money.

Now, some of our business logic had changed, which introduced a new state. The script had to be updated.

Mind you this didn’t introduce a bug yet, the code got worse, but it was still correct. State 3 was readyForMail, state 5 was also ready for mail, but with another precondition. A colleague was quick to point out that we now have an enum like object for user state, and that it would be more logical to use that. So I updated the code, the merge request got approved, merged, and deployed.

Can you spot the bug?

Now all accounts with the normal state would also receive the mail. The correct state that should have been used was AccountState::preConditionReadyForMail(). It was discovered quickly, but not before a lot of customers had received an email.

The tests

A mistake that could’ve been avoided, had I added tests. So then the job came of adding those tests. So what did I want to test? The user had to be fetched, the mail had to be sent, and the correct state had to be set back. The mock of the repo was configured like so:

I ran the tests, and then they passed. Then, just to be sure, I changed the code back to the ‘bad’ way, where all the normal accounts would also receive an email. But the tests stayed green. I could pass anything into the findAllByState method, and the mock would always return these two objects.

The method could be used wrong, and the mock would hide it.

Now you might say, that I could add a check on the mock, to make sure it was called with a specific object. I’ve found that those kinds of checks are very prone to just being copy-pasted from the original code. Especially if the values there are hardcoded.

We needed a better solution.

The solution

Stop using mocks. That’s it, that’s the fix. Or at least it’s half of the fix, we don’t want to use the database in our tests, so we still need something to use in the test. The solution is a new implementation of the UserRepository. The one in the code can be backed by a database, while the version for the test is based on an array.

And now we can use this repository in our tests, and when we use the wrong account state, the test fails.

By using a new version of the repository, we could make sure that the code using the repository does so correctly. You could argue that the test has now become an integration test, rather than a unit test. As it also tests the integration with the repository. And perhaps you are right. But I would still prefer this way of testing over mocks, as there is less chance of erroneous tests.

The other tests

We’re not done yet. The new MemoryUserRepository should work the same way as the other versions of the UserRepository. So, we need to add a test, to make sure all work the same. What I like to do is create a base TestCase class, where we define the tests. It has one abstract method, which will return an instance of the UserRepository to test against. This way we know the different repositories all work the exact same way. In the database version of this test, you could have a setUp and tearDown method to make sure the database gets reset to an empty state again.

So what do you think? Are you already using your own implementations during tests?

Or do you think mocks offer advantages over custom implementations?