Fix route helpers collision with asset helpers in view spec #1496
Conversation
| @@ -149,6 +149,22 @@ def _include_controller_helpers | |||
|
|
|||
| helper(*_default_helpers) | |||
|
|
|||
| app = ::Rails.application | |||
| if app.respond_to?(:routes) | |||
There was a problem hiding this comment.
Is there a version of rails that doesn't respond to routes? We only do defensive programming like this if theres a version inconsistency, and even then it's often better to make it a specific to the version of Rails, and often better to conditionally define a method e.g.
included do
...
setup_view_helpers
...
end
if ::Rails::VERSION < n
def setup_view_helpers
...
end
else
def setup_view_helpers
...
end
endAnd this pretty much goes for all the if statements in this PR
There was a problem hiding this comment.
To be honest, I was initially going with just doing a straight Rails.applicaiton.routes.url_helpers. However, then I saw this code in feature_example_group.rb is doing this defensive coding, so I just copied and pasted it. I saw it was introduced in 0fda98b#diff-07cac43fac355c3ef9dac2caf3a08574R11 by @jnicklas and @alindeman, so maybe they will have some insight if it's necessary.
There was a problem hiding this comment.
Try it without it, if the build passes its unnecessary.
|
@sikachu are you still interested in finishing up this PR? We'd love to see this merged. |
|
@samphippen I will work on this this Friday. Sorry for the delay. |
sikachu
force-pushed
the
fix-routes-helper
branch
2 times, most recently
from
1db2cab to
eb914d7
Compare
sikachu
force-pushed
the
fix-routes-helper
branch
4 times, most recently
from
e1718d3 to
96f8a7d
Compare
|
@samphippen I updated the code and test. This is ready for another look. |
| ::Rails.application.routes.draw { resources :images } | ||
| output = nil | ||
|
|
||
| RSpec::Core::ExampleGroup.describe "a view spec", :type => :view do |
There was a problem hiding this comment.
Rather than this, try to write this spec as below, where you create a class and include ViewExampleGroup::ExampleMethods.
This fixes the problem when calling a route helper method that has the same name as an asset helper method, such as `video_path` or `image_path`. This is done by re-including `Rails.application.routes.url_helpers` in the context of view specs. Note that the implementation to include `url_helpers` has been taken from `lib/rspec/rails/example/feature_example_group.rb`.
|
@JonRowe I've updated PR per your feedback. Do you mind give this another look? |
|
Thanks :) |
This fixes the problem when calling a route helper method that has the same name as an asset helper method, such as `video_path` or `image_path`. This is done by re-including `Rails.application.routes.url_helpers` in the context of view specs. Note that the implementation to include `url_helpers` has been taken from `lib/rspec/rails/example/feature_example_group.rb`.
This fixes the problem when calling a route helper method that has the same name as an asset helper method, such as
video_pathorimage_path. This is done by re-includingRails.application.routes.url_helpersin the context of view specs.Note that the implementation to include
url_helpershas been taken fromlib/rspec/rails/example/feature_example_group.rb.The bug I found was that when I tried to do
video_path(@video), I would get"/videos/#<Video:0x007ff92ad2aa48>"returned instead of"/videos/42". This is because the asset helper method was being called instead of routes helper.By the way, I spent some time trying to write the failing unit spec instead of integration spec (using Cucumber) but couldn't get the example within the example to run. Please let me know if I should try again to rewrite it to be a unit spec, or if this is already okay.