Skip to content

modify error message when redirected but expecting to render a template #1440

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 3 commits into from
Aug 2, 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
5 changes: 5 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
### 3.4.0 Development
[Full Changelog](http://github.com/rspec/rspec-rails/compare/v3.3.3...master)

Enhancements:

* Improved the failure message for `have_rendered` matcher on a redirect
response. (Alex Egan, #1440)

Bug Fixes:

* Fix another load order issued which causes an undefined method `fixture_path` error
Expand Down
23 changes: 21 additions & 2 deletions lib/rspec/rails/matchers/have_rendered.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,37 @@ def initialize(scope, expected, message = nil)
@expected = Symbol === expected ? expected.to_s : expected
@message = message
@scope = scope
@redirect_is = nil
end

# @api private
def matches?(*)
match_unless_raises ActiveSupport::TestCase::Assertion do
match_check = match_unless_raises ActiveSupport::TestCase::Assertion do
@scope.assert_template expected, @message
end
check_redirect unless match_check
match_check
end

# Uses normalize_argument_to_redirection to find and format
# the redirect location. normalize_argument_to_redirection is private
# in ActionDispatch::Assertions::ResponseAssertions so we call it
# here using #send. This will keep the error message format consistent
# @api private
def check_redirect
Copy link
Member

Choose a reason for hiding this comment

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

This needs a YARD doc comment.

response = @scope.response
return unless response.respond_to?(:redirect?) && response.redirect?
@redirect_is = @scope.send(:normalize_argument_to_redirection, response.location)
end
Copy link
Member

Choose a reason for hiding this comment

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

When the YARD doc is added, can you include some details about this normalize_argument_to_redirection method and why we need to use send?


# @api private
def failure_message
rescued_exception.message
if @redirect_is
rescued_exception.message[/.* but /] +
"was a redirect to <#{@redirect_is}>"
else
rescued_exception.message
end
end

# @api private
Expand Down
18 changes: 18 additions & 0 deletions spec/rspec/rails/matchers/have_rendered_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,24 @@ def assert_template(*); raise "oops"; end
end.to raise_exception("oops")
end
end

context "when fails with a redirect" do
let(:response) { ActionController::TestResponse.new(302) }
def assert_template(*)
message = "expecting <'template_name'> but rendering with <[]>"
raise ActiveSupport::TestCase::Assertion.new(message)
end
def normalize_argument_to_redirection(response_redirect_location)
"http://test.host/widgets/1"
end
Copy link
Member

Choose a reason for hiding this comment

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

Instead of def normalize_argument_to_redirection(*) could we update this to def normalize_argument_to_redirection(_descriptive_name_of_what_this_is)?

it "gives informative error message" do
response = ActionController::TestResponse.new(302)
response.location = "http://test.host/widgets/1"
expect do
expect(response).to send(template_expectation, "template_name")
end.to raise_exception("expecting <'template_name'> but was a redirect to <http://test.host/widgets/1>")
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.

❤️ yay for specs!

Copy link
Member

Choose a reason for hiding this comment

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

Normally we'd have spacing around those methods and the it too, feel like cleaning it up @cupakromer ?

Copy link
Member

Choose a reason for hiding this comment

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

Done 😄

Copy link
Member

Choose a reason for hiding this comment

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

❤️

end
end
end