Rails view helperとテストのリファクタリング：モックをcode smellと感じること

はじめに

自称、Ruby on Rails フロントエンドエンジニアのnaofumiです。X @naofumiでは色々勝手なことを書いていますが、最近Qiitaも頑張り始めました。

先日のview helperテストのリファクタリング

先日、Railsのviewテストを書こうって記事を書いたが、view helperおよびそのテストのコードがあまりイケてなかった。

そこで今日は、view helperテストのリファクタリングを一緒に考えていきたいと思う。

対象のコード

作ろうとしているordering_arrow helperメソッドは、下記のようなテーブルの並べ替えに使う三角ボタンの箇所。

これを汎用的に使えるコンポーネントにしたいので、helperメソッドに切り出している。下記のようにviewの中で使う

<%= ordering_arrow('started_at_order', :asc) do %>
  <span>▲</span>
<% end %>

Helperのコードの課題

まずはHelperのコード。問題箇所は

1. (params[param_name] == direction.to_s) || (params[param_name].blank? && default)がわかりにくい
2. ==とか||とか&&とか三項演算子とかがあって、見かけ以上に条件分岐が多い。これはわかりにくい

# ヘルパー

  def ordering_arrow(param_name, direction, default: false, &block)
    selected = (params[param_name] == direction.to_s) || (params[param_name].blank? && default)

    html_options = selected ? {} : { style: 'color: #CCC;' }
    link_to({ param_name.to_sym => direction }, html_options, &block)
  end

テストコードの問題

後述するようにHelperのコードの問題に起因するのだが、パッとみてこのテストコードには下記の問題がある。

1. Railsのviewにあるparamsメソッドをモックしなければならなくなっている
2. Railsのlink_toヘルパーメソッドをモックして、どのような引数で呼び出されたかを確認することでassertionをしている

わかりにくい以前の問題として、モックをいくつも用意しないといけないこと自体がcode smell (コードの臭い）と考えて良い。基本的には避けた方が良い。

でもモックを用意しなければならないのは、元のview helperのコードがよくないからであって、テスト手法の技巧で解決できない。そこでview helperとテストをセットでリファクタリングしたいと思う。

# テスト

RSpec.describe OrderingHelper do
  describe '#ordering_arrow' do
    before { allow(view).to receive(:link_to) }

    it 'params[:order] = nil, default true should not be grayed' do
      allow(params).to receive(:[]).with('order').and_return(nil)

      helper.ordering_arrow('order', :desc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :desc }, {})
    end

    it 'ordering_arrow :asc, params[:order] = asc should not be grayed' do
      allow(params).to receive(:[]).with('order').and_return('asc')

      helper.ordering_arrow('order', :asc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :asc }, {})
    end

    it 'ordering_arrow :desc, params[:order] = asc should be grayed' do
      allow(params).to receive(:[]).with('order').and_return('asc')

      helper.ordering_arrow('order', :desc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :desc }, { style: 'color: #CCC;' })
    end
  end
end

ロジックとプレゼンテーションを分離する

もう一度、view helperのコードを示す。コードの問題点をコメントで記載している。

# ヘルパー

  def ordering_arrow(param_name, direction, default: false, &block)
    # 条件文が複雑すぎる。
    selected = (params[param_name] == direction.to_s) || (params[param_name].blank? && default)

    html_options = selected ? {} : { style: 'color: #CCC;' }
    # 複雑な条件判断とHTMLの出力を同じ関数の中に書いてしまっている
    link_to({ param_name.to_sym => direction }, html_options, &block)
  end

view helperをリファクタリングした結果

下記は複雑な条件分岐とHTMLの出力を分けるようにview helperをリファクタリングしたものである。

