戻り値がないメソッドのテスト方法が間違っていた話

エンジニア雑記

nekoroll.hatenablog.com
これの続き

そもそもvoidとして受け取ることは無理だった

https://www.php.net/manual/ja/migration71.new-features.php#migration71.new-features.void-functions voidではなくnullになる

そもそもやりたかったのは

  1. assertionがなくてriskyテストになるのを避けたかった
  2. doesNotPerformAssetionsだとearly return削った時にテストがコケなくて怖い
  3. どうしよう…

という感じだった

じゃあどう対処するべきか？を悩んでいた所、色々教えてもらった

sji先生やLS先生に教えてもらった

LS先生

sji先生

以上を踏まえて何をやるべきか

まずは対象となるコード

<?php

class Hoge
{
  public function fuga(int $id): void
  {
    if (1 === $id) {
      // early return
      return;
    }
    // なんかいい感じにデータベース操作できるやつ
    $db = new ORM();
    // なんか適当にDBインサート処理とか処理する
    $db->insert($id);
  }
}

※例が適当すぎてよくなかったので、DBアクセスはインスタンス生成して処理するようにした

これに対してテストを書く

対応できそうなアプローチは以下

  1. $dbをMockにして呼ばれないことを担保する
    1. Mockにできないなら処理を呼び出すプロキシ的なメソッドを用意してMockにする
  2. そもそも処理内容をテストする必要がある(呼び出し側から検知する必要がある)場合、voidではなくboolなり何かを返すべき

が適切そうだった

DB操作をMockにする

まずMockにしやすいようにDIパターンを適用する

<?php

class Hoge
{
  // なんかいい感じにデータベース操作できるやつ
  private $db = ORM;

  public function __construct(ORM $db)
  {
    $this->db = $db;
  }

  public function fuga(int $id): void
  {
    if (1 === $id) {
      // early return
      return;
    }
    // なんか適当にDBインサート処理とか処理する
    $this->db->insert($id);
  }
}

呼び出されないことを検証する

<?php

class Hoge
{
  use PHPUnit\Framework\TestCase;

  /**
    *@test
    */
  public function idが1の時は処理しないこと()
  {
    // モック化して呼ばれないことを検証
    $db_mock = \Mockery::mock(ORM::class);
    $db_mock->shouldNotReceive('insert');
    $hoge = new Hoge($db_mock);
    $hoge->fuga(1);

    // PHPUnitとしてはassertionが無いと怒るので必要
    $this->doesNotPerformAssertions();
  }
}

これでearly returnが削除された場合shouldNotReceive('insert')で検出できるようになった

モックにしたいけど出来ない場合

プロキシ的なメソッドを用意する

<?php

class Hoge
{
  public function fuga(int $id): void
  {
    if (1 === $id) {
      // early return
      return;
    }
    // プロキシメソッドを介して呼ぶようにする
    $this->proxyMethod($id);
  }

  public function proxyMethod($id): void
  {
    // なんかいい感じにデータベース操作できるやつ
    $db = new ORM();
    // なんか適当にDBインサート処理とか処理する
    $db->insert($id);
  }
}

呼び出されないことを検証する

<?php

class Hoge
{
  use PHPUnit\Framework\TestCase;

  /**
    *@test
    */
  public function idが1の時は処理しないこと()
  {
    // 一部のみモック化して検証したいのでパーシャル化
    $hoge_mock = \Mockery::mock(Hoge::class)->makePartial();
    $hoge_mock->shouldNotReceive('proxyMethod');
    $hoge_mock->fuga(1);

    // PHPUnitとしてはassertionが無いと怒るので必要
    $this->doesNotPerformAssertions();
  }
}

同様にshouldNotReceive('proxyMethod')で検出できるようになった
※パーシャルの扱いイマイチだけど多分これでいいはず

戻り値を与える

voidからboolに変更

<?php

class Hoge
{
  public function fuga(int $id): bool
  {
    if (1 === $id) {
      // インサートしていない を示すfalse
      return false;
    }
    $db = new ORM();
    // インサートに成功した場合boolを返すと仮定
    return $db->insert($id);
  }
}

戻り値があるのでテストが書ける

<?php

class Hoge
{
  use PHPUnit\Framework\TestCase;

  /**
    *@test
    */
  public function idが1の時は処理しないこと()
  {
    $hoge = new Hoge($db_mock);
    $result = $hoge->fuga(1);
    
    // インサートされていないこと(falseであること)を検証できる
    $this->assertFalse($result);
  }
}

まとめ

  • mockにするのが個人的にはベスト。テスタビリティ高いしわかりやすい
  • mockが無理ならプロキシメソッドを用意して呼び出す手法もあり
  • 処理内の実行結果を検証したい場合voidは誤りかもしれない
    • ただしvoidが必ずしも誤りというわけではない
      例としてバリデーション処理でエラーなら例外、OKならvoidとなったりもする
  • doesNotPerformAssetionsは結局必要になる
    コメントを添えておくとテストコードとしてとても優しいと思う　ラブアンドピース

バリデーション処理の例外の場合も結局expectExceptionで検証しつつ
正常系はdoesNotPerformAssetionsを付ける形になりそう

いろいろ考えて、DIパターンにしつつ全部publicにしていいんじゃないかな…と思った
呼び出される範囲考えてprivateにしてたけど、public/protectedでも良い気がする
でも可視性は適切につけるべきだから、プロキシメソッドを用意するのがベストなのかな
これもうわかんねぇな、お前どう？