Little and Often

What do you call refactoring without unit tests?

It’s called breaking the code!

But it’s easy to say we should have had unit tests when you don’t. So it’s pointless.

A Story From The Trenches

There is a very talented developer who wrote a JavaScript AWS Lambda a few months ago. He’s no longer with the team, and we miss him greatly. This is true even though he didn’t leave us any unit tests in that particular piece of code.

However, the Lambda in question, simple though it was, hit a couple of glitches in production, and we wanted to see if there was any way we could prevent it doing so in future. Our standard for that sort of code is now TypeScript, so we thought that maybe now might be the time to migrate to TypeScript.

Similarly, we prefer the rather delightful async/await pattern in our JavaScript code, rather than the harder-to-fathom Promise technique, even though one is essentially based on the other.

Attempt 1 – a Rewrite

Another member of the team ported the code across to TypeScript, added a load of improvements and dropped a PR.

As there were a lot of changes, and the filenames had change, the changed was not even slightly comparable with the old code.

Given this was a rewrite, was impossible to review with any diff view, AND there were no tests, it set off some alarm bells. To be honest, it set off loads.

Even though it was a small amount of code, we didn’t like the idea of only being able to test it in a deployed environment, and weren’t sure how many edge cases we’d see if we just ran it.

Attempt 2 – Lift and Shift a Tiny Bit

What if we don’t change the code at all? Just shift it to TypeScript and fix any major dealbreaker linting issues? Will that work?

We had a go at this – well the team member whose first rewrite got put on ice did – and tried to review it.

It was more reviewable, but then we started worrying about how certain native TypeScript types would interplay with the string-based JSON coming into the application. We still couldn’t predict if this port had introduced any awkward edge cases that would have been covered in the old code. Worse than that, we felt that maybe the old code was being overzealous with some of its dynamic type handling and that maybe it could be simplified in TypeScript, but we couldn’t be sure.

Attempt 2.5 – The Hot Tub Time Machine

If only we had unit tests…

Except we could solve that by going back in time and writing some.

All we had to do was go back to master where the code was unchanged and write some unit tests against the original JavaScript. So we did.

Life got better more quickly after that.

Attempt 2.75 – The Hot Tub Machine 2

Reviewing the now tested TypeScript code, we noticed a few areas where some unit tests were missing, and where there were some potential edge cases.

We went back in time again and added more test cases for what we felt were the hotspots.

Attempt 3 – Back To The Future

Confident that the code, barely changed, got the exact same results as the unit tests demonstrated previously, we merged it and deployed it.

It should be noted that we have a relatively quick CD process that pushes the code into a couple of environments. We also have automated tests that run traffic end to end across the whole application. It was a couple of button clicks to set everything running, and then we watched the graphs to see if the output came out as expected.

It worked!

Attempt N… N+1

Then we got into a rhythm of making small, useful changes. Having struggled with the code, we had a few key hotspots in mind where we wanted to improve the design/error handling and clarity.

The pattern was:

  • Make a small amount of change – only a few lines
  • Ensure the unit tests still passed
  • As code coverage was high, check the code coverage to see look for surprising paths becoming covered/uncovered
  • Review it, merge it, deploy it
  • Repeat

We kept changes small and self-contained and we used each change as a search for the new hotspot of confusion.

In one case we broke out a new component and added independent unit tests for it. We hadn’t quite understood what that component was doing before, and in the process were able to not only capture its spec with tests, but largely replace the code with library calls, making the code more self-explanatory.

Outcome

The code is better, but we had a purpose going into this. Some mysterious glitching had occurred and we didn’t quite know what error edge cases the original code was capable of.

By refactoring it, we discovered something it was doing that we wanted to reduce, and we also discovered that there was a potential race condition it allowed for that would make errors quite bad… sometimes… so we refactored that out.

The fresher code was generally a lot easier to reason about, and encouraged us to make the software more fault tolerant.

The tests started paying dividends immediately, and the coverage organically grew during the refactor as we were able to reduce the clunkiness of the code down to its bare essentials.

TL;DR

You can jump into refactor, but you may find it easier to write automated tests, rather than debug broken production code!

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s