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

Adds ability to inspect rails 5 test process method #3365

Merged
merged 1 commit into from Oct 7, 2016

Conversation

Projects
None yet
4 participants
@logicminds
Contributor

logicminds commented Aug 4, 2016

Rails 5 deprecates the use of http methods when testing controllers and adds the ability to use keyword arguments instead.

DEPRECATION WARNING: ActionDispatch::IntegrationTest HTTP request methods will accept
only the following keyword arguments in future Rails versions:
params, headers, env, xhr

This adds a new Rails cop to fix these notices because there is going to be a ton for everyone using rails5.

So this will test a rails 5 test method to ensure the use of the process method is used instead.

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Aug 4, 2016

Contributor

This still needs work, as I was unable to figure out how to only process files in particular file paths. I am also curious how we can only enable this cop when rails5 is detected. I know we detect rails but do we also get the version of rails?

Any other help is appreciated. Additional tests would also be need to cover headers, env, and xhr.

Contributor

logicminds commented Aug 4, 2016

This still needs work, as I was unable to figure out how to only process files in particular file paths. I am also curious how we can only enable this cop when rails5 is detected. I know we detect rails but do we also get the version of rails?

Any other help is appreciated. Additional tests would also be need to cover headers, env, and xhr.

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Aug 4, 2016

Collaborator

Nice. 😀

I take it you used Rails/FindEach as a template, because the top level comment and #ignored_by_find_each? method is still in there. 😀

Collaborator

Drenmi commented Aug 4, 2016

Nice. 😀

I take it you used Rails/FindEach as a template, because the top level comment and #ignored_by_find_each? method is still in there. 😀

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Aug 4, 2016

Contributor

ah thanks will replace that.

Contributor

logicminds commented Aug 4, 2016

ah thanks will replace that.

Show outdated Hide outdated lib/rubocop.rb Outdated

@logicminds logicminds changed the title from WIP: Adds ability to inspect rails 5 test process method to Adds ability to inspect rails 5 test process method Aug 4, 2016

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Aug 4, 2016

Contributor

@Drenmi any other issues? Just fixed 1083 warnings in my own rails app!

Contributor

logicminds commented Aug 4, 2016

@Drenmi any other issues? Just fixed 1083 warnings in my own rails app!

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Aug 5, 2016

Contributor

Looks like I need to go back and configure the process method to use the http method since rails5 doesn't official support the process method in controller tests. Lots of confusing docs and blogs.

Contributor

logicminds commented Aug 5, 2016

Looks like I need to go back and configure the process method to use the http method since rails5 doesn't official support the process method in controller tests. Lots of confusing docs and blogs.

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Aug 18, 2016

Contributor

@Drenmi would be great to get this merged in so I can stop rebasing.

Contributor

logicminds commented Aug 18, 2016

@Drenmi would be great to get this merged in so I can stop rebasing.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Aug 18, 2016

Collaborator

I'm on vacation and I haven't had time to review it yet. If someone reviews
it thoroughly this might expedite the process of merging the PR.

On Thursday, 18 August 2016, Corey Osman notifications@github.com wrote:

@Drenmi https://github.com/Drenmi would be great to get this merged in
so I can stop rebasing.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3365 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGVygJt7Ilu3wFsu7c5sseiihQ9f2epks5qg6t6gaJpZM4JcUHf
.

Collaborator

bbatsov commented Aug 18, 2016

I'm on vacation and I haven't had time to review it yet. If someone reviews
it thoroughly this might expedite the process of merging the PR.

On Thursday, 18 August 2016, Corey Osman notifications@github.com wrote:

@Drenmi https://github.com/Drenmi would be great to get this merged in
so I can stop rebasing.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3365 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGVygJt7Ilu3wFsu7c5sseiihQ9f2epks5qg6t6gaJpZM4JcUHf
.

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Aug 18, 2016

Collaborator

@logicminds: I am not on the core team. 😬 I'm also extremely cramped for time at the moment. 😞

Collaborator

Drenmi commented Aug 18, 2016

@logicminds: I am not on the core team. 😬 I'm also extremely cramped for time at the moment. 😞

Show outdated Hide outdated CHANGELOG.md Outdated
Show outdated Hide outdated CHANGELOG.md Outdated
Show outdated Hide outdated config/enabled.yml Outdated
Show outdated Hide outdated config/enabled.yml Outdated
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Aug 20, 2016

Collaborator

I took a cursory glance at the cop and I'd say it needs some cleanup and polish, before we can merge it.

Collaborator

bbatsov commented Aug 20, 2016

I took a cursory glance at the cop and I'd say it needs some cleanup and polish, before we can merge it.

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Aug 22, 2016

Contributor

@bbatsov I made all the corrections and change the name to HttpPositionalArguments after what rubocop is actually checking for.

On a side note for some reason I was not able to rename to HttpPositionalArgs as ruby errors kept telling me about class name errors. No idea why, so I went with the long form.
uninitialized constant RuboCop::Cop::Rails::HttpPositionalArgs (NameError)

Contributor

logicminds commented Aug 22, 2016

