When you refactor, do you have unit tests covering you? …If not, why not? If so, how do you know?
To me, it seems that the state of refactoring has gotten worse across the industry. Both managers and programmers and managers say the word “refactoring” more than ever. But they almost always mean, “I'm going to change a bunch of stuff. Then at the end, we need to make sure I didn't break anything.”
Improve your test writing “Flow.”
Sign up to get my test-oriented code snippets.
But that's not refactoring. That's rewriting.
What Is Refactoring?
Refactoring is moving in small steps, with each step verified by unit tests.
To see a demo of what I mean, watch my screencast Refactoring Demo: Is It More than Just Changing Stuff? In the comments, Vui Nguyen shared,
Wow, this is like “iterative refactoring”, which is a term I just made up :) I like the idea of refactoring so that the code still works every step of the way, instead of leaving things broken until the end.
If her term “iterative refactoring” helps you, use it. To me, it's simply “refactoring.” But since the term has been so misunderstood, call it “iterative refactoring” if it helps communicate the idea:
- You can break a large refactoring into small moves.
- A single refactoring move has many steps.
- After any step that may introduce a failure, we run the unit tests.
The key thing is that we don't refactor without tests, and the tests should never fail. We start in green, and end in green. And we check often.
Disclosure: The book links below are affiliate links. If you buy anything, I earn a commission, at no extra cost to you.
I keep saying this: The Refactoring book changed the way I code.
The Code Smell Almost Everyone Missed
Let’s return to the Code Smells exercise. I asked, “How many code smells do you see below?”
override func viewDidLoad() {
super.viewDidLoad()
if viewModel.isAlternateStyle {
// Alternate style needs different placeholder text.
textField1.placeholder = strings.field1Hint
textField2.placeholder = strings.field2Hint
setUpAlternateView()
} else {
textField1.placeholder = strings.field1Placeholder
textField2.placeholder = strings.field2Placeholder
setUpNormalView()
}
}Graham Lee, author of Test-Driven iOS Development, left a two-word reply:
No tests.
Code without tests is, by definition, legacy code. And maybe now you can see why. Without tests, we can’t do any refactoring.*
* With one exception: when you need to refactor something to make it testable. That’s when I pull out my copy of Working Effectively with Legacy Code.
That’s why, before I begin any refactoring, I first check to see if the code in question has tests …But how does one do that?
Code Coverage Can Lie
I started by enabling code coverage and running all unit tests. And look at that, I see 100% coverage for that method! So we’re good, right?
But the code coverage was a lie.
I knew that because when I first came across this viewDidLoad() method, it had no tests. And I wanted to start by extracting the helpers setUpNormalView() and setUpAlternateView(). (Reminder: I anonymized this code. Those aren’t the real method names.) So I wrote tests to cover both. One test sets isAlternateStyle on the view model, and the other doesn’t.
Now do you see the problem?
The lines that set the placeholder values aren’t tested. They just happen to be incidentally hit on the way through each branch. Because we test both sides of the conditional, we have “100% coverage.” But it doesn’t tell us if the code is safe to refactor!
Five years ago when I released my XcodeCoverage tool, I wrote this:
If the coverage shows a hole, I know that area is lacking unit tests.
In other words, where there is no coverage, we know that no tests cover that line. But the opposite is not true! When a line shows it has coverage, it doesn’t mean tests cover it in any meaningful way.
How Can We Check for Meaningful Coverage?
There is a way to see whether the tests actually check the code we want to refactor. Here’s what I wrote in Who Tests the Tests? Increase Confidence in Your Tests:
There’s a simple way to test the tests: Deliberately break something in production code.
So that’s what I did for the code in question. Look at the placeholder values:
override func viewDidLoad() {
super.viewDidLoad()
if viewModel.isAlternateStyle {
// Alternate style needs different placeholder text.
textField1.placeholder = "BOGUS" // 
textField2.placeholder = "BOGUS" //
setUpAlternateView()
} else {
textField1.placeholder = "BOGUS" //
textField2.placeholder = "BOGUS" //
setUpNormalView()
}
}As you can see, I’m simply setting all the placeholder values to "BOGUS". Then I ran the unit tests.
There were no test failures.
This proved to me that despite the 100% coverage, the tests didn’t actually cover those lines.
(This is “mutation testing.” On other platforms, mutation testing is automatic. The testing program introduces a change in the production code, and reruns the tests. For example, adding a ! to reverse a conditional, or swapping && with ||. Or as I did, assigning bogus values. I’d love to see automatic mutation testing for Swift, wouldn’t you?)
Don’t Refactor without Tests!
Refactoring without tests is like crossing a busy street blindfolded.
— Allen Holub (@allenholub) April 15, 2020
Once I knew that the placeholder lines weren’t covered, the path was clear: I added tests for them.
These tests failed until I restored the original code. This gave me the confidence that now, I had enough test coverage to begin refactoring.
Before any refactoring, do you ask: how good are the tests for that section of code?
Let’s summarize:
- Refactoring is moving in small steps, with each step verified by unit tests.
- Unit tests are essential for refactoring.
- Before any refactoring, ask: how good are the tests for that section of code?
- Lack of code coverage tells us that no tests cover a section of code.
- But having code coverage doesn’t tell us anything about the tests.
- Check test quality by deliberately breaking a section of code to see if any tests fail.
- Add any missing tests before starting to refactor.
- While refactoring, start in green, stay in green, and end in green.
Next time, we’ll start to refactor those code smells, but using small, small steps.
What questions do you have? What challenges do you face as you begin applying these refactoring principles? Please share in the comments below.
All Articles in this series
This is awesome, Jon! I’d always wondered how to verify that tests are strong enough, and intentionally breaking the production code makes perfect sense. Thanks!
Hi Jon,
Thank you for the post. I have a question on below rule.
“Before any refactoring, ask: how good are the tests for that section of code?”
How to refactor code (legacy code) which doesn’t have any unit tests?
Balaji, that’s a really good question. Because quite often, in order to make something testable, it needs to be refactored! I felt stuck on this question until I read Working Effectively with Legacy Code.
Let me summarize a few things:
Use automated refactorings
“Lean on the compiler” through type safety
Make snapshot tests for views. This is a type of “characterization test”.
When all else fails, do a manual test that confirms your change.
I’d say that you can refactor without tests. Unfortunately, at the same time, you can’t be sure if it was a refactor or a change. That’s why tests are very important.
The tricky parts are with the code that can’t be tested. Working Effectively With Legacy Code is a good source for many cases. However, legacy code contains such a variety of “solutions”, that no book is enough to cover them all.
Good tips about making sure that the tests are covering enough!
The particular techniques in Working Effectively with Legacy Code are important. But even more important is the mindset. The techniques are just demonstrations of that mindset. We have to continue to find new applications.
So, keep it up, Aleksi. When you bump into something that can’t be tested, find a way to carefully transform it into something that can.