PHPUnitのテストコード比較 (PHP7.0.4, PHPUnit5.0.10)

# はじめに
今回はPHPのtime()のテストコードを書こうと思って、この辺をみつつ([PHPUnitでtime()をテストする](http://qiita.com/ritukiii/items/c475f646d0d69e325a4a))幾つかのパターンで書いてみたのですが実際に書いてみると良い悪いが見えてきたので一例としてご紹介します。

# 環境
|PHP|7.0.4|
|:--|--:|
|**PHPUnit**|**5.0.10**|
環境設定についてはこちらをご参考いただければと思います。
[MacにPHP7.0.4,PHPUnit5.0.10を入れる](http://qiita.com/hikarut/items/026e7087326ea0861bd6)

# テストするもの
例えば引数に指定した時間(秒)を返すメソッドのテストをします。ただし、引数に指定した時間と翌日0時までの時間を比べて翌日0時までの時間の方が短かったらそちらの時間を返すとします。
(例)
600秒を指定(現在時刻 23:45:59) → 600秒を返す
600秒を指定(現在時刻 23:55:00) → 300秒を返す
みたいなイメージです。例えば基本は10分キャッシュするものを0時のタイミングでキャッシュが切れるような時間を設定する時などに使うイメージです。

# パターン1
まずは元のコードです

```php:Time.php
/*
 * 翌日0時までと指定した時間の短い方を取得 
 *
 * @param int $time 時間(sec)
 */
public function getTime($time, $now = null, $tomorrow = null)
{
    // 引数が空なら現在時間を取得
    if ($now === null) {
        $now = time();
    }
    
    // 引数が空なら翌日0時の時間を取得
    if ($tomorrow === null) {
        $tomorrow = strtotime('tomorrow');
    }
    // 翌日0時と現在時刻の差分
    $tomorrowTime = $tomorrow - $now;
    
    // 指定した時間と比べて短い方を返す
    return min($tomorrowTime, $time);
}
```
time()やstrtotimeはそのままではテストできないため、引数として渡すパターンにしました。`getTime(600)`と使えば実行時の現在時刻、翌日0時が設定されるので使う側は第2,3引数を意識する必要がありません。
これに対するテストコードです。

```php:TimeTest.php
<?php
require_once 'Time.php';
class TimeTest extends PHPUnit_Framework_TestCase
{
    /**
     * @covers Time::getTime
     * @test
     * @dataProvider getTimeDataProvider
     */
    public function getTimeTest($time, $now, $tomorrow, $expected)
    {
        $obj = new Time();
        $this->assertEquals($expected, $obj->getTime($time, $now, $tomorrow));
    }
    public function getTimeDataProvider()
    {
        return [[600, strtotime('2016-04-15 23:40:59'), strtotime('2016-04-16 00:00:00'), 600],
                [600, strtotime('2016-04-15 23:50:01'), strtotime('2016-04-16 00:00:00'), 599],
                [600, strtotime('2016-04-15 23:59:59'), strtotime('2016-04-16 00:00:00'), 1]];
    }
}
```
先ほど使わなかった第2,3引数をテストコード上で指定してあげればちゃんとテストできます。

コード →　https://github.com/hikarut/dev/tree/master/testCode/TimeSample1

# パターン1の何が悪いか？
最初は一番簡単だと思ったパターン1の方法を思いついたのですが、よくよく考えるとこれはいけてないなと思いました。。何がまずでしょうか、、？
色々と突っ込みどころはあると思いますが自分が思ったところとしては

* 1.自由度が高くバグを生む可能性が高い

最初使う側は`getTime(600)`とすれば良いと言いましたが、第2,3引数が定義されている以上、誰がどう使うかはわかりません。そうなった時によく知らない人が`getTime(600,600,600)`とか使われる可能性もあります。
本来は第2引数(現在時間)、第3引数(翌日0時の時間)は使う側で指定されるべきではないです。これは使われ方によってはバグを生むきっかけになるかなと思います。

* 2.メソッド全体のカバレッジを担保できてない

ここでテストしたかったのは例えば現在時刻が23:55:00だった時に600(10分)を指定しても300(5分)で返ってくるか、や境界値テストだったりします。なのでそれに関していえばちゃんとテストできるとは思います。
しかしメソッドの最初にif分を入れたことによってこのif分に関してはテストが実行できない状態です。

```
    if ($now === null) {
        $now = time();
    }
```
要は結局この部分のテストができない状況です。ここをテストするには第2引数を指定しないかnullを指定する必要があり、その場合は結局テストの実行時間によってtime()の値が変わるためそれによって得られる結果も分からないためテストできません。
テストコードを書く時はメソッドのリターンだけを考えるのではなく、メソッド内の1行1行をちゃんと意識する必要があると思います。なのでこれだとカバレッジ率を低下させる原因になります


# パターン2
では上記課題を踏まえて改善版としてパターン2ではこちらで実装してみました

```php:Time.php

<?php
class Time
{
    /*
     * 翌日0時までと指定した時間の短い方を取得 
     *
     * @param int $time 時間(sec)
     */
    public function getTime($time)
    {
        // 翌日0時と現在時刻の差分
        $tomorrowTime = $this->getTomrrow() - $this->getNow();
        
        // 指定した時間と比べて短い方を返す
        return min($tomorrowTime, $time);
    }
    /*
     * 翌日0時の時間を取得
     *
     */
    public function getTomrrow()
    {
        return strtotime('tomorrow');
    }
    /*
     * 現在時間を取得
     *
     */
    public function getNow()
    {
        return time();
    }
}
```
先ほどは引数としていた現在時間と翌日0時の時間をメソッドとして外出ししています。
これに対するテストコードです。


```php:TimeTest.php
<?php
require_once 'Time.php';
class TimeTest extends PHPUnit_Framework_TestCase
{
    /**
     * @covers Time::getTime
     * @test
     */
    public function getTimeTest()
    {
        // Timeクラスのモックを作成
        $mock = $this->getMockBuilder('Time')
                     ->setMethods(['getNow', 'getTomrrow'])
                     ->getMock();
        // getNowメソッドのモック
        $mock->method('getNow')
             ->will($this->onConsecutiveCalls(strtotime('2016-04-15 23:40:59'),
                                              strtotime('2016-04-15 23:50:01'),
                                              strtotime('2016-04-15 23:59:59')));
        // getTomrrowメソッドのモック
        $mock->method('getTomrrow')
             ->will($this->onConsecutiveCalls(strtotime('2016-04-16 00:00:00'),
                                              strtotime('2016-04-16 00:00:00'),
                                              strtotime('2016-04-16 00:00:00')));
        $this->assertEquals(600, $mock->getTime(600));
        $this->assertEquals(599, $mock->getTime(600));
        $this->assertEquals(1, $mock->getTime(600));
    }
}
```
これはPHPUnitのgetMockBuilderを使ってメソッドのリターンを上書きするようなイメージです。モックについてはこちらに詳細が書かれています
[第9章 テストダブル](https://phpunit.de/manual/current/ja/test-doubles.html)

注意点としてはgetMockBuilderを使うとデフォルトではクラスにあるメソッド全てが置きかわります。メソッドについて指定がない場合は全てnullが返るメソッドになってしまいます。
なので必要なメソッドだけ置き換える(パーシャルモック)にする場合は明示的にメソッドを指定してあげます`->setMethods(['getNow', 'getTomrrow'])`

あとは`->will($this->onConsecutiveCalls`でモック化したメソッドのリターンを書いてあげて、実際にテストしたいメソッドを実行するだけです。`$mock->getTime(600)`

こんな感じにすればパターン1で課題だった引数にすることで自由度が高まることもありません。引数が1つになったので意図しない使われ方はしなくなります。
またメソッドのif分も消えたのでカバレッジ率も上がるはずです。

コード →　https://github.com/hikarut/dev/tree/master/testCode/TimeSample2


# 終わりに
本来であればテストが書けるコード=メンテナンスしやすい良いコードとなる場合が多いとは思いますが、今回のパターン1のように書くとかえってバグも生みやすくなる気もするので逆によくないような気もしました。テストコード書く時はとりあえずかければいいというよりはちゃんとその後のことも考えて書かないとだなーと個人的に思いました。