I remember when I first discovered automated testing. I immediately wanted to apply it to all the projects that I was working on, but it didn’t work as well as I expected. In fact, it was a disaster, which is why so many developers shy away from tests after a few failed attempts. It turns out that adding tests to a project that never had any tests is a much bigger challenge than testing new software. People get thrown at the deep end of the pool, then either learn to swim or get scarred for life.
The problem with legacy applications is that they almost never follow SOLID or clean code principles. This makes them hard to unit-test. You’d need to refactor the code before you can test it, but how do you know that the refactoring won’t break existing functionality? You’d need tests to ensure that the refactoring goes smoothly. We are in a deadlock… or are we?
Characterization Tests
When I want to refactor code, I don’t start with unit tests. I start with characterization tests, which are meant to characterize the current software’s behavior. I usually write them by calling an HTTP endpoint or batch script, then inspecting the output.
For example, I want to make sure that product prices are correctly synchronized based on a daily CSV file that we fetch from an FTP server. I will write a test for that and when I refactor the underlying code, this test would still pass, because it does not concern itself with the internal working of the code. It only cares that the end result is the same. Characterization tests will survive a refactoring operation.
Note that these tests are meant to ensure that the existing behavior doesn’t change, not that the behavior is correct. This means that if I uncover bugs, I’ll document them in tickets and continue testing.
Later, when refactoring is done, I can update these tests to capture the correct behavior that is expected, then fix the bugs that pop up, along with any unit tests that I have written during the refactoring. They will then become acceptance tests.
Exploratory Testing
Once I built myself a safety net with characterization tests, I need to understand how the existing code works. For this, I use exploratory testing. These are often throw-away tests that I only use to understand the code, its design and where things can be refactored.
I don’t refactor everything at once, as that would be a 2-year effort on some projects. I try to find smaller components and make them testable, but not so small that the refactoring will be pointless. With some practice, I eventually found the sweet spot.
To decide how to refactor, I look at difficult tests. The following testing points usually indicate an underlying code design flaw.
Hard Dependencies
These are easy to spot. When my test makes me mock a static call or a new keyword, I know that I will need to move these things to constructor dependencies.
I will also extract an interface from these dependencies, so that my code would respect the “D” in SOLID: one should depend upon abstractions, not concretions. Dependency inversion is the first thing that I will introduce into any legacy to make my tests sane and any code changes a lot less painful.
Uncontrolled Variables
Sometimes, my test will depend on things that I cannot control, like the current time or a user’s IP address. This is a great opportunity to refactor the code to not explicitly depend on these things.
- I will find all places that use
mktime(), new DateTime(), etc.
- I will create and inject a
Clock interface as a dependency for these classes.
- I will replace the time creation with
$this->clock->now().
- I will write an implementation for it using
DateTimeImmutable and another using a value that comes from a database, which I can later leverage for controlling the time in acceptance tests. The acceptance test writes a date to the DB and the application under test reads it.
Not only do these interfaces allow for robust and repeatable unit tests, but I can also simulate things like cache expiry or other interval-based logic.
Mixed Concerns
Sometimes, a class requires me to mock 15 dependencies, but only a tiny fraction of them is used in each method. This often happens in the context of an MVC framework where it’s common to have one controller class with many actions.
$controller = new ProductController($dep1, $dep2, ...);
Perhaps this class is mixing too many concerns and should be split into smaller classes. Group the classes by the dependencies that they have, even if it means one method per class. Instead of having a class called ProductController, you’ll have ProductListHandler, ProductViewHandler, etc. The resulting classes will be much easier to test, and the code will be easier to debug and modify.
Long Methods
Is a method too long and requires 200 unit tests, each with a ridiculous setup? Myself from 10 years ago would have written several thousand lines of unit tests and then hoped to never have to understand them again.
Today, I will split that into small private methods, group them by dependencies and move whatever makes sense into separate classes.
Let’s say that each comment in this example corresponds to about 20 lines of code:
public function synchronizePrices(): void
{
// load CSV from file
// parse CSV
// create product array
// if product doesn't exist, throw exception
// look up product in database
// if price is different, update it
}
As a first step, I’ll extract the code into private methods and ensure that my characterization tests still pass:
public function synchronizePrices(): void
{
// This depends on the filesystem.
$csv = $this->loadCsvFromFile();
$parsedCsv = $this->parseCsv($csv);
$products = $this->getProductsFromArray($parsedCsv);
foreach ($products as $product) {
// These two depends on the database.
$this->findProduct($product);
$this->updatePrice($product);
}
}
Now I should move the code from those methods into new classes that I’ll inject through the constructor. More than 100 lines of code turn into this:
public function __construct(
ProductRepository $sourceProductRepository,
ProductRepository $targetProductRepository
) {
$this->sourceProductRepository = $sourceProductRepository;
$this->targetProductRepository = $targetProductRepository;
}
public function synchronizePrices(): void
{
$products = $this->sourceProductRepository->getAll();
foreach ($products as $product) {
$this->targetProductRepository->updatePrice($product);
}
}
Notice that both our repositories are implementations of the ProductRepository interface. Basically, I want to be able to synchronize between two repositories. This class doesn’t need to care how things are stored. I’ll just instantiate it with the CSV implementation on one side and the DB implementation on the other.
Maybe the third party will one day stop uploading a CSV and instead I’ll need to fetch the prices from a REST API, which I know just requires an additional implementation that can be done in an afternoon or two.
Of course, nothing prevents me from splitting things further inside of those concrete classes if they are still too complex. I will end up with smaller, testable classes.
More Refactoring
This is by no means an exhaustive list of all the refactoring that can be done. For more ideas, please read “Clean Code” by Robert C. Martin, “Working Effectively with Legacy Code” by Michael Feathers and “Modernizing Legacy Applications in PHP” by Paul M. Jones.
However, don’t try to do everything at once. As soon as you have a small class that depends on just a handful of interfaces and a rather short method, like in the synchronizePrices example, go write unit tests.
Unit Tests
Refactoring will make the synchronizePrices method much easier to test. Here is what the exploratory test might have looked like:
protected function setUp(): void
{
// 50 lines to mock hard dependencies
$this->synchronizer = new Synchronizer();
}
public function testSynchronizePrices(): void
{
// 50 lines to build CSV content
// create the CSV file and upload to FTP
// 200 lines of mocks for the ORM calls
}
Here is how I would generally write the test after the refactoring:
protected function setUp(): void
{
$this->csvProductRepository = $this->createStub('ProductRepository');
$this->databaseProductRepository = $this->createMock('ProductRepository');
$this->synchronizer = new Synchronizer(
$this->csvProductRepository,
$this->databaseProductRepository
);
}
public function testSynchronizePrices_WithProducts_WillUpdatePrices(): void
{
$this->csvProductRepository
->method('getAll')
->willReturn([$product1, $product2]);
$this->databaseProductRepository
->expects($this->exactly(2))
->method('updatePrice')
->withConsecutive([
[$product1],
[$product2],
]);
$this->synchronizer->synchronize();
}
In the unit test, I instantiate the class with a stub and a mock, then make sure to cover the two execution paths (with and without products). If I expect updatePrice to throw an exception, I can decide to add yet another test case and then implement a try/catch in the code.
As you gain a better understanding of what constitutes good code design, your tests will become increasingly easier to write.
Helpful Tools
In addition to all these tests, I also rely heavily on PHPStorm, which offers automated refactoring tools and a plethora of inspections that highlight any potential error that I’m making, like trying to call a method on a property that is possibly null: $entity->relationship->getName().
I go even further with PHPCS, PHPStan and Psalm, all at once, to break my CI in case I write something that has even a remote possibility of being incorrect. Because legacy code lights up my CI like a Christmas tree, these tools can be configured to only apply on files that you have edited.
I also had some great experience with RectorPHP, which can automatically improve things like moving static calls to constructor dependencies, add type declarations, etc. There are many great tools in there to give you a refactoring boost.
If you have a problem, there is a good chance that someone solved it years ago, wrote a book and maybe even created a tool for it. Keep exploring and have fun!
