Adding Tests to Legacy Code Part 5

This is the fifth and final part to my Adding Tests to Legacy Code post. The other 4 parts can be found here:

After the tests and code we built, there’s just a few more parts we need to work on. There’s a few tests for the user parameter’s getter and setter and we need to make tests for the rest of parseParameters.

We’ll start off with the user parts. The $user_id property is set via the identifyUser method. We need to test this part. Here’s what the code could look like:

    /**
     * Ensures that identifyUser method sets a user id on the request model
     *
     * @return void
     *
     * @test
     */
    public function identifyUserSetsUserIdForValidHeader()
    {
        $request   = new \Request();
        $mockOauth = $this->getMock('OAuthModel', array(), array(), '', false);
        $mockOauth->expects($this->once())
            ->method('verifyAccessToken')
            ->with('authPart')
            ->will($this->returnValue('TheUserId'));

        $request->setOauthModel($mockOauth);

        $request->identifyUser(null, 'oauth authPart');

        $this->assertEquals('TheUserId', $request->user_id);
        $this->assertEquals('TheUserId', $request->getUserId());
    }

There’s a bit of new in this test. We’ve already created a mock like this in a previous test — we’re building the mock OAuthModel without calling the constructor, but the next series of calls are new.

In this case, we need to make the mock do something so we can test the logic in the identifyUser method. PHPUnit’s mock object has a fluent interface which means the next few lines are all setting expectations and behaviors on the mock. PHPUnit ensures that all the expectations we set must be met or the test will fail.

