Skip to content

Commit

Permalink
Restrict block matchers use with value expectations
Browse files Browse the repository at this point in the history
https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
https://rspec.rubystyle.guide/#implicit-block-expectations

Spec changes are due to:

    matcher.matches?(invalid_value)

doesn't work with block-only matchers, as no block is passed in, and it
fails with a message:

    1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message`
       Failure/Error: expect(message).to include("detailed inspect")
         expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect"

The redundant (due to existing check in ExpectationTarget) `Proc ===
@event_proc` checks could not be removed safely as well, since
@actual_after is not initialized yet when we haven't executed the block:

     RuntimeError:
       Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized

Also see:
  #1139
  #1125
  • Loading branch information
pirj committed Feb 7, 2021
1 parent f656188 commit 774f332
Show file tree
Hide file tree
Showing 10 changed files with 26 additions and 56 deletions.
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Breaking Changes:
* Remove `include_chain_clauses_in_custom_matcher_descriptions` option
and make it the default. (Phil Pirozhkov, #1279)
* Remove support for present-tense dynamic predicate. (Phil Pirozhkov, #1286)
* Restrict block matchers use with value expectations. (Phil Pirozhkov, #1285)

Enhancements:

Expand Down
17 changes: 7 additions & 10 deletions lib/rspec/expectations/expectation_target.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,16 +111,13 @@ def not_to(matcher=nil, message=nil, &block)
def enforce_value_expectation(matcher)
return if supports_value_expectations?(matcher)

RSpec.deprecate(
"expect(value).to #{RSpec::Support::ObjectFormatter.format(matcher)}",
:message =>
"The implicit block expectation syntax is deprecated, you should pass " \
"a block rather than an argument to `expect` to use the provided " \
"block expectation matcher or the matcher must implement " \
"`supports_value_expectations?`. e.g `expect { value }.to " \
"#{RSpec::Support::ObjectFormatter.format(matcher)}` not " \
"`expect(value).to #{RSpec::Support::ObjectFormatter.format(matcher)}`"
)
raise ArgumentError,
"The implicit block expectation syntax is not supported, you should pass " \
"a block rather than an argument to `expect` to use the provided " \
"block expectation matcher or the matcher must implement " \
"`supports_value_expectations?`. e.g `expect { value }.to " \
"#{RSpec::Support::ObjectFormatter.format(matcher)}` not " \
"`expect(value).to #{RSpec::Support::ObjectFormatter.format(matcher)}`"
end

def supports_value_expectations?(matcher)
Expand Down
3 changes: 0 additions & 3 deletions lib/rspec/matchers/built_in/change.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,10 @@ def raise_block_syntax_error
end

def positive_failure_reason
return "was not given a block" unless Proc === @event_proc
"is still #{@actual_before_description}"
end

def negative_failure_reason
return "was not given a block" unless Proc === @event_proc
"did change from #{@actual_before_description} " \
"to #{description_of change_details.actual_after}"
end
Expand Down Expand Up @@ -171,7 +169,6 @@ def supports_value_expectations?
private

def failure_reason
return "was not given a block" unless Proc === @event_proc
"was changed by #{description_of @change_details.actual_delta}"
end
end
Expand Down
2 changes: 0 additions & 2 deletions lib/rspec/matchers/built_in/output.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,11 @@ def captured?
end

def positive_failure_reason
return "was not a block" unless Proc === @block
return "output #{actual_output_description}" if @expected
"did not"
end

def negative_failure_reason
return "was not a block" unless Proc === @block
"output #{actual_output_description}"
end

Expand Down
1 change: 0 additions & 1 deletion lib/rspec/matchers/built_in/raise_error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ def format_backtrace(backtrace)
end

def given_error
return " but was not given a block" unless Proc === @given_proc
return " but nothing was raised" unless @actual_error

backtrace = format_backtrace(@actual_error.backtrace)
Expand Down
1 change: 0 additions & 1 deletion lib/rspec/matchers/built_in/throw_symbol.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ def expects_call_stack_jump?
private

def actual_result
return "but was not a block" unless Proc === @block
"got #{caught}"
end

Expand Down
29 changes: 5 additions & 24 deletions lib/rspec/matchers/built_in/yield.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ module BuiltIn
class YieldProbe
def self.probe(block, &callback)
probe = new(block, &callback)
return probe unless probe.has_block?
probe.probe
end

Expand All @@ -26,10 +25,6 @@ def initialize(block, &callback)
self.yielded_args = []
end

def has_block?
Proc === @block
end

def probe
assert_valid_expect_block!
@block.call(self)
Expand Down Expand Up @@ -90,13 +85,12 @@ class YieldControl < BaseMatcher
# @private
def matches?(block)
@probe = YieldProbe.probe(block)
return false unless @probe.has_block?
expected_count_matches?(@probe.num_yields)
end

# @private
def does_not_match?(block)
!matches?(block) && @probe.has_block?
!matches?(block)
end

# @api private
Expand Down Expand Up @@ -124,7 +118,6 @@ def supports_value_expectations?
private

def failure_reason
return ' but was not a block' unless @probe.has_block?
return "#{count_expectation_description} but did not yield" if @probe.num_yields == 0
count_failure_reason('yielded')
end
Expand All @@ -137,13 +130,12 @@ class YieldWithNoArgs < BaseMatcher
# @private
def matches?(block)
@probe = YieldProbe.probe(block)
return false unless @probe.has_block?
@probe.yielded_once?(:yield_with_no_args) && @probe.single_yield_args.empty?
end

