• 14
name

A PHP Error was encountered

Severity: Notice

Message: Undefined index: userid

Filename: views/question.php

Line Number: 187

Backtrace:

File: /home/prodcxja/public_html/questions/application/views/question.php
Line: 187
Function: _error_handler

File: /home/prodcxja/public_html/questions/application/controllers/Questions.php
Line: 433
Function: view

File: /home/prodcxja/public_html/questions/index.php
Line: 315
Function: require_once

Say you have a method which ultimately boils down to

class Pager
{
    private $i;

    public function next()
    {
        if ($this->i >= 3) {
            throw new OutOfBoundsException();
        }

        $this->i++;
    }
}

How would you unit test this class. I.e. test whether the exception gets thrown on the third call of next() using PHPUnit? I've added my attempt as an answer, but I'm not sure whether this really is the way to go.

      • 1
    • @PeeHaa - it seems you missed the 'phpunit' tag. I guess the question is how to unit test that the exception occurs on the third call.

What about testing for null on the first two calls and also test for the exception being thrown like follows:

class PagerTest
{
    public function setUp()
    {
        $this->pager = new Pager();
    }

    public function testTooManyNextCalls()
    {
        $this->assertNull($this->pager->next());
        $this->assertNull($this->pager->next());
        $this->assertNull($this->pager->next());

        $this->setExpectedException('OutOfBoundsException');
        $this->pager->next();
    }
}
  • 8
Reply Report
      • 2
    • You should really test whether this works as intended. Because I'm not 100% sure of the top of my head whether this will pass or fail when the exception is thrown on the first or second next() call.
      • 1
    • @PeeHaa - re your comment: Can the setExpectedException call be moved to after the first two calls, so that it doesn't expect the exception until the third call?
      • 2
    • If the contract for Pager::next is "You are allowed to call it up to three times but not more," this is the cleanest solution. Note that since $i starts equivalent to zero the exception is thrown on the fourth call.
      • 2
    • @DavidHarkness The fact that $i is zero based is nowhere in OP, but I have updated my answer nonetheless.

It's very important when unit-testing to avoid testing implementation details. Instead, you want to limit yourself to testing only the public interface of your code. Why? Because implementation details change often, but your API should change very rarely. Testing implementation details means that you'll constantly have to rewrite your tests as those implementations change, and you don't want to be stuck doing this.

So what does this mean for the OP's code? Let's look at the public Pager::next method. Code that consumes the Pager class API doesn't care how Pager::next determines if an exception should be thrown. It only cares that Pager::next actually throws an exception if something is wrong.

We don't want to test how the method arrives at it's decision to throw an OutOfBoundsException -- this is an implementation detail. We only want to test that it does so when appropriate.

So to test this scenario we simulate a situation in which the Pager::next will throw. To accomplish this we simply implement what's called a "test seam." ...

<?php
class Pager
{
    protected $i;

    public function next()
    {
        if ($this->isValid()) {
            $this->i++;
        } else {
            throw new OutOfBoundsException();
        }
    }

    protected function isValid() {
        return $this->i < 3;
    }
}

In the above code, the protected Pager::isValid method is our test seam. It exposes a seam in our code (hence the name) that we can latch onto for testing purposes. Using our new test seam and PHPUnit's mocking API, testing that Pager::next throws an exception for invalid values of $i is trivial:

class PagerTest extends PHPUnit_Framework_TestCase
{
    /**
     * @covers Pager::next
     * @expectedException OutOfBoundsException
     */
    public function testNextThrowsExceptionOnInvalidIncrementValue() {
        $pagerMock = $this->getMock('Pager', array('isValid'));
        $pagerMock->expects($this->once())
                  ->method('isValid')
                  ->will($this->returnValue(false));
        $pagerMock->next();
    }
}

Notice how this test specifically doesn't care how the implementation method Pager::isValid determines that the current increment is invalid. The test simply mocks the method to return false when it's invoked so that we can test that our public Pager::next method throws an exception when it's supposed to do so.

The PHPUnit mocking API is fully covered in Test Doubles section of the PHPUnit manual. The API isn't the most intuitive thing in the history of the world, but with some repeated use it generally makes sense.

  • 4
Reply Report
      • 1
    • At some point you must test isValid and the overall contract for Pager::next which client code does care about. If the validity test were external to Pager I would use this method but not here.
    • @DavidHarkness Testing isValid should be understood; it must be done. The point is that you shouldn't test it directly, you should gain test coverage on isValid by testing the public interface in a scenario where you aren't mocking its results. We don't need to directly test PHP's ability to evaluate $i >=3. This can also be easily achieved with a custom stub class extending Pager, but the example uses PHPUnit's mocking API to demonstrate its capabilities. Obviously, this is a very simple scenario ... one in which full-on test seams feels like overkill. The concept is what's important.
      • 2
    • Without editing the answer I'd like to draw attention to encapsulation issues. Normally you'll want to keep private things private, but note that you can't mock private instance methods. In cases where lowering the accessibility of an implementation method or property can significantly improve testability (e.g. to allow mocking), it's my strong opinion that protected is helpful and encapsulation concerns can be disregarded.

Here's what I have at the moment, but I was wondering if there were any better ways of doing this.

class PagerTest
{
    public function setUp()
    {
        $this->pager = new Pager();
    }

    public function testTooManyNextCalls()
    {
        for ($i = 0; $i < 10; $i++) {
            try {
                $this->pager->next();
            } catch(OutOfBoundsException $e) {
                if ($i == 3) {
                    return;
                } else {
                    $this->fail('OutOfBoundsException was thrown unexpectedly, on iteration ' . $i);
                }
            }

            if ($i > 3) {
                $this->fail('OutOfBoundsException was not thrown when expected');
            }
        }
    }
}
  • 1
Reply Report

You could use something like:

class PagerTest extends PHPUnit_Framework_TestCase {

    /**
     * @expectedException OutOfBoundsException
     */
     public function testTooManyNextCalls() {
         $this->pager = new Pager();

         $this->pager->next();
         $this->pager->next();
         $this->pager->next();

         $this->assertTrue(false);
    }
}

If an exception is thrown in the 3rd method call, the always-failing assert-statement should never be reached and the test should pass. on the other hand if no exception gets thrown, the test will fail.

  • 1
Reply Report
    • you're right - an exception from either call would make that test pass. a second test calling only 2 times could work around this but still that's not an optimal solution.

You may pass the value $this->i to the exception instantiation, which will then be the message of the exception.

class Pager
{
    private $i;

    public function next()
    {
        if ($this->i >= 3) {
            throw new OutOfBoundsException($this->i);
        }

        $this->i++;
    }
}
$a=new Pager();
$a->next();
$a->next();
$a->next();
$a->next();

//outputs: "Exception: 3"
  • 0
Reply Report