Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use kwargs in ActionController::TestCase and ActionDispatch::Integration HTTP methods #18323

Merged
merged 1 commit into from Jan 29, 2015

Conversation

@kirs
Copy link
Member

@kirs kirs commented Jan 4, 2015

As @dhh suggested, controller HTTP methods are a prime candidate for kwargs.
post url, nil, nil, { a: 'b' } doesn't make sense.
More explicit way would be: post url, params: { y: x }, session: { a: 'b' }.

@rafaelfranca
rafaelfranca reviewed Jan 4, 2015
View changes
actionpack/lib/action_controller/test_case.rb Outdated
session = args[0][:session]
flash = args[0][:flash]
else
parameters, session, flash = args

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 4, 2015
Member

We should deprecate this path or we will not be able to use kwargs for real here in the future.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 4, 2015
Member

Since we are deprecating all documentation and tests should use the kwarg implementation and we can remove the old implementation from the documentation keeping only a few tests to make sure it works and it is deprecated.

This comment has been minimized.

@kirs

kirs Jan 7, 2015
Author Member

Good point, thanks.

Here is rdoc syntax for usual arguments:

- +action+: The controller action to call.
- +http_method+: Request method used to send the http request. Possible values
  are +GET+, +POST+, +PATCH+, +PUT+, +DELETE+, +HEAD+. Defaults to +GET+.
- +parameters+: The HTTP parameters. This may be +nil+, a hash, or a
  string that is appropriately encoded (+application/x-www-form-urlencoded+
  or +multipart/form-data+).
- +session+: A hash of parameters to store in the session. This may be +nil+.
- +flash+: A hash of parameters to store in the flash. This may be +nil+.

Any suggestions how it will look like with kwargs? Couldn't find it in docs.

This comment has been minimized.

@dhh

dhh Jan 7, 2015
Member

New form should be:

process action, method: 'GET', params: {}, session: {}, flash: {}

And then we add on with:

post/get/put/patch/delete/head action, params: {}, session: {}, flash: {}

This comment has been minimized.

@kirs

kirs Jan 7, 2015
Author Member

Got it. I meant the rdoc syntax for describing kwargs

@dhh
Copy link
Member

@dhh dhh commented Jan 4, 2015

We should do the same treatment to ActionDispatch::IntegrationTest.

@kirs
Copy link
Member Author

@kirs kirs commented Jan 4, 2015

@dhh in the same PR or in the separate?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 4, 2015

In the same

On Sun, Jan 4, 2015, 15:04 Kir Shatrov notifications@github.com wrote:

@dhh https://github.com/dhh in the same PR or in the separate?


Reply to this email directly or view it on GitHub
#18323 (comment).

@dhh
Copy link
Member

@dhh dhh commented Jan 4, 2015

We can do a separate PR for that. Just that we don’t want the two APIs to diverge in a release.

On Jan 4, 2015, at 9:04 AM, Kir Shatrov notifications@github.com wrote:

@dhh https://github.com/dhh in the same PR or in the separate?


Reply to this email directly or view it on GitHub #18323 (comment).

@dhh
Copy link
Member

@dhh dhh commented Jan 4, 2015

Ha, I don't really care if it's the same or separate. But yeah, @rafaelfranca is probably right that we might as well treat it as one change. Since it's the same API. Although IntegrationTests also have env.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 4, 2015

Yeah, they can be different commits but I'd like to have the whole set of
changes grouped so we can link is release notes or even revert if needed.

On Sun, Jan 4, 2015, 15:08 David Heinemeier Hansson <
notifications@github.com> wrote:

Ha, I don't really care if it's the same or separate. But yeah,
@rafaelfranca https://github.com/rafaelfranca is probably right that we
might as well treat it as one change. Since it's the same API. Although
IntegrationTests also have env.


Reply to this email directly or view it on GitHub
#18323 (comment).

@kirs
Copy link
Member Author

@kirs kirs commented Jan 19, 2015

