5
\$\begingroup\$

I've just created my first tests using mock classes. So far my tests are running much faster and now I guess I'm solely focusing on a single class (in this case I'm testing UserTable and mocking the DatabaseAdapter instance I inject into it)

I haven't proceeded to do the write tests for the other methods of this class (just testing "create" method for now), I was hoping someone could comment on my tests for this method. It simply creates a new record in the user table. I'm trying to test all possibilities that I should be testing. I'm a little new to this and not quite sure if I've got it right yet.

<?php

use app\models\UserTable;
use app\models\User;

/**
*    UserTableTest
*/
class UserTableTest extends PHPUnit_Framework_TestCase
{
    protected $dbAdapter;
    protected $userTable;

    public function setUp()
    {
        // create mock database adapter
        $this->dbAdapter = $this->getMockBuilder('framework\Db\DatabaseAdapter')
            ->disableOriginalConstructor()
            ->getMock();

        // create our instance of the class to be tested
        $this->userTable = new UserTable;
    }

    // create method

    public function testCreateMethodWhenSuccessfullyInsertsRow()
    {
        // set mock class

        // set the mock object's methods for this test
        $this->dbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(true) );

        $this->userTable->setDatabaseAdapter($this->dbAdapter);

        // perform test

        // assert user is created when valid argument is passed
        $result = $this->userTable->create( array() );

        $this->assertTrue($result);
    }

    public function testCreateMethodWhenFailsToInsertsRow()
    {
        // set mock class

        // set the mock object's methods for this test
        $this->dbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(false) );

        $this->userTable->setDatabaseAdapter($this->dbAdapter);

        // perform test

        // assert user is created when valid argument is passed
        $result = $this->userTable->create( array() );

        $this->assertFalse($result);
    }

    /**
    * @expectedException InvalidArgumentException
    */
    public function testCreateMethodWithInvalidArguments()
    {
        // set mock class

        // set the mock object's methods for this test
        $this->dbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(false) );

        $this->userTable->setDatabaseAdapter($this->dbAdapter);

        // perform test

        // pass invalid argument, should be an array of values
        $result = $this->userTable->create('invalid argument type');
    }

    /**
    * @expectedException RuntimeException
    */
    public function testCreateMethodWithMissingDatabaseAdapter()
    {
        // assert user is created
        $result = $this->userTable->create( array() );
    }
}
\$\endgroup\$

migrated from programmers.stackexchange.com Aug 13 '14 at 17:42

This question came from our site for professionals, academics, and students working within the systems development life cycle.

5
\$\begingroup\$

Except for some (mostly experience related issues) I commend your efforts. Well done!

The Short Version

Specific Points

  • use app\models\User; is not needed as the User class is not used (or mocked) in this test.
  • The comments before your asserts don't always seem to match what you are actually asserting. I think you'd be better of using those comments as the test method name rather that having them as a comment in the function.
  • On the subject of names, I'd suggest using words like should and when to make the test method name state exactly what it expects the system under test to do. That way when a test fails you'll rarely have to look in the test to understand what is broken.
  • The first two test methods are basically testing the same thing. Whether or not the output of the create method matches that of the insertRow mock. This could be simplified into one single test method fed by a dataProvider, more on this below.
  • You don't need to catch the result of the create in the methods that test for exceptions. Unused variables in code are bad and test code is still code.

Although it is impossible to tell without the class under test, you seem to have covered all logic paths:

  • Insert Succeeds
  • Insert Fails
  • Valid Arguments Provided (implicitly through previous methods)
  • Invalid Arguments Provided
  • DbAdapter Provided (implicitly)
  • DbAdapter Not Provided

You could consider making the implicit tests more explicit.

Incorporating these points your code would look a bit like this:

<?php

namespace app\models;

/**
 * Tests for the UserTable class
 *
 * @coversDefaultClass app\models\UserTable
 * @covers ::<!public>
 */
class UserTableTest extends PHPUnit_Framework_TestCase
{
    ////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    protected $userTable;


    /**
     * create our instance of the class to be tested
     */
    final public function setUp()
    {
        $this->userTable = new UserTable;
    }

    /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    /**
     * @covers ::create
     * @covers ::setDatabaseAdapter
     * 
     * @dataprovider provideBooleans
     */
    final public function testUserTableShouldOnlyCreateUserWhenInsertRowSucceeds($insertRowStatus)
    {
        $userTable = $this->userTable;

        // set mock class
        $mockDbAdapter = $this->getMockAdapter();

        // set the mock object's methods for this test
        $mockDbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue($insertRowStatus) );

        $userTable->setDatabaseAdapter($mockDbAdapter);

        // perform test

        // assert user is created when valid argument is passed
        $actual = $userTable->create( array() );

