Handle unknown HTTP status codes for RSpecRails::HttpStatus cop

[viralpraxis]

Consider the following snippet

it { expect(response).to have_http_status("some-string") }

RSpecRails::HttpStatus (configured with default symbolic style) reports this as an offense:

C: [Correctable] RSpecRails/HttpStatus: Prefer nil over "some-string" to describe HTTP status code. (https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/HttpStatus)
    it { expect(response).to have_http_status("some-string") }

Autocorrection looks like have_http_status(nil) which leads to runtime error.

I expect the cop to ignore unexpected objects passed as http status code.

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

[pirj]

I think we still need to register an offence, but with a different message, that doesn’t mention nil, and do not autocorrect.

How does this spec work before correction?

[viralpraxis]

I think we still need to register an offence, but with a different message, that doesn’t mention nil, and do not autocorrect.

I agree, it seems to be even better.

How does this spec work before correction?

expect_offense(<<~RUBY)
  it { is_expected.to have_http_status("some-custom-string") }
                                        ^^^^^^^^^^^^^^^^^^^^ Prefer `nil` over `"some-custom-string"` to describe HTTP status code.
RUBY

[pirj]

I mean how does the have_http_status("some-custom-string") work, will it ever match?

[viralpraxis]

Ah, I got you. That example is artificial. I have a project where have_http_status is extended/patched to work with some specifial values (don't ask why), so it works. I understand that rubocop-rspec_rails should expect that this matcher only works with symbols/strings/integers, but the error message (and, especially, autocorrection) still seems to be wrong

[pirj]

Understood. Would it fix your case to still raise an offence but don’t autocorrect unknown string arguments?

[viralpraxis]

Yep. How should the message for this case look like?

[viralpraxis]

Actually the issue may happen with any EnforcedStyle configured; fixed it (with message Unknown status code. and autocorrection disabled).

[viralpraxis]

Could you please take a look? @pirj

[pirj]

Perfect, thank you!

[ydah]

Could you please add your comments and Bad case about this case?

rubocop-rspec_rails/lib/rubocop/cop/rspec_rails/http_status.rb

Lines 12 to 59 in 8ea4ef0

	# Enforces use of symbolic or numeric value to describe HTTP status.
	#
	# This cop inspects only `have_http_status` calls.
	# So, this cop does not check if a method starting with `be_*` is used
	# when setting for `EnforcedStyle: symbolic` or
	# `EnforcedStyle: numeric`.
	#
	# @example `EnforcedStyle: symbolic` (default)
	# # bad
	# it { is_expected.to have_http_status 200 }
	# it { is_expected.to have_http_status 404 }
	# it { is_expected.to have_http_status "403" }
	#
	# # good
	# it { is_expected.to have_http_status :ok }
	# it { is_expected.to have_http_status :not_found }
	# it { is_expected.to have_http_status :forbidden }
	# it { is_expected.to have_http_status :success }
	# it { is_expected.to have_http_status :error }
	#
	# @example `EnforcedStyle: numeric`
	# # bad
	# it { is_expected.to have_http_status :ok }
	# it { is_expected.to have_http_status :not_found }
	# it { is_expected.to have_http_status "forbidden" }
	#
	# # good
	# it { is_expected.to have_http_status 200 }
	# it { is_expected.to have_http_status 404 }
	# it { is_expected.to have_http_status 403 }
	# it { is_expected.to have_http_status :success }
	# it { is_expected.to have_http_status :error }
	#
	# @example `EnforcedStyle: be_status`
	# # bad
	# it { is_expected.to have_http_status :ok }
	# it { is_expected.to have_http_status :not_found }
	# it { is_expected.to have_http_status "forbidden" }
	# it { is_expected.to have_http_status 200 }
	# it { is_expected.to have_http_status 404 }
	# it { is_expected.to have_http_status "403" }
	#
	# # good
	# it { is_expected.to be_ok }
	# it { is_expected.to be_not_found }
	# it { is_expected.to have_http_status :success }
	# it { is_expected.to have_http_status :error }
	#

[viralpraxis]

Could you please add your comments and Bad case about this case?

rubocop-rspec_rails/lib/rubocop/cop/rspec_rails/http_status.rb

Lines 12 to 59 in 8ea4ef0

	# Enforces use of symbolic or numeric value to describe HTTP status.
	#
	# This cop inspects only `have_http_status` calls.
	# So, this cop does not check if a method starting with `be_*` is used
	# when setting for `EnforcedStyle: symbolic` or
	# `EnforcedStyle: numeric`.
	#
	# @example `EnforcedStyle: symbolic` (default)
	# # bad
	# it { is_expected.to have_http_status 200 }
	# it { is_expected.to have_http_status 404 }
	# it { is_expected.to have_http_status "403" }
	#
	# # good
	# it { is_expected.to have_http_status :ok }
	# it { is_expected.to have_http_status :not_found }
	# it { is_expected.to have_http_status :forbidden }
	# it { is_expected.to have_http_status :success }
	# it { is_expected.to have_http_status :error }
	#
	# @example `EnforcedStyle: numeric`
	# # bad
	# it { is_expected.to have_http_status :ok }
	# it { is_expected.to have_http_status :not_found }
	# it { is_expected.to have_http_status "forbidden" }
	#
	# # good
	# it { is_expected.to have_http_status 200 }
	# it { is_expected.to have_http_status 404 }
	# it { is_expected.to have_http_status 403 }
	# it { is_expected.to have_http_status :success }
	# it { is_expected.to have_http_status :error }
	#
	# @example `EnforcedStyle: be_status`
	# # bad
	# it { is_expected.to have_http_status :ok }
	# it { is_expected.to have_http_status :not_found }
	# it { is_expected.to have_http_status "forbidden" }
	# it { is_expected.to have_http_status 200 }
	# it { is_expected.to have_http_status 404 }
	# it { is_expected.to have_http_status "403" }
	#
	# # good
	# it { is_expected.to be_ok }
	# it { is_expected.to be_not_found }
	# it { is_expected.to have_http_status :success }
	# it { is_expected.to have_http_status :error }
	#

Done.

[viralpraxis]

@ydah Could you please take a look? 🙂 I updated cop's docs

[ydah]

Looks good!