@kirs kirs changed the title Use kwargs in ActionController::TestCase HTTP methods Use kwargs in ActionController::TestCase and ActionDispatch::Integration HTTP methods Jan 19, 2015
@dhh
dhh reviewed Jan 19, 2015
View changes
actionpack/test/controller/integration_test.rb Outdated
end
end
end
# class MetalIntegrationTest < ActionDispatch::IntegrationTest

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

Why is this test commented out?

@dhh
dhh reviewed Jan 19, 2015
View changes
actionpack/test/controller/live_stream_test.rb Outdated
end

capture_log_output do |output|
get :exception_in_view_after_commit, format: :json

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

format should be a stand-alone thing, but part of the params.

This comment has been minimized.

@kirs

kirs Jan 20, 2015
Author Member

Fixed.

@dhh
dhh reviewed Jan 19, 2015
View changes
actionpack/lib/action_dispatch/testing/integration.rb Outdated
# be +nil+,
# a Hash, or a String that is appropriately encoded
# (<tt>application/x-www-form-urlencoded</tt> or
# <tt>multipart/form-data</tt>).
# - +headers_or_env+: Additional headers to pass, as a Hash. The headers will be
# - headers_or_env: Additional headers to pass, as a Hash. The headers will be

This comment has been minimized.

@dhh

dhh Jan 19, 2015
Member

Let's just call this headers. Hate that "or" setup. The headers get turned into env, that's fine. Don't need to spell that out further.

Actually, even better, let's do both, and just merge them together. Then it'll be even clearer in the code what the intent is.

@rafaelfranca
rafaelfranca reviewed Jan 20, 2015
View changes
actionpack/lib/action_dispatch/testing/integration.rb Outdated
# xhr :get, '/feed', params: { since: 201501011400 }
def xml_http_request(request_method, path, *args)
if kwarg_request?(*args)
# TODO slice

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

This TODO need to be solved

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 20, 2015

Should we update the testing guide?

@kirs
Copy link
Member Author

@kirs kirs commented Jan 20, 2015

@rafaelfranca TODO is fixed, thanks! I will update the guide soon.

@kirs
Copy link
Member Author

@kirs kirs commented Jan 20, 2015

@rafaelfranca guide updated.

@rafaelfranca
rafaelfranca reviewed Jan 20, 2015
View changes
actionpack/lib/action_controller/test_case.rb Outdated
# be +nil+, a hash, or a string that is appropriately encoded
# - action: The controller action to call.
# - params: The hash with HTTP parameters that you want to pass. This may be +nil+.
# - body: The request body with a string that is appropriately encoded
# (<tt>application/x-www-form-urlencoded</tt> or <tt>multipart/form-data</tt>).
# - +session+: A hash of parameters to store in the session. This may be +nil+.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

If we removed the fixed size of fonts of the other arguments we should do the same with session and flash.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2015
Member

But I really not sure if we should remove it @fxn?

This comment has been minimized.

@kirs

kirs Jan 21, 2015
Author Member

What do you mean under "fixed size of fonts"?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 21, 2015
Member

The +action+ over action.

This comment has been minimized.

@fxn

fxn Jan 21, 2015
Member

Yep, we should keep it.

This comment has been minimized.

@fxn

fxn Jan 21, 2015
Member

@kirs when you refer to variables, keyword arguments, etc. you use fixed-width font for them. That is assigns, rather than just assigns. Have a look at the API guidelines.

This comment has been minimized.

@kirs

kirs Jan 21, 2015
Author Member

Thanks for review and explanation, I will fix it.

@kirs
Copy link
Member Author

@kirs kirs commented Jan 24, 2015

@rafaelfranca I added the CHANGELOG entry. Ready for merge?

@robin850
robin850 reviewed Jan 25, 2015
View changes
actionpack/lib/action_controller/test_case.rb Outdated
@@ -491,58 +491,64 @@ def determine_default_controller_class(name)
end
end

# Simulate a GET request with the given parameters.
# Simulate a GET request with the given keyword arguments.

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

"parameters" is maybe a better fit here ; "keyword arguments" is more an implementation detail and we are still passing the action as a normal parameter, not as a keyword argument. What do you think ?

