Prevent Breaking Changes in Existing Systems

Safely Refactor Old Code: Part 1

Published in

ITNEXT

9 min readNov 24, 2018

In this series, we go over the conceptual design patterns of safely refactoring old code step-by-step. I’ve written this guide to accommodate any kind of refactoring scenario, and the example project is Node.js with RxJS.

This is the first article in a 3-part series:

History

I created a Halloween smart light project last year to give my house a bit of haunted-zest when the kids come trick-or-treating.

Since I want to release this project as an npm package, I need to both update it for current library versions and make it presentable. It’s only been a year, lots of things have changed with RxJS and my experience with it so there’s plenty of refactor work I’ll need to do.

This app is mission critical. It has to go off on Halloween and only during the hours kids are around. If it fails to do that, we lose out on the effect completely.

The First Step: Unit Tests

When doing a refactor, you need to first make sure you don’t break or change the existing functionality. Moving files around is one thing, but upgrading packages and changing code can cause some major regressions.

The very first thing you need to do is create unit tests for your existing code. If you already have them, you can skip this step; although, it’s possible those old unit tests were lacking. First make sure they pass. Next, make sure your existing tests cover the areas you’ll be refactoring.

In this particular project, I didn’t have any unit tests. It was a quick one-off project, and I had absolutely no clue how to unit test RxJS at the time. To add unit tests, the code needs to be testable. As with most projects lacking tests, they’re going to be completely untestable without some minor refactoring.

As a goal, I need to write unit tests against the current codebase without major architectural changes. After I’ve committed everything in a working, unit-tested state, then I can begin to do the real refactor and upgrade library versions.

While you can totally skip adding unit tests and go straight to the refactor, you will inevitably wind up with code that only breaks once it’s in production in front of real users. Worse yet, you could’ve missed requirements you didn’t even know where there! I’ve done this plenty of times on past projects and suffered every time because of it.

While the Halloween lights project is ridiculously simple and really doesn’t need all this overhead, I’ll use it to show you the issues you’d come up against in a real production project.

Adding Unit Tests

This is the gist of the code as it stands right now:

We can’t write unit tests around it, but we can probably unit test all the individual functions in our filter and map transducers.

Starting Simple

Let’s start with isHalloween:

You’d think this super simple boolean function would be just as simple to unit test, but it’s not. We can’t have this unit test passing or failing based on today’s date; it needs to be consistent no matter when we run the test. When unit testing this function, we need to be able to pass in a date and see if it properly believes that date is within the range of Halloween night.

The only real way to test this code right now is to wait for Halloween night and call the function hoping it returns true. Since we’re calling moment directly and not passing in a date, we’re missing a crucial piece to unit testing; pure functions and reproducibility.

As you can see, we can’t unit test it. We literally have to refactor the function to unit test even though the very act of refactoring has the potential to break the code.

Trying Something Else

It’s possible this is an isolated issue, so let’s see if anything else can be unit tested:

Instead of passing in colorSets, we’re closing over it in both getColorSetAtIndex and getRandomColorSetIndex, but only getColorSetAtIndex is testable. getRandomColorSetIndex, on the other hand, is impossible to test because the function will return something different every time you call it.

Even though getRandomColorSetIndex isn’t pure by nature, we could still try to unit test it. It takes no inputs, so we merely need to ensure the output is an index in our colorSets array. If we run the function 10–30 times, we could get a good sampling of the random data and see if it’s returning values we want. I don’t recommend this in any scenario though. It doesn’t cover edge cases and may only seem to work.

And if you did sample the data and run the function a bunch, you’d be testing Math.random along with your code. You don’t want to be testing other peoples’ code in your unit tests nor should your tests work inconsistently. Sampling a bunch of random numbers means it’s possible your test will break just as randomly.

A better idea is to make the function pure by passing in a randomizer function instead of implicitly relying on Math.random. That way, you can be absolutely sure how it handles whatever numbers you throw at it. This type of mock is referred to as dependency injection and is necessary in situations where we don’t have pure functions.

AJAX Functions

Lastly we’ll look at our AJAX methods: doScaryLightFlash and getDataFromPromise.

We have yet another Math.random in getCycles, and an AJAX call in doScaryLightFlash using fetch directly. Because we’re not using dependency injection and passing in fetch ourselves, we have no way to mock the AJAX call.

That leaves us with getDataFromPromise. This function is completely testable! fromPromise is taking a promise and turning it into an observable so as long as we mock an AJAX response promise from fetch, we can test this no problem.

Not Meeting Our Goal

Of the 5 functions in our observable chain, we’ve only got 2 we can unit test. Worse yet, those two don’t contain any of the app’s core business logic.

Our goal is to safely refactor by first writing unit tests, and we keep getting stuck just about every step of the way! For a relatively simple one-process app, you’d think this would be a lot easier to refactor. Lesson learned.

Since the core business logic is elsewhere, I’d argue against unit testing those two functions. Frankly, it’d be a waste of time. We’re gonna need to look at the bigger picture and figure out a way to test our core business logic altogether.

