xshoji's blog

15 min read

単体テスト ベストプラクティス集

#java #unit test

目次

Modern Best Practices for Testing in Java
https://phauer.com/2019/modern-best-practices-testing-java/

テスト時に何気なく気をつけている事が根拠とともに具体的に示されていて、 自分自身がテストする時はもちろん、コードレビュー時でも注意・指摘したい内容がまとめられています。

タイトルには「Java」と書かれていますが、プラクティスとしては言語に依存しない内容がほとんどなので、 特定の言語に限らない有益なプラクティスだと思いました。

それでは、それぞれの中身をまとめていきます。

一般的なプラクティスの話

テストメソッドでは、以下の基本ルールを守ると良いみたいです。

  • (1) 1つの空行で区切られた3つのブロックを含むようにする
    • Given (Input): データ作成やモックの設定などのテスト準備
    • When (Action): テストしたいメソッドやアクションを呼び出す
    • Then (Output): アサートを実行して、アクションの正しい出力や動作を検証する
  • (2) 等値アサートで変数を使用する場合は、変数名の前に actual と expected を付ける
    • 実際に取得される値: 変数名の前に actual をにつける
    • 取得される値の期待値: 変数名の前に expected をつける
    • モチベーション: 比較時の意図が明確になり間違いが減るし、可読性もあがるため
  • (3) ランダムな値（特に時刻など）は使わない。固定値を使って再現性を高める

(1) 1つの空行で区切られた3つのブロックを含むようにする

(2) 等値アサートで変数を使用する場合は、変数名の前に actual と expected を付ける

最初に、(1), (2)をまとめると、以下の形が理想的です。

  @Test
  public void getTest() {
    // Given
    UserGetRequest request = new UserGetRequest(1111L);
    UserDao dao = new UserDao();

    // When
    User actualUser = dao.get(request);

    // Then
    User expectedUser = new User(1111L, "John");
    assertThat(actualUser, is(expectedUser));
  }

GroovyのSpock frameworkを使っていると、構文としてこのあたりが用意されていて自然と意識する形になっていますよね。

Spock Framework Reference Documentation
http://spockframework.org/spock/docs/1.3/all_in_one.html

spock-workshop/02_basics.md at master · yamkazu/spock-workshop
https://github.com/yamkazu/spock-workshop/blob/master/docs/02_basics.md#setup

こういうフレームワークに関わらず、テストする時これらのステップごとにブロックを分けてちゃんと記述しようねってことです。

(3) ランダムな値（特に時刻など）は使わない。固定値を使って再現性を高める

結果が毎回変わるような、ランダム化される値が登場するテストは行わないようにします。

以下が悪い例です。

  /**
   * ダメな例
   */
  @Test
  public void formatBadTest() {
    // Given
    Instant currentInstant = Instant.now(); // example => 1601739774
    EpochSecondFormatter formatter = new EpochSecondFormatter();

    // When
    String actualDate = formatter.format(currentInstant); // example:  => DATE: 20201003

    // Then
    String expectedDate = createExpectedString(currentInstant);
    assertThat(actualDate, is(expectedDate));
  }

こういうテストはアサートが失敗した場合に出るログの値が毎回変わるのでデバッグが困難になります。 また、コメントに記載されている値も役に立ちません。

以下のように、固定値を使い、毎回同じ結果になるようにします。

  /**
   * 良い例
   */
  @Test
  public void formatGoodTest() {
    // Given
    Instant fixedInstant = Instant.ofEpochSecond(1601739774);
    EpochSecondFormatter formatter = new EpochSecondFormatter();

    // When
    String actualDate = formatter.format(fixedInstant); // DATE: 20201003

    // Then
    String expectedDate = createExpectedString(fixedInstant);
    assertThat(actualDate, is(expectedDate));
  }

こうすることでいつ実行しても毎回同じ結果になる再現性の高いテストになります。

