FLINTERS Engineer's Blog

FLINTERSのエンジニアによる技術ブログ

テストコードを粛々とリファクタリングした話

勉強会 Java JUnit TDD リファクタリング

元記事はこちらです。最新版もこちらにあります。
「テストコードを粛々とリファクタリングした話」

h0ng0yut0です。
今回は、「テストコードを粛々とリファクタリングした話」を書かせていただきます。
（※掲載されているソースコード等はこの記事以外に使用されておりません。）

読んでほしい方

  • 僕と同じく、テストコード触りたてで、これから頑張っていこうと思っている方
  • チームで導入してみたものの、なんか突っ走りすぎちゃってる気がしている人

テストコードを粛々とリファクタリングした話

チームに参加した直後のテストコード

// ーーーーもろもろインポートーーーー
@RunWith(Seasar2.class)
public class UserCommentDaoTest {

    UserCommentDao userCommentDao = new UserCommentDao();

    /**
     * テストの準備
     */
    TestContext ctx;
    public void before(){
        ctx.setPreparationType(PreparationType.ALL_REPLACE);
    }

    public void testGetUserCommentList() {
        //変数
        String userId = null;
        String from = null;
        String to = null;
        String comment = null;
        String orderByItem = null;
        int limit = 50;
        int page = 1;
        List<UserCommentListDto> result = new ArrayList<UserCommentListDto>();

        //正常系：userId1のコメントを取得
        userId = "1";
        from = null;
        to = null;
        comment = null;
        orderByItem = "createdAt";
        result = new ArrayList<UserCommentListDto>();
        result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
        assertThat(result.size(), is(6));

        //正常系：期間指定でuserId1のコメントを取得
        userId = "1";
        from = "2014-11-01 00:00:00";
        to = "2014-11-05 00:00:00";
        orderByItem = "createdAt ASC";
        result = new ArrayList<UserCommentListDto>();
        result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
        assertThat(result.size(), is(6));
        assertNotSame(result.get(0).userId, result.get(1).userId);

        //正常系：コメント(部分一致)でuserId1のコメントを取得
        userId = "1";
        comment = "サンプルコメント";
        result = new ArrayList<UserCommentListDto>();
        result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
        assertThat(result.size(), is(1));

        //正常系：フィルタリング結果なし
        userId = "1";
        comment = "サンポーコメンツ";
        result = new ArrayList<UserCommentListDto>();
        result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
        assertThat(result.size(), is(0));

        //異常系：存在しないuserId
        userId = "999";
        result = new ArrayList<UserCommentListDto>();
        result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
        assertThat(result.size(), is(0));

        //異常系：userIdがnull
        result = new ArrayList<UserCommentListDto>();
        result = userCommentDao.getUserCommentList(null, from, to, comment, orderByItem, limit, page);
        assertThat(result.size(), is(0));
    }

    // ----こんな感じで、メソッドごとにたくさんのアサーション等が…----

}

１メソッドに対して１テストメソッドが用意されており、その中に複数のテストケース、複数のアサーションが混在している形でした。

含まれているアンチパターンについて

アンチパターン このコードにおいて なにが困るの？
嘘つき 嘘つきというか、取得してきたresultのサイズでのアサーションとなっており、検索条件に合致したデータを取得できているのかというアサーションが存在しない。 正しくそのメソッドが動作していることを保証したテストコードにはなっていないので、テストコードの意味自体がとても薄れてしまう。
タダ乗り タダ乗りというか、テストケースごとにテストメソッドを書いておらず、メソッドの全機能を１つのテストメソッドでチェックしようとしている。 途中のアサーションで予期せぬ結果が出た場合に以降のアサーションは判断されないので、どんなテストケースで失敗しているのかを１度のユニットテスト実行で判断できない。
のろまな動き どうやら、ユニットテストを回す際に、CSVデータをmysqlに毎回つっこんでテストをしているらしい DB繋ぐ回数が多いと、テスト遅くなるよね。

修正の方針

リファクタリング後のテストコード

@RunWith(Enclosed.class)
public class UserCommentDaoTest {

    UserCommentDao userCommentDao = new UserCommentDao();

    private static void initializeDaoTest() throws Exception {
        TestHelper.preparingData(userCommentDao, new String[] { "user", "user_comment" });
    }

    static {
        try {
            initializeDaoTest();
        } catch (Exception e) {
            e.printStackTrace();
            fail(e.toString());
        }
    }

    @RunWith(Seasar2.class)
    public static class testGetUserCommentList {

        String userId;
        String from;
        String to;
        String searchComment;
        String orderByItem;
        int limit;
        int page;
        List<UserCommentListDto> result;

        @Before
        public void before() {
            userId = "1";
            from = null;
            to = null;
            searchComment = null;
            orderByItem = "createdAt";
            limit = 50;
            page = 1;
            result = new ArrayList<UserCommentListDto>();
        }

