Building on top of @SDC's answer, I recommend the following
- split the tests even further
- avoid using instance properties to reference the system under test
Splitting the tests further
There's a problem with multiple assertions in a single test if the assertions are not related to the same behaviour: you cannot name the test properly, you might even end up using and
within the test method name. If that happens, split the tests into separate tests
Avoid using instance properties for the SUT
When I started writing tests, I felt that there's an opportunity to reduce code duplication when arranging the system under test (SUT) in setUp
, and then referencing the SUT via the corresponding instance properties in the individual tests.
This is tempting, but after a while, when you start extracting collaborators from the SUT, you will have the need to set up test doubles. In the beginning this might still work for you, but then you will start setting up test doubles differently in different tests, and all the duplication that was aimed to avoid earlier comes back at you: you will end up setting up both the test doubles, and arranging the SUT in your test again.
When I encounter this in code reviews, I like to reference
and I recommend reading it.
The important point is, you want to make it easy to write and maintain tests. Maintaining tests (or any code, if you will) means mostly making the code easily readable. If you read a bit of code, let's say, a class method, you want to easily understand what is about, and ideally, the method should do what you would expect it to do from its class name. If you are testing different behaviours, make it obvious by creating different test methods.
This also has the advantage that if you run your tests with
$ phpunit --testdox
you end up with a nice list of expected behaviours, see
Example based on your Question
Note The comments I provide in this example are only to illustrate the idea of further splitting the tests, in actual code I would not have them.
/**
* The name of this method suggests a behaviour we expect from the
* constructor of DependencyContainer
*/
public function testCanOverrideShellCommanderConfiguration()
{
$container = new DependencyContainer(__DIR__ . '/../../Resources/valid_json.json');
$this->assertEquals(
'overriden',
$container->getConfig('shell_commander')['pygmentize_command']
);
}
/**
* While the idea of using a data provider is good, splitting the test
* further makes sense for the following reasons
*
* - running tests with --testdox option as lined out above
* - modifying the behaviour independently
* Currently, a generic Exception is thrown, but you might
* consider using a more specific exception from the SPL library,
* (see http://php.net/manual/en/spl.exceptions.php),
* or creating your own NonExistentConfigurationException class,
* and then a data provider might not make sense anymore)
*/
public function testConstructorRejectsNonExistentConfigurationFile()
{
$path = 'unexisting_file';
$this->setExpectedException(\Exception::class, sprintf(
'Configuration file at path "%s" doesn\'t exist.',
$path
));
new DependencyContainer($path);
}
public function testConstructorRejectsInvalidConfigurationFile()
{
$path = __DIR__ . '/../../Resources/invalid_json.json';
$this->setExpectedException(
\Exception::class,
'Configuration JSON file provided is not valid.'
);
new DependencyContainer($path);
}
Note I would also recommend to take a look at