小規模で具体的なテストを書こうという話

  • (1) 繰り返し使うコードは専用のメソッドに切り出し、分かりやすい記述的な名前を付ける
  • (2) 複数回使用される値を変数に抽出するのは実はやらない方が良い
  • (3) 期待される動作についてのテストメソッドをそれぞれ個別に用意する
    • 期待する動作が何なのかが分かりやすいテストメソッド名にする
    • 1つのでかいテストメソッドの中でいろんなコーナーケースのテストはやらないほうが良い
  • (4) テストしたい部分だけをアサートする

(1) 繰り返し使うコードは専用のメソッドに切り出し、分かりやすい記述的な名前を付ける

テスト用のオブジェクトを作るだけのメソッド、みたいな、みんなよく無意識のうちにやってることだとは思います。

ただ、ここで大切なのは、「テストに関係するフィールドをパラメータとして指定させるメソッド」にするとより見通しが良いみたいです。

  @Test
  public void getByIdGreaterThanTest() {
    // Given
    UserFilter userFilter = new UserFilter(Arrays.asList(
            createUserWithId(10L),
            createUserWithId(20L),
            createUserWithId(30L)
    ));

    // When
    List<User> actualUsers = userFilter.getByIdGreaterThan(15L);

    // Then
    assertThat(actualUsers.size(), is(2));
    assertThat(actualUsers, is(containsInAnyOrder(createUserWithId(20L), createUserWithId(30L))));
  }

ここでは、IDで User クラスを絞り込む処理のテストをしているので、 User.name フィールドは何でも良いです。 なので、 createUserWithId(Long id) というIDを指定して User を返してくれる（ User.name の値は何でも良い）テスト用のメソッドを用意します。

(2) 複数回使用される値を変数に抽出するのは実はやらない方が良い

これは意外でした。

A usual reflex of a developer is to extract values that are used multiple times to variables.
複数回使用される値を変数に抽出するのが開発者の常套手段です。

よくありがちというか、むしろそうした方が良いという人がいそうなくらいの話ですが、 この元記事では、実は変数化のやりすぎは良くないと紹介されていました。

というのも、アサートが失敗した時のログからテストコードを辿る際、 変数化されていると実際に問題がある行までトレースするのに時間がかかってしまうから、ということのようです。ふーん。

この「複数回使用される値を変数に抽出する」良くない例が以下です。

  @Test
  public void getByIdGreaterThanVariableIdTest() {
    // Given
    Long id1 = 10L;
    Long id2 = 20L;
    Long id3 = 30L;
    UserFilter userFilter = new UserFilter(Arrays.asList(
            createUserWithId(id1),
            createUserWithId(id2),
            createUserWithId(id3)
    ));

    // When
    List<User> actualUsers = userFilter.getByIdGreaterThan(15L);

    // Then
    assertThat(actualUsers.size(), is(2));
    assertThat(actualUsers, is(containsInAnyOrder(createUserWithId(id2), createUserWithId(id3))));
  }

これJUnitだと分かりづらいんですかね？会社ではSpock Framework使ってますが、Groovyには Power assert っていう便利な機能がついていて、

The Apache Groovy programming language - Testing guide
https://www.groovy-lang.org/testing.html#_power_assertions

例えば、以下の様に失敗するテストを実行すると…

  def "Groovyの場合"() {
    setup: 
    Long id1 = 10L
    Long id2 = 20L
    Long id3 = 30L
    UserFilter userFilter = new UserFilter(Arrays.asList(
            createUserWithId(id1),
            createUserWithId(id2),
            createUserWithId(id3),
    ));


    when:
    List<User> actualUsers = userFilter.getByIdGreaterThan(15L)

    then: 
    assert actualUsers.size() == 2
    assert actualUsers[0].id == id2
    assert actualUsers[1].id == id1
  }

以下のような感じでエラーがあった行とその値をあわせて表示してくれます。

// Console output

actualUsers[1].id == id1
|          |   |  |  |
|          |   30 |  10
|          |      false
|          io.github.xshoji.samplecode.bestpractice.testingtarget.User@c868802
[io.github.xshoji.samplecode.bestpractice.testingtarget.User@c860008, io.github.xshoji.samplecode.bestpractice.testingtarget.User@c868802]

