TDD Rules!

These are some rules I like to follow when doing TDD. You can follow them too! Rules are fun!

  • Write your tests first. If you can’t, spike a solution, throw it away, and try again.
  • Test units in isolation. Use mocks to verify interaction between units. If this makes your tests brittle, refactor.
  • You don’t need to isolate your unit from simple value objects. So use more value objects.
  • If you feel like you can’t keep everything in your head, ask yourself if you really need to keep it all in your head. If you do, you need to refactor.
  • Each branch of logic should be covered by a unit test. If that makes you feel like you have too many tests, your logic is too complicated. Refactor.
  • If you ever feel the need to only run part of the unit test suite, it’s too slow and refactoring is needed.
  • Unit tests should be written as if they are a set of requirements – or “specs” – for the unit being tested.
  • Each test should test one and only one concept. That doesn’t always mean only one assertion.
  • When fixing bugs, make sure there is a test that fails without your fix, and passes with it.
  • Never push commits that contain failing tests. This makes it harder to revert, cherry-pick, and debug problems (e.g. with git bisect).

Solve tough problems with spikes

Sometimes I’m approaching a problem where I lack some understanding that would let me start with nice little unit tests. So instead, I start with a high-level functional test. Then I start getting the code to work by any means necessary. I do this without writing any more tests. I just cowboy-code my way to a solution that works. Extreme Programming calls this a Spike Solution.

When my functional test is green, I have much more understanding. I’ve been googling, looking up and using new libraries, and usually have a better idea of what a clean solution might look like. This is when I throw my code away.

Well, most of it. I keep the functional test. And if I have any particularly tricky code I might want for reference, I keep it separate from the project code, but available to refer to if I need it. Then I jump down into my unit tests and try to make my functional test green again using a proper test-driven approach.

It can be very hard to discard working code, but when I think back to every time I’ve lost some writing work unintentionally – an essay, blog post, homework assignment – the second draft is always better. I think the same is true for writing code.

Why is “Legacy” a negative word?

Programming might be the only place where “legacy” is a negative word. It seems backwards that we’re an industry where things tend to get worse instead of better as they get older and more people work on them. How does that happen?

Imagine you’re given a task to make a change to a piece of code that works, but no one wants to touch. It’s a gnarly mess of quick fixes and rushed, untested features that’s impossible to understand.

But you’re relieved because the change you need to make is super tiny. You can dive in, add a new if block to the 500-line method and get outta there.

“Who made that mess? I wish my managers would give me time to rewrite this thing” you think as you wash your hands of that “legacy code”.

But how did the code get like that? No one sets out to write a giant, confusing method that no one wants to work in. It’s a group effort. It takes lots of little “quick fixes” and “tiny changes” like the one you just made.

It’s not up to your managers to “let” you pay down that “technical debt”. A rewrite won’t fix that problem. To really fix it, we need to change our attitude about code like this.

We need to realize that the problem isn’t some other programmer who wrote this big thing. It’s us. We all worked together to make this mess because no one thinks their one small change is causing the problem.

When we finish working on a piece of the system, it shouldn’t just do what we want, it should also be a little bit easier for the next person to come in and make more changes. Our job isn’t just to build new things, it’s to enable change. To do that, we should leave each piece of code we touch a little better than we found it.

Uncle Bob calls this The Boy Scout Rule. Martin Fowler calls it Opportunistic Refactoring.

Imagine working on a team where this is the norm. Imagine working on a code base that gets better as it ages. Imagine if “legacy” was a good thing.

That doesn’t mean there will never be a mess. But it means the trend will be towards cleaner code. Quick hacks and technical debt will no longer be excuses to continue cutting corners. They will be strategic decisions made when the trade-off is worth it. And we don’t need to get management approval to pay down that debt, because it won’t be a huge undertaking. It will be how we normally work: pay it down a little bit at a time as we continue working on the codebase.

Let’s start leaving behind a good legacy.

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