🙅

テストで無闇にAllMatch的な処理をするのはやめよう

に公開
Java
Test
JUnit
tech

なんの話？

テストを書いていることは大前提として、 そのテストが失敗した時のことまで考えて書いていますか？ という話です。
具体例に関しては諸説あります。

どういう状況？

例えばユーザー一覧を取得した時に、 IDが50以下のユーザーだけ取得できていること をテストしたいとします。
（実際にこんなテストをするかどうかは今は気にしない）

ユーザーは id を持ちます。

public class User {
    private final Integer id;
    // コンストラクタ・ゲッターは省略
}

allMatchを使った実装

いざ、 getUserList というメソッドを使いユーザーを取得して、IDが50以下であることをアサートしましょう！

public class UserListTest {
    @Test
    public void test_getUserList() {
        List<User> userList = getUserList();
        assertTrue(userList.stream().allMatch(user -> user.getId() <= 50));
    }
}

できました！
めちゃめちゃシンプルに書けてるし、いい感じなのでは 🤔

何が悪いの

このようなテストだと、テストが失敗した時に何が間違っていたのか把握することが難しくなります。
例えば id = 60のユーザーがリストに含まれている時、下記のようなログが出力されます。

expected: <true> but was: <false>

これだとどんなデータがuserList含まれていたのか分からず、なぜテストが失敗したのか良く分かりませんね。

こうしてみよう

public class UserListTest {
    @Test
    public void test_getUserList() {
        List<User> userList = getUserList();
        List<User> filtered = userList.stream()
            .filter(user -> !(user.getId() <= 50))
            .collect(Collectors.toList());
        assertEquals(filtered, Collections.emptyList());
    }
}

このようにIDが50以下のユーザーを取り除き、その結果が空のリストと等しいことをアサートします。
行数も増え条件が否定系になるので少しだけ可読性は下がるかもしれませんが、テストが失敗した時のログは分かりやすくなります。

expected: <[]> but was: <[User(id=60)]>

まとめ

  • テストが失敗した時にどんなログが出力されるのか考えながらテストを書きましょう
  • そのために、 assertTrue ではなく assertEquals を使うようにしましょう

メモ

  • サンプルリポジトリ: https://github.com/sanopi/dont-use-allmatch/
  • サンプルコードではListの具体的な型が異なる可能性がありますが、アサーションには問題ありません。気になる方はassertEqualsの実装や、java.util.List#equalsのドキュメントを参照してみてください。

Discussion