@robin850
robin850 reviewed Jan 25, 2015
View changes
actionpack/lib/action_controller/test_case.rb Outdated
#
# You can also simulate POST, PATCH, PUT, DELETE, and HEAD requests with
# +post+, +patch+, +put+, +delete+, and +head+.
# Example sending parameters, session and setting a flash message:
#
# get :show

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

There's a missing comma here.

@robin850
robin850 reviewed Jan 25, 2015
View changes
actionpack/lib/action_controller/test_case.rb Outdated
@@ -567,40 +573,68 @@ def paramify_values(hash_or_array_or_value)
#
# - +action+: The controller action to call.
# - +http_method+: Request method used to send the http request. Possible values

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

It should be +method:+, no ? Also a nit-picky thing but maybe we can use "HTTP" instead of "http".

@robin850
robin850 reviewed Jan 25, 2015
View changes
actionpack/lib/action_controller/test_case.rb Outdated
#
# To simulate +GET+, +POST+, +PATCH+, +PUT+, +DELETE+ and +HEAD+ requests
# prefer using #get, #post, #patch, #put, #delete and #head methods
# respectively which will make tests more expressive.
#
# Note that the request method is not verified.
def process(action, http_method = 'GET', *args)

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

✂️

@robin850
robin850 reviewed Jan 25, 2015
View changes
actionpack/lib/action_dispatch/testing/integration.rb Outdated
@@ -12,12 +12,14 @@ module RequestHelpers
#
# - +path+: The URI (as a String) on which you want to perform a GET
# request.
# - +parameters+: The HTTP parameters that you want to pass. This may
# - +parameters:+ The HTTP parameters that you want to pass. This may

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

It should be +params:+, no ?

@robin850
robin850 reviewed Jan 25, 2015
View changes
actionpack/lib/action_dispatch/testing/integration.rb Outdated
# params: { ref_id: 14 },
# headers: {"X-Test-Header" => "testvalue"}
def request_via_redirect(http_method, path, *args)
if kwarg_request?(*args)

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

It looks like you are duplicating some logic from process_with_kwargs, is it intended ? Otherwise, the method could look like:

def request_via_redirect
  process_with_kwargs(http_method, path, *args)

  follow_redirect! while redirect?
  status
end
@robin850
robin850 reviewed Jan 25, 2015
View changes
guides/source/testing.md Outdated
* An optional hash of request parameters to pass into the action (eg. query string parameters or article variables).
* An optional hash of session variables to pass along with the request.
* An optional hash of flash values.
* `params:` option with a hash of request parameters to pass into the action (eg. query string parameters or article variables).

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

Missing dot after the "e" of "e.g.". Also could you please wrap your lines around 80 chars editing the guides ? That would be awesome!

@robin850
robin850 reviewed Jan 25, 2015
View changes
guides/source/testing.md Outdated
* An optional hash of flash values.
* `params:` option with a hash of request parameters to pass into the action (eg. query string parameters or article variables).
* `session:` option with a hash of session variables to pass along with the request.
* `flash` option with a hash of flash values.

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

Nit-picky missing colon. 😄

@robin850
robin850 reviewed Jan 25, 2015
View changes
guides/source/testing.md Outdated
* `session:` option with a hash of session variables to pass along with the request.
* `flash` option with a hash of flash values.

All the key arguments are optional.

This comment has been minimized.

@robin850

robin850 Jan 25, 2015
Member

s/key/keyword

@robin850
Copy link
Member

@robin850 robin850 commented Jan 25, 2015

Awesome work @kirs !

There's just a tiny missing change here to avoid displaying a deprecation warning running the test suite. Also, regarding deprecation warnings, you may want to use strip_heredoc instead of squish to keep the formatting. At the moment, it displays like this:

DEPRECATION WARNING: ActionDispatch::Integration::TestCase HTTP request methods will accept only keyword arguments in future Rails versions. Examples: get '/profile', params: { id: 1 }, headers: { 'X-Extra-Header' => '123' }, env: { 'action_dispatch.custom' => 'custom' } xhr :post, '/profile', params: { id: 1 }. (called from block (3 levels) in run at ...).

But well, that's just styling concerns, there's no big deal. Thank you !

@connorshea
Copy link
Contributor

@connorshea connorshea commented Jul 20, 2016

