Skip to content

Move the have_rendered matcher to RSpec public API. #1968

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Move the have_rendered matcher to RSpec public API. #1968

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
69 changes: 44 additions & 25 deletions lib/rspec/rails/matchers/have_rendered.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,49 @@ module Matchers
# Matcher for template rendering.
module RenderTemplate
# @private
class RenderTemplateMatcher < RSpec::Matchers::BuiltIn::BaseMatcher
def initialize(scope, expected, message = nil)
@expected = Symbol === expected ? expected.to_s : expected
class RenderTemplateMatcher < RSpec::Matchers::DSL::Matcher
Copy link
Member

Choose a reason for hiding this comment

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

Why are you subclassing RSpec::Matchers::DSL::Matcher? Using the matcher DSL is part of our public API, but directly subclassing this class, and dealing with internals such as what args are passed to its initializer, is not part of our public API. Besides that, it forces you to deal with some weird concerns that don't really belong here (such as name: the DSL matcher needs to know what the matcher method name was, but that's static in your case), and define logic using a weird declarations lambda in initialize, which is far harder to follow than just defining the protocol methods. There's also risk of your instance variables conflicting with the matcher DSLs since they live in the same namespace.

Instead, either use the matcher DSL directly, or define a class that doesn't subclass any rspec-expectations class, and fully implements the matcher protocol.

Copy link
Member Author

Choose a reason for hiding this comment

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

one cannot capture the scope part without directly calling the DSL. My understanding was that inheriting DSL::Matcher was public api. @myronmarston do you have a preferred way to capture the Rails scope given what APIs are available?

Copy link
Member

Choose a reason for hiding this comment

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

one cannot capture the scope part without directly calling the DSL.

That's not true. Consider that this matcher was previously capturing the scope w/o using the DSL at all. And as it happens, RSpec::Matchers::BuiltIn::BaseMatcher doesn't do anything related to the scope.

As far as I can see, the DSL does only 3 things related to the scope:

  1. In the matcher method it defines, it passes self (the scope, as you're calling it) to the constructor when instantiating the matcher. You're doing this part manually in have_rendered and render_template, so you're not even using the DSL for this.
  2. In initialize, the DSL class stores the scope in an instance variable (called @matcher_execution_context). You're doing this part manually do, storing it in a variable called @scope.
  3. The DSL implements respond_to_missing? and method_missing to delegate to the scope so that DSL-defined matchers can call methods defined in the example scope. As far as I can tell, you don't need this, considering the old base class (RSpec::Matchers::BuiltIn::BaseMatcher) did not define this, but if you want it, you can define those methods yourself.

Given you do #1 and #2 yourself already, and it doesn't appear you need #3, I can't see what, if anything, the DSL base class is doing for you with respect to the scope.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use the define_matcher macro to do this without breaking into the class?

def initialize(name, scope, expected, message = nil)
@message = message
@scope = scope
@expected = Symbol === expected ? expected.to_s : expected
@rescued_exception = nil
@redirect_is = nil

declarations = lambda do |_|
match do |_actual|
match_check = match_unless_raises ActiveSupport::TestCase::Assertion do
scope.assert_template @expected, @message
end
check_redirect unless match_check
match_check
end

failure_message do |_|
if @redirect_is
@rescued_exception.message[/(.*?)( but|$)/, 1] +
" but was a redirect to <#{@redirect_is}>"
else
@rescued_exception.message
end
end

failure_message_when_negated do |_|
"expected not to render #{@expected.inspect}, but did"
end
end
super(name, declarations, scope, expected)
end

# matches unless a matcher raises one of a specified set of exceptions
# @api private
def matches?(*)
match_check = match_unless_raises ActiveSupport::TestCase::Assertion do
@scope.assert_template expected, @message
def match_unless_raises(*exceptions)
exceptions.unshift Exception if exceptions.empty?
begin
yield
true
rescue *exceptions => @rescued_exception
false
end
check_redirect unless match_check
match_check
end

# Uses normalize_argument_to_redirection to find and format
Expand All @@ -31,32 +59,23 @@ def check_redirect
return unless response.respond_to?(:redirect?) && response.redirect?
@redirect_is = @scope.send(:normalize_argument_to_redirection, response.location)
end

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

# @api private
def failure_message_when_negated
"expected not to render #{expected.inspect}, but did"
end
end

# Delegates to `assert_template`.
#
# @example
# expect(response).to have_rendered("new")
def have_rendered(options, message = nil)
RenderTemplateMatcher.new(self, options, message)
RenderTemplateMatcher.new("have_rendered", self, options, message)
end

alias_method :render_template, :have_rendered
# Delegates to `assert_template`.
#
# @example
# expect(response).to render_template("new")
def render_template(options, message = nil)
RenderTemplateMatcher.new("render_template", self, options, message)
end
end
end
end
Expand Down