なので、ここで指摘されている懸念は意識したことありませんでした。 Javaを主軸にした記事なのでそこは仕方ないですね。SpockはGroovyなんで。

話を戻して、変数にまとめないと何度も同じ値を書くことになるのでDRY原則に反しそうですが、この場合は

KISS. Keep It Simple,Stupid > DRY. Don’t Repeat Yourself.

という考えのようです。まぁ、コードは読む時間のほうが圧倒的に長いですからね。 とはいえ、変数化自体はやったほうが良い場合ももちろんあるので、ここでは複雑化させる要因にもなり得るから気をつけよう、程度で抑えておけば良さそう。

(3) 期待される動作についてのテストメソッドをそれぞれ個別に用意する

これの逆を結構やってしまいがち。GetTest() とかいうテストメソッド内で通常系とかエラーとか色々テストしちゃってるというパターン。 特に、機能追加などで既存のメソッドの役割が膨らんでいった場合に、 元のテストメソッドにどんどんパターンを追加していっちゃう、みたいなやつ。

Yes, it’s more writing effort but you can create a tailored and clear test, that only test the relevant behavior.
そうです、それはより多く書く労力が必要ですが、関連する動作のみをテストするような、カスタマイズされた明確なテストを作成することができます。

テストメソッド内でやってることが多すぎて、修正しようにもどこ直せば良いかわからない！となるのを防ぐため、 1テストメソッドを1つの観点だけにしぼり、全体の各テストメソッドで使われる共通の処理は どんどんヘルパーメソッドに切り出していこう、という話です。

テストが失敗した時になんで失敗したかっていうのが分かりやすくあるべきという事のようです。

(4) テストしたい部分だけをアサートする

So we should only check the relevant field to clearly state and document the scope of the logic under test.
そのため、関連するフィールドだけをチェックして、テスト対象のロジックの範囲を明確に示し、文書化する必要があります。

関係ないところも一応アサートしておくか、みたいなの割とやってしまいがちですが、意味がないのでやめましょうってことですね。 特に、色んなメソッドで使われてる共通のビジネスロジックは、複数のテスト内で何度も同じ観点のアサートしちゃってるケースは多いので、 (3)のテストメソッドを分離する話と合わせて、

  • 共通でアサートすべきところ
  • コーナーケースとして追加でアサートすべきところ

を分離した構成にすることが求められそうです。

自己完結型テストの話

  • (1) テストコードの読者にメソッドの定義へのジャンプを強制させない
    • テストで制御する必要があるパラメータをヘルパーメソッドの引数にする
    • 事前データの準備機能（JUnitだと @Before）は使わず、ヘルパーメソッドを明示的に呼び出す形にする
  • (2) 継承よりも合成を優先する

(1) テストコードの読者にメソッドの定義へのジャンプを強制させない

これは「繰り返し使うコードは専用のメソッドに切り出し分かりやすい記述的な名前を付ける」 に出てきた内容と少し被りますが、事前データを用意するためのヘルパーメソッドの引数は、 必ずテストに関係する引数を渡すようにする、ということです。

// 悪い例
User user = createUser();

// 良い例
User user = createdUserWithId(11111L);

悪い例の引数なしのヘルパーメソッドだと、中で何が作られるのか定義に飛んで確認しないと読んだ人は分かりません。

良い例の場合、少なくともIDが 11111 のUserが作られていることは定義に飛ばなくても分かります。常にこっちの形になるようにしましょう。

また、共通の事前処理を仕込むための仕組み（JavaのJUnitだと @Before アノテーションをつけたメソッドは各テストの実行前に必ず自動で呼び出してくれる）はできるだけ使わないようにし、そういった共通処理はヘルパーメソッド名に適切な名前をつけ、 各テストメソッド内で個別に呼ぶようにします。

以下、だめな例です。（完全にダメではないけど、良くない例）

  private static List<User> targetUsers;

  /**
   * ダメな例
   */
  @Before
  public void setup() {
    targetUsers = Arrays.asList(
            createUserWithId(10L),
            createUserWithId(20L),
            createUserWithId(30L)
    );
  }

