特に後半は細かいプラクティスの話が多くなり、どうしても人によって「そうかな？」と思わされる内容も多そうだった。とはいえ大事な考え方も書かれているので一読の価値は間違いなくあると感じた。

repositoryのインターフェイスを用意するか

この本のテストでのDBの扱いに関しての主張は以下のようなものだった

複雑なビジネスロジックを持つドメインモデルとプロセス外依存を扱うクラスの連携を指揮する「コントローラ」に分類されるコードに対して統合テストを書くべき
統合テストはテストケースを同時実行できるようにしようとすると保守コストが大きくなってしまうため、単体テストと違いテストケースを隔離しなくともよく、実際のDBにアクセスしてもよい
テスト対象のアプリケーションを通してしかアクセスされないDBは「実装の詳細」だと考えられ、実装の詳細をモックしてしまうとテストは壊れやすくなりリファクタリングへの耐性を失うため、モックすべきでない
統合テストは本番環境のアプリケーションと同じ処理を実行させることが大事なので、インメモリな実装に置き換えたrepositoryを代わりに使うこともすべきではない
実装が1つしかなく、モックするわけでもないクラスはインターフェイスを用意すべきではない(YAGNI原則に違反するため)

特にインメモリでDBを模したrepositoryについては、私も「そのrepositoryの実装にミスがあったら破綻するのでは？」と比定寄りの考えを持っていた。となると、たしかにusecase層がinfrastructure層に依存しないよう依存性逆転を行うという目的こそあるが、それも形骸化してしまいいよいよrepositoryのインターフェイスを作成する意味がよくわからなくなってくる。ただ、私はそれでもusecase層からrepositoryを呼び出させることを意図しているのであればインターフェイスを用意すべきだし、そうでないなら用意しないべきではないかと思った。

生成方法が複雑なドメインオブジェクトはその生成過程をfactoryに隠蔽するが、infrastructure層にfactoryだけあっても使う側がそれに気付けないと意味がない。しかしDomain層でドメインモデルと同じパッケージにfactoryのインターフェイスが置いてあれば「これを使えばいいんだな」とわかる。 Scalaなら、repositoryをfactory以外から呼び出してほしくない場合はinfrastructure層のパッケージの外に公開しないこともできる。

このように、インターフェイスはusecase層に操作を公開するという役割もあり、テストにおいてモックすることが望ましくないとはいえ必要に応じて用意しておいた方がいいのではないかという気がした。

テストで本番と違う実装を使う

ここで思い出してほしいのが、テストでは本番環境とまったく同じ方法でテスト対象のコードとやり取りをしなくてはならない、ということです。つまり、テストだからと言って特別なことが許されるわけではないのです。

『単体テストの考え方/使い方プロジェクトの持続可能な成長を実現するための戦略』, Vladimir Khorikov, マイナビ出版, 2022-12-28, 須田智之訳, p.378

これは、テストのためにprivateメソッドを公開してしまうのは、実装の詳細とテストが結びついてしまい壊れやすいテストになってしまうためアンチパターンであるという文脈で書かれていたことだ。

privateメソッドの公開に限らずテストのために特別なことをやってしまわないよう気をつけたいが、必ずしもそういうわけには行かないのではないかと感じた。私は少し前にこのルールを破らなければならないことがあった。

つい最近、顧客の要望でakka-quartz-schedulerを使ってバッチ処理を実装する機会があった。その際のバッチ処理の内容は以下のようなものだった。

class SampleBatchProcessService(
  repository: CustomerInfoRepository
) {
  def process(customerId: Long): Unit =
    repository
      .find(customerId)
      .foreach { config =>
        // DBの変更を含む色々な処理
      }
}

class CustomerInfoRepository {
  def find(customerId: Long): Option[CustomerInfo]
}

case class CustomerInfo(foo: Long, bar: String)

このSampleBatchProcessServiceがActorから呼び出されていた。ここで、顧客から「バッチ処理をしばらく止めたくなることがあるかもしれない」と言われたので、バッチ処理が有効になっているかをTINYINTで持つbatch_process_enabledというカラムをCustomerInfoを構成するためのテーブルの一部に追加し、それが1でない場合はCustomerInfoRepository.find()で見つからないようにすることでバッチ処理を止められるようにした。

