So, another legacy code base just fell into your lap. Just when you thought you were done with that rusty old app and you’d move to greener pastures, life had other plans. And, since it’s not your first time around, you know what comes next:

• Looking for documentation, a greater quest than the search for the holy grail.
• Hours of cutting through the grass, trying to understand what each class is supposed to do.
• Asking yourself, “Is that even a word?” more times than you thought possible.

And all that before getting into the “fun” parts, setting up the local environment…oh, what a nightmare, right? Ok, ok, let’s not get all caught up in the drama, shall we?

Perhaps there’s a slightly better approach you can take to make this experience less painful (Even enjoyable?) Maybe there is something else you can try besides going step-by-step with XDebug and hoping for the best. Before you go any further, I want to be clear about what I’m offering, as it is not a universal silver bullet but rather a way to approach this task with a method.

Introducing: Characterisation tests.

What is a characterisation test?

A characterisation test is designed not to prove the correct functioning of a piece of code, but rather to discover what such code is doing. There are many tools and approaches you can take for this, mainly depending on the level of zoom you want/can take.

For the sake of brevity, I’ll discuss a kind of micro approach and then mention a few complementary tools/methodologies for you to look at by the end. The principles you’ll learn along the way will apply just the same.

So, let me introduce you to your new best friend:

<?php

namespace TripServiceKata\Trip;

use TripServiceKata\User\User;
use TripServiceKata\User\UserSession;
use TripServiceKata\Exception\UserNotLoggedInException;

class TripService
{
   /**
    * @throws UserNotLoggedInException
    */
   public function getTripsByUser(User $user) {
       $tripList = array();
       $loggedUser = UserSession::getInstance()-&gt;getLoggedUser();
       $isFriend = false;
       if ($loggedUser != null) {
           foreach ($user-&gt;getFriends() as $friend) {
               if ($friend == $loggedUser) {
                   $isFriend = true;
                   break;
               }
           }
           if ($isFriend) {
               $tripList = TripDAO::findTripsByUser($user);
           }
           return $tripList;
       } else {
           throw new UserNotLoggedInException();
       }
   }
}

While this class and all the related ones were created for teaching purposes, they’re heavily inspired by real-life ones, so you’ll feel right at home with them. When going through this article, try to resist the temptation of running the code in your head. Instead, follow along as I show you this approach to dealing with a piece of legacy.

First of all, let’s discuss our goal: to turn this spaghetti into nice SOLID-compliant code. But (of course there’s a but!) we wouldn’t be doing it in a very safe way if we didn’t start by building some test coverage around it, would we?

You must be thinking: “How can I test this if I have no idea what this code should do?”. Exactly. That’s the whole point of this exercise. We’ll be both figuring that out and building coverage at the same time. Is that cool or what?

Alright, let’s jump right into it. To get you started on the right foot, I created this Dockerfile you can use:

FROM php:7-cli-alpine
LABEL authors="Mauro Chojrin"

RUN apk update \
&& apk upgrade \
&& mkdir "/app/"\
&& wget https://raw.githubusercontent.com/composer/getcomposer.org/76a7060ccb93902cd7576b67264ad91c8a2700e2/web/installer -O - -q | php -- --quiet \
&& mv composer.phar /usr/local/bin/composer \
&& apk add --no-cache --virtual .build-dependencies $PHPIZE_DEPS \
&& pecl install xdebug-3.1.0 \
&& docker-php-ext-enable xdebug \
&& pecl clear-cache \
&& apk del .build-dependencies \
&& echo "xdebug.mode = coverage" >> /usr/local/etc/php/conf.d/docker-php-ext-xdebug.ini

WORKDIR /app

Start by cloning the repo and checking out the initial state tag:

• git clone [email protected]:mchojrin/TripServiceKata.git
• git checkout start_here.

Next:

• Build the image with docker build . -t trip-service
• Install dependencies with docker run –rm -v $(pwd):/app -it trip-service composer install.

Once the basics are in place, check your tools via docker run –rm -v $(pwd):/app -it trip-service bin/phpunit

