From 4eeb989b510d4d240c2b9306e669968050fb7566 Mon Sep 17 00:00:00 2001 From: ydah <13041216+ydah@users.noreply.github.com> Date: Thu, 8 Feb 2024 00:52:34 +0900 Subject: [PATCH] Add configuration option `ResponseMethods` to `RSpec/Rails/HaveHttpStatus` Fix: https://github.com/rubocop/rubocop-rspec/issues/1760 --- CHANGELOG.md | 1 + config/default.yml | 4 ++ docs/modules/ROOT/pages/cops_rspec_rails.adoc | 32 +++++++++++++- .../cop/rspec/rails/have_http_status.rb | 42 +++++++++++++++---- .../cop/rspec/rails/have_http_status_spec.rb | 33 +++++++++++++++ 5 files changed, 101 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e88b3368..d01f6caa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - Support correcting `assert_not_equal` and `assert_not_nil` in `RSpec/Rails/MinitestAssertions`. ([@G-Rath]) - Fix a false positive for `RSpec/ExpectActual` when used with rspec-rails routing matchers. ([@naveg]) - Add new `RSpec/RepeatedSubjectCall` cop. ([@drcapulet]) +- Add configuration option `ResponseMethods` to `RSpec/Rails/HaveHttpStatus`. ([@ydah]) ## 2.26.1 (2024-01-05) diff --git a/config/default.yml b/config/default.yml index e60e75ffd..abf3db8e0 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1132,8 +1132,12 @@ RSpec/Rails/AvoidSetupHook: RSpec/Rails/HaveHttpStatus: Description: Checks that tests use `have_http_status` instead of equality matchers. Enabled: pending + ResponseMethods: + - response + - last_response SafeAutoCorrect: false VersionAdded: '2.12' + VersionChanged: "<>" Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Rails/HaveHttpStatus RSpec/Rails/HttpStatus: diff --git a/docs/modules/ROOT/pages/cops_rspec_rails.adoc b/docs/modules/ROOT/pages/cops_rspec_rails.adoc index d9aead83c..14f2038e8 100644 --- a/docs/modules/ROOT/pages/cops_rspec_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rspec_rails.adoc @@ -48,22 +48,50 @@ end | Yes | Yes (Unsafe) | 2.12 -| - +| <> |=== Checks that tests use `have_http_status` instead of equality matchers. === Examples +==== ResponseMethods: ['response', 'last_response'] (default) + [source,ruby] ---- # bad expect(response.status).to be(200) -expect(response.code).to eq("200") +expect(last_response.code).to eq("200") # good expect(response).to have_http_status(200) +expect(last_response).to have_http_status(200) +---- + +==== ResponseMethods: ['foo_response'] + +[source,ruby] ---- +# bad +expect(foo_response.status).to be(200) + +# good +expect(foo_response).to have_http_status(200) + +# also good +expect(response).to have_http_status(200) +expect(last_response).to have_http_status(200) +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| ResponseMethods +| `response`, `last_response` +| Array +|=== === References diff --git a/lib/rubocop/cop/rspec/rails/have_http_status.rb b/lib/rubocop/cop/rspec/rails/have_http_status.rb index 0b8716ed4..7235a58d3 100644 --- a/lib/rubocop/cop/rspec/rails/have_http_status.rb +++ b/lib/rubocop/cop/rspec/rails/have_http_status.rb @@ -6,20 +6,32 @@ module RSpec module Rails # Checks that tests use `have_http_status` instead of equality matchers. # - # @example + # @example ResponseMethods: ['response', 'last_response'] (default) # # bad # expect(response.status).to be(200) - # expect(response.code).to eq("200") + # expect(last_response.code).to eq("200") # # # good # expect(response).to have_http_status(200) + # expect(last_response).to have_http_status(200) + # + # @example ResponseMethods: ['foo_response'] + # # bad + # expect(foo_response.status).to be(200) + # + # # good + # expect(foo_response).to have_http_status(200) + # + # # also good + # expect(response).to have_http_status(200) + # expect(last_response).to have_http_status(200) # class HaveHttpStatus < ::RuboCop::Cop::Base extend AutoCorrector MSG = - 'Prefer `expect(response).%s have_http_status(%s)` ' \ - 'over `%s`.' + 'Prefer `expect(%s).%s ' \ + 'have_http_status(%s)` over `%s`.' RUNNERS = %i[to to_not not_to].to_set RESTRICT_ON_SEND = RUNNERS @@ -28,26 +40,38 @@ class HaveHttpStatus < ::RuboCop::Cop::Base def_node_matcher :match_status, <<~PATTERN (send (send nil? :expect - $(send (send nil? :response) {:status :code}) + $(send $(send nil? #response_methods?) {:status :code}) ) $RUNNERS $(send nil? {:be :eq :eql :equal} ({int str} $_)) ) PATTERN - def on_send(node) - match_status(node) do |response_status, to, match, status| + def on_send(node) # rubocop:todo Metrics/MethodLength + match_status(node) do + |response_status, response_method, to, match, status| return unless status.to_s.match?(/\A\d+\z/) - message = format(MSG, to: to, status: status, + message = format(MSG, response: response_method.method_name, + to: to, status: status, bad_code: node.source) add_offense(node, message: message) do |corrector| - corrector.replace(response_status, 'response') + corrector.replace(response_status, response_method.method_name) corrector.replace(match.loc.selector, 'have_http_status') corrector.replace(match.first_argument, status.to_s) end end end + + private + + def response_methods?(name) + response_methods.include?(name.to_s) + end + + def response_methods + cop_config.fetch('ResponseMethods', []) + end end end end diff --git a/spec/rubocop/cop/rspec/rails/have_http_status_spec.rb b/spec/rubocop/cop/rspec/rails/have_http_status_spec.rb index aaf54d48d..9eebeda7a 100644 --- a/spec/rubocop/cop/rspec/rails/have_http_status_spec.rb +++ b/spec/rubocop/cop/rspec/rails/have_http_status_spec.rb @@ -34,6 +34,17 @@ RUBY end + it 'registers an offense for `expect(last_response.status).to eql("200")`' do + expect_offense(<<~RUBY) + it { expect(last_response.status).to eql("200") } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `expect(last_response).to have_http_status(200)` over `expect(last_response.status).to eql("200")`. + RUBY + + expect_correction(<<~RUBY) + it { expect(last_response).to have_http_status(200) } + RUBY + end + it 'does not register an offense for `is_expected.to be(200)`' do expect_no_offenses(<<~RUBY) it { is_expected.to be(200) } @@ -51,4 +62,26 @@ it { expect(response.status).to eq("404 Not Found") } RUBY end + + context 'when configured with ResponseMethods: [foo_response]' do + let(:cop_config) { { 'ResponseMethods' => %w[foo_response] } } + + it 'registers an offense for `expect(foo_response.status).to be(200)`' do + expect_offense(<<~RUBY) + it { expect(foo_response.status).to be(200) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `expect(foo_response).to have_http_status(200)` over `expect(foo_response.status).to be(200)`. + RUBY + + expect_correction(<<~RUBY) + it { expect(foo_response).to have_http_status(200) } + RUBY + end + + it 'does not register an offense for ' \ + '`expect(response.status).to be(200)`' do + expect_no_offenses(<<~RUBY) + it { expect(response.status).to be(200) } + RUBY + end + end end