# @private
def does_not_match?(block)
!matches?(block) && @probe.has_block?
!matches?(block)
end

# @private
Expand All @@ -169,13 +161,11 @@ def supports_value_expectations?
private

def positive_failure_reason
return 'was not a block' unless @probe.has_block?
return 'did not yield' if @probe.num_yields.zero?
"yielded with arguments: #{description_of @probe.single_yield_args}"
end

def negative_failure_reason
return 'was not a block' unless @probe.has_block?
'did'
end
end
Expand All @@ -196,14 +186,13 @@ def matches?(block)
@actual_formatted = actual_formatted
@args_matched_when_yielded &&= args_currently_match?
end
return false unless @probe.has_block?
@probe.probe
@probe.yielded_once?(:yield_with_args) && @args_matched_when_yielded
end

# @private
def does_not_match?(block)
!matches?(block) && @probe.has_block?
!matches?(block)
end

# @private
Expand Down Expand Up @@ -236,7 +225,6 @@ def supports_value_expectations?
private

def positive_failure_reason
return 'was not a block' unless @probe.has_block?
return 'did not yield' if @probe.num_yields.zero?
@positive_args_failure
end
Expand All @@ -246,9 +234,7 @@ def expected_arg_description
end

def negative_failure_reason
if !@probe.has_block?
'was not a block'
elsif @args_matched_when_yielded && !@expected.empty?
if @args_matched_when_yielded && !@expected.empty?
'yielded with expected arguments' \
"\nexpected not: #{surface_descriptions_in(@expected).inspect}" \
"\n got: #{@actual_formatted}"
Expand Down Expand Up @@ -300,12 +286,11 @@ def matches?(block)
yield_count += 1
end

return false unless @probe.has_block?
args_matched_when_yielded && yield_count == @expected.length
end

def does_not_match?(block)
!matches?(block) && @probe.has_block?
!matches?(block)
end

# @private
Expand Down Expand Up @@ -342,16 +327,12 @@ def expected_arg_description
end

def positive_failure_reason
return 'was not a block' unless @probe.has_block?

'yielded with unexpected arguments' \
"\nexpected: #{surface_descriptions_in(@expected).inspect}" \
"\n got: [#{@actual_formatted.join(", ")}]"
end

def negative_failure_reason
return 'was not a block' unless @probe.has_block?

'yielded with expected arguments' \
"\nexpected not: #{surface_descriptions_in(@expected).inspect}" \
"\n got: [#{@actual_formatted.join(", ")}]"
Expand Down
2 changes: 1 addition & 1 deletion spec/rspec/matchers/built_in/change_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1038,7 +1038,7 @@ def invalid_block
end

RSpec.describe RSpec::Matchers::BuiltIn::ChangeRelatively do
it_behaves_like "an RSpec block-only matcher", :disallows_negation => true, :skip_deprecation_check => true do
it_behaves_like "an RSpec block-only matcher", :disallows_negation => true do
let(:matcher) { change { @k }.by(1) }
before { @k = 0 }
def valid_block
Expand Down
4 changes: 2 additions & 2 deletions spec/rspec/matchers/built_in/yield_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ def each_arg(*args, &block)
# this example group overrides the default definition of expectations
# and lambdas that take the expectation target in a way that they accept
# a probe.
RSpec.shared_examples "an RSpec probe-yielding block-only matcher" do |*options|
include_examples "an RSpec block-only matcher", { :expects_lambda => true }.merge(options.first || {}) do
RSpec.shared_examples "an RSpec probe-yielding block-only matcher" do |**options|
include_examples "an RSpec block-only matcher", **options do
let(:valid_expectation) { expect { |block| valid_block(&block) } }
let(:invalid_expectation) { expect { |block| invalid_block(&block) } }

Expand Down
22 changes: 10 additions & 12 deletions spec/support/shared_examples/block_matcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,27 +69,25 @@
it 'fails gracefully when given a value' do
expect {
expect(3).to matcher
}.to fail_with(/was not( given)? a block/)

}.to raise_error(/implicit block expectation syntax is not supported/)
unless options[:disallows_negation]
expect {
expect(3).not_to matcher
}.to fail_with(/was not( given)? a block/)
}.to raise_error(/implicit block expectation syntax is not supported/)
end
end

it 'prints a deprecation warning when given a value' do
expect_warn_deprecation(/The implicit block expectation syntax is deprecated, you should pass/)
expect { expect(3).to matcher }.to fail
end unless options[:skip_deprecation_check] || options[:expects_lambda]
expect { expect(3).to matcher }
.to raise_error(/implicit block expectation syntax is not supported/)
end

it 'prints a deprecation warning when given a value and negated' do
expect_warn_deprecation(/The implicit block expectation syntax is deprecated, you should pass/)
expect { expect(3).not_to matcher }.to fail
end unless options[:disallows_negation] || options[:expects_lambda]
expect { expect(3).not_to matcher }
.to raise_error(/implicit block expectation syntax is not supported/)
end unless options[:disallows_negation]

it 'allows lambda expectation target' do
allow_deprecation
expect(valid_block_lambda).to matcher
it 'allows a Proc for an expectation target' do
expect(&valid_block_lambda).to matcher
end
end

0 comments on commit 774f332

Please sign in to comment.