If everything works, you should see something like:

And now the real work begins.

The first test

This probably feels really awkward, I know. Bear with me for a while, I promise it will pay off. What should your first test be named?

Here’s a little secret: it doesn’t really matter at this point. Just create a test like it_does_something() because, let’s face it, you don’t have much more information yet.

YOU LOVE PHP?

Explore the PHP Core Track

Learn more

Looking from the outside, the first thing you see, by analyzing the method’s signature, is the fact that it takes a User instance as a parameter and returns an array, so… let’s poke it and see what comes out!

How about a little test that goes like this:

<?php

namespace Test\TripServiceKata\Trip;

use PHPUnit\Framework\TestCase;
use TripServiceKata\Trip\TripService;

class TripServiceShould extends TestCase
{
   /**
    * @test
    */
   public function do_something(): void
   {
       $tripService = new TripService();
       $this->assertEquals([], $tripService->getTripsByUser(null));
   }
}

Nothing fancy, it’s just about getting your toe in the water and seeing if it’s hot. Remember: we’re not trying to verify the system’s behavior but to understand it or, more technically speaking, characterise it.

If you run this test, you’ll get this result:

Now it’s time to take that information and retrofit it into our test, so let’s dissect the output we got. The most important part, of course, is this:

TypeError: Argument 1 passed to TripServiceKata\Trip\TripService::getTripsByUser() must be an instance of TripServiceKata\User\User, null given, called in /app/test/TripServiceKata/Trip/TripServiceShould.php on line 16

Which means we need to provide a valid User instance to the getTripsByUser method. Ok, yes, we could have seen that right from the start, but again, I want to take this step-by-step to make it as explicit as possible.

So, let’s move one step up the complexity ladder. What is the simplest User instance we can create? No better way to tell than to look at the class constructor:

public function __construct($name)
{
   $this->name = $name;
   $this->trips = array();
   $this->friends = array();
}

Aha! We need to supply a name, which we can safely assume is a string, so our test will end up like this:

<?php

namespace Test\TripServiceKata\Trip;

use PHPUnit\Framework\TestCase;
use TripServiceKata\Trip\TripService;
use TripServiceKata\User\User;

