rubocop の警告が煩わしいと思うことがありませんか?
面倒だから警告を diable にして、警告を消してOKにしてませんか?
逆に深く考えずに警告を修正して、警告が消えたから、それでOKなんてことしてませんか?
ちょっと待ってください。警告は自分のスキルアップにつながるチャンスかも知れません。警告にどんな意味があるか、どう修正すべきか調べることで、自分の知識を深めることにつながるかも知れません。今日は、そんな自分の経験をいくつかご紹介したいと思います。
なお、ここでは、 rubocop と rubocop-rspec gem を使っています。
以下のコードと spec を例にして説明します。
# frozen-string-literal: true
require 'open-uri'
# Web ページのクラス
class Web
def initialize(url)
@url = url
end
def read
URI.open(@url, &:read)
end
end
# Web ページをダウンロードするクラス
class WebDownloadService
def run(web, io = $stdout)
io.write web.read
end
end
# frozen_string_literal: true
require_relative '../web_download'
describe WebDownloadService do
describe '#run' do
let(:io) { StringIO.new }
let(:service) { described_class.new }
it 'run calls the web#read method' do
web = double(Web)
expect(web).to receive(:read)
service.run(web, io)
end
end
end
Security/Open
まずは、 Web
クラスの rubocop の警告から見ていきましょう。
class Web
...
def read
URI.open(@url, &:read)
end
end
rubocop で以下のような警告が出ます。
web_download.rb:12:9: C: Security/Open: The use of URI.open is a serious
security risk.
URI.open(@url, &:read)
^^^^
rubocop Security/Open
で検索してみると https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Security/Open が見つかります。
ページには次のように書かれています。
Kernel#open
andURI.open
enable not only file access but also process invocation by prefixing a pipe symbol (e.g.,open(“| ls”)
). So, it may lead to a serious security risk by using variable input to the argument ofKernel#open
andURI.open
. It would be better to useFile.open
,IO.popen
orURI.parse#open
explicitly.
URI.open
ではなく、 URI.parse#open
を使えということのようです。この理由をもう少し詳しく調べてみましょう。
例えば、Web クラスを使うときに |
を使って
url = '| rm very_important_file'
Web.new(url).read
とすると、 rm
コマンドが実行できてしまい、 very_important_file
が削除できてしまいます。URI.open
にはこういったリスクがあるということですね。
では、説明にあるように URI.parse
を使うように書き換えます(ここでは、 open
を使わずに書き換えることができます)。
class Web
...
def read
URI.parse(@url).read
end
end
今度は、
url = '| rm very_important_file'
Web.new(url).read
としても、以下の例外が発生します。 very_important_file
は削除されません。
bad URI(is not URI?): "| rm very_important_file" (URI::InvalidURIError)
URI.open
より、 URI.parse#open
を使った方が良いことが、理由も含めて理解できました。
RSpec/VerifiedDoubles
次に WebDownloadService
クラスと、その spec を例にとります。
# Web ページをダウンロードするクラス
class WebDownloadService
def run(web, io = $stdout)
io.write web.read
end
end
spec は、 WebDownloadService#run
メソッドが Web#read
を呼び出していることをテストしています。
describe WebDownloadService do
describe '#run' do
let(:io) { StringIO.new }
let(:service) { described_class.new }
it 'run calls the web#read method' do
web = double(Web)
expect(web).to receive(:read)
service.run(web, io)
end
end
end
rubocop で spec/web_download_service_spec.rb
をチェックすると次の警告が出ます。
spec/web_download_service_spec.rb:10:13: C: RSpec/VerifiedDoubles: Prefer
using verifying doubles over normal doubles.
web = double(Web)
^^^^^^^^^^^
RSpec/VerifiedDoubles rubocop
で検索すると https://www.rubydoc.info/gems/rubocop-rspec/1.3.1/RuboCop/Cop/RSpec/VerifiedDoubles が見つかります。
double
よりも、 instance_double
の方を使えということのようですが、どうしてでしょうか?
https://relishapp.com/rspec/rspec-mocks/docs/verifying-doubles を読めばわかります。
instance_double
は、引数で指定したクラスのメソッドが実際に存在するかどうかをチェックしてくれますが、 double
はチェックしてくれません。
例えば、 Web#read
を Web#body
にメソッド名を変更したとしましょう。
class Web
...
# read -> body にメソッド名を変更
def body
URI.open(@url, &:read)
end
end
class WebDownloadService
def run(web, io = $stdout)
io.write web.read # 存在しない read メソッドを呼び出している
end
end
このとき、実際にコードを実行すると WebDownloeadService#run
メソッドは、存在しない Web#read
を呼び出すため、エラーになります。
ところが、specを実行しても、エラーにはならないのです
spec は、Web#read
を呼び出すことをテストしますが、 Web#read
が実際に存在するかチェックしないからです。
it 'run calls the web#read method' do
web = double(Web)
# Web#read を呼ぶかどうか確認する。 Web#readが存在することをチェックをしない。
expect(web).to receive(:read)
service.run(web, io)
end
double
を instance_double
に変更してみます。
it 'run calls the web#read method' do
web = instance_double(Web)
expect(web).to receive(:read)
service.run(web, io)
end
instance_double
を使うと、 spec は、呼び出すメソッドが実際に存在することをチェックします。
spec を実行すると、 Web#read
が存在しないため spec が失敗します。
実際にコードを動かさなくても、spec を実行しただけで、コードに何か問題がありそうだとわかります。
$ bundle exec rspec
F
Failures:
1) WebDownloadService#run run calls the web#read method
Failure/Error: expect(web).to receive(:read)
the Web class does not implement the instance method: read
# ./spec/web_download_service_spec.rb:11:in `block (3 levels) in <top (required)>'
double
よりも instance_double
が推奨される理由がわかりました。
RSpec/MessageSpies
spec には、 rubocop の警告がまだあります。
spec/web_download_service_spec.rb:11:22: C: RSpec/MessageSpies:
Prefer have_received for setting message expectations.
Setup web as a spy using allow or instance_spy.
expect(web).to receive(:read)
^^^^^^^
have_received
に変更してみます。
it 'run calls the web#read method' do
web = instance_double(Web)
expect(web).to have_received(:read)
service.run(web, io)
end
rubocop の警告は消えますが、今度は spec がエラーになります。
$ bundle exec rspec
F
Failures:
1) WebDownloadService#run run calls the web#read method
Failure/Error: expect(web).to have_received(:read)
#<InstanceDouble(Web) (anonymous)> expected to have received read, but that object is not a spy or method has not been stubbed.
# ./spec/web_download_service_spec.rb:11:in `block (3 levels) in <top (required)>'
では、 allow
を使ってみましょう。
it 'run calls the web#read method' do
web = instance_double(Web)
allow(web).to have_received(:read)
service.run(web, io)
end
これも rubocop の警告は消えますが、 spec はエラーになります。
$ bundle exec rspec
F
Failures:
1) WebDownloadService#run run calls the web#read method
Failure/Error: allow(web).to have_received(:read)
Using allow(...) with the `have_received` matcher is not supported as it would have no effect.
# ./spec/web_download_service_spec.rb:11:in `block (3 levels) in <top (required)>'
では、 have_received
を receive
に変えてみましょう。
it 'run calls the web#read method' do
web = instance_double(Web)
allow(web).to receive(:read)
service.run(web, io)
end
spec も通るし、rubocop の警告も消えました。これですべてクリア...ではないんですねー。
このテストだと、 次のように Web#read を呼ばないコードに変更しても spec が通ってしまいます。
class WebDownloadService
def run(web, io = $stdout)
io.write web # Web#read を呼ばないが、specは通る。
end
end
allow
は実際に Web#read が呼ばれたことを確認しないからです。
次のように書くのが正解です。
it 'run calls the web#read method' do
web = instance_double(Web)
allow(web).to receive(:read)
service.run(web, io)
expect(web).to have_received(:read)
end
service.run(web, io)
を実行した後で、 expect(web).to have_received(:read)
で、実際に Web#read
が呼ばれたことを確認するのが正解です。
修正前のコードに対して、 rubocop が警告を出す理由を少し考えてみます。
修正前のコードと修正後のコードを比較してみましょう。
修正前の spec は、Web#read
が呼ばれることをテストするよ、と先に書いて、 service.run(web, io)
を実行するよ(すると Web#read
が呼ばれるはず)、と後で書いてます。
メソッドの呼び出し (service.run(web, io)
) が後、メソッドを呼び出した結果どうなるかの確認 (Web#read
が呼ばれること) が先に登場するのです。
...
# Web#read が呼ばれることをテストするよ
expect(web).to receive(:read)
...
# WebDownloadService#run を実行するよ(すると `Web#read` が呼ばれるはず)
service.run(web, io)
...
修正後の spec は、 service.run(web, io)
を実行するよ、その結果、 Web#read
が呼ばれたことをテストするよ、と順番に上から下に素直に読むことができます。
...
# WebDownloadService#run を実行するよ
service.run(web, io)
# Web#read が呼ばれたことをテストするよ
expect(web).to have_received(:read)
...
修正後の spec の方が順を追って流れを理解しやすいことから、 rubocop は警告を出すのだと思います。
修正後のコード
少し、spec のリファクタリングもして、最終的なコードは以下のようになりました。
# frozen-string-literal: true
require 'open-uri'
# Web ページのクラス
class Web
def initialize(url)
@url = url
end
def read
URI.parse(@url).read
end
end
# Web ページをダウンロードするクラス
class WebDownloadService
def run(web, io = $stdout)
io.write web.read
end
end
# frozen_string_literal: true
require_relative '../web_download'
describe WebDownloadService do
describe '#run' do
let(:io) { StringIO.new }
let(:web) { instance_double(Web) }
let(:service) { described_class.new }
before do
allow(web).to receive(:read)
end
it 'calls the Web#read method' do
service.run(web, io)
expect(web).to have_received(:read)
end
end
end
まとめ
rubocop の警告が出た場合に安易に無効にしたり、言われた通りに機械的に修正するだけではなく、一度くらいは、自分の知識を更に深める良い機会だと考えて、調べてみてはいかがでしょうか?
修正後のコードと修正前のコードは以下で確認できます。
修正前:
https://github.com/suketa/rubocop_sample/tree/initial_code
修正後:
https://github.com/suketa/rubocop_sample/tree/final_code