このバッチ処理だが、結構な高頻度で実行されており、バッチ処理が実行されている間にbatch_process_enabledの値を変更しなければならないことも十分考えられた。その場合、途中で処理を止めてしまうとDBの中身がおかしな状態になってしまう可能性が無いとは言えないため、既に走っているバッチ処理については最後まで通常どおり実行し、次回以降の実行はbatch_process_enabledが1に戻るまで行わないという動作になっていてほしい。

これは統合テストで見ておくシナリオだと思ったのだが、テストを書くにあたって1つ問題があった。実際の手順を再現するのであればバッチ処理を行うActorにメッセージを送った直後にbatch_process_enabledの値を0にし、Actorから完了を伝えるメッセージが返ってきたらDBがバッチ処理を行なったあとの状態として正しい内容になっているかを確認すればよいのだが、テストコードではActorの処理がすぐ終わってしまい、なおかつバッチ処理の実行中にbatch_process_enabledが変わろうが変わるまいが期待するバッチ処理実行後のDBの状態は変わらないため、本当に

Actorがメッセージを受け取り、SampleBatchProcessServiceが呼び出され、CustomerInfoRepositoryからCustomerInfoが見つかる
batch_process_enabledが0に変更される
SampleBatchProcessServiceの処理が最後まで実行される

という順番で処理されているかわからなかった。なので、以下のようにspyを使ってCustomerInfoRepositoryのふるまいを変更して注入した。

val spyRepository = spy(new CustomerInfoRepository())
doAnswer(new Answer[AnyRef] {
  override def answer(invocation: InvocationOnMock): AnyRef = {
    val info = invocation.callRealMethod()

    // batch_process_enabledを0にするコード

    info
  }
}).when(spyRepository).find(any)

一応invocation.callRealMethod()で実際の実装が使われるが、完全に同じ実装というわけには行かなかった。そもそもこの本の主張からすれば、テスト対象のプロジェクト以外からアクセスされることが全くないDBは管理下にある依存なので、モックすること自体が望ましくない。

ただ、一方で「バッチ処理の実行中にバッチ処理が行われないよう切り替える」というのはいかにもそのうち1回は行われそうな操作であり、しかもそんなことが行われる場合は既に何か他の問題が起きていることも考えられる。そのような状況で更にデータを壊すようなバグを引き起こしてしまうというのはなるべく避けたく、ぜひとも統合テストでカバーしておきたいシナリオの1つだった。

繰り返しになるが、たしかに統合テストはビジネス上のシナリオで想定される操作をテストでも再現する必要がある。ただし、このようなビジネス上必ず検証しておく必要があるシナリオがあるが検証が難しい場合は、なるべく実際の操作から離れないようにしつつもある程度セオリーを破る必要もあるのではないかという気がした。

統合テストとpackage privateどちらをとるか

第4章で見たように、退行に対する保護はテスト中に実行されるコードの量によって変わります。そのため、管理下にない依存とのコミュニケーションの流れの中で、モックに置き換えられるコンポーネントをアプリケーションの境界に近いものにすれば、テストを実施する際に経由されるクラスの数が増え、より強力な退行に対する保護を得られるようになります。

『単体テストの考え方/使い方プロジェクトの持続可能な成長を実現するための戦略』, Vladimir Khorikov, マイナビ出版, 2022-12-28, 須田智之訳, p.313-314

とても大事な、原則と言ってもよさそうなルールだと感じたのだが、このルールに従うと1つだけ残念なことが起きた。

そもそもこの話が出てきたのは以下のようなクラスがあった場合どちらをモックすべきかという文脈だった。

class SlackWebApiClient(client: WSClient) {
  val WebApiBaseUri = new URI("https://slack.com/api")

  def postMessage(
    authorizationToken: String,
    channelName: String,
    message: String,
  )(implicit ec: ExecutionContext): Future[Unit] =
    client
      .url(WebApiBaseUri.resolve("chat.postMessage").toString)
      .withHttpHeaders(
        "Authorization" -> s"Bearer $authorizationToken",
        "Content-Type" -> "application/json",
      )
      .post(
        Json.obj(
          "channel" -> channelName,
          "text" -> message,
        )
      )
      .map { response =>
        // SlackWebAPIはレスポンスのJSONのokプロパティで成否を判定する
        Try((response.json \ "ok").as[Boolean]) match {
          case Success(true) =>
            ()
          case Success(false) =>
            throw new Exception("SlackWebAPIの実行に失敗しました。")
          case Failure(_) =>
            throw new Exception(
              "SlackWebAPIから想定していない形式のレスポンスを受け取りました。"
            )
        }
      }
}

