この記事は「Rails初級者が一ヶ月研修を受けて得た学びをまとめた - Qiita」のうち分量が多くなりすぎたコード品質担保（LinterやRSpecなど）に関する項目を分割した記事です。
概要などについては親記事を参照してください。

code-checkとGithubAction

PR時にRSpectとともにLinter/Formatterが走るようにした
導入したものは以下
- rubocop
- erblint
- brakeman
- rspec

GithubActions

こちらを参照

rubocop

action-rubocopを使おう

指摘箇所があるとPRにコメントしてくれる
reviewdog/action-rubocop: Run rubocop with reviewdog 🐶
そして先輩の一人がメンテナをされているらしい、心強すぎる

pre-commitで走らせると良さげ

【Ruby on Rails】rubocop と pre-commit を利用して git commit 時にコーディングチェックを行う - きゃまなかのブログ

frozen_string_literal: true について

「Ruby3 では文字列がデフォルトで immutable になる」と書いたが、「Ruby3 では文字列をデフォルトで immutable にはしない」という決定がMatzによってなされた。
Ruby2.3で導入されたfrozen_string_literalマジックコメントでImmutable Stringを実現する - Hack Your Design!

application.rbに`require_relative 'boot'`をつける

Rails の初期化プロセス - Railsガイド

unique_index

model_validatesuniquenessをつけるカラムはunique indexをつける

 app/models/label.rb:4:3: C: Rails/UniqueValidationWithoutIndex: Uniqueness validation should have a unique index on the database column.
   validates :label_name, presence: true, uniqueness: true, length: { maximum: 10 }
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

参考：[Rails] uniquness バリデーションの警告 - Qiita

Style/OptionalArguments は入れる

オプショナル引数は省略できることに意味があるので、最後にないと意味がない（先輩のサンプルをそのまま引用。）
```
 # このメソッドは引数`a`, `b`両方を与えなければいけないので`a`をオプショナルにしている意味がない
 def bad(a = '', b)
 end

 # このメソッドには引数`b`を与えなくてもよい
 def good(a, b = '')
 end
```
オプショナル引数を最後に置くほうがメソッドの使い勝手が良いので従いたいルールですね。

Rails/HelperInstanceVariable 入れよう

これはhelperメソッドで@インスタンス変数を定義すると怒られるもの
可能な限りメソッドを局所化すべきであり、あちこちで同じ変数を参照できる状態が増えると、複雑性や可読負荷が高まりバグの原因になる

プログラムにおいては「状態を持つ」というのが複雑さや認知負荷（読みづらい・保守しづらい）を生む大きな要因です。なので極力状態を気にするモジュールを局所化するのが良いプラクティスとされています。
たとえばcontroller, view, helperすべてで同じ変数を参照していると、いつどこで変数が代入されるかによって影響を受ける箇所が多くなってしまいます。こうした事態を避けるためhelperではメソッドをただの関数となるようにしたく、こうしたルールが存在しています。

なにを有効化すべきか？

基本はデフォルトベースで調整する
Linterは問題点検出が目的なので可能な限り全て有効化する
formatterはプロジェクト内で統一したフォーマットにするために必要なものであるので状況による
- 基本的にはデフォルトベースで良い
- フォーマット（インデントなど…）は個人の好みであり、そこで議論することは開発の本質ではない
- 特にプロジェクトに途中から参加してformatterを有効にするタスクを任されたときはデフォルトのままで行くのが望ましい
- 自分の好みを入れるとそこで議論が起きてしまう

余談：rubocopのバグにぶち当たる

https://minne.com/items/2715511

Rubocopの導入中、突然動かなくなった。（何もしていないのに壊れました）
おま環かと思い慌てたがどうもバグっぽい
ビビりながらrubocopにissue立てたら、バグで爆速で直してもらえた。
Can't execute v1.34.0 with error "cannot load such file -- rubocop-rails" · Issue #10893 · rubocop/rubocop
教訓：慣れない環境構築中はversionが上がってないか注意しようね。問題が切り分けられなくなるぞ

Rspec

ファイル名はわかりやすく

task_controller_spec.rb とかにしておくとIDEでの検索時にmodelなのかcontrollerなのかわかりやすい

`.rspec`のオプション変更

--require rails_helper を記述することで、specファイルのrequire 'rails_helper'は書かなくてよくなる

describe/context/example/itをしっかり使い分けよう

RSpecの(describe/context/example/it)の使い分け - Qiita
Jestの経験があるのでなんとなくは掴んでいた。PJごとに文化などあったら確認

controllerSpecよりrequestSpecの方が推奨

今回テスト方針としてendpointが設定されているcontrollerメソッドのカバレッジを100％にすることが目標となっていた
RSpec - Controller or Request Specs?

requestSpecでチェックした項目

HTTPstatus, flash, bodyレンダリング結果

 expect(response).to have_http_status(200)
 expect(flash[:notice]).to be_present
 expect(response.body).to include('flashの内容')
 expect(response.body).to include('xxx')

redirectされる場合はredirect_toの宛先も確認する
記述順序は統一しよう

エンドポイントテストでカバーできない範囲は別メソッドに切り出して単体テスト、あるいはmodel側のテストでカバーする

