-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Expose path to view specs #1402
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
require 'rspec/rails/view_assigns' | ||
require 'rspec/rails/view_spec_methods' | ||
require 'rspec/rails/view_path_builder' | ||
|
||
module RSpec | ||
module Rails | ||
|
@@ -159,6 +161,12 @@ def _include_controller_helpers | |
controller.controller_path = _controller_path | ||
controller.request.path_parameters[:controller] = _controller_path | ||
controller.request.path_parameters[:action] = _inferred_action unless _inferred_action =~ /^_/ | ||
controller.request.path = ViewPathBuilder.new(::Rails.application.routes).path_for(controller.request.path_parameters) | ||
ViewSpecMethods.add_to(::ActionView::TestCase::TestController) | ||
end | ||
|
||
after do | ||
ViewSpecMethods.remove_from(::ActionView::TestCase::TestController) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need these hooks to add the Would that be an alternative API to use? I may be missing some sort of glaring issue with how merging that with the path setup could cause issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, we're before any of the user's code, so they haven't set params yet. That means that when they do set params, later, we need to re-run the path creation. It felt odd to override There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only concern is adding a new API to the spec. If for some reason my view does look at I thought about suggesting we override p controller.request.path
render Instead the inspection would need to occur after I'm honestly a bit torn between the extra API and having to shim around |
||
end | ||
|
||
let(:_default_file_to_render) do |example| | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
module RSpec | ||
module Rails | ||
# Builds paths for view specs using a particular route set. | ||
class ViewPathBuilder | ||
def initialize(route_set) | ||
self.class.send(:include, route_set.url_helpers) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this always re-add the route helpers to the self.extend route_set.url_helpers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After a 2nd look at the class, is this used somewhere in the class? Is this how we're getting access to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's how we get |
||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a benefit to using this over storing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine. |
||
|
||
# Given a hash of parameters, build a view path, if possible. | ||
# Returns nil if no path can be built from the given params. | ||
# | ||
# @example | ||
# # path can be built because all required params are present in the hash | ||
# view_path_builder = ViewPathBuilder.new(::Rails.application.routes) | ||
# view_path_builder.path_for({ :controller => 'posts', :action => 'show', :id => '54' }) | ||
# # => "/post/54" | ||
# | ||
# @example | ||
# # path cannot be built because the params are missing a required element (:id) | ||
# view_path_builder.path_for({ :controller => 'posts', :action => 'delete' }) | ||
# # => nil | ||
def path_for(path_params) | ||
url_for(path_params.merge(:only_path => true)) | ||
rescue => e | ||
e.message | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ on the very thorough docs! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are your thoughts on something like: def path_for(path_params)
url_for(path_params.merge(:only_path => true))
rescue => e
e.message
end I suggest this because we've had requests (for example #1332) to provide better info into fields which may or may not get set. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
module RSpec | ||
module Rails | ||
# Adds methods (generally to ActionView::TestCase::TestController). | ||
# Intended for use in view specs. | ||
module ViewSpecMethods | ||
module_function | ||
|
||
# Adds methods `extra_params=` and `extra_params` to the indicated class. | ||
# When class is `::ActionView::TestCase::TestController`, these methods | ||
# are exposed in view specs on the `controller` object. | ||
def add_to(klass) | ||
return if klass.method_defined?(:extra_params) && klass.method_defined?(:extra_params=) | ||
|
||
klass.module_exec do | ||
# Set any extra parameters that rendering a URL for this view | ||
# would require. | ||
# | ||
# @example | ||
# | ||
# # In "spec/views/widgets/show.html.erb_spec.rb": | ||
# before do | ||
# widget = Widget.create!(:name => "slicer") | ||
# controller.extra_params = { :id => widget.id } | ||
# end | ||
def extra_params=(hash) | ||
@extra_params = hash | ||
request.path = | ||
ViewPathBuilder.new(::Rails.application.routes).path_for( | ||
extra_params.merge(request.path_parameters) | ||
) | ||
end | ||
|
||
# Use to read extra parameters that are set in the view spec. | ||
# | ||
# @example | ||
# | ||
# # After the before in the above example: | ||
# controller.extra_params | ||
# # => { :id => 4 } | ||
def extra_params | ||
@extra_params ||= {} | ||
@extra_params.dup.freeze | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I see the intent for this is to simply read the extra params, I'm afraid that people will instead use this as: controller.extra_params[:id] = 1 This would appear to work, but the path will not get updated. On the surface this seems like the same issue you raised about overriding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @cupakromer do you think a comment here to the effect of "Don't do that" would suffice? |
||
end | ||
end | ||
|
||
# Removes methods `extra_params=` and `extra_params` from the indicated class. | ||
def remove_from(klass) | ||
klass.module_exec do | ||
undef extra_params= if klass.method_defined?(:extra_params=) | ||
undef extra_params if klass.method_defined?(:extra_params) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
module RSpec::Rails | ||
RSpec.describe ViewSpecMethods do | ||
before do | ||
class ::VCSampleClass; end | ||
end | ||
|
||
after do | ||
Object.send(:remove_const, :VCSampleClass) | ||
end | ||
|
||
describe ".add_extra_params_accessors_to" do | ||
describe "when accessors are not yet defined" do | ||
it "adds them as instance methods" do | ||
ViewSpecMethods.add_to(VCSampleClass) | ||
|
||
expect(VCSampleClass.instance_methods.map(&:to_sym)).to(include(:extra_params=)) | ||
expect(VCSampleClass.instance_methods.map(&:to_sym)).to(include(:extra_params)) | ||
end | ||
|
||
describe "the added #extra_params reader" do | ||
it "raises an error when a user tries to mutate it" do | ||
ViewSpecMethods.add_to(VCSampleClass) | ||
|
||
expect { | ||
VCSampleClass.new.extra_params[:id] = 4 | ||
}.to raise_error(/can't modify frozen/) | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a Ruby version reason to have both string and symbol checks here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Older versions of ruby return them as strings whilst later as symbols) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this just normalize instead of using the either or check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dealt with |
||
end | ||
|
||
describe "when accessors are already defined" do | ||
before do | ||
class ::VCSampleClass | ||
def extra_params; end | ||
|
||
def extra_params=; end | ||
end | ||
end | ||
|
||
it "does not redefine them" do | ||
ViewSpecMethods.add_to(VCSampleClass) | ||
expect(VCSampleClass.new.extra_params).to be_nil | ||
end | ||
end | ||
end | ||
|
||
describe ".remove_extra_params_accessors_from" do | ||
describe "when accessors are defined" do | ||
before do | ||
ViewSpecMethods.add_to(VCSampleClass) | ||
end | ||
|
||
it "removes them" do | ||
ViewSpecMethods.remove_from(VCSampleClass) | ||
|
||
expect(VCSampleClass.instance_methods).to_not include("extra_params=") | ||
expect(VCSampleClass.instance_methods).to_not include(:extra_params=) | ||
expect(VCSampleClass.instance_methods).to_not include("extra_params") | ||
expect(VCSampleClass.instance_methods).to_not include(:extra_params) | ||
end | ||
end | ||
|
||
describe "when accessors are not defined" do | ||
it "does nothing" do | ||
expect { | ||
ViewSpecMethods.remove_from(VCSampleClass) | ||
}.to_not change { VCSampleClass.instance_methods } | ||
end | ||
end | ||
end | ||
end | ||
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.
Yay cukes! 💙 I recognize that the style for these mirrors the existing spec. If you'd be so kind to update them with a newer style we working towards that would be great!
request.fullpath
request.fullpath
set"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.
This is resolved, and it's a hangover comment that github didn't squash.