概要
原著者の許諾を得て翻訳・公開いたします。
- 英語記事: RubyOnRails testing antipatterns — part 1/2
- 原文公開日: 2018/01/03
- 著者: Błażej Kosmowski
訳文見出しには番号とアンカーを追加しました。
Rails: テスティングアンチパターン --前編（翻訳）
テストスイートを準備していると、つい誘惑に負けて近道を選んでしまい、テストの可読性や理解しやすさはもちろん、実装を先に進めるための柔軟性までがっくり下がってしまうことがあったりします。本記事では、よくある手法の中から、テストスイートを健全に保つうえで避けるべきものをリストアップしてみたいと思います。本文中の例の多くは、問題に焦点を当てるためにかなり簡素化してありますのでご了承ください。
アンチパターン01: privateメソッドをテストする
privateメソッドのテストがよくないのは、privateメソッドを自然に使う状況は実際には起きないはずだからです。代わりにpublicインターフェイスをテストすべきです。privateメソッドのテストが書かれているということは、実装の後でテストを書いた（これ自体アンチパターンです）か、publicインターフェイスがどこかの時点でprivateインターフェイスに変わってしまった可能性があります。privateメソッドをテストしてしまうと、実装とテストの結合が密になってしまい、コードがリファクタリングを嫌がるようになってしまいます。
privateメソッドのテストに直接対抗するには、それらを実際に使うpublicインターフェイスの方を常にテストすべきです（つまりprivateメソッドのテストは間接的に行われます）。simplecovなどのテストカバレッジツールを使うと、テストされていない（つまり今後使われない可能性のある）privateメソッドを手軽に検出できます。TDD（テスト駆動開発）も、publicインターフェイスのみテストを無理なく推進する手法のひとつです。
（Gist）
# 残念なspec
# ==========
describe CloseOrder do
# テスト対象オブジェクトのdouble（身代わり）を半端に使って
# #callをテストすると不自然になり、コードが重複している感じがする
describe '#call' do
it do
order = create(:order)
service = CloseOrder.new(order)
allow(service).to receive(:send_notification)
service.call
expect(service).to have_received(:send_notification)
end
end
describe '#send_notification' do
it '注文のクローズを顧客に通知する' do
order = create(:order, customer_email: 'tony@stark.com')
service = CloseOrder.new(order)
expect {
service.send(:send_notification) # We are forced to use #send to test private method
}.to change { ActionMailer::Base.deliveries.count }.by(1)
notification = ActionMailer::Base.deliveries.last
expect(notification).to have_attributes(subject: 'Order closed!', recipients: ['tony@stark.com'])
end
end
end
# よいspec
# ==========
describe CloseOrder do
describe '#call' do
it '注文のクローズを顧客に通知する' do
order = create(:order, customer_email: 'tony@stark.com')
service = CloseOrder.new(order)
expect {
service.call
}.to change { ActionMailer::Base.deliveries.count }.by(1)
notification = ActionMailer::Base.deliveries.last
expect(notification).to have_attributes(subject: 'Order closed!', recipients: ['tony@stark.com'])
end
end
end
# 実装側
# ==============
class CloseOrder
def initialize(order)
@order = order
end
def call
send_notification
end
private
def send_notification
OrderMailer.order_closed_notification(@order).deliver_now
end
end
アンチパターン02: フレームワークのメソッドをスタブ化する
フレームワークのメソッドをスタブ化すると、多くの場合実装の自由がほとんど失われてしまうので、よくありません。簡単のため、以下のスニペットで考えてみましょう。
allow(User).to receive(:where).with(name: "Tony") {
[instance_double(User, name: "Tony")]
}
フレームワーク（ここではActiveRecordのクエリインターフェイス）をスタブ化すると、要件を実装する方法が1種類に縛られてしまい、リファクタリングの余地がほぼなくなってしまいます。
フレームワークの機能をスタブ化するのではなく、ファクトリーなどの手法を用いて必要なデータをフレームワークに与えるだけにとどめましょう。以下の例では、シンプルなcreate(:user, name: "Tony")で事足ります（Gist）。
# 残念なspec
# ==========
describe PurgeDomainUsers do
describe '.call' do
# このテストでは実装の自由が完璧に失われてしまう
it '指定のドメインに一致するメールを持つユーザーをすべて削除する' do
users = instance_double(:users_collection)
allow(User).to receive(:where) { users }
allow(users).to receive(:destroy_all)
PurgeDomainUsers.call(domain: 'stark.com')
expect(User).to have_received(:where).with('email ILIKE :domain_query', domain_query: "%@stark.com"))
expect(users).to have_received(:destroy_all)
end
end
end
# よいspec
# ==========
describe PurgeDomainUsers do
describe '.call' do
it '指定のドメインに一致するメールを持つユーザーをすべて削除する' do
tony = create(:user, email: 'tony@stark.dev')
create(:user, email: 'bruce@wayne.dev')
howard = create(:user, email: 'howard@stark.dev')
create(:user, email: 'peter@parker.dev')
expect {
PurgeDomainUsers.call(domain: 'stark.dev')
}.to change { User.count }.from(4).to(2)
expect { tony.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { howard.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
# 実装側
# ==============
class PurgeDomainUsers
def self.call(domain:)
User.
where('email ILIKE :domain_query', domain_query: "%@#{domain}").
destroy_all
end
end
アンチパターン03: ライブラリメソッドをスタブ化する
ライブラリメソッドのスタブ化は、フレームワークメソッドのスタブ化とよく似ていますが、こちらはまた別の問題を持ち込みます。ライブラリの進化はフレームワークよりも速く、頻繁に変更される傾向があります。また、ライブラリが何らかの理由で別のライブラリに差し替えられることもありえます。ライブラリのpublicインターフェイスのスタブ化に頼ってしまうと、ライブラリが進化したり差し替えられたりするたびにテストをごっそり更新しなければならなくなるかもしれません。
この問題を軽減するよい方法のひとつとして、ライブラリのラッパー（facade）を作成し、そのライブラリによって導入されるインターフェイスだけをスタブ化する方法が考えられます。後はテスト内でライブラリメソッドのラッパーをスタブ化すればよいでしょう。ただしたいていの場合、実際のライブラリが依存する対象（HTTPトラフィックなど）をモック化する方がずっとよい結果になります。
（Gist）
# 残念なspec
# ==========
# ライブラリの使い方と、対象のクラスに期待する振る舞いの2つがテストの対象に混在している
describe ImportInvestigations do
describe '.call' do
it '取調べ結果をデータベースにインポートする' do
stub_const('ImportInvestigations::IMPORT_URL', 'http://gcpd.dev/investigations.json')
investigations_json = [
{ uid: '123', name: 'Joker: 脱獄' },
{ uid: 'abc', name: 'Dr. Strange: 放火' }
].to_json
faraday_response = instance_double(Faraday::Response, body: investigations_json)
allow(Faraday).to receive(:get) { faraday_response }
expect {
ImportInvestigations.call
}.to change { Investigation.count }.from(0).to(2)
expect(Investigation.all).to include(
have_attributes(uid: '123', suspect_name: 'Joker', case_subject: '脱獄'),
have_attributes(uid: 'abc', suspect_name: 'Dr. Strange', case_subject: '放火')
)
expect(Faraday).to have_received(:get).with('http://gcpd.dev/investigations.json')
end
end
end
# 改善されたspec
# ============
# 使われている特定のライブラリにテストが依存していない
describe ImportInvestigations do
describe '.call' do
it '取調べ結果をデータベースにインポートする' do
stub_const('ImportInvestigations::IMPORT_URL', 'http://gcpd.dev/investigations.json')
investigations_json = [
{ uid: '123', name: 'Joker: 脱獄' },
{ uid: 'abc', name: 'Dr. Strange: 放火' }
].to_json
stub_request(:get, "http://gcpd.dev/investigations.json").to_return(status: 200, body: investigations_json)
expect {
ImportInvestigations.call
}.to change { Investigation.count }.from(0).to(2)
expect(Investigation.all).to include(
have_attributes(uid: '123', suspect_name: 'Joker', case_subject: 'prison escape'),
have_attributes(uid: 'abc', suspect_name: 'Dr. Strange', case_subject: 'arsony')
)
end
end
end
# 最善のspec
# ==========
# レコードの取り出しをfacadeで抽象化している（facadeは独自のテストセットを得る）
describe ImportInvestigations do
describe '.call' do
it '取調べ結果をデータベースにインポートする' do
allow(GcpdWrapper).to receive(:get_investigations) {
[
double(uid: '123', suspect_name: 'Joker', case_subject: '脱獄'),
double(uid: 'abc', suspect_name: 'Dr. Strange', case_subject: '放火')
]
}
expect {
ImportInvestigations.call
}.to change { Investigation.count }.from(0).to(2)
expect(Investigation.all).to include(
have_attributes(uid: '123', suspect_name: 'Joker', case_subject: '脱獄'),
have_attributes(uid: 'abc', suspect_name: 'Dr. Strange', case_subject: '放火')
)
end
end
end
アンチパターン04: 重要性の低いテスト
テストにはいくつかの種類がありますが、中には実施する価値がほとんどない、実装の細かな部分をねちねちチェックするだけのテストもあります。この種のテストはとっとと削除して、細かい部分は別のテストで間接的にチェックすべきです。属性が存在するかどうかのテストや、単純なマイグレーションやリレーションシップのテストは、実行する価値がほとんどなく、テストスイートの行数を無駄に増やすspecの好例です。
アンチパターン05: テストのための実装
「テストのための実装」とは、specをパスさせるためだけに存在する、機能やビジネスロジックにまったく貢献しないコード片を指します。if Rails.env.test?のようなひどすぎる書き方をコードベースで見かけたら、常にこのアンチパターンを疑うべきです。test環境のみという条件付きで実行されるコードがあると、無駄に複雑になるばかりでなく、テストの信頼性そのものが疑われてしまいます。したがって、プロジェクトの設定や初期化に関連するテストの実装やコードを除いて、このような条件文は許容すべきではありません。
アンチパターン06: 設計を損なうテスト
設計を損なうテストは、前項と少し似た面のある問題です。手っ取り早く言うと、テストの独立性を高めたり、テストファーストをやりやすくしたり、テスト実行を高速化したりすることに頑張りすぎた結果、コードやアーキテクチャが読みにくく、わかりにくくなってしまうことです。これについては、このトピックを詳しく追求したDHHの良記事をご覧ください。