テストコードのリファクタリングを進めている話

こんにちは竹田です。

今回は採用係長のプロダクトの一部のテストコードを少しずつ改善している話です。プロダクト内の実際にあったテストコードのアンチパターンの紹介と現在どのようにコードを改善しようとしているのか紹介したいと思います。テストフレームワークはPHPUnitです。

テストメソッド名が単純すぎて何をテストしているのか読み取れない

例えば　sum()という関数のテストメソッドがtest_sum()になっているようなパターンです

このテストの問題点はsum()のテストをすること以外の情報が何も得られない事です。仮にこのテストが失敗した場合のテスト実行時ログから得られる情報が少なくなります。

例えば以下のようなテストがあるとします

    public function test_sum()
    {
        $result = sum(3,4);
        $this->assertEquals($result,8);
    }

実行時のログです

1) Test::test_sum
Failed asserting that 8 matches expected 7.
FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

仮にテストコードについて何も知らない人がテスト結果だけを見た時に内容（何と何を足したのか）を読み取ることができません。なのでテストコードを以下のように関数名を日本語使うように修正してしまいます。

    public function test_3＋4が7になること()
    {
        $result = sum(3,4);
        $this->assertEquals($result,8);
    }

この修正によりエラーメッセージを見た人がテスト内容を失敗のログから理解できるようになります。

1) Test::test_3＋4が7になること
Failed asserting that 8 matches expected 7.
FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

＊テストメソッドの日本語化においては賛否あると思われますが、ネットオンでは以下のメリットが大きいと考え取り入れることにしました。デメリットとしてはコード補完されない等です。

テスト名を考えるのが簡単
テスト名から内容がわかりやすい
現在は国内の方しかテストメソッドを触ることがない

テスト内にロジックが入ってしまっている。

以下は極端な例かもしれませんがアサーションをfor文の中で使うと以下のような単純な足し算のコードでさえ複雑になってしまいます。DRY[繰り返しを避ける]なコードよりもDAMP[明確でわかりやすい]なコードを目指しましょう

    public function test_sum()
    {
        $test_array = [
            [1,2,3],
            [4,5,9],
            [30,100,130]];
        foreach ($test_array as $key => $value) {
            $this->assertEquals(sum($value[0],$value[1]),$value[2]);
        }
    }

冗長に感じますがテストコードにおいてはこの書き方が明確に相手に伝わります。

    public function test_1＋2＝3()
    {
            $this->assertEquals(sum(1,2),3);
    }
    public function test_4＋5＝9()
    {
            $this->assertEquals(sum(4,5),9);
    }
    public function test_30＋100＝130()
    {
            $this->assertEquals(sum(30,100),130);
    }

データベースが絡んだテストにてデータセットの挿入過程がない

以下のテストではどこからともなくユーザーのデータをDBから取得しているテストになっています。テストケースを見た時にどういうデータを挿入しているか不明な状態だと、このテストが失敗した場合の修正に時間がかかります。

    public function test_点数が正しいこと()
    {
        //データベースからユーザーID1のデータを取得する
        $test_data = Model_score::find(["user_id",1]);
        $this->assertEquals($test_data->score,90);
        
    }

以下のように明確にDBにデータを挿入するコードがあると明確です。

    public function test_点数が正しいこと()
    {
        $score = Model_score::forge([
            "user_id" =>1,
            'score' => 90
        ]);
        $score->save();
        $test_data = Model_score::find(["user_id",1]);
        $this->assertEquals($test_data->score,90);  
    }

データベースを共有しており各テストが後処理をしていない

先ほどデータべースが絡む処理の例を上げましたが、Model_score内のデータがそのままになっているの場合はフィクスチャで指定して削除しておきましょう。

https://phpunit.readthedocs.io/ja/latest/fixtures.html

    protected function setUp()
    {
　　　　	\DBUtil::truncate_table('score');
    }

終わりに

いかがだったでしょうか？私は特にDRY原則よりDAMP原則を意識することを結構強く意識しています。つい繰り返しを避けてしまうのが癖になっています。今回紹介したものだけでも意識するだけでコードの読みやすさが一気に上がると思いますぜひ参考にしてみてください。

Post Views: 871