I have a class similar to this (some logic removed for brevity):
class FooCollection {
protected $_foos;
public function __construct() {
$this->_foos = new SplObjectStorage();
}
public function addFoo(FooInterface $foo) {
$this->_foos->attach($foo);
}
public function removeFoo(FooInterface $foo) {
$this->_foos->detach($foo);
}
}
I'd like to test the addFoo()
and removeFoo()
methods using PHPUnit, and I was wondering what the best strategy would be to do this? As far as I can tell, I have only a few options:
hasFoo(FooInterface $foo)
and check this after the add.getFoos()
that directly returns the SplObjectStorage
instance and check if $foo
is in it after the add.removeFoo($foo)
after addFoo($foo)
and check for an exception.$_foos
a public property and check it directly after the add (bad, bad, bad...).Options #1 and #2 are changing the public interface solely for testing purposes, and I'm not sure how I feel about that. They seem like pretty generic, useful methods to have anyway on the surface, but in my particular case, I never have a need to check for the existance of a particular Foo
instance in the collection, nor retrieve all of the instances, so it really would just be bloat. Also, it seems like if I'm testing multiple parts of the interface in one test, I'm not really testing a "unit," but that's more or less just a philosophical hangup.
Option #3 seems awkward to me.
Option #4 is a really bad idea, and I shouldn't have even listed it, as I wouldn't do it even if it were suggested here.
You didn't post a public accessor for getting the collection, but I am sure you have one, otherwise adding/removing foos to an array which is publically inaccessible makes no sense. So you could try something like (phpunit 3.6, php 5.4):
public function setUp()
{
$this->NumbersCollection = new NumbersCollection;
}
public function tearDown()
{
unset($this->NumbersCollection);
}
public function testNumbersCollection()
{
$this->NumbersCollection->addNumber(1);
$this->NumbersCollection->addNumber(2);
$this->assertSame(3, $this->NumbersCollection->sum());
$this->assertSame(2, $this->NumbersCollection->product());
$this->NumbersCollection->removeNumber(1);
$this->NumbersCollection->addNumber(7);
$this->assertSame(9, $this->NumbersCollection->sum());
$this->assertSame(14, $this->NumbersCollection->product());
}
Why not create a mock SplObjectStorage
object that you pass in to the constructor? Then you can assert that the attach
and detach
methods are called on the mock.
function testAttachFOO() {
$mockStorage = $this->getMockBuilder('SplObjectStorage')
->setMethods(array('attach'))
->getMock();
$mockFoo = $this->getMock('FooInterface');
$mockStorage->expects($this->once())
->method('attach')
->with($mockFoo);
$collection = new FooCollection($mockStorage);
$collection->addFoo($mockFoo);
}
And something similar for removeFoo
.
Doing this does require that you change the constructor so that you can inject the dependency. But IMO this makes the code clearer as to what is going on. Also makes testing things easier.
So the constructor becomes:
public function __construct(SPLObjectStorage $storage) {
$this->_foos = $storage;
}
If the class becomes difficult to build doing this, it is a sign that the class is doing too much and should be refactored into more, smaller classes.