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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix with_routing test helper to make it work with api-only controllers #27371
Conversation
Build failures are unrelated to this PR. |
754cb89
to
ee52160
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the 馃敟, @yukideluxe
@spastorino you're the API person, what do you think of the fix?
@@ -170,6 +176,16 @@ def test_string_constraint | |||
end | |||
end | |||
|
|||
def test_with_routing_works_with_api_only_controllers | |||
@controller = ApiOnlyController.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, I think we could skip the ApiOnlyController
class with: @controller = Class.new(ActionController::Api).new
Cool thanks for your work @ yukideluxe. @kaspth seems OK to me at least for now. Maybe at some point in several places we could structure the code a bit differently. Perhaps in a lot of cases for API only we could have classes or modules with the basic functionality and for regular controllers we could reuse that code and enhance in subclasses or with extra modules or something like that I think would be better than having a lot of if statements. |
ee52160
to
8e9c8a1
Compare
Thanks for the feedback! @kaspth I've updated the test with your suggestion 鉁岋笍 |
with_routing do |set| | ||
set.draw do | ||
get "wow", to: "action_pack_assertions#nothing" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a process
and assert
like what we do here https://github.com/yukideluxe/rails/blob/8e9c8a19afc3f41b5bb9596ad604c7f386a903d0/actionpack/test/controller/action_pack_assertions_test.rb#L220
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I've changed the test so it does the same as the one you put as example :)
I didn't do the assertion before because the test was failing by just using with_routing
fab90b6
to
9f54e42
Compare
@spastorino yup, agreed about the future improvements. @yukideluxe looks good! Can you squash your commits and add an entry in the Action Pack changelog? 馃榿 |
9f54e42
to
f28d073
Compare
@kaspth done! 馃挭 馃帀 鉂わ笍 |
fix with_routing test helper to make it work with api-only controllers
@yukideluxe BOOM! 馃帺鉂わ笍馃挭 Backported to 5-0-stable: c484e21 |
Hi again! 馃憢
I found another regression while migrating my api-only app to Rails 5 (I'm on fire 馃敟) The
with_routing
test helper doesn't work for controllers that inherit fromActionController::API
because theActionView::Rendering
module is not longer included as it was in therails-api
gem https://github.com/rails-api/rails-api/blob/master/lib/rails-api/action_controller/api.rb#L128Hope you like the fix! 鉂わ笍