The first call is to ->expects($this->once(). This literally means that PHPUnit is going to make sure that we call this mock’s method exactly once. We’ve not told it what method yet, but that’s the very next call: ->method(‘verifyAccessToken’). So we want PHPUnit to make sure that verifyAccessToken is called exactly one time during this test. Next is a ->with(‘authPart’) call. This tells PHPUnit to make sure that when the method is called, a string of ‘authPart’ will be sent in. Lastly, the call ->will($this->returnValue(‘TheUserId’)) means that when this method is called it’s going to return a string of ‘TheUserId’.

The next thing we do is inject our mock OAuthModel into the request object. This means when identifyUser calls getOauthModel, it will get our mock object instead of building a new, real, OAuthModel.

Then we call identifyUser, passing in null and ‘oauth authPart’. We can pass in null and not mock out the database because we’ve already got a mock OAuthModel and the db adapter is optional if it’s already set. We’re using the ‘oauth authPart’ for the header because the method requires a two word header string with oauth (case-insensitive). The second word is what is passed into the verifyAccessToken and our mock expects it to be ‘authPart’.

The return value we told the mock to send back is what identifyUser sets into the $user_id property. So our first assert is the make sure it’s set to ‘authPart’. The second assert will ensure that our getter (which doesn’t yet exist) will also return the same value.

When we run PHPUnit now, it will *almost* pass, but you’ll see a fatal error calling a method ‘getUserId’ which doesn’t exist. So we’ll add it to the Request class:

    /**
     * Retrieves the user id that's been set on the request
     *
     * @return string|null
     */
    public function getUserId()
    {
        return $this->user_id;
    }

Now PHPUnit will pass. Next we want to make a couple of tests for the setter, one to make sure it sets the value we pass in, and the second to make sure it is fluent. These are pretty much like the other setter tests we’ve made so there’s no need for much explanation.

    /**
     * Ensures that the setUserId method is fluent
     *
     * @return void
     *
     * @test
     */
    public function setUserIdIsFluent()
    {
        $request = new \Request();
        $this->assertSame($request, $request->setUserId('TheUserToSet'));
    }

    /**
     * Ensures that setUserId can set a user id into the model that can be
     * retrieved with getUserId
     *
     * @return void
     *
     * @test
     */
    public function setUserIdAllowsForSettingOfUserId()
    {
        $request = new \Request();
        $user = uniqid();

        $request->setUserId($user);
        $this->assertEquals($user, $request->getUserId());
    }

Since PHPUnit shows two failures at this point, we can write some more code. The setter is pretty much what you’re probably expecting:

    /**
     * Sets a user id
     *
     * @param string $userId User id to set
     *
     * @return Request
     */
    public function setUserId($userId)
    {
        $this->user_id = $userId;

        return $this;
    }

The last part we will be building tests for is the parseParamters method. This method is called at the end of constructor. It’s responsible for parsing the query string (which we’ve already tested) and it’s also responsible for loading up the request body (from JSON) if the request is a POST or a PUT. Here’s the code:

    /**
     * What format/method of request is this?  Figure it out and grab the parameters
     * 
     * @access protected
     * @return boolean true
     */
    protected function parseParameters() {
        // first of all, pull the GET vars
        if (isset($_SERVER['QUERY_STRING'])) {
            parse_str($_SERVER['QUERY_STRING'], $parameters);
            $this->parameters = $parameters;
            // grab these again, keep them separate for building page hyperlinks
            $this->paginationParameters = $parameters;
        }

        if(!isset($this->paginationParameters['start'])) {
            $this->paginationParameters['start'] = 0;
        }
        if(!isset($this->paginationParameters['resultsperpage'])) {
            $this->paginationParameters['resultsperpage'] = 20;
        }

        // now how about PUT/POST bodies? These override what we already had
        if($this->verb == 'POST' || $this->verb == 'PUT') {
            $body = file_get_contents("php://input");
            if(isset($_SERVER['CONTENT_TYPE']) && $_SERVER['CONTENT_TYPE'] == "application/json") {
                $body_params = json_decode($body);
                if($body_params) {
                    foreach($body_params as $param_name => $param_value) {
                        $this->parameters[$param_name] = $param_value;
                    }
                }
            } else {
                // we could parse other supported formats here
            }
        }
        return true;
    }

Looking through this method, there’s a few parts that stand out as being hard or impossible to test or that point to new things we need to build. The first part is the pagination parameters. They are simple and straight-forward though so I won’t cover them in this post. The hard part is within the section of code that loads up the request body and parses the json. The body of the request is fetched with this call:

$body = file_get_contents("php://input");

This is an external dependency that will be really hard to test with. If we replace it with a method call, we’ve got hope though:

$body = $this->getRawBody();

The code for getRawBody is pretty simple:

    /**
     * Returns the raw body from POST or PUT calls
     *
     * @return string
     */
    public function getRawBody()
    {
        return file_get_contents('php://input');
    }

Now in case you’re wondering how this helps, I’ve got some explaining to do. PHPUnit allows you to create mocks where some of the methods are still real. So while that’s true, we’ve still got a little bit of a problem. Firstly, the parseParameters method is protected which means we cannot call it directly within the Request. Secondly it’s called within the constructor.

If it’s only called within the constructor, we have to call the constructor in order to call it. But the constructor is called when we first create the mock but before we’ve set up the reset of the expectations, mock methods, etc which means we cannot mock out the method we need to by the time it’s needed.

In general it’s not recommended that you test your protected or private methods. There are good reasons for this. For one, testing these methods means you’re tied into a specific implementation. By sticking to testing your public methods you can ensure that the interface that all the other code uses to interact with your class is solid. As long as it’s working, you should be free to change how the internals work. As long as the same inputs result in the same outputs, no other code should care.

Testing private and protected methods directly means you’ve now made it hard to change the implementation. With all that said, PHPUnit does have a way to test private and protected methods. I’d rather change parseParameters to be public.

    /**
     * What format/method of request is this?  Figure it out and grab the parameters
     *
     * @return boolean true
     *
     * @todo Make paginationParameters part of this object, add tests for them
     */
    public function parseParameters()
    {
        ...

I also want to make one more change. This time it’s to the constructor. I’d like the option to create a Request object but not run parseParameters on construction. That will allow mocking of part of the object we’re testing and testing parseParameters in isolation.

Here’s the test:

    /**
     * Ensures that if a json body is provided on a POST or PUT request, it
     * gets parsed as parameters
     *
     * @param string $method Method to use
     *
     * @return void
     *
     * @test
     * @dataProvider postPutProvider
     */
    public function jsonBodyIsParsedAsParameters($method)
    {
        $body = json_encode(
            array(
                'a'     => 'b',
                'array' => array('joind' => 'in')
            )
        );

        $inside        = new \stdClass();
        $inside->joind = 'in';

        $_SERVER['REQUEST_METHOD'] = $method;
        $_SERVER['CONTENT_TYPE']   = 'application/json';
        /* @var $request \Request */
        $request = $this->getMock('\Request', array('getRawBody'), array(false));
        $request->expects($this->once())
            ->method('getRawBody')
            ->will($this->returnValue($body));

        $request->setVerb($method);
        $request->parseParameters();

        $this->assertEquals('b', $request->getParameter('a'));
        $this->assertEquals($inside, $request->getParameter('array'));
    }

There’s some newness here as well so I’ll step through what’s happening in this test. Firstly it’s a data provider test, but I’m just using that to test that the json decoding stuff will happen for both POST and PUT.

    /**
     * Provider for methods
     *
     * @return array
     */
    public function postPutProvider()
    {
        return array(
            array('POST'),
            array('PUT')
        );
    }

Back in the test method. I’m using json_encode to create a json string that I can put in the request body for testing.

The $inside stuff is because when we encode and then decode the json, it ends up being an object. So the $inside variable is going to correspond to the array(‘joind’ => ‘in’) part.

Next we set the method into the $_SERVER[‘REQUEST_METHOD’] variable. I could also have called our new setVerb method on the request object after mocking it. After that I’m setting the content type to ‘application/json’. This is because in order to get to the actual decoding, $_SERVER[‘CONTENT_TYPE’] must be set and it must be set to ‘application/json’.

Next is the creation of the mock. In this case we’re mocking Request itself. I want it to all be real code except for the getRawBody that we added earlier. That will be mocked so we can return whatever we want. We can now set the expectation that the getRawBody method will be called once and it will return the json string we created in the $body variable.

The third parameter is array(false) which is means it will pass in false to the original Request class’s constructor. The false value will tell the constructor to not run the parseParameters method on construction. The modification to the constructor looks like this:

    /**
     * Builds the request object
     *
     * @param bool $parseParams Set to false to skip parsing parameters on construction
     */
    public function __construct($parseParams = true)
    {
        ...
        if ($parseParams) {
            $this->parseParameters();
        }
    }

By putting in a new argument in the constructor and setting the default to true, everything that’s already creating the Request the old way doesn’t need to change and it will work the same. For our test though, we can pass in false and it will skip running parseParameters.

Back in the test, the next thing I’m doing is setting the verb. Looking at it now, I think I intended to remove the super global setting and just use setVerb, but probably forgot to take out the super global. Finally I’m calling parseParameters manually.

The last two lines in the test are the assertions. The first makes sure the ‘a’ parameter is set to ‘b’ which was the first part of the array we json_encode’d at the top. The last is making sure we got the $inside object back for the ‘array’ parameter.

With that, the Request class has 100% code coverage. However, 100% code coverage does not mean that we’ve tested everything. It just means that every line has been run during the course of the tests. In order to feel completely confident that I’ve got everything thoroughly tested, I’d want to throw some tests around the paginationParameters variable that is created and set in parseParameters.

The bottom line is that there are likely uses of $paginationParameters, user_id and other public parameters in other classes that use Request. The $paginationParameters property is an array with ‘start’ and ‘resultsperpage’, but it’s also set to all the parameters that came in via the query string. So beyond adding a getter and setter for $paginationParameters, it may be good to add specific parameters (and getters and setters) for start and resultsperpage. Depending on what else may be used in the rest of the project, additional parameters may be added as well.

And with that, you’ve reached the end of this series. I hope it gives some good ideas about how you can start introducing tests to legacy code and refactoring to make code more testable. Please feel free to ask any questions you might have in the comments or leave suggestions or requests for future topics to cover.

Adding Tests to Legacy Code Part 4

This is part 4 in the series of adding test to legacy code. The other parts are here:

In part three, we left off with tests for the setter and getter for scheme. In the constructor, the only part left is some additional methods around authentication which don’t deal with any of the parameters set in the constructor. Without any further adieu, let’s get on to testing authentication.

The identifyUser method looks like this:

    public function identifyUser($db, $auth_header) {
        // identify the user
        $oauth_pieces = explode(' ', $auth_header);
        if(count($oauth_pieces) <> 2) {
            throw new Exception('Invalid Authorization Header', '400');
        }
        if(strtolower($oauth_pieces[0]) != "oauth") {
            throw new Exception('Unknown Authorization Header Received', '400');
        }
        $oauth_model = new OAuthModel($db);
        $user_id = $oauth_model->verifyAccessToken($oauth_pieces[1]);
        $this->user_id = $user_id;
        return true;
    }

Looking through this method, I see a few potential issues. The first is that the method is throwing the base PHP Exception. This is a no-no as it means we have no way to catch just the exceptions that this piece of code might throw since catching Exception will catch every exception. One piece I can do is to change the exception that’s thrown to something more specific.

The next issue is that we’re creating a brand new OAuthModel within the method. This makes it impossible for us to test the Request object’s identifyUser method without also testing a real OAuthModel object. Depending on the work that is done in the model this can be very undesirable. Fortunately we can make a small change which will allow us to inject a fake OAuthModel object for testing.

We simply need to change this line:

$oauth_model = new OAuthModel($db);

to

$oauth_model   = $this->getOauthModel($db);

This simple change means that we can provide something other than a literal new OAuthModel. Of course we’ll need a setter in addition to the getter for the OAuthModel. However, before we even get to the point where we’ll need that, there’s two code paths that cause exceptions to be thrown before we need the new line, so we’ll go ahead and write tests for those.

The first exception is thrown if $auth_pieces is not equal to two, and $auth_pieces is built by exploding the $auth_header string on space. Therefore if we pass in a header that has one word or more than two, we’ll get the first exception. We can write the test like this:

    /**
     * Ensures that an exception is thrown if the authorization header
     * doesn't have two parts
     *
     * @return void
     *
     * @test
     * @expectedException \InvalidArgumentException
     * @expectedExceptionMessage Invalid Authorization Header
     * @expectedExceptionCode 400
     */
    public function ifIdentificationDoesNotHaveTwoPartsExceptionIsThrown()
    {
        $request = new \Request();
        $request->identifyUser(null, 'This is a bad header');
    }

The test itself is very simple. We create a new Request and then we call the identifyUser method. The first parameter can be null since we don’t need a database to get to the exception. The “header” has five words which will cause an exception to be thrown.

The new parts are three new annotations. The first one:

@expectedException \InvalidArgumentException

This tells PHPUnit that we’re expecting that somewhere along the way in the test, an \InvalidArgumentException will be thrown. PHPUnit will take care of catching the exception and making sure it’s of the right type.

The next annotation is:

@expectedExceptionMessage Invalid Authorization Header

This tells PHPUnit to make sure that when it catches that exception that the message in the exception starts with the phrase “Invalid Authorization Header”. I say starts with because even if there’s a dynamic part at the end of your exception message, you can use this annotation to make sure the message is correct, at least up to the dynamic part.

The third annotation is:

@expectedExceptionCode 400

This annotation makes sure the exception code is as expected, in this case, 400. So even though there’s no calls to $this->assert*, the annotations set up assertions on our behalf.

If we run PHPUnit right now, there’s a failure because the exception PHPUnit catches is not of the right type. We can fix that by changing the code in the first “if” block to throw an \InvalidArgumentException. After that change the test will pass.

The next test is to make sure we can get the second exception. This exception occurs if there are two parts to the string (we need that to pass the first if statement) and the first part of the string is not OAuth. So we can make the second test look like this:

    /**
     * Ensures that an exception is thrown if the authorization header doesn't
     * start with oauth
     *
     * @return void
     *
     * @test
     * @expectedException \InvalidArgumentException
     * @expectedExceptionMessage Unknown Authorization Header Received
     * @expectedExceptionCode 400
     */
    public function ifIdentificationHeaderDoesNotStartWithOauthThrowException()
    {
        $request = new \Request();
        $request->identifyUser(null, 'Auth Me');
    }

Again, this test is very similar. Only the auth header we’re sending in and the expected message have changed. Of course when it’s run, it should fail because the code is still throwing a \Exception instead of an \InvalidException. Now we can change the exception thrown and run the tests and everything is happy again.

We now need to test the remainder of the method. The method retrieves our OAuthModel, sending in the database, and calls the verifyAccessToken method, passing in the second part of the auth header. That method returns the user id and sets it into the Request object.

So we have a few dependencies we’ll need to take care of. The first is the OAuthModel. We haven’t even built the getOauthModel method so we’ll need to write some tests for that. After that, we’ve got the database which needs to be faked out as well, or it will be, but I’ll get to that in a bit too.

First, let’s build the tests for the getter and setter for the OAuthModel. The constructor for the real OAuthModel typehints on a PDO object so it makes sense that we’d want to typehint PDO for the OAuthModel getter parameter. So first we want to make sure that if we call getOauthModel we’ll get back an OauthModel. The test looks like this:

    /**
     * Ensures that if getOAuthModel is called, an instance of OAuthModel
     * is returned
     *
     * @return void
     *
     * @test
     */
    public function getOauthModelProvidesAnOauthModel()
    {
        // Please see below for explanation of why we're mocking a "mock" PDO
        // class
        $db      = $this->getMock('\JoindinTest\Inc\mockPDO', array());
        $request = new \Request();
        $result  = $request->getOAuthModel($db);

        $this->assertInstanceOf('OAuthModel', $result);
    }

There’s some new parts in here. We need to mock out the PDO database adapter, but there’s a problem. PHPUnit’s getMock method has the option to pass in parameters to the real object’s constructor or to not call it at all. If we pass in parameters, they need to be real because the PDO constructor is doing work (connecting). If we tell PHPUnit to not call the PDO constructor, we end up with a different problem.

PHPUnit’s getMock method uses a trick where it creates a string that is interpreted as a serialized object of the type we want to mock and then unserializes it. Well, unserializing uses the PHP magic method __wakeup which in PDO is marked as final, so PHPUnit cannot extend the class.

To get around this, I created a “real” mock class that extends PDO which has an empty constructor so that we can call the constructor but not actually do anything. So at the bottom of the test file, I created a new class:

/**
 * Class to allow for mocking PDO to send to the OAuthModel
 */
class mockPDO extends \PDO
{
    /**
     * Constructor that does nothing but helps us test with fake database
     * adapters
     */
    public function __construct()
    {
        // We need to do this crap because PDO has final on the __sleep and
        // __wakeup methods. PDO requires a parameter in the constructor but we don't
        // want to create a real DB adapter. If you tell getMock to not call the
        // original constructor, it fakes stuff out by unserializing a fake
        // serialized string. This way, we've got a "PDO" object but we don't need
        // PHPUnit to fake it by unserializing a made-up string. We've neutered
        // the constructor in mockPDO.
    }
}

And yes, I actually did put all those comments in the code. Now, back to the test we need to write.

    /**
     * Ensures that if getOAuthModel is called, an instance of OAuthModel
     * is returned
     *
     * @return void
     *
     * @test
     */
    public function getOauthModelProvidesAnOauthModel()
    {
        // Please see below for explanation of why we're mocking a "mock" PDO
        // class
        $db      = $this->getMock('\JoindinTest\Inc\mockPDO', array());
        $request = new \Request();
        $result  = $request->getOAuthModel($db);

        $this->assertInstanceOf('OAuthModel', $result);
    }

The new part here is creating mock objects. Mock objects are objects that can be used in place of other objects. You can make them return values or do things that you want them to that may be very hard to make happen in a real-world, end-to-end testing scenario.

For instance, imagine trying to test code that uses the database. Suppose you want to make sure that the code can deal with the situation when the database is down or the connection cannot be established. If everything is real, the only way to get that test to work is to turn off the database server or pull the plug or block the traffic. None of these are acceptable things to do. But we can make a mock database connection that throws an exception or otherwise indicates that the connection failed and by using that we can determine if our code does the right thing when the database is down.

It works in the other way as well. Suppose you’re running tests that rely on a database. Suppose there’s a network glitch or the server goes down or something else that causes an unexpected result to be returned to the test code. So now we have a situation when our tests break but it’s not because the code is wrong. By mocking out these external dependencies we can ensure that the “database” always returns what we want or any other dependency does whatever good thing (or bad thing) we want.

Enough of that segue, back to the code. When we call the getMock method, the first parameter is the class we want to mock, in this case, we’re mocking our mock PDO, \JoindinTest\Inc\mockPDO. The second parameter is an array of the methods we need to mock out. In our case, passing in an empty array actually tells PHPUnit to mock all of the methods. Since we don’t need any of them, there’s no problem.

The simplest code that would make our test pass right now is this:

public function getOauthModel(PDO $db)
{
    return new OAuthModel($db);
}

Running unit tests should pass at this point, but right now we’ve just moved the problem of building a real OAuthModel from the identifyUser method into a new method. We want to be able to fake out the model with a mock which means we should be returning something we can set. To keep the interface simple, we’ll make sure that the getOauthModel can return a real object in case nothing has been set already.

If the model has been set, we can just return it and there’s no reason to need the $db value passed in, so we can make the optional, but if there’s no set model it requires a db adapter. We’ll make sure that an exception will be thrown if there’s no model set and no database adapter provided. We’ll write a test for that:

    /**
     * Ensures that if the getOauthModel method is called and no model is already
     * set, and no PDO adapter is provided, an exception is thrown
     *
     * @return void
     *
     * @test
     * @expectedException \InvalidArgumentException
     * @expectedExceptionMessage Db Must be provided to get Oauth Model
     */
    public function callingGetOauthModelWithoutADatabaseAdapterThrowsAnException()
    {
        $request = new \Request();
        $request->getOauthModel();
    }

Since there’s nothing new here, I’m not going to spend time explaining it.

We can now create the getOauthModel method.

    /**
     * Retrieves or builds an OauthModel object. If it is already built/provided
     * then it can be retrieved without providing a database adapter. If it hasn't
     * been built already, then you must provide a PDO object to put into the
     * model.
     *
     * @param PDO $db [optional] PDO db adapter to put into OAuthModel object
     *
     * @return OAuthModel
     * @throws InvalidArgumentException
     */
    public function getOauthModel(PDO $db = null)
    {
        if (is_null($this->oauthModel)) {
            if (is_null($db)) {
                throw new \InvalidArgumentException('Db Must be provided to get Oauth Model');
            }
            $this->oauthModel = new OAuthModel($db);
        }

        return $this->oauthModel;
    }

Of course we need to build the setter as well so let’s write more tests. I want to make sure the setter is fluent and that I can set an OAuthModel. So two more tests. First is the test for a fluid interface:

    /**
     * Ensures that the setOauthModel method is fluent
     *
     * @return void
     *
     * @test
     */
    public function setOauthModelMethodIsFluent()
    {
        /* @var $mockOauth \OAuthModel */
        $mockOauth = $this->getMock('OAuthModel', array(), array(), '', false);
        $request   = new \Request();

        $this->assertSame($request, $request->setOauthModel($mockOauth));
    }

Since I want the setOauthModel to type hint on OAuthModel, I need to make a mock. You might notice a few more parametes. As before, the first parameter to getMock is the class we’re mocking. The second is an empty array indicating all the methods should be mocked. The third parameter is what we want to pass to the original constructor, in this case nothing. The empty string is the default for specifying the name of the mock class. Since we don’t care about the name, sending in ” allows PHPUnit to set it. The final parameter, false, indicates that we want PHPUnit to mock the object without calling the original constructor. Other than that, this is a standard test for a fluent interface.

The second test ensure that what we pass in for the OAuthModel is what is returned from the getter.

    /**
     * Ensures that the setOauthModel method allows for an OAuthModel
     * to be set and retrieved
     *
     * @return void
     *
     * @test
     */
    public function setOauthModelAllowsSettingOfOauthModel()
    {
        /* @var $mockOauth \OAuthModel */
        $mockOauth = $this->getMock('OAuthModel', array(), array(), '', false);
        $request   = new \Request();
        $request->setOauthModel($mockOauth);

        $this->assertSame($mockOauth, $request->getOauthModel());
    }

This one is the same as far as getting the mock is concerned. We’re just calling the setter and then ensuring that what we passed in is what we got back out.

With these failing tests in place, we can build the setter.

    /**
     * Sets an OAuthModel for the request to use should it need to
     *
     * @param OAuthModel $model Model to set
     *
     * @return Request
     */
    public function setOauthModel(OAuthModel $model)
    {
        $this->oauthModel = $model;
        return $this;
    }

With that and since it’s getting late, I’ll wrap up part 4. Part 5 will be the conclusion as we’ll build tests for and code for a getter and setter for the user id that is set be the identifyUser method, and we’ll get into the rest of the functionality of the parseParameters method and lay out what the plans would be for future changes and refactoring that I didn’t originally do. Thank you for reading.

Adding Tests to Legacy Code Part 3

In part 1 of this article, I introduced the project and the purpose of this refactor and unit testing effort. Part 2 continued that effort and we added a few more tests and a couple more methods to the Request class. In this part, we’ll continue that effort and add more tests and do more refactoring to help with testability.

In the last parts, we were progressing through the constructor to make sure each bit of work the constructor does is covered and any methods in the Request class that deal with the variables that are set or introduced are also covered.

The next bit of work the constructor does is to split the $_SERVER[‘HTTP_ACCEPT’] value into an array and stores it in $this->accepts.

        if (isset($_SERVER['HTTP_ACCEPT'])) {
            $this->accept = explode(',', $_SERVER['HTTP_ACCEPT']);
        }

Looking through the rest of the Request class for uses of $this->accept, I found two methods: accepts and preferredContentTypeOutOf. We need to add some tests for each of those to ensure they are working properly and continue to work properly.

The first method, accepts, is here:

    public function accepts($header)
    {
        $result = false;
        foreach ($this->accept as $accept) {
            if (strstr($accept, $header) !== false) {
                return true;
            }
        }
    }

This method loops over the accept array and uses strstr on the value to determine if some $header pattern is found. The accept header will look something like this:

text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

Since the constructor explodes the header on a comma, we’ll have individual accept headers that look something like this inside of $this->accept:

$this->accept = array(
    'text/html',
    'application/xhtml+xml',
    'application/xml;q=0.9',
    '*/*;q=0.8'
);

The strstr function returns part of the string starting with the first match. Since the method is just using it to determine if the header exists somewhere, we could probably get away with using strpos which is even what the documentation recommends. I’ll wait until a later refactor to change that though. The goal here is to get everything I can tested and testable so that later on, larger refactors are easier and so I can do them with a high level of confidence that I haven’t broken anything.

To test this, I’d want to add both positive and negative tests. That means I want to make sure that if the browser doesn’t accept a certain mime type and we’re checking, we should get a false return value and if it does accept it, we should get a true value. Looking back at the method, you’ll see there’s a $result value that is set to false initially, but then never used again, and the only explicit return is if the header is found. We’ll fix that so that we’ll have a method that returns a boolean all the time.

    /**
     * Determines if the headers indicate that a particular MIME is accepted based
     * on the browser headers
     *
     * @param string $header Mime type to check for
     *
     * @return bool
     */
    public function accepts($header)
    {
        foreach ($this->accept as $accept) {
            if (strstr($accept, $header) !== false) {
                return true;
            }
        }

        return false;
    }

The method now looks pretty much like any standard “look for something in an array and return true if you find it or false otherwise” method.

To make tests for the accepts method, I pretty much want to make sure that the headers are correctly separated and the accepts method will correctly return whether the browser accepts the mime type or not.

I grabbed the accept header from my browser to use in testing:

text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

Presumably, we’re going to want to use accepts to make sure that the browser accepts something like json or html, so I wrote a series of asserts to make sure that the mime types I’d expect to be accepted are accepted and the ones that are not in there don’t come back as being accepted.

    /**
     * Ensures the accept headers are properly parsed
     *
     * @return void
     *
     * @test
     */
    public function acceptsHeadersAreParsedCorrectly()
    {
        $_SERVER['HTTP_ACCEPT'] = 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8';
        $request                = new \Request();

        $this->assertFalse($request->accepts('image/png'));
        $this->assertTrue($request->accepts('text/html'));
        $this->assertTrue($request->accepts('application/xhtml+xml'));
        $this->assertTrue($request->accepts('application/xml'));
        $this->assertTrue($request->accepts('*/*'));
    }

Since the accept header I set into the super-global doesn’t include ‘image/png’, the first assert makes sure that the accepts method returns false. All the others are in the header and they should return true.

In this article, I’ve been going back through the code I wrote and the thought processes I had when writing it, but while going through it, I’ve also noticed some parts that I would have probably done different or better. This is one of those test methods.

In this case, I’d turn this test into an @dataProvider test and do a single assertion in each. That code would look like this:

    /**
     * Ensures the accept headers are properly parsed
     *
     * @param string  $mimeType Mime type to look for
     * @param boolean $expected True if it should match, false otherwise
     *
     * @return void
     *
     * @test
     * @dataProvider mimeTypeProvider
     */
    public function acceptsHeadersAreParsedCorrectly($mimeType, $expected)
    {
        $_SERVER['HTTP_ACCEPT']
                 = 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8';
        $request = new \Request();

        $this->assertEquals($expected, $request->accepts($mimeType));
    }

Much simpler, and now easier to add additional test to as well. Since it’s a data provider test, I need to create a method that returns the data for this test to use. Since this is a slightly more complex example, I’ll include it because we’re dealing with a test that takes two parameters. When it comes right down to it though, data provider tests are pretty simple and the data provider itself is also easy. Usually it’ll be a method that returns a literal array of arrays, but in some cases you may want to programmatically create the data for the tests as well.

Here’s the new data provider method:

    /**
     * Provides mime types to the acceptsHeadersAreParsedCorrectly. The second value
     * is whether it should be found in the test or not.
     *
     * @return array
     */
    public function mimeTypeProvider()
    {
        return array(
            array('image/png', false),
            array('text/html', true),
            array('application/xhtml+xml', true),
            array('application/xml', true),
            array('*/*', true),
        );
    }

The second method that deals with the $this->accepts property is the preferredContentTypeOutOf method. The idea behind this method is that we pass in an array of formats that we want to or can deal with and if we find one we’ll return it. Otherwise we’ll return the string ‘json’. Here’s the code:

    /**
     * Determine if one of the accept headers matches one of the desired
     * formats and returns that format. If none of the desired formats
     * are found, it will return 'json'
     *
     * @param array $formats Formats that we want to serve
     *
     * @todo need some real accept header parsing here
     *
     * @return string
     */
    public function preferredContentTypeOutOf($formats)
    {
        foreach ($formats as $format) {
            if ($this->accepts($format)) {
                return $format;
            }
        }

        return 'json';
    }

The code to test this is pretty straight-forward. The plan is to do a positive test (make a match) and a negative test (get back ‘json’).

First, the positive test:

    /**
     * Ensures that if we're accepting something that the accept headers
     * say, then we get back that format
     *
     * @return void
     *
     * @test
     */
    public function preferredContentTypeOfReturnsADesiredFormatIfItIsAccepted()
    {
        $_SERVER['HTTP_ACCEPT'] = 'text/text,application/xhtml+xml,application/json;q=0.9,*/*;q=0.8';
        $request                = new \Request();

        $result = $request->preferredContentTypeOutOf(
            array('text/html', 'application/json')
        );

        $this->assertEquals('application/json', $result);
    }

For this test, I’m making sure that I don’t get the text/html because it’s not in the accept header, but I do get application/json which does exist.

For the non-matching test, it’s very similar but we’re checking to ensure that we get back the json default value. This test looks like:

    /**
     * Ensures that if the browser doesn't send an accept header we can deal with
     * we return json
     *
     * @return void
     *
     * @test
     */
    public function ifPreferredFormatIsNotAcceptedReturnJson()
    {
        $_SERVER['HTTP_ACCEPT'] = 'text/text,application/xhtml+xml,application/json;q=0.9,*/*;q=0.8';
        $request                = new \Request();

        $result = $request->preferredContentTypeOutOf(
            array('text/html'),
            array('application/xml')
        );

        $this->assertEquals('json', $result);
    }

The subtle difference in the two tests is that I’ve changed the accept header from the first test so that it’s no longer text/html, it’s now text/text. This causes the accept method to not find the mime type we’re looking for and we get back the ‘json’ default value.

At this point we’ve written tests for the $accepts property on the Request class. The next property manipulated in the constructor is $this->host. It’s a straight feed in from the super-global $_SERVER[‘HTTP_HOST’]. It is another public property though, with no getters or setters, so we’ll go ahead and make some tests for those as well.

The first test to add is very straight-forward.

    /**
     * Ensures host is set correctly from headers
     *
     * @return void
     *
     * @test
     */
    public function hostIsSetCorrectlyFromTheHeaders()
    {
        $_SERVER['HTTP_HOST'] = 'joind.in';
        $request              = new \Request();

        $this->assertEquals('joind.in', $request->host);
        $this->assertEquals('joind.in', $request->getHost());
    }

Without the last line of the test, things all pass. With the call to getHost() though, a method which does not currently exist, PHPUnit will have ended with a fatal error due to calling a method which doesn’t exist. Hooray TDD!

Back in the Request object, we’ll add the method.

    /**
     * Returns the host from the request
     *
     * @return string|null
     */
    public function getHost()
    {
        return $this->host;
    }

Now all the tests pass again. Since we’ve got a getter, we’ll need a setter. In sticking with the other setters we’ve built, I want to make sure that calling the setter sets the value and that it provides a fluent interface. This is nothing new, so here’s the tests:

    /**
     * Ensures that the setHost method is fluent
     *
     * @return void
     *
     * @test
     */
    public function setHostIsFluent()
    {
        $request = new \Request();
        $this->assertSame($request, $request->setHost(uniqid()));
    }

With this test in place, PHPUnit fails, but we can fix it by adding the following code to Request.php:

    public function setHost()
    {
        return $this;
    }

Here I’m writing the minimum code necessary to make the test pass. With the next test, the setter will be fully fleshed out.

    /**
     * Ensures that setHost can be used to set a host
     *
     * @return void
     *
     * @test
     */
    public function hostCanBeSetWithSetHost()
    {
        $host    = uniqid() . '.com';
        $request = new \Request();
        $request->setHost($host);

        $this->assertEquals($host, $request->getHost());
    }

With this test, there’s another failure because while the setter is returning the object, it is not actually setting anything. So we’ll need to make a small change to the setter:

    /**
     * Sets the host on the request
     *
     * @param string $host Host to set
     *
     * @return Request
     */
    public function setHost($host)
    {
        $this->host = $host;

        return $this;
    }

We’ve now got a fully functioning getter and setter for the host in the request. This is one step closer to the eventual refactoring of the Request class to make the public properties less visible. For now though, we’ll carry on with testing and minor refactors.

The next variable manipulated in the constructor is the scheme. The constructor looks for the $_SERVER[‘HTTPS’] value and if it’s on, sets the scheme to ‘https://’. Otherwise it’s ‘http://’. So we’re looking at probably two or three more pretty simple tests to add.

Looking through the code for uses of $this->scheme and other than use within another variable ($this->base) in the constructor, there is no reference to it. So we’ll need to create the tests for a getter and setter.

The first test ensures that we get a scheme of http:// by default:

    /**
     * Ensures that the scheme is set to http unless https is on
     *
     * @return void
     *
     * @test
     */
    public function schemeIsHttpByDefault()
    {
        $request = new \Request();

        $this->assertEquals('http://', $request->scheme);
        $this->assertEquals('http://', $request->getScheme());
    }

This of course fails when we run it because there’s no getScheme() method, so we’ll add it in the Request class.

    /**
     * Returns the scheme for the request
     *
     * @return string
     */
    public function getScheme()
    {
        return $this->scheme;
    }

Now the tests have passed so w00t! TDD FTW!

Next we need to make the tests for the setter for scheme. Again, I want it to be fluent and I want it to set the value that I pass in. Those two test look like this:

    /**
     * Ensures that the scheme is set to https:// if the HTTPS value is
     * set to 'on'
     *
     * @return void
     *
     * @test
     */
    public function schemeIsHttpsIfHttpsValueIsOn()
    {
        $_SERVER['HTTPS'] = 'on';
        $request          = new \Request();

        $this->assertEquals('https://', $request->scheme);
        $this->assertEquals('https://', $request->getScheme());
    }

    /**
     * Ensures setScheme provides a fluent interface
     *
     * @return void
     *
     * @test
     */
    public function setSchemeIsFluent()
    {
        $request = new \Request();
        $this->assertSame($request, $request->setScheme('http://'));
    }

In PHPUnit, we get a failure because the setScheme method doesn’t exist, but again the implementation is very simple:

    /**
     * Sets the scheme for the request
     *
     * @param string $scheme Scheme to set
     *
     * @return Request
     */
    public function setScheme($scheme)
    {
        $this->scheme = $scheme;

        return $this;
    }

I also want to ensure that the scheme can be set to valid schemes (even if I’m not going to validate the schemes are valid at this time. So I added another data provider test:

    /**
     * Ensures that the scheme can be set by the set scheme method
     *
     * @param string $scheme Scheme to set
     *
     * @return void
     *
     * @test
     * @dataProvider schemeProvider
     */
    public function schemeCanBeSetBySetSchemeMethod($scheme)
    {
        $request = new \Request();
        $request->setScheme($scheme);

        $this->assertEquals($scheme, $request->getScheme());
    }

The data provider itself is very simple as well.

    /**
     * Provides schemes for tests
     *
     * @return array
     */
    public function schemeProvider()
    {
        return array(
            array('http://'),
            array('https://'),
        );
    }

I think that about does it for part 3. In the next part we’ll get into some more advanced testing techniques including building and using mock objects to allow for faking of dependencies and some minor refactoring to make injecting these dependencies possible or at least easier.

Adding Tests to Legacy Code Part 2

In the first part of this series, I talked about how I started with building unit tests and refactoring code to make it more testable. There was a lot to introduce and cover so we only got through building a single test. In this part we will continue building tests and refactoring.

We started with testing the getParameter method in the Request class. There’s still another test to write there to ensure that we’re testing the behavior of the request code as thoroughly as we can though, and that’s to ensure that if the parameter is not set, we get back the default value we passed in. That test could look something like this:

    /**
     * Ensures that getParameter returns the default value if the parameter requested
     * was not set.
     *
     * @return void
     *
     * @test
     */
    public function getParameterReturnsDefaultIfParameterNotSet()
    {
        $uniq    = uniqid();
        $request = new \Request();
        $result  = $request->getParameter('samoflange', $uniq);

        $this->assertSame($uniq, $result);
    }

As mentioned in the previous post, the @test is there to make PHPUnit run it as a test. Inside the test, I’m creating a random string using uniqid. I’m then using that in the second parameter for getParameter which is where the default goes.

I’m setting a random value in here due to a habit I’ve gotten into with Test Driven Development. One of the goals in TDD is to write the minimum amount of code you can in order to get all the tests to pass. Technically, if this code was not written and I’d done something like pass in a default value of ‘foo’, I could write the getParameters code to just return the value ‘foo’ if the parameter didn’t pass. By changing the value of the default each time the test runs, I’m ensuring that the code cannot be something as simple and naive as that.

The assertion is that the default is what we get back since when the Request object is created, there’s no parameters, so we definitely should be getting the default.

Running PHPUnit results in:

PHPUnit 3.7.13 by Sebastian Bergmann.

..

Time: 0 seconds, Memory: 2.75Mb

OK (2 tests, 3 assertions)

So far so good, and at this point rather boring. Since getParameters is already in the original code I only have a few concerns that I’d need to check on later. Since $this->parameters is a public parameter, I’m hoping that everything that uses Request uses getParameters instead of accessing the $parameters property directly, but for the purposes of the first run at tests and refactor, I’m going to leave that alone. I would make a note that it’s something to check though.

It’s time to move on to the next part. In the constructor, the first value it looks at is $_SERVER[‘REQUEST_METHOD’]. This is where PHP will have GET or POST or PUT or DELETE or whatever HTTP verb is sent with the request. We’ve already wrapped this section with a check to ensure it’s set before using it, so that will avoid notices. It means this test should be pretty straight-forward as well.

    /**
     * Ensures that methods are properly loaded from the
     * $_SERVER['REQUEST_METHOD'] variable
     *
     * @param string $method Method to try
     *
     * @return void
     *
     * @test
     * @dataProvider methodProvider
     */
    public function requestMethodIsProperlyLoaded($method)
    {
        $_SERVER['REQUEST_METHOD'] = $method;
        $request                   = new \Request();

        $this->assertEquals($method, $request->verb);
    }

This test introduces a new concept, at least for this site. The new annotation @dataProvider tells PHPUnit that this test takes parameters and where it should get the parameters for the test. The “methodProvider” means we’ll need to create another method called “methodProvider” and give PHPUnit all the parameters it should use. I’ll cover that in just a minute. First let’s look at the contents of the test method.

Inside the test, we’re just setting the method to the super-global and instantiating the Request object. Then we’re ensuring that verb is set correctly on the object.

Here, I’ve got a concern. The $verb field is set to public and there’s no getter for it. With it being public, there’s no way we can guarantee anything, so refactoring should include adding a getter and setter for the $verb field. After that, we’ll want to go through all the other code that uses $request->verb and change it to use our getter. For the purposes of this set of tests though, we’ll just make sure the getter and setter are introduced and working so we can do the refactor part later.

To do this, I added another line to the test at the end:

$this->assertEquals($method, $request->getVerb());

If we run phpunit right now, we’ll have a problem:

PHPUnit 3.7.13 by Sebastian Bergmann.

..F

Time: 0 seconds, Memory: 3.00Mb

There was 1 failure:

1) Warning
The data provider specified for JoindinTest\Inc\RequestTest::requestMethodIsProperlyLoaded is invalid.
Method methodProvider does not exist


FAILURES!
Tests: 3, Assertions: 3, Failures: 1.

We need to create a dataprovider called methodProvider within our test. Dataprovider methods return an array of arrays. Each inner array represents a set of parameters that PHPUnit will send into the test method. In our case, for this method, we only need a single parameter for each time we want to run the test since there’s only a single parameter in the test method. The data provider will look like this:

    /**
     * Provides a list of valid HTTP verbs to test with
     *
     * @return array
     */
    public function methodProvider()
    {
        return array(
            array('GET'),
            array('POST'),
            array('PUT'),
            array('DELETE'),
            array('TRACE'),
            array('HEAD'),
            array('OPTIONS')
        );
    }

This means we’ll be running the requestMethodIsProperlyLoaded seven times and passing in one of the verbs each time. With the provider in place, we’ll run the tests again.

PHPUnit 3.7.13 by Sebastian Bergmann.

..PHP Fatal error:  Call to undefined method Request::getVerb() in /Users/dstockto/Projects/joind.in/src/api-v2/tests/inc/RequestTest.php on line 66

Fatal error: Call to undefined method Request::getVerb() in /Users/dstockto/Projects/joind.in/src/api-v2/tests/inc/RequestTest.php on line 66

This failure is a good thing in TDD. It is simply pointing to the fact that we haven’t created the getter yet. So back in the Request class, I added a getter:

    /**
     * Retrieves the verb of the request (method)
     *
     * @return string
     */
    public function getVerb()
    {
        return $this->verb;
    }

With that in place, we can run phpunit again:

PHPUnit 3.7.13 by Sebastian Bergmann.

.........

Time: 1 second, Memory: 2.75Mb

OK (9 tests, 17 assertions)

Things are looking good. Right now though, we’ve got no setter for the verb, so let’s add some tests (first) and then make a setter. I want the setter to allow setting a string into the verb field. I also want it to be fluent. This means that you can set a value into the Request object and then immediately call another method, such as $request->setVerb(‘POST’)->doSomethingElse();

First we’ll build the test that ensures that the value can be set:

    /**
     * Ensures that a verb can be set on the request with setVerb
     *
     * @param string $verb Verb to set
     *
     * @return void
     *
     * @test
     * @dataProvider methodProvider
     */
    public function setVerbAllowsForSettingRequestVerb($verb)
    {
        $request = new \Request();
        $request->setVerb($verb);

        $this->assertEquals($verb, $request->getVerb());
    }

This test is able to utilize the same data provider as we did in the last test, but it’s also really simple. We simply instantiate the object, call the setter and then make sure that the value we get from the getter is the same as the value we sent into the setter.

If I run PHPUnit now, I get a failure:

PHP Fatal error:  Call to undefined method Request::setVerb()

Again, in TDD, this is good, so now I can go write some code in Request:

    /**
     * Allows for manually setting of the request verb
     *
     * @param string $verb Verb to set
     *
     * @return void
     */
    public function setVerb($verb)
    {
        $this->verb = $verb;
    }

Now the tests are passing again.

PHPUnit 3.7.13 by Sebastian Bergmann.

................

Time: 0 seconds, Memory: 3.00Mb

OK (16 tests, 24 assertions)

So we now need to ensure the setter is fluent. We can do this by calling the setter and making sure it returns the object that we’re calling the setter on.

    /**
     * Ensure the setVerb method is fluent
     *
     * @return void
     *
     * @test
     */
    public function setVerbIsFluent()
    {
        $request = new \Request();
        $this->assertSame($request, $request->setVerb('GET');
    }

We’re making good progress and we’ve refactored slightly by adding a getter and a setter for the Request’s verb.

The next part of the constructor works with the $_SERVER[‘PATH_INFO’] variable. It puts values into the $path_info variable on the request object and explodes it into the $url_elements array. Both of these are public values, so again I want to make sure that I get some getters and setters in place so I can eventually make the variables protected.

Within the Request code, $path_info is set directly from the $_SERVER[‘PATH_INFO’] but not used anywhere else within the class. In a future refactor, I’d check if any of the other code uses that value and if not, I’d remove it. The $url_elements variable is another story though. It is used, so we need some tests. The getUrlElement method looks into this array and returns a part of it in a similar way to getParameter().

    /**
     * Retrieves a url element by numerical index. If it doesn't exist, and
     * a default is provided, the default value will be returned.
     *
     * @param integer $index Index to retrieve
     * @param string  $default
     *
     * @return string
     */
    public function getUrlElement($index, $default = '')
    {
        $index   = (int)$index;
        $element = $default;

        if (isset($this->url_elements[$index])) {
            $element = $this->url_elements[$index];
        }

        return $element;
    }

In this case, I want to make sure that the splitting happening in the constructor is working correctly and that I can get back the elements I want. I also want to make sure that returning the default value is working for url parts that don’t exist, so we’re looking at at least two more tests. Starting with the default value return, the test could look like this:

    /**
     * Ensures that the default value is returned if the requested index is
     * not found on getUrlElement
     *
     * @return void
     *
     * @test
     */
    public function getUrlElementReturnsDefaultIfIndexIsNotFound()
    {
        $request = new \Request();

        $default = uniqid();
        $result  = $request->getUrlElement(22, $default);

        $this->assertEquals($default, $result);
    }

In this case, I’m not setting any path, so the $request->url_elements would be an empty array. The number 22 was arbitrarily chosen as some value that’s bigger than 0 and thus won’t exist. I’m using uniqid() for the same reason as outlined before. Since no code is being introduced here, the tests still work (boring).

The next test is to set a path and then get back parts of it.

    /**
     * Ensures that url elements can be properly fetched with a call to
     * getUrlElement
     *
     * @return void
     *
     * @test
     */
    public function getUrlElementReturnsRequestedElementFromPath()
    {
        $_SERVER['PATH_INFO'] = 'foo/bar/baz';
        $request              = new \Request();
        $this->assertEquals('foo', $request->getUrlElement(0));
        $this->assertEquals('bar', $request->getUrlElement(1));
        $this->assertEquals('baz', $request->getUrlElement(2));
    }

In this case, I’m creating a path with three parts and then making sure that I get back those parts when I specify where they were. The constructor is exploding the string on ‘/’ so I’ll get foo in part 0, bar in part 1 and baz in part 2.

I think it would be a good idea to build additional functionality in the Request to allow for setting a path or even setting specific url elements, but since I didn’t do that in the pull request, I won’t here. Please feel free to do that and submit a pull request with tests if you’d like though. 🙂

And with that, you’ve reached the end of part two. I’ll continue going through the refactoring and testing of the Request object in part three.

Adding Tests to Legacy Code Part 1

One of the major challenges for building unit tests is when the code was built without tests. Often this code was not built with testing in mind but was built with speed and ease of development at the forefront.

In order to introduce unit tests and ensure you don’t break anything, you must work methodically, introducing new methods (using TDD if possible) and moving the code, step-by-step into a more testable form.

I spend a portion of my time contributing to a couple of open source projects and helping to clean up code and introduce good, high quality unit tests is one of the areas I try to focus on. It makes changing things and adding new features so much easier in the future.

One of the projects I contribute to is joind.in. Joind.in is a site for rating speakers and events. Users can enter information about their events (think conferences like ZendCon, php|tek or even your local user group) and the talks held at the events. Speakers can claim their own talks and attendees and can rate the speakers and give feedback to help them improve for future talks.

With that introduction, I’d like to walk you through the steps I did in order to refactor one small piece of code from the project into something that was more testable. Learning how to identify trouble areas in code and then work on a solution for how you can successfully refactor code is a valuable skill.

In the joind.in project there are a few main areas of the code. Most of the code for the website is a CodeIgniter based website with an API that allows for some external clients to retrieve data from the site. There’s another area with API V2 which is newer and more cohesive, but it wasn’t necessarily built with testability in mind either. That said, the project is moving more in that direction, so I felt my efforts would be better spent working on improving the testability of the V2 API.

Initially I wanted to work on files that rely on very few other files. That way if I’ve got tests in place for those, when I get to classes that use them, I’ll have tests and feel much more confident about refactoring other classes. So to start, I took a look at the Request class which the V2 API uses as an abstraction layer for information coming in from callers.

The original Request.php file is on github. There are a few things that should immediately stick out as potential issues.

Right at the top you should notice there’s a bunch of public variables. Public variables are great if you want to have easy access to everything and allow any part of your code to change the value of those variables to anything.

 public $verb;
 public $url_elements;
 public $path_info;
 public $accept = array();
 public $host;
 public $parameters = array();
 public $view;

Those are cause for some concern, but at least for the first pass, they are not going to affect the testability of the code, but the plan would be to introduce getters and setters (using TDD) which allow me to come back later and change the dependent classes to use those, and thus change the visibility modifier from public to protected.

Next up is the constructor. It’s fairly straight-forward and with PHPUnit’s ability to backup global variables, writing tests for it will not be too hard with a couple of exceptions. Here’s the code for reference:

   public function __construct()
    {
        $this->verb = $_SERVER['REQUEST_METHOD'];
        
        if (isset($_SERVER['PATH_INFO'])) {
            $this->url_elements = explode('/', $_SERVER['PATH_INFO']);
            $this->path_info = $_SERVER['PATH_INFO'];
        }

        if (isset($_SERVER['HTTP_ACCEPT'])) {
            $this->accept = explode(',', $_SERVER['HTTP_ACCEPT']);
        }

        if (isset($_SERVER['HTTP_HOST'])) {
            $this->host = $_SERVER['HTTP_HOST'];
        }

        if (isset($_SERVER['HTTPS']) && ($_SERVER['HTTPS'] == "on")) {
            $this->scheme = "https://";
        } else {
            $this->scheme = "http://";
        }

        $this->base = $this->scheme . $this->host;

        $this->parseParameters();
    }

The first line in the constructor sets the $this->verb value to that of a global $_SERVER[‘REQUEST_METHOD’]. That’s not terrible in and of itself, but if you look at almost all the rest, you’ll notice that they’re wrapped in an isset check which prevents notices should one of the values not exist. This is a good practice and I see little harm in fixing that right away, so I’d wrap that value.

        if (isset($_SERVER['REQUEST_METHOD'])) {
            $this->verb = $_SERVER['REQUEST_METHOD'];
        }

With that in place, the rest of the constructor looks like it’s pretty good with the exception of the very last line.

        $this->parseParameters();

That part is doing work and it’s doing it in the constructor. Doing work in the constructor is a practice you should strive to avoid if you can. It makes it difficult to test and will lead to problems later on, but we’ll come back to that later. For now, let’s get started testing each part of the constructor.

Since each of the ifs in the constructor are logically separated, we can test each part on its own. When writing tests it’s best to try and isolate each test so that if you get a failure, or perhaps a bunch of failures, you can see all of them at once and decide the best course of action. If we wrote a test that set all the $_SERVER values that are in use and then just did one assertion after another, the test would stop after the first failure. This means that even after we’d fixed that failure and we run the test again, we’ll get another new failure, but on the same test. This is not ideal.

So, on to the testing. Since there were no unit tests on the APIv2 code, I created a tests directory in the api-v2 directory and put in a few important bits. The first is the phpunit.xml file. This file configures PHPUnit with the directories and options you want to run it with which means instead of a hard to remember list of options, you can run phpunit simply by running

phpunit

The contents of the phpunit.xml can be seen on github. The file tells phpunit which directories have tests and which source code directories to pay attention to or ignore for code coverage.

Looking through the rest of the methods, it seems that for the most part, they are simple and are accessing variables that are set by the constructor. I feel that there’s two logical ways to go about getting tests written for the Request class. The first would be to write tests for the methods in the order they are in the file and the other would be to figure out what methods use the parameters in the constructor and build the tests for those. I’m not sure that it matters too much which way you choose.

Since there’s not a ton going on in the constructor short of the method mentioned above, I’ll just make my way through the methods and use code coverage at the end to ensure that I’ve run all the lines in the constructor.

The first method is this:

    public function getParameter($param, $default = '')
    {
        $value = $default;
        if (isset($this->parameters[$param])) {
            $value = $this->parameters[$param];
        }
        return $value;
    }

As you can see, the method is pretty simple. There are really only two code paths in the method. One is to ensure that we get back a parameter if the parameter is set. The other is to ensure we get back a default value if a particular parameter is not found.

Since we’re dealing with $this->parameters here, we need to find out how those get set so we can manipulate them for testing. Looking at the code, this work is done in the parseParameters() method. We only need to know enough about the method to determine how $this->parameters can get values. Right at the top is this code:

        // first of all, pull the GET vars
        if (isset($_SERVER['QUERY_STRING'])) {
            parse_str($_SERVER['QUERY_STRING'], $parameters);
            $this->parameters = $parameters;
            // grab these again, keep them separate for building page hyperlinks
            $this->paginationParameters = $parameters;
        }

The parseParameters method is going to take the value from $_SERVER[‘QUERY_STRING’], run it through parse_str and put the result in $this->parameters. We can use that to get value we want into it. We can start with the test for getting a value that exists.

To start with testing, I need to create a test class. I think it’s best to have one or more test classes per class you are testing and to only put a single class in a file. So to start, I created a test file called RequestTest.php in the /src/api-v2/tests/inc/ directory. Inside the tests directory, I make the test files and directory structure mirror the files and classes that I’ll be testing.

The filename is important as well. PHPUnit will find files that end with Test.php and use those as unit tests. If you don’t end the filename like that, PHPUnit won’t find it. You can still run them individually or you can configure your phpunit.xml file to run them, but this way, all my test files can be found automatically.

In the file, I created a namespace with a new class:

<?php
namespace JoindinTest\Inc;

require_once __DIR__ . '/../../inc/Request.php';

class RequestTest extends \PHPUnit_Framework_TestCase
{
}

We can now run the unit tests. By going into the tests directory and running phpunit, we’ll see something like this:

PHPUnit 3.7.13 by Sebastian Bergmann.

F

Time: 0 seconds, Memory: 2.75Mb

There was 1 failure:

1) Warning
No tests found in class "JoindinTest\Inc\RequestTest".


FAILURES!
Tests: 1, Assertions: 0, Failures: 1.

This is expected since we’ve got no tests yet. It’s now time to add some. As mentioned before, we’ll start by testing the getParameter method. The first test looks like this:

    /**
     * Ensures that if a parameter was sent in, calling getParameter for it will
     * return the value it was set to.
     *
     * @return void
     *
     * @test
     */
    public function getParameterReturnsValueOfRequestedParameter()
    {
        $queryString = http_build_query(
            array(
                'foo' => 'bar',
                'baz' => 'samoflange',
            )
        );

        $_SERVER['QUERY_STRING'] = $queryString;
        $request                 = new \Request();

        $this->assertEquals('bar', $request->getParameter('foo'));
        $this->assertEquals('samoflange', $request->getParameter('baz'));
    }

This test ensures that if there’s some “GET” parameters in a query string that parseParameters will be able to parse them and will put them into the $this->parameters field of the Request object. There’s a few things I want to make a note of as well though.

The first is in the doc comments at the top of the file. If you’re not familiar with this style of commenting for your php code, I’d recommend you take a look at docblox, php documentor or doxygen. All of these are capable of looking at your code, parsing comments like this and generating developer documentation of your code and tests. Most IDEs can also use information in the doc block comments to help with type aheads, return values and other useful information that can enable to you write code more quickly.

Within the doc block, there’s an annotation:

@test

PHPUnit doesn’t assume that every method in a test case is a test. It has two ways of determining that a method is a test method. One way is if the method starts with the word test. The second is if it has the @test annotation in the doc comment. I tend to use the @test annotation in almost every case. The reason has to do with the method name.

The method name, a few lines down, is, as you may have noticed, rather long.

getParameterReturnsValueOfRequestedParameter()

The reason for this is that PHPUnit can take camelCased method names and split them into words which can be output in an easy to read format, specifically if you use the ––testdox parameter for phpunit.

JoindinTest\Inc\Request
 [ ] Get parameter returns value of requested parameter

The value in the square brackets indicates whether the test passed or failed. If you name your tests appropriately, the testdox output can read like a list of specifications that your code should adhere to. An X in the square brackets means the test passed.

JoindinTest\Inc\Request
 [x] Get parameter returns value of requested parameter

So back to the test. I start out by building up a query string by using PHP’s http_build_query function. This results in a string that looks like

foo=bar&baz=samoflange

This is the sort of string that parse_str is going to parse.

Next I set this value into the $_SERVER[‘QUERY_STRING’] variable.

After that, I create a new Request object. Since the constructor is going to look at the $_SERVER[‘QUERY_STRING’] this should be all I need to get things working.

The next two lines are ensuring that there’s a parameter named ‘foo’ which has a value of bar and a parameter named ‘baz’ which contains a value of ‘samoflange’. PHPUnit has a whole slew of different ‘asserts’ which are used to test that values that come from the code we are testing match an expected value. For nearly every assert* call, the expected value is first, the value we are testing is second. The last parameter is an optional message that can be sent back if the test fails. The exception to this parameter ordering is when the value that’s being tested against is mentioned in the assert* method name. A few examples of this would be assertNull or assertTrue.

As this article is already getting long, I’ll end this here and I hope you’ll join me for the next part where I continue working through the Request object and building out the tests.

Benefits of TDD

If you read my other post, “What is test driven development?” I left off promising to talk about the benefits of TDD. Well, here we are.

I feel the benefits of TDD far outweigh the costs. I am also guessing that this will not be my only article about the benefits. As we discussed earlier, testing your code is a great thing and that’s definitely one of the benefits, I want to go a bit beyond that.

Of people who actually do write unit tests, I’d guess the majority of them do so after the code has been written. There’s a few issues with that. First of all, if the person writing the tests is not the person who wrote the code then the intention behind the code may not be understood. This can lead to tests that are written with little or no understanding of what the correct functionality of the code should be. Typically tests written in this way have a developer making a call into a method with some random parameters, making a note of the return value and then creating a test that asserts that when those values are passed in, the value that comes out is the one that came out.

This doesn’t help with exposing problems in the code and can actually cause tests to be written that solidify bugs in the code. As a trivial example, imagine a calculator class that has a bug where adding 2 and 2 returns 5. If the intention behind the code was not understood and a test was written to assure that 2+2=5 then it now becomes more difficult to fix the bug later because the test and the correct execution of the code are contrary to each other.

Help maintainers understand the intention behind the code

By writing the tests first, you help give the developers who are maintaining the code a clue to the intention of the code. This helps prevent what’s known as “programming by coincidence.”

When the tests are written first when the code is fresh on your mind, it’s much easier to think about the valid range of inputs, expected outputs and the expected actions the code should take when the input is outside the range of what is considered valid. This brings validation and proper handling of parameters to the forefront. If you’ve ever tested methods that didn’t bother with validation or defining how values will be handled, you might know that it can be difficult to test or change things as you may not be certain that some inadvertent functionality is not being used somewhere in the application.

Bring validation and proper data handling concerns to the forefront.

Next, writing the tests first is fun. Well, as fun as writing tests is. Writing tests after the fact is boring. Test code tends to be simple and straightforward and typically doesn’t require a lot of thought. When you’re writing tests first, you get to exercise the creative designing side of your brain. To me, the thinking, building, designing part of development is the most fun. Typing in tests is not all that intellectually stimulating. Plus making the red bar turn green is fun as well.

Writing the tests first is fun.

When you write your tests first and then build your code to make the tests pass, you’ll typically end up with a better designed, well-thought-out API. If your tests are hard to write because the APIs you created are hard to use, you’re not going to want to use them and neither will anyone else. Code that’s simple to test tends to be easier to use and easier to maintain.

Better APIs come from writing testable code.

As you write more tests, the wisdom of various software design principles like DRY, SOLID, KISS and others really make sense. Building your code to prefer composition over inheritance means that you can actually extend the functionality of the class in ways  that are much more difficult with inheritance as well as being easier to test.

When creating unit tests, true unit tests, only a single class should be under test at once. This means that if you’re building classes that need functionality from other classes (composition) such as databases, services, file systems, web service calls, etc, then being able to mock out these classes can be a huge benefit for testing. By designing your classes to use interfaces and then coding just to those interfaces you will allow others (or yourself) to come along later and provide a drop-in replacement for your initial classes in an easy and extensible way. Without the interfaces your or another developer would need to dig through how the code works in order to understand all of the functionality that a replacement class would need to have which means that often the code is stuck how it was originally developed.

By doing TDD and discovering how to make better, more testable and more maintainable code, you become more valuable as a developer. That’s right: TDD will make you a better developer.

TDD will make you a better developer.

There are many other benefits that I’m sure I’ll touch on in more detail in future essays as well, but I think these are some excellent benefits. Stay tuned for more TDD!

 

 

The Cost of Doing TDD

TDD is not a silver bullet. I am in no way promising that if you start doing TDD that your software will be perfect, your startup will be rolling in cash with investors beating down the door or that you’ll even like it. I *hope* you’ll like it, but if you decide you don’t like it, I’m not going to take it personally.

There are some costs to doing TDD. First of all, writing tests takes some time. If you follow the steps exactly as prescribed, it can sometimes take a lot of time, especially in the beginning. This time cost decreases the more you do it though. You’ll become more adept at writing tests, and designing and developing testable code.

However, despite getting better and faster at all of this, the fact remains that writing code and writing tests is slower than writing just code. A study in 2008 by Microsoft found that teams utilizing TDD took 15-35% longer to implement features compared to teams that didn’t use TDD. Other studies have come to similar conclusions. However the same studies also found that the teams that did do TDD also ended up with a defect count that was 40-90% lower than the teams that didn’t do TDD.

Considering that the majority of the cost of a software project is in the maintenance phase and not the development phase, I feel that this is an excellent tradeoff.

So basically, I feel that there’s two main costs in TDD. The first is the cost of changing mindset and ramping up skills to get used to the TDD process. The second is just the cost of writing the tests. Both of these may vary depending on your skill level and commitment level, but there will be some cost. Hopefully this ends up being the only downer post on the site. 🙂

What is Test Driven Development?

Test Driven Development, or TDD is a software development methodology that, as the name implies, has you writing tests before you write or change the code. You might be wondering how this is possible or how it makes sense. This blog aims to help you understand how TDD works and how it can work for you.

A wise man once said, “If debugging is the act of removing bugs from software, then programming must be the act of putting them in.”

I think nearly everyone can agree that testing your code is important to have a sense of whether it does what you want and just as importantly, doesn’t do what you don’t want it to do. Testing itself comes in two basic forms: manual testing and automated testing. Manual testing is done when humans use your software and make determinations about whether it’s doing what it should or not. Manual testing is the only way to detect some sorts of defects, so I’m certainly not advocating firing your QA team. However, manual testing also doesn’t scale very well. The larger and more complex your software gets, the harder it is to test manually. The only way to scale manual testing is to hire more people to do testing. That’s why the goal should be to automate as much of your testing as possible.

Computers are very good at doing repetitive, boring and tedious work. This work can include running the same sets of tests over and over and over. They can also do this very quickly, so the more work we can make the computer do, the better. It leaves the QA team free to find the really weird issues and to make sure things work well from the user’s perspective.

So, how do you do TDD anyway? Well, the standard way is to follow these steps:

  1. Write a test that should pass if your software does something it should do, or doesn’t do something it shouldn’t do.
  2. Run the test, see if fail
  3. Write or change the minimum amount of code to make the test pass
  4. Lather, rinse, repeat.

You keep going this way until the feature is built, the defect is fixed or whatever you’re trying to accomplish is done. At the end of it, you’ve got a suite of tests showing that your software behaves as it should. This is a different approach than writing tests after the fact. I think it has several great benefits which I plan on getting into in future posts.