        @Test
        public void userId1のコメントを取得() {
            result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);

            assertThat(result.size(), is(not(0)));
            for (UserCommentListDto comment : results) {
                assertThat(comment.userId, is(userId));
            }
        }

        @Test
        public void 期間指定でuserId1のコメントを取得() {
            from = "2014-11-01 00:00:00";
            to = "2014-11-05 00:00:00";
            result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);

            assertThat(result.size(), is(not(0)));
            for (UserCommentListDto comment : results) {
                createdTimeMills = comment.createdAt.getTime();
                fromTimeMills = from.getTime();
                toTimeMills = to.getTime(); 

                assertThat(createdTimeMills, greaterThanOrEqualTo(fromTimeMills));
                assertThat(createdTimeMills, lessThanOrEqualTo(toTimeMills));
            }
        }

        @Test
        public void コメントの部分一致でuserId1のコメントを取得() {
            searchComment = "サンプルコメント";
            result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);

            assertThat(results.size(), is(not(0)));
            for (UserCommentListDto comment : results) {
                assertThat(comment.commentText.contains(searchComment), is(true));
            }
        }

        @Test
        public void コメントの部分一致しない場合() {
            searchComment = "サンポーコメンツ";
            result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
            assertThat(result.size(), is(0));
        }

        @Test
        public void 存在しないuserIdの場合() {
            userId = "999";
            result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
            assertThat(result.size(), is(0));
        }

        @Test(expected = Exception.class)
        public void userIdがnullの場合() {
            userId = null;
            result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
        }

    }

    // ----他、同じように修正----
}

リファクタリングの解説

アンチパターンの解消に関して

アンチパターン このコードにおいて 利用したもの
嘘つき 様々なMatcher、様々なJavaのメソッドを調査して、しっかりとテストケースに合ったアサーションに書き換えた。 Matcher等
タダ乗り 構造化を行うことで、１つのメソッドに対して複数のテストケースとそれに対応したテストメソッドを記述した。（xSpec系等でよく言われているdescriveとcontextのような理想の形とは違うののの、落とし所だとは思っている。） Enclosed
のろまな動き 今回のソースには載せていないが、jdbcManagerの接続先をh2dbのmysqlモードに変更して対応している。（ついでにユニットテストに使うデータとローカル環境構築でmigrateするときに用意するデータの二元管理されていて、なにかと面倒くさいので一元管理に変更した。） "先輩"の記事のリンク

その他


なぜstaticイニシャライザなのか

デバッガとかSystem.out埋めての調査でアレですが、複数ファイルを同時にユニットテストを実行する場合、BeforeClassアノテーションをつけたメソッドが１番初めに呼ばれたBeforeClassアノテーションをつけたメソッドしか動作しなかったので。


テストスイートを作成しました

Enclosedを使用して構造化したのですが、テストメソッドが２重に実行されてしまう問題が起こったため。


テストコードをDRYにしすぎると、可読性が下がると言われていますが…

おそらくsetUpやtearDownのような仕組みを使ったり、独自メソッドを使ってDRYにすることはあると思います。
僕の場合よくやるのは、以下のような手法です。
1. setUpに正常系の場合の変数の値をセットする。
2. テストメソッド内で、テストケースに関わる変数に関して代入を行い、テスト対象のメソッドを実行している行と近い状態にしておく。

// Beforeアノテーションをつけたメソッドに正常系を用意

@Test(expected = Exception.class)
public void userIdがnullの場合() { // ←　テストケース
    userId = null;                // ←　テストケースに沿ったデータの用意
    result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
}

所感

結構ありがちなのが、『動いているプロダクションコードをあまり触りたくない』という感情だと思います。動いているプロダクションコードに触れる恐怖や不安を取り除く１つのツールがテストコードだとおもいます。そのツールを使いこなせるようになり、不安がなくなったときに「プロダクションコードをよりよい状態」にfearlessに変更できますね。

今回は、「プロダクションコードをよりよい状態」にするための準備の準備みたいなお話でした。

準備はできたので、これからレガシーコードに少しづつ挑んでいきます。

読んでいただいた方へ感謝

長い記事を読んでいただきありがとうございます。まだ新卒で入社してキャリア１年ほどのエンジニアなので、色々とアドバイス等いただけると嬉しいです！
http://qiita.com/h0ng0yut0

参考サイト

「TDDのアンチパターン」
http://www.hyuki.com/yukiwiki/wiki.cgi?TddAntiPatterns

追記

このQiitaの記事を書いた後、チームで「Light Lightning Talk（軽めのライトニングトーク）」として、以下リンクのスライドと共にチームメンバーにテストコードに関して共有いたしました。

Qiita記事をトリガにして、社内LTとかを文化にしてみても面白いと思いますヽ(´ー｀)ノ