We want the lights to do a cool animation when it’s Halloween. We can only be sure the code worked last year. Since I’ve I’ve upgraded Node.js, yarn, and pm2 over the last year, I really have no way of knowing if it’ll even work as-is.

Since we can’t unit test without refactoring, let’s go to step 2: integration tests. With integration tests, we should be able to write some kind of repeatable tests without changing much if any of the existing code.

Add Integration Tests

Integration tests are a way of unit testing a single set of data all together. These differ from end-to-end (E2E) tests in that we don’t want to make calls outside of our application and shouldn’t depend on side-effects.

To do any kind of integration testing, we’ll need to wrap our main code body in a function and pass it props. Those props could include the observable itself and any other external dependencies.

It’s not always necessary to pass in all your dependencies. For instance, we’re not going to use dependency injection for RxJS pipeline operators, but we need to mock side-effects like AJAX calls as they’re not testable otherwise.

As we’ve already seen, there are plenty of side-effects in this code, but the most-important one is calling out to the LIFX HTTP API. As long as we can capture the outbound data call, we can mock the response and be on our way.

For the purpose of writing a working integration test for this one project, I’ve tried to come up with a good pattern for hacking Node.js. I want something that won’t break when we refactor the code and properly tests the big-picture functionality.

Since we have no control over the fetch function, we’re limited to using a DNS proxy and our own HTTP server. We could modify the hosts file instead, but then we’d need to do that anytime we run our integration tests. Not gonna happen!

Failing to Add Integration Tests

After quite a bit of fiddling to get a DNS proxy working, I decided it wasn’t worth it. It required I had a valid TLS cert to fake the HTTPS call not to mention the fact that the integration test file ballooned in size. Without a ready-made library, I’d first have to write an integration test library, test it, then use it in this project. No way.

We’ve still got one more trick up our sleeve: E2E tests! But those won’t work either. E2E tests are supposed to be automated. For this particular project, that requires having a group of LIFX lights with color sensors attached in a clean room. It’s like writing software for a self-driving car to do a simple E2E test. It’s just not feasible.

The other way would be calling the AJAX function and pinging the lights to figure out if their current state matches our color calculations. That requires making sure we account for the Math.random calls. This idea’s not going to work in any reliable way either.

Reproducible Manual E2E Tests

I don’t have a dedicated QA person for this project, but I can definitely write reproducible tests that will run at my command. The manual part is that I’ll need to physically watch a light to see if it changes. While it’s not ideal, it’s the only solution that works.

There really aren’t any good ways of going about it with this app. It only triggers the light when it’s Halloween (to our knowledge) so it’d be pretty hard to test without commenting out isHalloween. Obviously, commenting out the Halloween check and committing it would make it so the lights flash no matter the date so we’ll need to do a tiny bit of refactoring to remove isHalloween from the pipeline only when running our tests.

After a small bit of non-destructive refactoring, we’ve got something testable:

lifxHalloween.js is our new index file. isHalloween has been moved to its own file, and flashRandomLight now contains the majority of our observable pipeline minus the isHalloween check. The only real change was the creation of the flashRandomLight function. Because we can test this function by itself now, this refactor is fairly safe.

Since we already noticed isHalloween and the rest of that observable chain requires some refactoring to get it testable, we’ll just have to assume it’s working as-expected.

Since everyone has their own opinions on which testing framework to use, I will use Node.js’s built-in assertion library for now. Normally, I’d use a test-runner as well, but in this case, we’re only testing one file so it’s fine.

I refer to this as a spec file (integration and E2E tests) rather than a test file (unit tests). Tests are for how individual modules should function whereas specs are high-level business specifications.

When it’s run, this test targets a particular light in your LIFX account: the one in your config file, and first checks the false case, then the true case. In both instances, it will log to the console immediately after running the test. This way, you can be sure which case caused the light to trigger.

But it all happens to fast for me to really tell so I ended up complicating the tests quite a bit and set them to run 2 seconds apart. At this point, I’m pretty upset I custom-wrote this with Node.js’s assertion library rather than using a more-familiar testing framework.

Good news, the code works! Now we’ve got something that is both reproducible and ensures, when refactoring, our code isn’t broken.

Conclusion

Write unit tests early-on or at least, write your code to be easily testable. Often, I don’t have time to unit test, but I’ve made sure to write my recent code as if I was going to unit test it.

In general, when refactoring, you need requirements. Refactoring code without the proper business requirements will almost always lead to missed requirements. Otherwise, you’ll end up having to make major architectural changes down the line when the business tells you the month you spent on that project didn’t solve the problem they wanted. It happened to a coworker of mine; don’t let it happen to you! Make sure you‘re aware of all the implications of the code you’re changing instead of assuming the business requirements.

Think about if you had to refactor this project. How much would you be willing to change if you didn’t know how it’d break?

Checkout Part 2 where we start our refactor!