Common testing fails - testing the right scope
When I started with phpspec I often wrote tests for already existing classes. In the last few weeks I read over some test cases from my phpspec beginnings and some of them really scared me. With this series of blog articles I’d like to share my learnings on general testing or phpspec specific fails.
An example
I would like to look at a pretty simple and very common task. Let’s fetch some data from an api or a database, transform the data into a DTO and return this object. Let’s assume we want to retreive a user object from a database, the corresponding service class code would look like this:
<?php
class UserService {
public function get(int $id)
{
$userData = $this->repository->get($id);
if (!empty($userData)) {
return $this->userFactory->create($userData);
}
throw new UserNotFoundException($id);
}
}
The $this->repository->get call will return an array of data or an empty array if no machting user data is found and the $this->userFactory->create call is going to build a User DTO from the array data.
Testing things out of scope
The first testcase was easy. If the repository does not return data, the UserNotFoundException gets thrown.
<?php
public function it_should_throw_an_exception_if_no_user_data_is_found()
{
$this->shouldThrow(UserNotFoundException::class)->duringGet(1);
}
Maybe I should consider to add another test case where the repository returns an empty array instead of null. But it should basically cover the bad case.
The test for the good case looked something similar to the following:
<?php
public function it_should_return_an_user(Repository $repository, UserFactory $userFactory)
{
$repository->get(1)->willReturn([
'id' => 1,
'firstname' => 'John',
'lastname' => 'Doe',
])->shouldBeCalled();
$user = new User(1, 'John', 'Doe');
$userFactory->create([
'id' => 1,
'firstname' => 'John',
'lastname' => 'Doe',
])->willReturn($user)->shouldBeCalled();
$response = $this->get(1);
$response->shouldHaveType(User::class);
$response->getId()->shouldReturn(1);
$response->getFirstname()->shouldReturn('John');
$response->getLastname()->shouldReturn('Doe');
}
Oh my god, this test case is completely broken and breaks many BDD rules (for me). I’m creating a User instance to return it from my $userFactory mock and I’m asserting the fields of the returned User instance. This doesn’t make sense at all and it creates a very strong binding to the implementation of the SUT and its dependencies. For example if I’m adding a field to the User model I need to adopt the User tests and also this test case and this is not what this test should do. It should test the behavior of the service method.
Rewriting the test case
To clean up the test case I should focus on the actual specification of the method. What is the goal I wanted to achieve? Fetch some data, bring it into the correct format and return the transformed data object. Let’s realize these simple steps in a simple and readable test case.
<?php
public function it_should_return_an_user(Repository $repository, UserFactory $userFactory)
{
$repository->get(1)->willReturn([
'id' => 1,
'firstname' => 'John',
'lastname' => 'Doe',
])->shouldBeCalled();
$userFactory->create([
'id' => 1,
'firstname' => 'John',
'lastname' => 'Doe',
])->willReturn(new User())->shouldBeCalled();
$this->get(1)->shouldHaveType(User::class);
}
Now this test case reflects the behavior of the service method, nothing more and nothing less.
Wrap-Up
Testing is hard, it needs a lot of practice and I’m learning new ways to test things efficiently every day. But one of the most important things to point out is that each of your test cases should have as few lines as possible. Heavy setup stuff or mocking indicates a wrong testing approach and/or bad method design. If you look on the example above again we must ensure that the Repository and the UserFactory are working correctly. Not in the way as it was done before but with seperate unit tests.
This might be also interesting
Do you enjoy reading my blog?