切り出さないと検索クエリなどのテストがやりづらい

 def index
  @user = current_user
  @tasks = @user.tasks.includes(:labels)
  unless params[:reset]
    @tasks = @tasks.where({ status: Task.statuses[params[:status]] }.compact)
    if params[:search_word]
      @tasks = @tasks.where('task_name LIKE ? OR detail LIKE ?', params[:search_word],
                            params[:search_word])
    end
  end
  @tasks = @tasks.order("#{sort_column(params[:sort])} #{sort_direction}").page(params[:page]).per(20)
end

この状態だとindexにリクエストを送るrequestSpecでは、responseのHTMLをperseして期待した順番にアイテムが並んでいるかチェック…としなければいけず、良い状態とは言えない
別メソッドに切り出してファイルも分けて管理した方がテストはしやすい

使い回す値はletとfactory_botで生成

let(:valName) { command.. }で定義できる
factory_botはテストデータの定義をしてくれており、それを呼び出すだけで簡単に作成できる
letと組み合わせて使うと便利
factory_bot/GETTING_STARTED.md at master · thoughtbot/factory_bot ←読み込んでおこう

HTTPステータスだけでなく描画結果も確認しよう

expect(response).to have_http_status(200) だけチェックしていたが中身も見るべき

実行しないテストは`xit`、`xcontect`にしておく

空行のみや、コメントのみの状態で放置しない

controllerテストではテストデータ作成の記述をそこまで実際のものに寄せる必要はない

controllerテストの目的はrequestを行い、正しいレスポンスが帰ってくるかの確認
なので、letでのモデルインスタンス作成時の記述にこだわる必要はなく、可読性が担保されていればよい
テストとコントローラーでインスンタンスの初期化方法が同じであることのメリットはない

Factory_bot初期値は実際に入りそうな文字列にしておくとイメージしやすい

たとえば　task_name { 'testTask1' }ではなくtask_name { 'お米を買う' }のような感じ

expectの値はハードコードした方が良い場合がある

bodyレンダリング結果を次のように検証するばあい、可読性の悪さ、テスト自体が間違っている際に気づきにくいというデメリットがある
- bad: expect(response.body).to include(default_task1.task_name,...
- good: expect(response.body).to include('testTask1', 'testTask2', ...)
前職でも言われていたことだったが完全に忘れてやってしまっていた…

redirect先の検証は`follow_redirect!`で

redirectを返すエンドポイントテストではfollow_redirect!を使うとリダイレクト先のresponseまで確認できる

共通処理はshared_contextで共通化しよう

例えばログインしてない状態で任意のurlに飛ぶと、全てログインページにリダイレクトさせるような処理をチェックする場合,このようにcontroller_specの中でshared_contextを定義してあげて

  shared_context 'redirect_to_login-page' do
    it 'is redirect to /users/sign_in' do
      send_request

      expect(response).to have_http_status(302)
      expect(response).to redirect_to('/users/sign_in')
      follow_redirect!
      expect(response).to have_http_status(200)
      expect(response.body).to include('Log in', 'Email', 'Password')
    end
  end

任意の場所でit_behaves_likeで呼び出してやる

  context 'is redirect to /users/sign_in' do
      context 'request GET #index' do
          subject(:send_request) { get '/' }
          it_behaves_like 'redirect_to_login-page'
      end
  ...
  end

subjectと組み合わせて使うと良い

共通処理はsubjetで定義する

subject(:subject-name) { get 'xx', params: {...} } のように使う

expectとis_expectedの違いなどなど

RSpec で subject & expect する時の注意点 - Qiita

modelSpec

shouldaを入れる

validationなどのmodel定義のテストをワンライナーでかける。
以下のサンプルはshouldaを利用したもの
ref: shoulda-matchersが便利すぎる - Qiita
ref: File: README — Documentation by YARD 0.9.20

validationができているかのテストなどはしなくて良い

rails自体のテストになってしまうため
validationが適用されているかを確認したければ validate_presence_ofをmodeltestで行う

enum定義されたもの以外が渡ったときエラーになるかの検証はしなくていい

先輩のサンプル

 describe XXX do
   describe 'Model attributes definition' do
     let(:task) { build(:task) } # factory_bot

     it { is_expected.to validate_presence_of(:priority) }

     it {
       is_expected.to define_enum_for(:priority)
 	.with_values({
 	  '低': 0,
 	  '標準': 1,
 	  '高': 2
       })
     }
   end
 end

validatesやenumのテストは、正しく設定ができていることを確認すれば良いので、下記のようなテストを実装することが多いですね

test改行ルールも統一

これは好みだがルールは統一しておくべき。
改行はLinterがやってくれないので人間が気をつけなければいけない

今回はこんな感じにした

  RSpec.describe LabelsController, type: :request do
  let(:user) { create(:user) }
  ...

      describe 'XXXX' do
  	before do
  	...
  	end

  	context 'YYYY' do
  		it 'ZZZZZ' do
  		    ...
  		end

  		it 'ZZZZZ' do
  		    ...
  		end
  	end

  	context 'YYY2' do
  	    ...
  	end
      end

      describe 'XXX' do
      ...
      end
  end

ブロックの開始と終わりは一行開ける
letなどの初期化行との間も一行開ける
end同士は改行しない