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

Start documenting ActionController::TestCase again #25862

Conversation

@prathamesh-sonpatki
Copy link
Member

@prathamesh-sonpatki prathamesh-sonpatki commented Jul 17, 2016

Summary

  • Rails 5 changed interface for passing arguments to request methods to
    keyword args for AC::TestCase but also hid the documentation.
  • But existing AC::TestCase tests need the new documentation about
    keyword args. So resurrected documentation and added a note about not
    using this for new tests.
  • The guides and other documentation is already updated to use
    ActionDispatch::IntegrationTest.

[Matthew Draper, Prathamesh Sonpatki]

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jul 17, 2016

@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Aug 10, 2016

@matthewd What do you think about this?

@kaspth kaspth added this to the 5.0.1 milestone Aug 14, 2016
@kaspth
kaspth reviewed Aug 14, 2016
View changes
actionpack/lib/action_controller/test_case.rb Outdated
# test a single controller action per test method. Rails discourages use of
# ActionController functional tests in favor of integration tests (see ActionDispatch::IntegrationTest).
# New Rails applications no longer generate ActionController style functional tests and they should
# only be used for backward compatibility. Use ActionDispatch::IntegrationTest instead.

This comment has been minimized.

@kaspth

kaspth Aug 14, 2016
Member

Maybe we should pull this out into its own == Use ActionDispatch::IntegrationTest instead section at the start?

This comment has been minimized.

@kaspth

kaspth Aug 14, 2016
Member

Action Controller and wrap ActionDispatch::IntegrationTest in backticks.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Aug 15, 2016
Author Member

@kaspth Re. changing ActionController to Action Controller --> here we are talking about implementation and not framework. AD::IntegrationTests are tests for Action Controller as well so saying Rails discourages use of Action Controller functional tests in favor of integration tests doesn't sound good to me.

@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Aug 15, 2016

@kaspth Please check now.

@kaspth
kaspth reviewed Aug 15, 2016
View changes
actionpack/lib/action_controller/test_case.rb Outdated
# Rails discourages use of `ActionController` functional tests in favor of integration tests
# (see ActionDispatch::IntegrationTest).
# New Rails applications no longer generate `ActionController` style functional tests and they should
# only be used for backward compatibility. Use `ActionDispatch::IntegrationTest` instead.

This comment has been minimized.

@kaspth

kaspth Aug 15, 2016
Member

Learned yesterday that our documentation doesn't handle backticks too well. So we'd have to go for + or <tt></tt>

See the section about request encoding here: http://api.rubyonrails.org/v5.0.0.1/classes/ActionDispatch/IntegrationTest.html (I've already fixed this in edge).

This comment has been minimized.

@kaspth

kaspth Aug 15, 2016
Member

We say both see and use ActionDispatch::IntegrationTest — why don't we merge them into one use recommendation?

@kaspth
kaspth reviewed Aug 15, 2016
View changes
actionpack/lib/action_controller/test_case.rb Outdated
#
# Rails discourages use of `ActionController` functional tests in favor of integration tests
# (see ActionDispatch::IntegrationTest).
# New Rails applications no longer generate `ActionController` style functional tests and they should

This comment has been minimized.

@kaspth

kaspth Aug 15, 2016
Member

I still disagree that we're referencing the constant here. In my opinion we are talking about Action Controller the library.

This comment has been minimized.

@kaspth

kaspth Aug 15, 2016
Member

As a reader, I'm more interested in the why than what the Rails generators do. We've moved to integration tests because they perform a request including executing the middleware stack and encoding the parameters, whereas functional controller tests merely simulate a request. Plus integration tests are faster and support as: :json and parsed_body to help ease API testing. Basically, I think it's a good thing to try to nudge people over to integration tests as best we can 😁

@kaspth
kaspth reviewed Aug 15, 2016
View changes
actionpack/lib/action_controller/test_case.rb Outdated
# different HTTP requests).
# test a single controller action per test method.
#
# == Use `ActionDispatch::Integration` tests instead of this.

This comment has been minimized.

@kaspth

kaspth Aug 15, 2016
Member

The Integration module is nodoc'ed best to rewrite as == Use integration tests over functional controller tests or similar.

@kaspth
kaspth reviewed Aug 15, 2016
View changes
actionpack/lib/action_controller/test_case.rb Outdated
#
# == Use `ActionDispatch::Integration` tests instead of this.
#
# Rails discourages use of `ActionController` functional tests in favor of integration tests

This comment has been minimized.

@kaspth

kaspth Aug 15, 2016
Member

discourages the use of

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:start-documenting-ac-testcase-again branch 3 times, most recently Aug 16, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Aug 16, 2016

@kaspth Please review again :)

@kaspth
kaspth reviewed Aug 16, 2016
View changes
actionpack/lib/action_controller/test_case.rb Outdated
#
# New Rails applications no longer generate functional style controller tests and they should
# only be used for backward compatibility. Integration style controller tests perform actual
# requests and are close to the request-response cycle in production, whereas

This comment has been minimized.

@kaspth

kaspth Aug 16, 2016
Member

and are close to the request-response cycle in production

I think we can just remove this sentence. That's not close to production that is what happens in production 😄

@kaspth
Copy link
Member

@kaspth kaspth commented Aug 16, 2016

One comment, then feel free to merge 👍

Nicely done!

- Rails 5 changed interface for passing arguments to request methods to
  keyword args for AC::TestCase but also hid the documentation.
- But existing AC::TestCase tests need the new documentation about
  keyword args. So resurrected documentation and added a note about not
  using this for new tests.
- The guides and other documentation is already updated to use
  `ActionDispatch::IntegrationTest`.

[Matthew Draper, Prathamesh Sonpatki]
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:start-documenting-ac-testcase-again branch to b556e2b Aug 17, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Aug 17, 2016

Thanks for the review!

@prathamesh-sonpatki prathamesh-sonpatki merged commit da06fb4 into rails:master Aug 17, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate Code Climate has skipped analysis of this commit.
Details
@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:start-documenting-ac-testcase-again branch Aug 17, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Aug 17, 2016

@rafaelfranca Please backport this commit, thanks!

rafaelfranca added a commit that referenced this pull request Aug 17, 2016
…ac-testcase-again

Start documenting ActionController::TestCase again
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 17, 2016

Backported in 405444c

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

5 participants