Anyone having trouble with this change specifically may want to take a look at the rails5-spec-converter gem

@hakanai
Copy link

@hakanai hakanai commented Aug 2, 2016

This warning was awfully hard to understand because by reading my code, it seemed like my params were already "keyword arguments". Now I understand that what it really means for all of my usages is that params can no longer be passed bare anymore.

@aaronrussell
Copy link

@aaronrussell aaronrussell commented Aug 2, 2016

Is there a way of writing ActionController::TestCase tests that will pass in both Rails 5 and 4.2? (for rails engines that need to run in both)

@connorshea
Copy link
Contributor

@connorshea connorshea commented Aug 2, 2016

@aaronrussell they'll still pass in both, but will print deprecation warnings on 5.

kaspth added a commit that referenced this pull request Aug 13, 2016
Prevent hitting integration tests users with two deprecation warnings by
clarifying how they should upgrade to the request helpers `get` and friends.

It's hard to imagine people having `xhr` calls that use keywords, why not
follow the xhr deprecation message? Why instead insert a middle layer of
migration?

They have to switch to something else anyway, so just show how that looks
and nudge them a bit more.

The code was originally added in #18323,
but then wasn't touched when deprecating xhr in
#18771.
edwardloveall added a commit to edwardloveall/pullfeed that referenced this pull request Nov 2, 2016
rails/rails#18323

Changes code like:

* get :show, params.merge(format: :atom)
* get(path, {}, headers)

to

* get :show, params: params.merge(format: :atom)
* get(path, params: {}, headers: headers)

Thanks to @rosmith76 and @asackofwheat
CGA1123 added a commit to CGA1123/F29SO that referenced this pull request Nov 28, 2016
Adds both `html` and `js` responses allowing the editting/updating of
`first_name`, `last_name`, & `location` on a users profile.

Introduces the `profile.edit` & `profile.edit.others` permissions.

Removes the `Rail/HttpPositionalArguments` Cop from `rubocop`, this is
due to the fact that we're using `rails-4.2.7.1` and that cop is
intended to catch behaviour that was deprecated in this [PR](rails/rails#18323)
however that deprecation does not affect our current `rails` version.
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 30, 2017
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 30, 2017
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 30, 2017
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 30, 2017
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 31, 2017
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 31, 2017
jfragoulis added a commit to skroutz/cogy that referenced this pull request May 31, 2017
hanmd82 added a commit to hanmd82/contribulator that referenced this pull request Oct 16, 2017
…ests#676

- update jsonapi initializer and corresponding JSON response keys
- add rails-controller-testing gem to support 'assert_template' and 'assigns'
  - see rspec/rspec-rails#1393
- update controller tests as Rails5 only accepts keyword arguments
  - see rails/rails#18323
skalee added a commit to riboseinc/synced_resources that referenced this pull request Oct 19, 2017
Rails 5 introduces a breaking change to request helpers synopsis.
Unfortunately, the modern way doesn't work in Rails 4, so we need to
choose between old and new helpers depending on Rails version.

See:
- rails/rails#18323
- https://blog.bigbinary.com/2016/04/19/changes-to-test-controllers-in-rails-5.html
ronaldtse added a commit to riboseinc/synced_resources that referenced this pull request Oct 20, 2017
Rails 5 introduces a breaking change to request helpers synopsis.
Unfortunately, the modern way doesn't work in Rails 4, so we need to
choose between old and new helpers depending on Rails version.

See:
- rails/rails#18323
- https://blog.bigbinary.com/2016/04/19/changes-to-test-controllers-in-rails-5.html
GabrielSandoval pushed a commit to GabrielSandoval/asyncapi-server that referenced this pull request Mar 13, 2018
- Use kwargs in HTTP request methods in rspec

See: rails/rails#18323
GabrielSandoval pushed a commit to GabrielSandoval/asyncapi-server that referenced this pull request Mar 14, 2018
- Use kwargs in HTTP request methods in rspec
See: rails/rails#18323
falusi94 added a commit to kir-dev/pek-next that referenced this pull request Aug 26, 2019
falusi94 added a commit to kir-dev/pek-next that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.