...

  @Test
  public void getByIdGreaterThanTestWithBeforeSetup() {
    // Given
    UserFilter userFilter = new UserFilter(targetUsers);

    // When
    List<User> actualUsers = userFilter.getByIdGreaterThan(15L);

    // Then
    assertThat(actualUsers.size(), is(2));
    assertThat(actualUsers, is(containsInAnyOrder(createUserWithId(20L), createUserWithId(30L))));
  }

これだと、 getByIdGreaterThanTestWithBeforeSetup のテストを読むためには、 事前処理でデータがセットアップされることを知らないと理解できません。 このため、事前処理が行われるメソッドの定義を探すこと、さらにそこに飛んで中身を読むことを強制されます。

この場合、以下の方が良いです。

  @Test
  public void getByIdGreaterThanTestIncludesSetup() {
    // Given
    List<User> targetUsers = createUsersHavingIdFrom10To30();
    UserFilter userFilter = new UserFilter(targetUsers);

    // When
    List<User> actualUsers = userFilter.getByIdGreaterThan(15L);

    // Then
    assertThat(actualUsers.size(), is(2));
    assertThat(actualUsers, is(containsInAnyOrder(createUserWithId(20L), createUserWithId(30L))));
  }

事前処理でやって良いのは、データベースのセットアップや、HTTP通信周りの設定など、 各テストケースに依存しない共通設定（テストケースの読者が詳細を意識しなくて良いこと）を書くべきです。

(2) 継承よりも合成を優先する

継承よりも委譲を使ったほうが良いという議論

Effective Java Tuesday! Favor Composition Over Inheritance - DEV
https://dev.to/kylec32/effective-java-tuesday-favor-composition-over-inheritance-4ph5

とは少し趣旨は違いますが、 言ってることは同じです。テストで継承を使わない方が良い理由は以下の通りです。

  • 現在のテストが必要としていないものを多く含むベーステストクラスを拡張する羽目になる
  • 一つ、あるいは複数の基底クラスを飛び回らなければならなくなる
  • 継承は柔軟性に欠ける

The Wrong Abstraction — Sandi Metz
https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction

“Prefer duplication over the wrong abstraction.” Sandi Metz.
“間違った抽象化よりも重複を優先する” サンディ・メッツ

具体的には、以下のようなテストの構成は避けましょう。

public class SelfContainedTestBase {
  protected Connection connection;
  @Before
  public void setup() {
    this.connection = this.setupDatabaseConnection();
    this.loadFixtures();
  }
  ...
}


public class SelfContainedTest extends SelfContainedTestBase {
  @Test
  public void extendedTest() {
    // Given ( fixture data is loaded in SelfContainedTestBase )
    UserDao dao = new UserDao(this.connection);
    
    // When
    List<User> actualUsers = dao.getAll();
    
    // Then
    User expectedUser = new User(1111L, "John");
    assertThat(actualUsers, is(containsInAnyOrder(expectedUser)));
  }
}

継承元の setup() の処理相当のクラスを個別に分離し、 以下のように継承をやめ、テストメソッド内でそれらを個別に呼び出す作りの方が良いです。

public class SelfContainedTest {
  @Test
  public void compositionTest() {
    // Given
    Connection connection =  new DatabaseConfiguration().setupConnection();
    FixtureLoader loader = new FixtureLoader(connection);
    loader.load();
    CompanyDao dao = new CompanyDao(connection);

    // When
    List<Company> actualCompanies = dao.getAll();

    // Then
    Company expectedCompany = new Company(1111L, "Apple");
    assertThat(actualCompanies, is(containsInAnyOrder(expectedCompany)));
  }
}

とても複雑な手続きが必要で、どうしても継承して共通処理にしたい！って場合でも、 その複雑な手続きを行うだけのクラスを別で用意し、各クラスのGivenで毎回呼ぶほうが良いと思います。 とにかく、コードを読む人が関心事となるテストメソッド以外の箇所を意識しなくて済むようにする、ってことですね。