class TripServiceShould extends TestCase
{
   /**
    * @test
    */
   public function do_something(): void
   {
       $tripService = new TripService();
       $this->assertEquals([], $tripService->getTripsByUser(new User('Mauro')));
   }

Now let’s hit that play button and see what comes out:

There’s good news and bad news. The good news is that the error changed. The bad news is that this doesn’t look like it’ll be easy to fix, does it?

Take a closer look. What does UserSession.getLoggedUser() should not be called in a unit test mean?

If you look at the method \TripServiceKata\User\UserSession::getLoggedUser it says it explicitly:

throw new DependentClassCalledDuringUnitTestException(
   'UserSession.getLoggedUser() should not be called in an unit test'
);

And where was it called during the test? Go back to \TripServiceKata\Trip\TripService::getTripsByUser and you’ll see

public function getTripsByUser(User $user) {
   $tripList = array();
   $loggedUser = UserSession::getInstance()->getLoggedUser();   
...

That looks like… a call to a Singleton! Oh no! It’s the nemesis of unit tests! We’re doomed! Not so fast: we still have a couple of tricks under the hood.

Breaking the dependencies

In order to introduce tests, first we need to make the class testable. To that end, let’s do a bit of refactoring. For starters, let’s create a new method in the TripService class:

protected function getLoggedUser(): ?User
{
   return UserSession::getInstance()->getLoggedUser();
}

And then, replace the direct call to the singleton in favor of the new addition:

public function getTripsByUser(User $user) {
   $tripList = array();
   $loggedUser = $this->getLoggedUser();   
...

Doesn’t look like much, does it? One small step for man…

But now the real magic begins. In case you didn’t notice, the method I asked you to create is protected, not private. This was no coincidence, as we will leverage OOP to make our mission possible.

How? By creating a new class that extends the original one, like this:

<?php

namespace Test\TripServiceKata\Trip;

use PHPUnit\Framework\TestCase;
use TripServiceKata\Trip\TripService;
use TripServiceKata\User\User;

class TestableTripService extends TripService
{


}

class TripServiceShould extends TestCase
{
   /**
    * @test
    */
   public function do_something(): void
   {
       $tripService = new TripService();
       $this->assertEquals([], $tripService->getTripsByUser(new User('Mauro')));
   }
}

And then overriding the new method:

class TestableTripService extends TripService
{
   protected function getLoggedUser(): ?User
   {
       return new User('John Doe');
   }
}

We have a hook to overcome the singleton problem. Basically, the idea is to test against the testable version of the TripService class, like this:

public function do_something(): void
{
   $tripService = new TestableTripService();
   $this->assertEquals([], $tripService->getTripsByUser(new User('Mauro')));
}

And now, when running the test:

🥳🥳🥳

Let’s do a little recap to try to make sense of what we have so far: We have a case where the method returns an empty array, which represents an empty trip list. So right there, we can make a small improvement to our test renaming it to a more accurate version: return_no_trips. Of course, that’s not the whole story, but we’ll improve it as we move along.

Next stop: coverage analysis. This will be our compass. Once we hit 100% coverage, we’ll be able to step into phase two: the actual refactoring.

Open the file coverage/report/Trip/TripService.php.html in a browser to see:

As you can see, there are a few lines that aren’t covered by our test. No surprise, really.

In principle, there are three paths for us to follow, each one corresponding to a scenario we’ll need to put together:

• One where the code will exercise lines 20-22
• One where the code will exercise line 26
• One where the code will exercise line 30

Of all the available options, the easiest one to pick seems to be the last, so let’s explore that angle.How can we make sure the exception is thrown? By looking at the code in line 18, it seems like all it would take for this to be the case would be to have $loggedUser be null.

How do we do that? The variable $loggedUser is initialized in line 16 with the value returned by the new family member.

In summary, let’s have the method \TripServiceKata\TestableTripService::getLoggedUser return null:

class TestableTripService extends TripService
{
   protected function getLoggedUser(): ?User
   {
       return null;
   }
}

Let’s run our test again…

Yay! There’s the exception we were looking for! 🙂 So now, let’s fix our test to make it pass:

<?php

namespace Test\TripServiceKata\Trip;

...
use TripServiceKata\Exception\UserNotLoggedInException;
...

   public function return_no_trips(): void
   {
       $this->expectException(UserNotLoggedInException::class);
       $tripService = new TestableTripService();
       $this->assertEquals([], $tripService->getTripsByUser(new User('Mauro')));
   }
}

Running the test:

Now we’re talking! 😎

Then again, let’s refine our test name. Let’s change it to: throw_exception_when_user_is_not_logged_in. If you refresh the coverage report, you’ll see:

It’s not a lot, but we are starting to see more green. For the next step, let’s try to come up with a new test that will help us cover lines 25-28.

For this, we’ll need:

• A $loggedUser that is not null
• The $isFriend variable to be false

The easiest way to achieve the latter will be to force $user->getFriends() to return an empty array so it will skip the loop altogether.

For that to happen, we need to pass a specific User (a particularly lonely one) as a parameter to the \TripServiceKata\Trip\TripService::getTripsByUser method. Of course, if we went straight to that, we’d lose our previous test, so we’ll need to find a way to keep the old test working and also get our new scenario ready.

What if we introduced a new property into our TestableTripService class and had the method return it? Like this:

class TestableTripService extends TripService
{
   public $loggedUser;

   protected function getLoggedUser(): ?User
   {
       return $this->loggedUser;
   }
}

This way, we can set the loggedUser to any value that suits the needs of the test at hand.

Before you run to the door, let me make a small clarification: I would think very carefully before having a class property be publicly accessible but, in this instance it makes my life easier and, most importantly, I am using this class as an intermediate step. It is not intended to be a part of the next pull request, so I can be a little more flexible than I’d be in other circumstances.

With these additions, there’s nothing to be changed to keep the existing test working, so let’s add a new one for the next scenario. My first attempt at this test would be something like:

/**
* @test
*/
public function do_something_when_user_has_no_friends(): void
{
   $tripService = new TestableTripService();
   $tripService->loggedUser = new User('John');
   $this->assertEquals([], $tripService->getTripsByUser(new User('Mauro')));
}

And hey, it looks like I got lucky as the returned value is precisely an empty array, so my next move is to rename the test to return_no_trips_when_user_has_no_friends.

Back at the coverage report, we see that we’re a step closer to our end goal:

Now it’s time to face the most difficult branch: we need to make the code execute line 26. For that to happen, $isFriend must be true, which implies that:

• The loop in line 19 must be executed at least once.
• The condition $friend == $loggedUser must be true at least once.

The first requirement is fairly straightforward: as long as the user passed as an argument has at least one friend, we’ve got this covered.

Now, about the second part, it all comes down to what $friend means.

$friend is the variable used in the loop, meaning it will iteratively be populated with the contents of $user->getFriends() so, to achieve our goal, we need $user to have a friend who happens to be the logged in user. In simple terms, we need $user to be a friend of the logged in user.

If we turn our attention to the User class, we’ll find that there’s a very convenient addFriend method in there. I believe we have enough to start building our next test.

As always, I’ll start with the simplest possible version of do_something, only this time I know a bit about the scenario, so I’ll jump ahead and name my test do_something_when_user_is_friend_of_loggedIn_user:

/**
* @test
*/
public function do_something_when_user_is_friend_of_loggedIn_user(): void
{
   $tripService = new TestableTripService();
   $tripService->loggedUser = new User('John');
   $user = new User('Mauro');
   $user->addFriend($tripService->loggedUser);
   $this->assertEquals([], $tripService->getTripsByUser($user));
}

If I run this test, the output is:

If we examine the file /app/src/TripServiceKata/Trip/TripDAO.php this is what we find:

<?php

namespace TripServiceKata\Trip;

use TripServiceKata\User\User;
use TripServiceKata\Exception\DependentClassCalledDuringUnitTestException;

class TripDAO
{
   public static function findTripsByUser(User $user)
   {
       throw new DependentClassCalledDuringUnitTestException('TripDAO should not be invoked on an unit test.');
   }
}

Hmmm… where have we seen this before? Oh yeah… the same happened with \TripServiceKata\User\UserSession, but this time you know what to do, so let me show you the updated version:

<?php

namespace TripServiceKata\Trip;

use TripServiceKata\User\User;
use TripServiceKata\User\UserSession;
use TripServiceKata\Exception\UserNotLoggedInException;

class TripService
{
   /**
    * @throws UserNotLoggedInException
    */
   public function getTripsByUser(User $user) {
       $tripList = array();
       $loggedUser = $this->getLoggedUser();
       $isFriend = false;
       if ($loggedUser != null) {
           foreach ($user->getFriends() as $friend) {
               if ($friend == $loggedUser) {
                   $isFriend = true;
                   break;
               }
           }
           if ($isFriend) {
               $tripList = $this->findTripsByUser($user);
           }
           return $tripList;
       } else {
           throw new UserNotLoggedInException();
       }
   }

   /**
    * @return User
    */
   protected function getLoggedUser(): ?User
   {
       return UserSession::getInstance()->getLoggedUser();
   }

   /**
    * @param User $user
    * @return mixed
    */
   protected function findTripsByUser(User $user)
   {
       return TripDAO::findTripsByUser($user);
   }
}

And, on the test side:

class TestableTripService extends TripService
{
   public $loggedUser;

   protected function getLoggedUser(): ?User
   {
       return $this->loggedUser;
   }
  
   protected function findTripsByUser(User $user)
   {
   }
}

Now the question is: what should this new method return? Judging by the usage of a TripDAO service, it looks like the goal of this method is to perform a database query to retrieve the trips done by the given user.

Based on this assumption, a good replacement seems to be $user->getTrips(), doesn’t it?

class TestableTripService extends TripService
{
   public $loggedUser;
  
   protected function getLoggedUser(): ?User
   {
       return $this->loggedUser;
   }

   protected function findTripsByUser(User $user)
   {
       return $user->getTrips();
   }
}

With these changes, we can make our tests even more relevant:

class TripServiceShould extends TestCase
{
   ...

   /**
    * @test
    */
   public function return_no_trips_if_user_has_no_friends(): void
   {
       $tripService = new TestableTripService();
       $tripService->loggedUser = new User('John');
       $aUser = new User('Mauro');
       $aTrip = new Trip();
       $aUser->addTrip($aTrip);
       $this->assertEquals([], $tripService->getTripsByUser($aUser));
   }
}

Re-run the tests to find:

This looks promising, doesn’t it? Now we’re ready to test the most complex scenario: when the users are friends:

/**
* @test
*/
public function do_something_when_user_is_friend_of_loggedIn_user(): void
{
   $tripService = new TestableTripService();
   $tripService->loggedUser = new User('John');
   $aUser = new User('Mauro');
   $aUser->addFriend($tripService->loggedUser);
   $aTrip = new Trip();
   $aUser->addTrip($aTrip);
   $this->assertEquals([], $tripService->getTripsByUser($aUser));
}

If we run this test the result would be:

Which means that the result was not an empty array, but contained an instance of Trip. Looking at the code for the TripService, the best candidate is an array containing the trips done by the user. So, this update to the test should do:

$this->assertEquals([$aTrip], $tripService->getTripsByUser($aUser));

Run the tests to confirm the assumption:

What does this tell us about the system? The TripService will return the trip list when users are friends. This is the updated version of the test, reflecting our newly acquired knowledge:

<?php

namespace Test\TripServiceKata\Trip;

use PHPUnit\Framework\TestCase;
use TripServiceKata\Exception\UserNotLoggedInException;
use TripServiceKata\Trip\TripService;
use TripServiceKata\User\User;
use TripServiceKata\Trip\Trip;

class TestableTripService extends TripService
{
   public $loggedUser;

   protected function getLoggedUser(): ?User
   {
       return $this->loggedUser;
   }

   protected function findTripsByUser(User $user)
   {
       return $user->getTrips();
   }
}

class TripServiceShould extends TestCase
{
   /**
    * @test
    *
    */
   public function throw_exception_when_user_is_not_logged_in(): void
   {
       $this->expectException(UserNotLoggedInException::class);
       $tripService = new TestableTripService();
       $this->assertEquals([], $tripService->getTripsByUser(new User('Mauro')));
   }

   /**
    * @test
    */
   public function return_no_trips_if_user_has_no_friends(): void
   {
       $tripService = new TestableTripService();
       $tripService->loggedUser = new User('John');
       $aUser = new User('Mauro');
       $aTrip = new Trip();
       $aUser->addTrip($aTrip);
       $this->assertEquals([], $tripService->getTripsByUser($aUser));
   }

   /**
    * @test
    */
   public function return_users_trips_if_is_friend_of_loggedIn_user(): void
   {
       $tripService = new TestableTripService();
       $tripService->loggedUser = new User('John');
       $aUser = new User('Mauro');
       $aUser->addFriend($tripService->loggedUser);
       $aTrip = new Trip();
       $aUser->addTrip($aTrip);
       $this->assertEquals([$aTrip], $tripService->getTripsByUser($aUser));
   }
}

There might be a more nuanced scenario to be covered: one where the user has trips and friends, but none of them is the logged-in user. While useful, it doesn’t add much to what I’ve already discussed, so I’ll leave it up to you to create it if you want. To see the evolution of the code one commit at a time, check out this and this.

What’s important about this is the fact that we have effectively:

• Built full coverage
• Understood the system’s functionality

Now, armed with these, we can safely approach our endeavor of making the system great again. That’s a topic for another article.

I’ll close with a few pointers to guide you forward. When dealing with the production code:

• Try to reduce the usage of temp variables.
• Extract loops and conditional bodies into their own methods.
• Add dependency injection to make testing/evolution easier.
• Refactor the tests to make them simpler to read/maintain.
• Have fun!