@bbatsov I made all the corrections and change the name to HttpPositionalArguments after what rubocop is actually checking for.

On a side note for some reason I was not able to rename to HttpPositionalArgs as ruby errors kept telling me about class name errors. No idea why, so I went with the long form.
uninitialized constant RuboCop::Cop::Rails::HttpPositionalArgs (NameError)

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Aug 31, 2016

Collaborator

On a side note for some reason I was not able to rename to HttpPositionalArgs as ruby errors kept telling me about class name errors. No idea why, so I went with the long form.
uninitialized constant RuboCop::Cop::Rails::HttpPositionalArgs (NameError)

That sounds pretty odd. Maybe you forgot to rename some class or something then?

The build is failing right now.

Collaborator

bbatsov commented Aug 31, 2016

On a side note for some reason I was not able to rename to HttpPositionalArgs as ruby errors kept telling me about class name errors. No idea why, so I went with the long form.
uninitialized constant RuboCop::Cop::Rails::HttpPositionalArgs (NameError)

That sounds pretty odd. Maybe you forgot to rename some class or something then?

The build is failing right now.

@denys281

This comment has been minimized.

Show comment
Hide comment
@denys281

denys281 Sep 7, 2016

@logicminds I think that you should rename you spec file from http_positional_arguments.rb to http_positional_arguments_spec.rb. At least I was able to run your spec file without NameError.

denys281 commented Sep 7, 2016

@logicminds I think that you should rename you spec file from http_positional_arguments.rb to http_positional_arguments_spec.rb. At least I was able to run your spec file without NameError.

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Sep 7, 2016

Contributor

@denys281 thanks for the suggestions. Not sure how the spec file was renamed without the spec but I'll just blame RubyMine. That fixed my issue. Just rebased so this should be all good now.

Contributor

logicminds commented Sep 7, 2016

@denys281 thanks for the suggestions. Not sure how the spec file was renamed without the spec but I'll just blame RubyMine. That fixed my issue. Just rebased so this should be all good now.

# # good
# get :new, params: { user_id: 1 }
class HttpPositionalArguments < Cop
MSG = 'Use `keyword arguments` instead of ' \

This comment has been minimized.

@bbatsov

bbatsov Sep 9, 2016

Collaborator

drop the backquotes around keyword arguments

@bbatsov

bbatsov Sep 9, 2016

Collaborator

drop the backquotes around keyword arguments

This comment has been minimized.

@logicminds

logicminds Sep 9, 2016

Contributor

I was just following others. Can you add what exactly you want from a MSG standpoint in a contributing doc to save future developers and yourself time?

@logicminds

logicminds Sep 9, 2016

Contributor

I was just following others. Can you add what exactly you want from a MSG standpoint in a contributing doc to save future developers and yourself time?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 9, 2016

Collaborator

I've added some more remarks, mostly concerning the tests. You might want to review http://betterspecs.org/, there's a ton of great advice regarding specs there.

Collaborator

bbatsov commented Sep 9, 2016

I've added some more remarks, mostly concerning the tests. You might want to review http://betterspecs.org/, there's a ton of great advice regarding specs there.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 5, 2016

Collaborator

@logicminds ping :-)

Collaborator

bbatsov commented Oct 5, 2016

@logicminds ping :-)

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Oct 5, 2016

Contributor

Its on my todo list, I'll try and refactor this week

Contributor

logicminds commented Oct 5, 2016

Its on my todo list, I'll try and refactor this week

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Oct 6, 2016

Contributor

@bbatsov just refactored and rebased.

Contributor

logicminds commented Oct 6, 2016

@bbatsov just refactored and rebased.

Show outdated Hide outdated config/enabled.yml Outdated
Show outdated Hide outdated CHANGELOG.md Outdated
Adds ability to inspect rails 5 test process method (#3370)
   * rails 5 introduces the ability to pass keyword arguments
     to the http method calls for test classes.
     Any rails 5 upgrade would likely see
     hundreds if not thousands of messages like:

     DEPRECATION WARNING: ActionDispatch::IntegrationTest HTTP request
     methods will accept
     only the following keyword arguments in future Rails versions:
     params, headers, env.

     This commit adds a new Rails/HttpPositionalArguments cop that detects
     and fixes those deprecated methods in tests by replacing
     with keyword args like params and headers.

@bbatsov bbatsov merged commit 4bbfc78 into rubocop-hq:master Oct 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@logicminds logicminds deleted the nwops:rails5_process branch Oct 8, 2016

Neodelf added a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016

Add the ability to inspect Rails 5 test process method (rubocop-hq#3370
…) (rubocop-hq#3365)

Rails 5 introduces the ability to pass keyword arguments
to the http method calls for test classes.
Any Rails 5 upgrade would likely see
hundreds if not thousands of messages like:

     DEPRECATION WARNING: ActionDispatch::IntegrationTest HTTP request
     methods will accept
     only the following keyword arguments in future Rails versions:
     params, headers, env.

This commit adds a new Rails/HttpPositionalArguments cop that detects
 and fixes those deprecated methods in tests by replacing
 with keyword args like params and headers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment