rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

Update RSpec/Rails/HttpStatus to support be_* syntax

Open bkuhlmann opened this issue 3 years ago • 4 comments

Overview

When writing Rails request specs, it'd be handy to auto-correct/enforce the "be_" syntax since it's shorter and easier to read instead of the existing have_http_status syntax.

Desired Behavior

I'd like to propose we update this cop (or make a new one if that makes sense) so we can promote the following:

# No
it "answers unauthenticated status" do
  get api_users_path
  expect(response).to have_http_status(:unauthorized)
end

# Yes
it "answers unauthenticated status" do
  get api_users_path
  expect(response).to be_unauthorized
end

Environment

1.13.0 (using Parser 3.0.1.1, rubocop-ast 1.5.0, running on ruby 3.0.1 arm64-darwin20)
  - rubocop-performance 1.11.1
  - rubocop-rake 0.5.1
  - rubocop-rspec 2.3.0

bkuhlmann avatar May 06 '21 20:05 bkuhlmann

Sounds reasonable. Would you like to hack on it?

pirj avatar May 06 '21 23:05 pirj

Yeah, it's been a while since I've had to write one of these but probably time I get back into this space. Yeah, might take me a while to get ramped up -- on top of everything else I'm doing -- but will see what I can do. :wink:

bkuhlmann avatar May 07 '21 14:05 bkuhlmann

Would it be better to add another EnforcedStyle - be_status - and maybe make it a default? This will allow users to use old style if they wish to.

tejasbubane avatar May 19 '21 21:05 tejasbubane

It seems that be_* matchers work with predicate methods however ActionDispatch::TestResponse doesn't have predicates for all the statuses, but only for most common ones.

sl4vr avatar Aug 30 '21 09:08 sl4vr

- expect(response).to have_http_status(200)
- expect(response).to have_http_status(:ok)
+ expect(response).to be_ok

Research:

$ rg --no-ignore --hidden --count-matches 'have_http_status.200' | wc -l
126 # files, some file have more than one match

$ rg --no-ignore --hidden --count-matches 'have_http_status.:ok' | wc -l
91 # files, some file have more than one match

$ rg --no-ignore --hidden --count-matches 'be_ok' | wc -l
33 # files, some file have more than one match. Just a few are unrelated to responses

$ rg --no-ignore --hidden --count-matches 'be_successful' | wc -l
365 # files, some file have more than one match. All are related to responses

I'd say it could become the default style right away with such a magnitude of adoption.

Mapping in Rack. More - needs to be checked if additional ones work fine with predicate matchers.

I'm uncertain if it should be a new cop or a style for HttpStatus/HaveHttpStatus.

It feels that the cop could be strict or relaxed, e.g. suggest be_ok for 200 or be_successful for 200/201/202/204. Should it enforce be_redirect for all 3xx codes? Or only in the relaxed mode?

pirj avatar Apr 04 '23 20:04 pirj