出力はハードコードされた期待値と比較しようというの話

  • (1) Production Code（アプリ本体のコード）をテストで再利用しない
  • (2) Production Logic（アプリ本体のロジック）をテストに持ち込まない
  • (3) テストメソッド内にロジックを書きすぎないようにする

(1) Production Code（アプリ本体のコード）をテストで再利用しない

テスト対象の処理以外のProduction Code（アプリ本体のコード）はテストで再利用するな、ということ。どういうことかというと、

  • アサートしやすくするためにProduction Codeのとあるメソッドを使って期待値を生成して比較する

のようなケース。以下、元記事のサンプルコード。

// Don't
boolean isActive = true;
boolean isRejected = true;
insertIntoDatabase(new Product(1, isActive, isRejected));

ProductDTO actualDTO = requestProduct(1);

// production code reuse ahead
List<State> expectedStates = ProductionCode.mapBooleansToEnumList(isActive, isRejected);
assertThat(actualDTO.states).isEqualTo(expectedStates);

ここでは、 requestProduct(1) のInとOutのみをテストすべきであって、期待値の生成のために ProductionCode.mapBooleansToEnumList というProduction codeを登場させてはいけないということ。

何が問題かというと、

If you reuse production code in a test, you might miss a bug that is introduced in the reused code because you don’t test this code anymore.
テストでProduction codeを再利用すると、このコードをテストしなくなったために、再利用したコードに導入されたバグを見逃してしまう可能性があります。

つまり、 requestProduct(1) の中で ProductionCode.mapBooleansToEnumList が使われていると、 ProductionCode.mapBooleansToEnumList にバグがあった場合に、間違った結果と間違った期待値同士での検証となってしまい、アサートをパスしてしまうため。 テストメソッドにおいては、

  • テスト対象の入力を分かりやすく設定する
  • 出力の期待値にはハードコードされた値を使う

が良いらしい。

(2) Production Logic（アプリ本体のロジック）をテストに持ち込まない

(1)に似てるけど、こっちは本番で使ってるコードをコピーしてテストクラスで使う、はやめておけってやつです。

特に、変換処理とか何かしら値を加工するようなシンプルな実装があった時、 テストでその変換処理が欲しくなってアプリ本体のコードを部分的にテストクラスに持ってくるみたいなことがあります。

そうするのではなく、変換結果をテストメソッド内にハードコードしてそれを期待値とせよ、ということみたいです。

(3) テストメソッド内にロジックを書きすぎないようにする

テストメソッドは、基本的にはInとOutの比較だけのはず。

例外、外れ値、異常値などいろいろなケースを1つのメソッド内でアサートし始めると、 for文やif文の嵐になるのでそうならないようにすべき。

現実に近いテストの話

ここまでは、単体テストについての話でしたが、最終的には「統合テスト」が一番威力が高い、信頼できるテストになる、という話です。

結局単体テストはMockを使ったり、クラス間をつないだ場合の動作は保証できないので、 リファクタリングなどによって内部の振る舞いが変わった場合でもテストでそれに気づけない場合があります。

「統合テスト」があれば、同じ動作をしているかどうかを単体テストよりも正確に保証できます。 ここでいう「統合テスト」とは、実際にアプリケーションを動作させて振る舞いの検証を行うテストのことです。

By “integration tests” (or “component test”) I mean putting all classes together (just like in production) and test a complete vertical slide going though all technical layers (HTTP, business logic, database).
「統合テスト」により、本番と同じようにすべてのクラスをまとめて、かつすべての技術層(HTTP、ビジネスロジック、データベース)を完全に上から下まで通るテストを実施できることを意味します。

この統合テストについては詳しく知りたいなら、別の記事見てちょって書いてあった…笑

Focus on Integration Tests Instead of Mock-Based Tests
https://phauer.com/2019/focus-integration-tests-mock-based-tests/

テスタブルな実装の話

ここはテスト方法というより、実装の話なのでさらっとだけ。

  • (1) Production codeで静的アクセスを使わない
  • (2) ロジックが持つ制限（上限値やしきい値など）はパラメタとして外から設定可能にする
  • (3) Instant.now() や new Date() を使わないようにする
  • (4) 非同期実行とビジネスロジックは分離する

