| « Getting past project inertia | What version of PHP should my package support? » |
The other day I came across the following code in a project:
class Users
{
public function __construct(PDO $pdo)
{
$this->pdo = $pdo;
}
public function getAllUsers()
{
$stmt = $this->pdo->prepare('SELECT * FROM users');
return $stmt->fetchAll();
}
}
And the following was a unit test written to test this bit of code:
class UserTest extends TestCase
{
public function testGetAllUsers()
{
$pdo = m::mock(PDO::class);
$stmt = m::mock(PDOStatement::class);
$pdo->shouldReceive(‘prepare’)->andReturn($stmt);
$pdoStmt->shouldReceive(‘fetchAll’)->andReturn($userArray);
$users = new Users($pdo);
$result = $users->getAllUsers();
$this->assertEquals($userArray, $users);
}
}
Note that I have omitted the rest of the User class, as well as the Users array that is returned in the test.
This test will in fact provide us with 100% code coverage of the getAllUsers() method. But unfortunately, for any practical purpose, this unit test is completely useless.
This unit test is useless because instead of testing the interworkings of the getAllUsers() method, it’s actually testing Mockery. From the first lines of the test, we’re creating mocks of PDO and PDOStatement, and we’re passing those mocks into the Users class. What we’re really testing is that Mockery properly returns an array; we never touch the database.
If getAllUsers() actually *did* something, like processed the users in some way, it would be worth mocking and testing appropriately for the various conditions of the algorithm. That is not the case here.
Too often, in the search for 100% unit test code coverage, I see tests like this get written. They don’t serve a practical purpose, except to meet the test coverage goal. Worse, they don’t actually improve the quality of the application.
Instead of writing a unit test here, we would be better served by writing an integration test, or a functional test. These tests would require us to interact directly with the database, but would provide far more valuable information about the health and status of our application. A useless unit test provides us with little if any benefit; a useful functional test provides us with tremendous advantages.
Writing useful tests is challenging and requires skill and determination. And avoiding these kinds of useless unit tests helps ensure that the tests that are written will be useful and beneficial in the future.
Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP
Posted on 1/16/2018 at 9:00 am
Steve Chamaillard wrote at 1/16/2018 1:34 pm:
I disagree with this post. As simple as the code tested is, this test is proving the collaboration between PDO and Users is done properly.
This is by no means testing that PDO works since we don’t need to test this, since by definition PDO should be tested itself. It’s just testing that Users is using PDO properly.
It’s missing some stuff though, such as telling prepare() should be called once only, with the right query passed as a parameter.
Brandon Savage (@brandonsavage) wrote at 1/16/2018 6:04 pm:
Testing your internals is a good way to make refactoring impossible. If you test the internals you can’t change the internals without your tests breaking, even if you’re accepting the same arguments and returning the same data. Unit tests should test black boxes as much as is practical.
Refactoring shouldn’t be interesting when interactions don’t change. Mocking lets you test the interactions and as long as they are correct, that means the logic is correct, and so the test is correct. Change the logic, change the interactions, change the test.
I understand what you say and that makes a lot of sense, but simply throwing off those tests because they’re not testing the inner parts of the code is too close minded.
Lee wrote at 1/17/2018 12:47 am:
I found that there are corollaries to this observation that I think are worth exploring:
1. If you intentionally aim to design parts your code like this so that it’s obviously not worth unit testing, that makes the still-necessary leftover code very easy and worthwhile to unit test.
2. This “leftover” code tends to be highly refactorable and can create well defined interfaces.
I haven’t been able to find cases yet that falsify these observations. I’d be interested if you find this to be true so far in your own experience.
Herberto Graça wrote at 1/17/2018 10:07 am:
I agree with Brandon on this specific case.
It is a repository method, and its only purpose should be to execute a query. How else are we going to be sure that the query is returning the intended result if we do not test it with the DB engine?
We need an integration test for this.
And if we already test it with an Integration test, the Unit test will be redundant.
Rasmus Schultz (@mindplaydk) wrote at 1/25/2018 2:28 am:
Spot on! The only interesting thing about the tested component in this example is “does it integrate correctly with the database?” – which is the only thing not covered by the unit test, and the integration test provides the same coverage. All the unit test does is marry you to implementation details, making it harder to internally refactor – for example, the integration test is still valid if you switch to a different database internally.
My rule of thumb is this: I write unit tests only if it helps me build and validate the design of the unit – if not, I write integration tests. I understand unit tests as being primarily a development artifact, to be honest – I often write integration tests after the units and unit tests are in place, and if the integration tests provide the same coverage (which they usually do, with a lot fewer lines) I simply scrap the unit tests, because this makes it easier to refactor the units internally.
Keith Mifsud wrote at 2/19/2018 12:08 am:
I agree with Brandon. In this case, the “Users” class is a data repository so the only unit worth testing within it, is the data retrieval. It will not make sense to test for data retrieval as a Unit because that test belongs to the PDO class not the User class. However, as Brandon mentioned, there may be a future requirement or scope to test this feature as an integration such as (for example only) if we want to test that once a user is added to the database, the getUsers method should include the latest user. This test will be spread across other layers of the system and thus it is not a Unit but an Integration.
| « Getting past project inertia | What version of PHP should my package support? » |