Deciding How To Test Bug Fixes post
Deciding How To Test Bug Fixes
"Hey Chris, I see that when Rusty went and updated his rotation for this week, there are two entries. Looks like a bug."
For my other hobby I also created a series of old-school-spaghetti-and-lasagna-style PHP scripts for doing the job of keeping track of rosters, player movement, and potential pitching rotations. It was a mess. I was initially inspired by Rasmus Lerdorf's "The no-framework PHP MVC framework" blog post. I did a really bad implementation.
But that was okay, because only a few people used it and I had extensive domain knowledge that made figuring out what the expected behaviour was supposed to be.
While I was funemployed during December 2022 I decided to go back and apply some structure to this application. It became a Slim + Doctrine application and I started working on refactoring it while also making sure existing functionality behaved as expected.
It had very few tests and I did set about writing tests for the refactored versions because who wants bugs? Except I forgot to do some for some refactored functionality.
At a high level, here is the bug:
- a user is presented with a form to add free-form text to specific rows
- when they click on the Save button, it is supposed to update details
- it was instead adding new details, not updating
No problem, I can fix this (and re-learn some things I had forgotten about Doctrine) and will also test this.
Manual vs. Automated
As many people who have listened to me talk, in my opinion there is a constant tension between "testing things manually" and "writing an automated test". Sometimes this tension is because the people involved are lazy. Sometimes it is due to time constraints. Sometimes it's because the current system is difficult to test in an automated way.
In this case I actually had two choices. I could work on code changes and manually test them using the application itself. This is actually how I normally do work on this particular application.
(As an aside, my normal policy is "if you pay me, I will automate as much of the testing as I can". For hobby projects I literally do whatever I feel like)
After looking at the refactored code, I realized that I would have to make a bunch of changes so this was a great opportunity to create an automated test that uses a real database.
This is very low-risk for this project as I am doing 99% of the development work on this code base. There is no build server running a series of checks. I need the real database to be there for me to test out the "customer-facing" side of this application so no new infrastructure is needed.
Real Tests
This is the actual test I wrote and what the code looks like now. I have been working on separating things out into Commands and Queries. This was a Command I had to fix.
So, as always, I started off with a test. I still do Arrange-Act-Assert as a way to organize the test. I thought about dependencies, like a Request object and a Response object.
$req = $this->createMock(Request::class);
$req->expects($this->once())
->method('getParsedBody')
->willReturn([
'week' => 99,
'franchise_id' => [1],
'rotation' => ['Moe, Larry, Curly'],
'current_rotation' => [''],
'rotation_id' => [0],
]
);
$response = $this->createMock(Response::class);
$response->expects($this->any())->method('withStatus')->will($this->returnSelf());
$response->expects($this->any())->method('withHeader')->will($this->returnSelf());
I also needed to make sure the database would work inside my test environment. I just cut-and-paste code from a bootstrapping section elsewhere in the application. Next time I write a test that needs to use the database, I will extract this code into a helper.
$container = new Container(require __DIR__ . '/../../config/settings.php');
$container->set(EntityManager::class, static function (Container $c): EntityManager {
$settings = $c->get('settings');
$cache = $settings['doctrine']['dev_mode'] ?
new ArrayAdapter() :
new FilesystemAdapter(directory: $settings['doctrine']['cache_dir']);
$config = ORMSetup::createAttributeMetadataConfiguration(
$settings['doctrine']['metadata_dirs'],
$settings['doctrine']['dev_mode'],
null,
$cache
);
return EntityManager::create($settings['doctrine']['connection'], $config);
});
Because the application uses a dependency injection container, I also needed to make sure my repository object I needed in the code would be available.
$container->set(RotationRepository::class, function (Container $c): RotationRepository {
return new RotationRepositoryUsingDoctrine($c->get(EntityManager::class));
});
/** @var RotationRepositoryUsingDoctrine $repo */
$repo = $container->get(RotationRepository::class);
With those all set, the test becomes:
- Given I have an existing rotation
- When I update that rotation
- I should only see that updated rotation in the database
$command = new UpdateRotationsCommand($repo);
$command->__invoke($req, $response);
$rotation = $repo->getByWeekAndFranchiseId(99, 1);
$req = $this->createMock(Request::class);
$req->expects($this->once())
->method('getParsedBody')
->willReturn([
'week' => 99,
'franchise_id' => [1],
'rotation' => ['Moe, Larry, Curly Joe'],
'rotation_id' => [$rotation->getId()],
'current_rotation' => ['Moe, Larry, Curly'],
]
);
$command = new UpdateRotationsCommand($container->get(RotationRepository::class));
$command->__invoke($req, $response);
$rotation = $repo->getByWeekAndFranchiseId(99, 1);
$this->assertEquals('Moe, Larry, Curly Joe', $rotation->getRotation());
This test took me a bit of work to get exactly right, but I didn't go an write any additional code just quite yet. I needed to:
- update the
UpdateRotationsCommand - add a new
getByWeekAndFranchiseIdmethod to my rotations repository object
final class UpdateRotationsCommand
{
public function __construct(private RotationRepository $repo) {}
public function __invoke(Request $req, Response $response): Response
{
$params = $req->getParsedBody();
$week = (int) $params['week'];
$franchiseIds = $params['franchise_id'];
$rotations = $params['rotation'];
$rotationIds = $params['rotation_id'] ?? [];
$currentRotations = $params['current_rotation'];
foreach ($franchiseIds as $idx => $franchiseId) {
if ($rotations[$idx] !== $currentRotations[$idx]) {
if (isset($rotationIds[$idx]) && (int)$rotationIds[$idx] !== 0) {
$rotation = $this->repo->getById((int)$rotationIds[$idx]);
$rotation->setId((int)$rotationIds[$idx]);
} else {
$rotation = new Rotation();
}
$rotation->setWeek($week);
$rotation->setRotation($rotations[$idx]);
$rotation->setFranchiseId((int) $franchiseId);
$this->repo->save($rotation);
}
}
return $response
->withHeader('Location', '/rotation_management?week=' . $week)
->withStatus(302);
}
}
I remain convinced I can improve that code in the foreach but my goal is
always to make sure the bug no longer exists and then, if I want, go back
and make it "nicer".
public function getByWeekAndFranchiseId(int $week, int $franchiseId): Rotation
{
return $this->em
->getRepository(Rotation::class)
->findOneBy(['week' => $week, 'franchiseId' => $franchiseId]);
}
So now I have an automated test that verifies that the bug was reported no longer exists. Are there any new bugs lurking? No idea. Only three people use this application so there are a whole series of tests I probably will never have to write.
Here is the complete test:
declare(strict_types=1);
use Symfony\Component\Cache\Adapter\ArrayAdapter;
use Doctrine\ORM\EntityManager;
use Doctrine\ORM\ORMSetup;
use PHPUnit\Framework\TestCase;
use Slim\Psr7\Request;
use Slim\Psr7\Response;
use Symfony\Component\Cache\Adapter\FilesystemAdapter;
use UMA\DIC\Container;
use Webreg\Command\UpdateRotationsCommand;
use Webreg\Repository\RotationRepository;
use Webreg\Repository\RotationRepositoryUsingDoctrine;
final class UpdateRotationsCommandTest extends TestCase
{
/**
* @test
*/
public function itUpdatesExistingRecordsCorrectly(): void
{
$req = $this->createMock(Request::class);
$req->expects($this->once())
->method('getParsedBody')
->willReturn([
'week' => 99,
'franchise_id' => [1],
'rotation' => ['Moe, Larry, Curly'],
'current_rotation' => [''],
'rotation_id' => [0],
]
);
$response = $this->createMock(Response::class);
$response->expects($this->any())->method('withStatus')->will($this->returnSelf());
$response->expects($this->any())->method('withHeader')->will($this->returnSelf());
$container = new Container(require __DIR__ . '/../../config/settings.php');
$container->set(EntityManager::class, static function (Container $c): EntityManager {
$settings = $c->get('settings');
$cache = $settings['doctrine']['dev_mode'] ?
new ArrayAdapter() :
new FilesystemAdapter(directory: $settings['doctrine']['cache_dir']);
$config = ORMSetup::createAttributeMetadataConfiguration(
$settings['doctrine']['metadata_dirs'],
$settings['doctrine']['dev_mode'],
null,
$cache
);
return EntityManager::create($settings['doctrine']['connection'], $config);
});
$container->set(RotationRepository::class, function (Container $c): RotationRepository {
return new RotationRepositoryUsingDoctrine($c->get(EntityManager::class));
});
/** @var RotationRepositoryUsingDoctrine $repo */
$repo = $container->get(RotationRepository::class);
$command = new UpdateRotationsCommand($repo);
$command->__invoke($req, $response);
$rotation = $repo->getByWeekAndFranchiseId(99, 1);
$req = $this->createMock(Request::class);
$req->expects($this->once())
->method('getParsedBody')
->willReturn([
'week' => 99,
'franchise_id' => [1],
'rotation' => ['Moe, Larry, Curly Joe'],
'rotation_id' => [$rotation->getId()],
'current_rotation' => ['Moe, Larry, Curly'],
]
);
$command = new UpdateRotationsCommand($container->get(RotationRepository::class));
$command->__invoke($req, $response);
$rotation = $repo->getByWeekAndFranchiseId(99, 1);
$this->assertEquals('Moe, Larry, Curly Joe', $rotation->getRotation());
}
}
So now I know that manually testing things is the last option to verify behaviour, not my only option.