        $this->assertEquals($insertRowStatus, $actual);
    }

    /**
     * @covers ::create
     * @covers ::setDatabaseAdapter
     * 
     * @expectedException InvalidArgumentException
     *
     * @dataprovider provideInvalidArgumentTypes
     */
    final public function testUserTableShouldOnlyCreateUserWhenGivenValidArguments($invalidArgumentType)
    {
        $userTable = $this->userTable;
        // set mock class
        $mockDbAdapter = $this->getMockAdapter();

        // set the mock object's methods for this test
        $mockDbAdapter->expects( $this->any() )
            ->method('insert')
            ->will( $this->returnValue(false) );

        $userTable->setDatabaseAdapter($mockDbAdapter);

        // perform test

        // pass invalid argument, should be an array of values
        $userTable->create($invalidArgumentType);
    }

    /**
     * @covers ::create
     * 
     * @expectedException RuntimeException
     */
    final public function testUserTableShouldOnlyCreateUserWhenGivenDatabaseAdapter()
    {
        $userTable = $this->userTable;
        $userTable->create( array() );
    }

    ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    final public function getMockAdapter()
    {
        // create mock database adapter
        $mockDbAdapter = $this->getMockBuilder('framework\Db\DatabaseAdapter')
            ->disableOriginalConstructor()
            ->getMock()
        ;
    }

    /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
    final public function provideBooleans() 
    {
        return array(
            'true' => array(true),
            'false' => array(false),
        );
    }

    final public function provideInvalidArgumentTypes() 
    {
        return array(
            'string' => array('invalid argument type'),
            'boolean - false' => array(false),
            'boolean - true' => array(true),
            'null' => array(null),
        );
    }    
}

The Long Version

Should you feel in the mood for a bit more depth and education, I have a more thorough review below. These are all things I'd point out to my teammates, if this code had passed my desk at work.

This may be a bit much to take in, keep in mind I'm a rather critical developer and I always find room for improvement, even (or maybe especially) in my own code. Feel free to ask for clarity if any point is unclear.

Also, just ignore any points that don't resonate with you. There's no use in taking advice you don't believe in.

Points in General

Don't Start test functions with the word test

Use annotations to tell PHPUnit that this function is a @test. This allow the name of the test function to be more descriptive.

I've actually changed my mind on this one. By starting test method names with test they are both more explicit and recognizable. The same goes for starting provider methods with provides.

Don't create your mock in the setup function

Unless you need to insert the mock into the constructor of your class under test, don't create your mock until you need it. I'd just create a getDatabaseAdapterMock function and call that from the test functions. You could consider making specific methods for the various states the mock is in but (depending on the size and complexity of the class under test) that might be a step to far.

Add @covers annotations to your docblocks

Not only does this set you up to understand which parts of your code might still need attention, it also means you don't have to mention the name of the method you are testing in the name of function in your test class.

As of version 4, PHPUnit will tell you if "risky" code has run for a test. That is to say, code that either isn't mentioned by a @covers block or code that is mentioned but didn't run.

Another trick I tend to use is adding a @coversDefaultClass to the class-level docblock along with a @covers ::<!public> (see the @covers docs). That way you only need to specify public API in the test method doc blocks.

Don't use $this->any() when the count is important

A mock is also an assert. You want to be as specific with your assertions of (public) API as you can. If it does not matter whether insert is called once or a hundred times than it's fine to use any. In that case you should add a test proving that there is no difference and that no nasty side-effect occur. If it does matter, for instance double rows might be inserted or existing rows might overwritten several times (causing your application to become slow), then I would strongly advocate using $this->exactly($count). Personally I also prefer using $this->exactly(1) over $this->once() for the sake of things looking the same regardsless of how often a method should be called.

Be specific with native PHP exceptions

Using @expectedException is a good start but, again, being specific really pays of when code breaks and tests start failing. My advice would be to also add an @expectedExceptionMessage annotation to make sure you are getting the right exception. If you want to keep things really clean you could put the message of Exceptions you throw yourself in class constants and just use the constant to check against like so (quoting straight from the manual here):

 /**
  * @expectedException        MyException
  * @expectedExceptionMessage MyClass::ERROR_MESSAGE
  */

Misc.

Use a namespace

Your classes live in a namespace, so should your test classes. Some people prefer to have an explicit Test namespace, others (me included) prefer to have the namespace match between the test class and class under test (as the class name already clearly states which one is the test...).

I'd say, pick one and see what you like best.

Avoid $this in the body of test functions.

To some it does not matter, other folks (like me) prefer a "setup" of sorts at the beginning. That way there are no distracting signatures in the important code. The bit you'll be looking at most will (most likely) look more like it will in "normal" code (that is to say, non-test code). You can see this in the example code posted above.

Use @dataproviders

If you run into a situation where you feel inclined to use a foreach loop in your test, or feed it just one single value, refactor the code to use a dataprovider instead. This is especially useful to make a distinction in which datasets are "good" versus "bad".

Another benefit is that developers often only test "happy path", even for things like exceptions. Instead of merely testing against one invalid argument, for instance, you could use a dataprovider and test against many. The results may surprise you, especially when testing against scalar values or obtional object parameters when fed a NULL.

It also allows such sets of data to be re-used from several tests functions. (DRY FTW!)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.