Skip to content

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

Merged
merged 2 commits into from
Dec 17, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Bug fixes:
internal to helper classes. (Sam Phippen, Peter Swan, #1499).
* Warn if a fixture method is called from a `before(:context)` block, instead of
crashing with a `undefined method for nil:NilClass`. (Sam Phippen, #1501)
* Expose path to view specs (Ryan Clark, Sarah Mei, Sam Phippen, #1402)

### 3.4.0 / 2015-11-11
[Full Changelog](http://github.com/rspec/rspec-rails/compare/v3.3.3...v3.4.0)
Expand Down
49 changes: 40 additions & 9 deletions features/view_specs/view_spec.feature
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Feature: view spec

View specs live in spec/views and render view templates in isolation.

Scenario: passing spec that renders the described view file
Scenario: View specs render the described view file
Given a file named "spec/views/widgets/index.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -24,7 +24,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing spec with before and nesting
Scenario: View specs can have before block and nesting
Given a file named "spec/views/widgets/index.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -51,7 +51,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing spec with explicit template rendering
Scenario: View specs can explicitly render templates
Given a file named "spec/views/widgets/widget.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -73,7 +73,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing spec with a description that includes the format and handler
Scenario: View specs can have description that includes the format and handler
Given a file named "spec/views/widgets/widget.xml.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand Down Expand Up @@ -105,7 +105,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing spec with rendering of locals in a partial
Scenario: View specs can render locals in a partial
Given a file named "spec/views/widgets/_widget.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -127,7 +127,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing spec with rendering of locals in an implicit partial
Scenario: View specs can render locals in an implicit partial
Given a file named "spec/views/widgets/_widget.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -149,7 +149,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing spec with rendering of text
Scenario: View specs can render text
Given a file named "spec/views/widgets/direct.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -166,7 +166,7 @@ Feature: view spec
When I run `rspec spec/views`
Then the examples should all pass

Scenario: passing view spec that stubs a helper method
Scenario: View specs can stub a helper method
Given a file named "app/helpers/application_helper.rb" with:
"""ruby
module ApplicationHelper
Expand Down Expand Up @@ -199,7 +199,7 @@ Feature: view spec
When I run `rspec spec/views/secrets`
Then the examples should all pass

Scenario: request.path_parameters should match Rails by using symbols for keys
Scenario: View specs use symbols for keys in `request.path_parameters` to match Rails style
Given a file named "spec/views/widgets/index.html.erb_spec.rb" with:
"""ruby
require "rails_helper"
Expand All @@ -212,3 +212,34 @@ Feature: view spec
"""
When I run `rspec spec/views`
Then the examples should all pass

Scenario: View spec actions that do not require extra parameters have `request.fullpath` set
Given a file named "spec/views/widgets/index.html.erb_spec.rb" with:
"""ruby
require "rails_helper"

RSpec.describe "widgets/index" do
it "has a request.fullpath that is defined" do
expect(controller.request.fullpath).to eq widgets_path
end
end
"""
When I run `rspec spec/views`
Then the examples should all pass

Scenario: View spec actions that require extra parameters have `request.fullpath` set when the developer supplies them
Given a file named "spec/views/widgets/show.html.erb_spec.rb" with:
"""ruby
require "rails_helper"

RSpec.describe "widgets/show" do
it "displays the widget with id: 1" do
widget = Widget.create!(:name => "slicer")
controller.extra_params = { :id => widget.id }

expect(controller.request.fullpath).to eq widget_path(widget)
end
end
"""
When I run `rspec spec/views`
Then the examples should all pass
Copy link
Member

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!

  • Use backticks for code in descriptions (including scenarios) as in normal markdown, so the scenarios should use request.fullpath
  • Scenarios should start capitalized; does not apply when starting with a code fragment
  • Avoid "should" opting for more direct and present language. For example "Actions which do not require extra parameters have request.fullpath set"

Copy link
Member

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.

8 changes: 8 additions & 0 deletions lib/rspec/rails/example/view_example_group.rb
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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these hooks to add the extra_params? I ask as there is access to params which is public in the view spec. We don't appear to have a feature scenario demoing it 😬

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.

Copy link
Author

Choose a reason for hiding this comment

The 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 params#[] to do that, so we opted to add new functionality. We can do the params thing instead if you'd rather.

Copy link
Member

Choose a reason for hiding this comment

The 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 params, say params[:id], and looks at the path (I know not something that should be done, but it still could happen), the id value will need to get set twice. That seems a bit odd to me.

I thought about suggesting we override render in the spec, but then I realized that my standard debugging technique wouldn't necessarily work there:

p controller.request.path
render

Instead the inspection would need to occur after render. Which may not be an issue unless render causes an exception in which case inspecting the path wouldn't be possible without more hackery.

I'm honestly a bit torn between the extra API and having to shim around params.

end

let(:_default_file_to_render) do |example|
Expand Down
29 changes: 29 additions & 0 deletions lib/rspec/rails/view_path_builder.rb
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this always re-add the route helpers to the ViewPathBuilder class? I'm a little tired, I think I am missing something. If this uses extend on the object does it accomplish the same thing without affecting ViewPathBuilder?

self.extend route_set.url_helpers

Copy link
Member

Choose a reason for hiding this comment

The 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 url_for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's how we get url_for.

end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a benefit to using this over storing the route_set and calling route_set.url_for below?

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ on the very thorough docs!

Copy link
Member

Choose a reason for hiding this comment

The 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
56 changes: 56 additions & 0 deletions lib/rspec/rails/view_spec_methods.rb
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
Copy link
Member

Choose a reason for hiding this comment

The 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 params. I think I may be missing something. 😸 Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The 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
72 changes: 72 additions & 0 deletions spec/rspec/rails/view_spec_methods_spec.rb
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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Older versions of ruby return them as strings whilst later as symbols)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this just normalize instead of using the either or check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly.

Copy link
Member

Choose a reason for hiding this comment

The 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