Rails/HttpPositionalArguments autocorrect in 0.56.0

[6temes]

Context:

it 'returns http success' do
   headers = { FOO: 'bar' }
   get '/foo', headers: headers
   expect(response).to have_http_status :ok
end

Code:

get '/foo', headers: headers

Is auto corrected to:

get '/foo', params: { headers: headers } # Wrong

Expected result:

get '/foo', headers: headers  # Ok. Does not chage

or

get '/foo', params: {}, headers: headers # Ok

Versions:

• ruby 2.5.1p57
• Rails 5.2.0
• Rubocop 0.56.0

[ChrisBr]

This got changed in #5865
headers: seems not be a valid keyword argument anymore. Please use session: instead.

@bbatsov does it make sense to implement an autocorrection for headers -> session?

[6temes]

Hi @ChrisBr and @bbatsov ,

I updated my issue to make it more generic, I think that the valid_session variable name was confusing.

I have been unsuccessfully trying to find where the methods get, post, patch, etc. come from, but I reviewed the documentation for Rspec and I found something that may be relevant:

https://relishapp.com/rspec/rspec-rails/v/3-7/docs/request-specs/request-spec (at the bottom of the page)

According to them, the arguments params and headers in Rails pre5 were positional. From Rails 5, they are named arguments. So, if I understood well, in Rails 5, calling get '/foo', headers: headers is valid if you want to perform a get request without params but with special headers.

[chuckd]

Hi guys, I've just run into this too. Switching the session: key leads to an unknown keyword: session error. I believe the problem is the change made in #5865 was wrong as it confused controller specs with request specs - the headers key is completely valid and necessary for request specs, and not related to the session key in controller specs.

The relevant rspec-rails/actionpack code is completely non obvious but here's my attempt at tracing it. For controller specs:

include ActionController::TestCase::Behavior

And request specs:

include ActionDispatch::IntegrationTest::Behavior

ActionController::TestCase::Behavior#process has a session argument but no way to set headers. ActionDispatch::IntegrationTest::Behavior#process on the other hand is the only one with a headers argument.

[rrosenblum]

@6temes the request methods all call into process which is defined https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L460 and https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/testing/integration.rb#L210. When I made the changes in #5865, I didn't realize that process was defined differently for ActionDispatch. It sounds like the false positive can be fixed by adding headers and env back to the list of valid keyword arguments.

This cop appears to be unsafe in nature because the auto-correction for ActionController::TestCase and ActionDispatch::IntegrationTest are 2 different things. We would have to start checking what type of test the class is to know how to correct the issue. This could be difficult because the check would be different for Minitest and Rspec and could be prone to issues if a custom test type is used. Maybe this cop should be split into 2 cops and users would rely on telling RuboCop which cop to use based on the Includes option in the config. This approach would mainly assume that a user's test suite is appropriately split by directories that are easy to identify.