Symfony Advent Calendar 2018 6日目の記事です！
昨日は @okapon_pon さんの Symfonyフレームワークにおけるデザインパターン活用 でした。
Symfony Advent Calendarはまだまだ枠に余裕があります！何かSymfonyネタのある方はこの機会にどうぞ！
前提
カルテット開発部のPHPチームは、通常、エンティティのprimary keyとなる $id にsetterを書きません。（習慣的に）
特に規約があるわけではないですが、primary keyはデータベース側でauto_increment/serial型で入れることがほとんどなので、アプリケーション側からIDをセットしないということを表すために、自然と習慣化されてきました。
IDのsetterがなくて困ること
ズバリ、エンティティを操作するサービスのユニットテスト書くときに困っています。
- エンティティのインスタンスを複数扱うときにそれぞれ違うIDを指定したい
- エンティティのID以外のフィールドにも値を出し入れする必要がある
という場合に、ベストプラクティスというべき方法がなかったのです。
サンプルコード
具体的には、idとstatusという2つのフィールドを持つ Job エンティティがあり、
<?php
namespace Quartetcom\DecBlogDemo\Entity;
class Job
{
/**
* @var int
*/
private $id;
/**
* @var string
*/
private $status;
/**
* @return int
*/
public function getId()
{
return $this->id;
}
/**
* @return string
*/
public function getStatus()
{
return $this->status;
}
/**
* @param string $status
* @return $this
*/
public function setStatus(string $status): Job
{
$this->status = $status;
return $this;
}
}
Job::$id と Job::$status を操作するサービスを作っているケースです。
<?php
namespace Quartetcom\DecBlogDemo;
use Doctrine\ORM\EntityManagerInterface;
use Quartetcom\DecBlogDemo\Entity\Job;
class DecBlogDemo
{
/**
* @var EntityManagerInterface
*/
private $em;
/**
* @var StatusCheckerInterface
*/
private $statusChecker;
/**
* @var StatusNotifierInterface
*/
private $notifier;
/**
* @param EntityManagerInterface $em
* @param StatusCheckerInterface $statusChecker
* @param StatusNotifierInterface $notifier
*/
public function __construct(
EntityManagerInterface $em,
StatusCheckerInterface $statusChecker,
StatusNotifierInterface $notifier
) {
$this->em = $em;
$this->statusChecker = $statusChecker;
$this->notifier = $notifier;
}
/**
* @param Job[] $jobs
*/
public function notifyIfStatusHasChanged(array $jobs): void
{
$updatedIds = [];
foreach ($jobs as $job) {
$previousStatus = $job->getStatus();
$currentStatus = $this->statusChecker->computeCurrentStatus($job);
if ($previousStatus !== $currentStatus) {
$job->setStatus($currentStatus);
$updatedIds[] = $job->getId();
}
}
$this->em->flush();
if (count($updatedIds) > 0) {
$this->notifier->notify($updatedIds);
}
}
}
下記のようにステータス更新がある場合・ない場合の動作をテストしたいのですが、テスト時の Job のインスタンスをどのように作ればよいでしょうか？
<?php
namespace Quartetcom\DecBlogDemo;
use Doctrine\ORM\EntityManagerInterface;
use PHPUnit\Framework\TestCase;
use Prophecy\Argument;
use Prophecy\Prophecy\ObjectProphecy;
use Quartetcom\DecBlogDemo\Entity\Job;
class DecBlogDemoTest extends TestCase
{
/**
* @var EntityManagerInterface|ObjectProphecy
*/
private $em;
/**
* @var StatusCheckerInterface|ObjectProphecy
*/
private $statusChecker;
/**
* @var StatusNotifierInterface|ObjectProphecy
*/
private $statusNotifier;
public function setUp()
{
$this->em = $this->prophesize(EntityManagerInterface::class);
$this->statusChecker = $this->prophesize(StatusCheckerInterface::class);
$this->statusNotifier = $this->prophesize(StatusNotifierInterface::class);
}
public function tearDown()
{
$this->em = null;
$this->statusChecker = null;
$this->statusNotifier = null;
}
public function test_ステータス更新あり()
{
$job1 = $this->getJob(1, 'pending');
$job2 = $this->getJob(2, 'running');
$this->statusChecker->computeCurrentStatus($job1)->willReturn('running')->shouldBeCalled();
$this->statusChecker->computeCurrentStatus($job2)->willReturn('running')->shouldBeCalled();
$this->em->flush()->shouldBeCalled();
$this->statusNotifier->notify([1])->shouldBeCalled();
$this->getSUT()->notifyIfStatusHasChanged([$job1, $job2]);
}
public function test_ステータス更新なし()
{
$job1 = $this->getJob(1, 'pending');
$job2 = $this->getJob(2, 'running');
$this->statusChecker->computeCurrentStatus($job1)->willReturn('pending')->shouldBeCalled();
$this->statusChecker->computeCurrentStatus($job2)->willReturn('running')->shouldBeCalled();
$this->em->flush()->shouldBeCalled();
$this->statusNotifier->notify(Argument::any())->shouldNotBeCalled();
$this->getSUT()->notifyIfStatusHasChanged([$job1, $job2]);
}
private function getJob(int $id, string $prevStatus): Job
{
// どう書く？
}
private function getSUT()
{
/** @var EntityManagerInterface $em */
$em = $this->em->reveal();
/** @var StatusCheckerInterface $statusChecker */
$statusChecker = $this->statusChecker->reveal();
/** @var StatusNotifierInterface $statusNotifier */
$statusNotifier = $this->statusNotifier->reveal();
return new DecBlogDemo($em, $statusChecker, $statusNotifier);
}
}
手法あれこれ
テストをどう書けば良いか、PHPチーム内で議論して様々なやり方を検討しました。
手法その1 getId()をスタブにする
エンティティのモックをJobとして使い、 getId() をスタブにする手法です。
サンプルコード1 PHPUnitのMockObjectを使う
<?php
// ...
class DecBlogDemoTest extends TestCase
{
// ...
private function getJob(int $id, string $prevStatus): Job
{
$job = $this->createMock(Job::class);
$job->expects($this->once())->method('getId')->willReturn($id);
$job->expects($this->once())->method('getStatus')->willReturn($prevStatus);
return $job;
}
テスト対象のサービスが依存する他のサービス StatusCheckerInterface や StatusNotifierInterface のテストには prophecy を使っているので、ここだけ突然MockObjectが出てくるとやや混乱しますね。
また、 Job::setStatus() の呼び出しは条件によって違うので記述していませんが、もし記述するとしたら $prevStatus !== $currentStatus という実際のサービス内にあるのと同じif文がここにも登場することになってしまいます。
サンプルコード2 prophecyを使う
<?php
// ...
class DecBlogDemoTest extends TestCase
{
// ...
public function test_ステータス更新あり()
{
- $job1 = $this->getJob(1, 'pending');
- $job2 = $this->getJob(2, 'running');
+ $job1 = $this->getUpdatedJob(1, 'pending', 'running');
+ $job2 = $this->getPreservedJob(2, 'running');
// ...
}
public function test_ステータス更新なし()
{
- $job1 = $this->getJob(1, 'pending');
- $job2 = $this->getJob(2, 'running');
+ $job1 = $this->getPreservedJob(1, 'pending');
+ $job2 = $this->getPreservedJob(2, 'running');
// ...
}
private function getUpdatedJob(int $id, string $prevStatus, string $currentStatus): Job
{
$job = $this->prophecy(Job::class);
$job->getId()->willReturn($id)->shouldBeCalled();
$job->getStatus()->willReturn($prevStatus)->shouldBeCalled();
$job->setStatus($currentStatus)->shouldBeCalled();
return $job->reveal();
}
private function getPreservedJob(int $id, string $prevStatus): Job
{
$job = $this->prophecy(Job::class);
$job->getId()->willReturn($id)->shouldBeCalled();
$job->getStatus()->willReturn($prevStatus)->shouldBeCalled();
return $job->reveal();
}
prophecyに統一した版です。 prophecyでは複数のメソッド呼び出しをすべて記述することになるので、やや冗長になります。
手法その2 機能テストだけにする
エンティティを対象にしたテストなので実際の値の出し入れそのものを確認してしまおうという手法です。
サンプルコード
<?php
// ...
use Liip\FunctionalTestBundle\Test\WebTestCase;
class DecBlogDemoTest extends WebTestCase
{
/**
- * @var EntityManagerInterface|ObjectProphecy
+ * @var EntityManagerInterface
*/
private $em;
// ...
public function setUp()
{
+ $this->loadFixtureFiles([]);
- $this->em = $this->prophesize(EntityManagerInterface::class);
+ $this->em = $this->getContainer()->get('doctrine')->getManager();
// ...
}
// ...
private function getJob(int $id): Job
{
return $this->em->find(Job::class, $id);
}
// ...
}
悪くはないですが、エンティティを操作するすべてのサービスについて機能テストを書いていくとなると、アプリケーション全体のテストが完了するまでの時間がどんどん長くなっていくのでオススメできません。
また、このサービスを使うコントローラやコマンドのテストでも機能テストを書くことになれば、重複したテストとなってしまいます。
手法その3 setId()を書く
習慣を捨ててエンティティに対してIDのsetterを追加しておき、テストには実際のエンティティのインスタンスを使う手法です。
サンプルコード
Job::setId() を追加して、
<?php
class Job
{
// ...
/**
* FOR TEST
* @param int $id
* @return Job
*/
public function setId(int $id): Job
{
$this->id = $id;
return $this;
}
}
テストではsetId()を使います。
<?php
// ...
class DecBlogDemoTest extends TestCase
{
// ...
private function getJob(int $id, string $prevStatus): Job
{
$job = new Job();
$job
->setId($id)
->setStatus($prevStatus)
;
return $job;
}
ID以外のフィールド操作はわざわざスタブにする必要がなくなり、良くなりました！ なお、setId()にはFOR TESTとコメントしておき、プロダクションコードから使わないように注意する必要があります。 人の注意力に依存するのはイマイチですね。
手法その4 IDをコンストラクタで渡せるようにする
コンストラクタでIDを指定できるようにし、テスト時のみ使うようにする手法です。
サンプルコード
Job のコンストラクタでIDをセットできるようにして、
<?php
class Job
{
// ...
/**
* @param int $id FOR TEST
*/
public function __construct(int $id = null)
{
$this->id = $id;
}
}
テストではコンストラクタからIDを注入します。
<?php
// ...
class DecBlogDemoTest extends TestCase
{
// ...
private function getJob(int $id, string $prevStatus): Job
{
$job = new Job($id);
$job->setStatus($prevStatus);
return $job;
}
setId()を作ったときと同様、ID以外のフィールド操作は良いですね。 コンストラクタでIDをセットするというのはプロダクションコードで書くことはあまりないでしょうが、やはりコンストラクタのphpdocのparam欄にはFOR TESTとコメントしておき、うっかり使わないように注意する必要があるでしょう。人の注意力依存がやめられませんでした…。
手法その5 エンティティの形を変えずにIDをreflectionでセットする
エンティティのIDはprivateのまま、setterを作らずコンストラクタからも注入させないまま、リフレクションを使ってテスト時のみ特別にIDをセットする手法です。
サンプルコード
<?php
// ...
class DecBlogDemoTest extends TestCase
{
// ...
private function getJob(int $id, string $prevStatus): Job
{
$job = new Job();
$job->setStatus($prevStatus);
$reflection = new \ReflectionObject($job);
$idProperty = $reflection->getProperty('id');
$idProperty->setAccessible(true);
$idProperty->setValue($job, $id);
return $job;
}
ID以外のフィールド操作は良い状態を保ったまま、実際のプロダクションコードに副作用なくテストできるようになりました。
まとめ
開発部PHPチームの現時点での結論は 手法5: エンティティの形を変えずにIDをリフレクションでセットする がベストプラクティスということになりました。
Lisketの開発開始から現在までの間に、各自の裁量で書いたテスト（それぞれの時点ではレビューも通っていた）の中には、手法1〜4で書かれたものも多いため、今後、技術的負債返済の時間を使って少しずつベストプラクティスに揃えていきたいと考えています。
カルテット開発部では、単に要件を満たせばよいのではなく、より良いコードの書き方・テストの書き方についても議論しながら開発を進めています。
このような環境が楽しそうだと思った方は、ぜひ一度 開発部Slackに遊びに来てください 