1. 複雑な条件分岐のロジックはordering_arrow_selected?に集約した
2. ordering_arrow helperメソッドの中にあるのは１つのif-else文だけで、これだったらテストを書くまでもなく、パッと見で何をやっているかがわかる
3. なおordering_arrow_selected?はprivateとしているが、Moduleをincludeするときにprivate宣言するのはほぼ意味がない。今回はコメント代わりにprivateと書くことで、「ordering_arrow_selected?は外から使わないでね！」って警告している。(そもそもRubyのprivateはドキュメンテーションの代わり程度だという話もある）

  def ordering_arrow(param_name, direction, default: false, &block)
    html_options = if ordering_arrow_selected?(params[param_name], direction, default)
                     {}
                   else
                     { style: 'color: #CCC;' }
                   end

    link_to({ param_name.to_sym => direction }, html_options, &block)
  end

  private

  def ordering_arrow_selected?(param_value, direction, default)
    (param_value == direction.to_s) || (param_value.blank? && default)
  end

テストの書き方にどういうインパクトがあるか？

上記のように view helperをリファクタリングすると、複雑な条件分岐をモックなしでテストできるようになる。

テストのリファクタリング

view helperをリファクタリングしたおかげで、下記のようなテストが書けるようになった。

1. #ordering_arrow_selected?はモックを全く必要としないので、非常に簡単にテストが書ける
2. 結果として、より網羅的なテストを書く意欲も湧いた（リファクタリングついでにテストを追加した）
3. #ordering_arrowの中のロジックは随分と減ったので、本当はテストもいらないぐらいになった。一応、今回は１つだけ残した

なお条件分岐のすべてのケースを網羅しようと思うと面倒になるので、「ここまで書いておけば十分だろう」って留めていることもわかっていただけるかと思う。

テストの目的はあくまでも正しく動くコードを書くためなので、そこにさえ到達できれば、100%のテストカバレッジを目指すのは割と不毛だと私は思っているし、一般にそう言われている。

RSpec.describe OrderingHelper do
  describe "#ordering_arrow_selected?" do
    it 'should return true if params_value is nil and default is true' do
      # privateなメソッドを呼び出すのは`send`を使うだけで良い
      expect(helper.send(:ordering_arrow_selected?, nil, "asc", true)).to be_truthy
    end

    it 'should return false if params_value is nil and default is false' do
      expect(helper.send(:ordering_arrow_selected?, nil, "asc", false)).to be_falsey
    end

    it 'should return true if params_value matches direction and default is false' do
      expect(helper.send(:ordering_arrow_selected?, "asc", "asc", false)).to be_truthy
    end

    it 'should return false if params_value does not match direction and default is true' do
      expect(helper.send(:ordering_arrow_selected?, "desc", "asc", true)).to be_falsey
    end
  end

  describe '#ordering_arrow' do
    before { allow(view).to receive(:link_to) }

    it 'ordering_arrow :desc, params[:order] = asc should be grayed' do
      allow(params).to receive(:[]).with('order').and_return('asc')

      helper.ordering_arrow('order', :desc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :desc }, { style: 'color: #CCC;' })
    end
  end
end

結論

今回は下記のことを実施した

1. テストコードにモックが２つも入っているのを見て、「これはcode smellだ」って判断した。要は、テストの書きにくさをきっかけに、リファクタリングの必要性を痛感した形になっている
2. モックが２つも必要だったのは、テスト対象のview helperメソッドの中で、複雑な条件分岐とHTML出力が同居しているためと考え、そこを分離した
3. テストの方では、複雑な条件分岐とHTML出力を別々にテストした。おかげで特に条件分岐のテストがわかりやすくなった

このように、テストが書きにくいのは、しばしば元のコードに原因がある。テストを先に書いたわけではないけど、テスト駆動開発(設計)の思想に則り、テストしやすさを指標としてコードを改善できた。

viewをテスト駆動開発でやるのはなかなか難しく、私もテストを先に書くことは基本的にしない。でもcode smellに敏感になることで、設計の改善というメリットは結果として享受できると考えている。

皆さんもテストを書くのがしんどいと感じることは多いと思う。その時、設計をどう改善するとテストが書きやすくなるかを考えて、実際にそのようにリファクタリングすることをお勧めする！

今回はこれまで！

また何か取り上げて欲しい話題があれば、コメントに残すか、Xでお知らせください！