case class BatchResult(taskName: String, customerName: String)

/**
 * 顧客に関する何らかのバッチ処理の結果を顧客のSlackチャンネルに
 * 通知する用途に特化したSlackWebApiClientのラッパー
 */
class BatchResultNotifier(client: SlackWebApiClient) {
  val AuthorizationToken = "foo"
  val Channel = "notification"

  def notifyComplete(result: BatchResult): Future[Unit] =
    client.postMessage(
      AuthorizationToken,
      Channel,
      s"${result.customerName} 様: ${result.taskName}が完了しました。"
    )
}

エラーハンドリングがガバガバだったり、認証用トークンやら通知先のチャンネル名やらがハードコーディングしてあるのはサンプルコードなので見逃してほしい。この時、BatchResultNotifierを使うコードの統合テストではBatchResultNotifierをモックするのではなく、最終的に顧客のSlackに投稿を行うSlackWebApiClientをモックすべきだというのが本書の主張だ。

なお、実際に最終的にSlackへの投稿を行なっているのはWSClientじゃないかと思うかもしれないが、本書ではサードパーティ製のライブラリについては直接モックするのではなくアダプタとなるクラスを作成し、それをモックすることを推奨している。実際、WSClientはBuilderパターンを採用しており、url()やwithHttpHeaders()などのメソッドが全て新しいWSClientのインスタンスを返すようになっているため、モックしようとすると面倒なことこの上ない。

たしかに、SlackWebApiClientのメソッドに渡される引数を確認しておけば、SlackWebApiClientに変更が入りでもしない限りは顧客に最終的に送られるメッセージの内容が変わってしまう可能性は低く、強力な対抗への保護が手に入る。

ただ、気にする人もそこまでいないのかもしれないがこのルールはScalaの機能と少し折り合いが悪い点がある。 Scalaにはprivate[foo] class Bar {}というように書くことでfooパッケージの中でのみBarを公開するということができる。私はこのようなアクセス制御はコード補完のサジェスト汚染を回避する意味合いでもなるべくちゃんとやっておきたいと考えている。上のコードで言えば、SlackWebApiClientを全体に公開してしまうと、認証用のトークンをどう取得するかといったビジネス上あまり重要でない処理がinfrastructure層の外で書かれてしまう可能性がある。なので、当然SlackWebApiClientはinfrastructure層に該当するパッケージの中でのみ公開し、外部からはBatchResultNotifierのような用途ごとのラッパーを作らせてそちらを使わせたかった。

すると何が起こるか。統合テストが書かれるパッケージからはSlackWebApiClientが参照できなくなるのでモックできなくなるのである。 SlackWebApiClientのインターフェイスを用意しそちらを公開すればよいのではないかと思うかもしれないが、PlayFrameworkを使った開発でGuiceを使っている場合はインターフェイスさえあれば実質SlackWebApiClientの機能が呼び出せてしまうので何の解決にもならない。結局、品質の高いテストを書くことを優先し、SlackWebApiClientはpublicにしたが、何かうまいやり方は無いかとモヤモヤしてしまった。

感想

「単体テストの考え方/使い方」というタイトルに反し、優れた統合テストを書くための指針についても書いてくれているありがたいパートだった。特にモックについてはこれまで「テストが速く終わる方がいいんだ！」と無闇やたらにRepositoryをモックにしていたと思えば、でも今のRepositoryの設計がイケていないのでいつか作り直す時のためにモックではなくDBを使いたいと考え始めたり、使い方に明確な指針を持てていなかったのでとても参考になった。

10章は、DBを使った統合テストを行うためにはスキーマをVCSで管理できるようにしておくだとか、開発者ごとに個別のDBインスタンスを用意するだとか、マイグレーションツールを使っているのであれば自ずと達成されているであろう内容も多かったが、むしろRDB以外を使った統合テストを書かなければならない時にこういったことを考える必要があるのかもしれないと思った。

p-kino’s blog

「単体テストの考え方/使い方」第3部, 4部を読んで考えたこと

repositoryのインターフェイスを用意するか

テストで本番と違う実装を使う

統合テストとpackage privateどちらをとるか

感想