(1) Production codeで静的アクセスを使わない

staticアクセスしちゃうと、Mockに差し替えたりできなくなるので、テストがしづらくなります。 なので、依存がなくstatic的に実装できそうな処理であっても、静的アクセスにはせず、依存があるクラスにDIして呼び出す形で実装するのが良い、という話です。

(2) ロジックが持つ制限（上限値やしきい値など）はパラメタとして外から設定可能にする

これも↑と同じっちゃ同じで、クラス内に何らかの制限値（例：上限値、下限値など）をハードコードで持たせちゃうと、 テストのときにも本当にその制限値に従った事前データ、あるいは入力が必要になってしまうので大変。 加えて、その値に変更が入った場合テストを修正しないといけなくなる。

本来テストでは、その制限値自体はどうでもよくて、その制限値に従った制御がうまく動作するのかを確認したいはず。 このため、テスト時に都合が良い制限値を設定できる作りにすべき、という話。 一度設定すると書き換えできないようにするのが理想なので、コンストラクタで値を設定できるようにしておけば良い。それだけ。

(3) Instant.now() や new Date() を使わないようにする

例えば、DBに値を保存するメソッド内で、更新日時に Instant.now() した値を設定して保存、ということはよくあると思います。 が、これだとDBに保存された際の更新日時がシステム日時に依存してしまうので、良くないです。

Javaには java.time.Clock というクラスが用意されており、このClockをインジェクションできる作りにすることで、 テスト時のみ固定の時刻が設定される状態を作れます。

テストしづらい実装と、テストしやすい実装例を以下に示します。

/**
 * テストしづらい...
 */
public class UnTestableDao {
  ...
  public User updateUser(User user) {
    user.setDateTime(LocalDateTime.now());
    updateDatabase(user);
    return user;
  }
  ...
}

/**
 * テストしやすい！
 */
public class TestableDao {
  private final Clock clock;
  public TestableDao(Clock clock) {
    this.clock = clock;
  }
  public User updateUser(User user) {
    user.setDateTime(LocalDateTime.now(this.clock));
    updateDatabase(user);
    return user;
  }
  ...
}

何が困るのか、何が嬉しいか、実際のテストコードを以下に示します。

  /**
   * テストしづらい...
   */
  @Test
  public void updateUserTest() {
    // Given
    UnTestableDao dao = new UnTestableDao();
    User user = createUserWithId(10L);

    // When
    User actualUser = dao.updateUser(user);

    // Then
    assertThat(actualUser.getDateTime(), is( ??? )); // 期待値がわからない
  }

  /**
   * テストしやすい
   */
  @Test
  public void updateUserTest() {
    // Given
    Instant fixedInstant = Instant.parse("2020-10-04T00:00:00.00Z");
    Clock fixedClock = Clock.fixed(fixedInstant, ZoneId.systemDefault());
    TestableDao dao = new TestableDao(fixedClock);
    User user = createUserWithId(10L);

    // When
    User actualUser = dao.updateUser(user);

    // Then
    assertThat(actualUser.getDateTime(), is( LocalDateTime.ofInstant(fixedInstant, ZoneId.systemDefault()) )); // 期待値が固定化される
  }

(4) 非同期実行とビジネスロジックは分離する

ここは同期的な部分と非同期的な部分を混ぜると、とたんにテストしづらくなるので、 同期、非同期を意識してロジックを分離しておいたほうが良い、という話。ここ具体例難しいなぁ。

JVMの設定、Junit5などの話

ざっくりまとめると

  • JVMのオプションに-noverify -XX:TieredStopAtLevel=1オプションつけるとちょっとテスト早くなる
  • assertTrue() and assertFalse() は避けたほうが良い
  • テストのグループ化
  • Mock Remote Service（HTTPのテスト時にhttpレイヤをMock化できる機能）

について説明されています。 ここはJava依存の話なので、Java好きの人は元の記事をご参照ください….!!!