I know it sounds weird, why should I test for what isn't happening if my tests are passing? The simple answer is that like the rest of your code, your tests can have bugs. Likewise, the logic that implements pieces of your code can introduce unanticipated behaviors that your tests are not designed for. Let's set the scene:
- Your made-up app is a to-do list that allows for family accounts with multiple users.
- You have an improvement to allow a secondary account owner.
- Your primary and secondary owner's contact information is updated through their user settings but also updates data in the main account settings
- Like always features need to be done yesterday and your backlog is breathing down your neck.
- Your tests are mainly implementation tests that focus on validating individual methods and assume some of the data state coming into them.
Some Mock Code to Set the Stage
I'm going to write these examples in PHP since that is my daily driver, but this can happen in any code base and to any project. Your code starts off looking something like this.
class User_Account_Logic extends Logic_Base {
public function updateUserAccountInformation(array $user_information): array {
if ($this->userDataIsNotValid($user_information)) {
return $this->errors;
}
$this->User_Model->save($user_information);
if ($this->userShouldUpdateMainAccount($user_information)) {
$this->Main_Account_Logic->updateContactInformation($user_information);
}
return ['success' => true, 'message' => 'Successfully updated user information!'];
}
public function userShouldUpdateMainAccount(array $user): bool {
if ($user['account_type'] === $User_Model::OWNER) {
return true;
}
return false;
}
}
class Main_Account_Logic extends Logic_Base {
public function updateContactInformation(array $user_information): bool {
if ($user_information['account_type'] === User_Model::OWNER) {
return $this->updateMainAccountContact($user_information);
}
return false;
}
}
//Somewhere in your unit tests....
class User_Account_LogicTest extends TestCase {
public function getUserAccountMock() {
$Mock_Main_Account = $this->getMockBuilder(Main_Account_Logic)
->disableOriginalConstructor()
->onlyMethods(['updateContactInformation'])
->getMock();
$Mock_Main_Account->method('updateContactInformation')->willReturn(true);
$Mock_User_Logic = new class extends User_Account_Logic;
$Mock_User_Logic->Main_Account_Logic = $Mock_Main_Account;
return $Mock_User_Logic;
}
public function testThatFamilyMembersCanUpdateTheirSettings() {
$User_Account_Logic = $this->getUserAccountMock():
$Mock_User_Model = $this->getMockBuilder(User_Model::class)
->disableOriginalConstructor()
->onlyMethods(['save'])
->getMock();
$Mock_User_Model->expects($this->once())->method('save')->willReturn(true);
$User_Account_Logic->User_Model = $Mock_User_Model;
$result = $User_Account_Logic->updateUserAccountInformation([
'name' => 'foo bar',
'account_type' => User_Model::MEMBER.
]);
$this->assertTrue($result['success']);
}
}
Nothing too crazy, you're updating a user, and if they are the primary you are updating the main account information as well. You have unit tests with a handy helper function to build your mock user account class and everything is working wonderfully. You don't mind that the helper assumed the return of the main account code because you are implementation testing that as well so what could go wrong?
What goes wrong
You're in a rush to get this feature out and you make some changes allowing a secondary owner to update the main accounts information for the secondary owner. This owner can do everything the main account holder can and this is a big feature that has been asked for. So one of the changes you make is the following.
class User_Account_Logic extends Logic_Base {
public function updateUserAccountInformation(array $user_information): array {
if ($this->userDataIsNotValid($user_information)) {
return $this->errors;
}
$this->User_Model->save($user_information);
if ($this->userShouldUpdateMainAccount($user_information)) {
$this->Main_Account_Logic->updateContactInformation($user_information);
}
return ['success' => true, 'message' => 'Successfully updated user information!'];
}
public function userShouldUpdateMainAccount(array $user): bool {
if ($user['account_type'] >= $User_Model::SECONDARY) {
return true;
}
return false;
}
}
class Main_Account_Logic extends Logic_Base {
public function updateContactInformation(array $user_information): bool {
if ($user_information['account_type'] === User_Model::OWNER) {
return $this->updateMainAccountContact($user_information);
}
return $this->updateSecondaryAccountInformation($user_information);
}
}
In these files, that's all you do is one quick change of a === in the User_Account_Logic method userShouldUpdateMainAccount and added a call to update the secondary account contact information in the Main_Account_Logic class. What you just did was make any account user other than the owner the secondary account owner. That quick typo of a > instead of a < just gave any user the ability to update the main account secondary user to their account.
Testing will catch it... right?
No, it won't because you don't update your tests because they still pass, you just changed which accounts hit the update main account method so nothing should fail. Since you're focused on implementation testing and not behavior testing you mock the response from Main_Account_Logic and Main_Account_LogicTest assumes the state of the data it is given so all your tests pass. Good enough, push that bad boy live.
How could testing have stopped this?
You could have added one thing to prevent this test from passing when it shouldn't. That one thing is testing for what shouldn't happen.
class User_Account_LogicTest extends TestCase {
public function getUserAccountMock(int $main_account_hits) {
$Mock_Main_Account = $this->getMockBuilder(Main_Account_Logic)
->disableOriginalConstructor()
->onlyMethods(['updateContactInformation'])
->getMock();
$Mock_Main_Account->expects($this->exactly($main_account_hits))
->method('updateContactInformation')
->willReturn(true);
$Mock_User_Logic = new class extends User_Account_Logic;
$Mock_User_Logic->Main_Account_Logic = $Mock_Main_Account;
return $Mock_User_Logic;
}
public function testThatFamilyMembersCanUpdateTheirSettings() {
$User_Account_Logic = $this->getUserAccountMock(0):
$Mock_User_Model = $this->getMockBuilder(User_Model::class)
->disableOriginalConstructor()
->onlyMethods(['save'])
->getMock();
$Mock_User_Model->expects($this->once())->method('save')->willReturn(true);
$User_Account_Logic->User_Model = $Mock_User_Model;
$result = $User_Account_Logic->updateUserAccountInformation([
'name' => 'foo bar',
'account_type' => User_Model::MEMBER.
]);
$this->assertTrue($result['success']);
}
}
Adding that $this->exactly(0) would have caused this test to fail. PHPUnit would have reported an error since you expected that method to be called 0 times and it was called once. Then you would have caught your mistake and you would have prevented your user's son who has a terrible password and gets his account compromised from giving the attacker owner-level access to your user's account.
Conclusion
Making sure that you also test for what shouldn't be happening can help improve the reliability of your tests and catch bugs earlier. Testing is not only about what should happen but what should not happen to your system and overlooking what shouldn't happen can end up with code that acts the complete opposite of how you assumed.
Top comments (0)