Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for #230 #243

Closed
wants to merge 2 commits into from

4 participants

Myron Marston David Chelimsky alindeman Loren Segal
Myron Marston

Clarify relationship between different method stub actions.

In 2.13.0 only `and_yield` and `and_return` could be combined, since
that was the only combination case that was specified by an example.
This was a regression, as reported by a user in #230.

I tried here to fully specify all of the various combinations of stub
actions. Notes:

* `and_return`, `and_raise` and `and_throw` are "terminal" actions
  in the sense that they terminate the method. They _must_ happen last
  and it is impossible to support more than one of these. Hence, we allow
  only one of these, and allow them to be overridden. We also return `nil`
  from these methods to discourage further stub configuration.
* `and_call_original` is a special case that doesn't make sense to be
  combined with any of the others. Once you've set it up, this causes
  any further instructions to raise an error.
* `and_yield` is treated as an "initial" action. Yielding doesn't exit
  a method the way the terminal actions do. It is the only initial action.
  Calling it multiple times sets up multiple yields.
* Setting a block implementation (possible by passing a block to almost
  any method on the fluent interface) sets the block as the "inner" action.
  It runs between the configured yields (if there are any) and the configured
  terminal action (if there is one). My thinking here is that in many cases,
  users use a block implementation to specify a return value, essentially
  making it like a terminal action (so it should come after the `and_yield`
  actions), but in other cases, the user may just use a block for a side
  effect, and may configure an terminal action as well. Only one block
  implementation is supported and it can be overridden.

@lsegal -- want to try this to see if it solves the problem you reported in #230 (I believe it will). If you have any feedback about the PR I'd love to hear.

@dchelimsky / @alindeman -- it'd be great to get a code review from one of you before I merge this.

Myron Marston myronmarston referenced this pull request
Closed

Regression from #245 #251

Myron Marston
Owner

I think the perf hit is the price of not being lazy. Spent some time on this yesterday but didn't come up w/ a universally workable solution (i.e. solved some problems but not all).

Is this comment intended for #264?

David Chelimsky
Owner

What comment? ;)

alindeman
Collaborator

This is really elegant in my opinion. Nice use of polymorphism. I'm :+1: to merge.

myronmarston added some commits
Myron Marston myronmarston Refactor implementation pieces in prep for #230.
This splits each implementation piece into a separate object
so that it is easier to combine them.
11e7e68
Myron Marston myronmarston Clarify relationship between different method stub actions.
In 2.13.0 only `and_yield` and `and_return` could be combined, since
that was the only combination case that was specified by an example.
This was a regression, as reported by a user in #230.

I tried here to fully specify all of the various combinations of stub 
actions. Notes:

* `and_return`, `and_raise` and `and_throw` are "terminal" actions
  in the sense that they terminate the method. They _must_ happen last
  and it is impossible to support more than one of these. Hence, we allow
  only one of these, and allow them to be overridden. We also return `nil`
  from these methods to discourage further stub configuration.
* `and_call_original` is a special case that doesn't make sense to be
  combined with any of the others. Once you've set it up, this causes
  any further instructions to raise an error.
* `and_yield` is treated as an "initial" action. Yielding doesn't exit
  a method the way the terminal actions do. It is the only initial action.
  Calling it multiple times sets up multiple yields.
* Setting a block implementation (possible by passing a block to almost
  any method on the fluent interface) sets the block as the "inner" action.
  It runs between the configured yields (if there are any) and the configured
  terminal action (if there is one). My thinking here is that in many cases,
  users use a block implementation to specify a return value, essentially
  making it like a terminal action (so it should come after the `and_yield`
  actions), but in other cases, the user may just use a block for a side
  effect, and may configure an terminal action as well. Only one block
  implementation is supported and it can be overridden.
c0ee734
Myron Marston
Owner

I rebased and merged this manually since it had gotten out of date and couldn't be merged on the github UI.

Loren Segal

Great! Sorry we didn't have a chance to check it out on the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2013
  1. Myron Marston

    Refactor implementation pieces in prep for #230.

    myronmarston authored
    This splits each implementation piece into a separate object
    so that it is easier to combine them.
  2. Myron Marston

    Clarify relationship between different method stub actions.

    myronmarston authored
    In 2.13.0 only `and_yield` and `and_return` could be combined, since
    that was the only combination case that was specified by an example.
    This was a regression, as reported by a user in #230.
    
    I tried here to fully specify all of the various combinations of stub 
    actions. Notes:
    
    * `and_return`, `and_raise` and `and_throw` are "terminal" actions
      in the sense that they terminate the method. They _must_ happen last
      and it is impossible to support more than one of these. Hence, we allow
      only one of these, and allow them to be overridden. We also return `nil`
      from these methods to discourage further stub configuration.
    * `and_call_original` is a special case that doesn't make sense to be
      combined with any of the others. Once you've set it up, this causes
      any further instructions to raise an error.
    * `and_yield` is treated as an "initial" action. Yielding doesn't exit
      a method the way the terminal actions do. It is the only initial action.
      Calling it multiple times sets up multiple yields.
    * Setting a block implementation (possible by passing a block to almost
      any method on the fluent interface) sets the block as the "inner" action.
      It runs between the configured yields (if there are any) and the configured
      terminal action (if there is one). My thinking here is that in many cases,
      users use a block implementation to specify a return value, essentially
      making it like a terminal action (so it should come after the `and_yield`
      actions), but in other cases, the user may just use a block for a side
      effect, and may configure an terminal action as well. Only one block
      implementation is supported and it can be overridden.
This page is out of date. Refresh to see the latest.
2  Changelog.md
View
@@ -26,6 +26,8 @@ Bug fixes
when the wrong number of args are passed (Jon Rowe).
* Fix `double` on 1.9.2 so you can wrap them in an Array
using `Array(my_double)` (Jon Rowe).
+* Allow a block implementation to be used in combination with
+ `and_yield`, `and_raise`, `and_return` or `and_throw` (Myron Marston).
Deprecations
158 lib/rspec/mocks/message_expectation.rb
View
@@ -10,7 +10,7 @@ class MessageExpectation
# @private
def initialize(error_generator, expectation_ordering, expected_from, method_double,
- expected_received_count=1, opts={}, &implementation)
+ expected_received_count=1, opts={}, &implementation_block)
@error_generator = error_generator
@error_generator.opts = opts
@expected_from = expected_from
@@ -24,8 +24,9 @@ def initialize(error_generator, expectation_ordering, expected_from, method_doub
@args_to_yield = []
@failed_fast = nil
@eval_context = nil
- @implementation = implementation
- @values_to_return = nil
+
+ @implementation = Implementation.new
+ self.inner_implementation_action = implementation_block
end
# @private
@@ -76,11 +77,12 @@ def and_return(*values, &implementation)
if implementation
# TODO: deprecate `and_return { value }`
- @implementation = implementation
+ self.inner_implementation_action = implementation
else
- @values_to_return = values
- @implementation = build_implementation
+ self.terminal_implementation_action = AndReturnImplementation.new(values)
end
+
+ nil
end
# Tells the object to delegate to the original unmodified method
@@ -98,7 +100,7 @@ def and_call_original
if @method_double.object.is_a?(RSpec::Mocks::TestDouble)
@error_generator.raise_only_valid_on_a_partial_mock(:and_call_original)
else
- @implementation = @method_double.original_method
+ @implementation = AndCallOriginalImplementation.new(@method_double.original_method)
end
end
@@ -128,7 +130,8 @@ def and_raise(exception = RuntimeError, message = nil)
exception = message ? exception.exception(message) : exception.exception
end
- @implementation = Proc.new { raise exception }
+ self.terminal_implementation_action = Proc.new { raise exception }
+ nil
end
# @overload and_throw(symbol)
@@ -142,7 +145,8 @@ def and_raise(exception = RuntimeError, message = nil)
# car.stub(:go).and_throw(:out_of_gas)
# car.stub(:go).and_throw(:out_of_gas, :level => 0.1)
def and_throw(*args)
- @implementation = Proc.new { throw(*args) }
+ self.terminal_implementation_action = Proc.new { throw(*args) }
+ nil
end
# Tells the object to yield one or more args to a block when the message
@@ -154,7 +158,7 @@ def and_throw(*args)
def and_yield(*args, &block)
yield @eval_context = Object.new.extend(RSpec::Mocks::InstanceExec) if block
@args_to_yield << args
- @implementation = build_implementation
+ self.initial_implementation_action = AndYieldImplementation.new(@args_to_yield, @eval_context, @error_generator)
self
end
@@ -174,8 +178,8 @@ def invoke(parent_stub, *args, &block)
@order_group.handle_order_constraint self
begin
- if @implementation
- @implementation.call(*args, &block)
+ if implementation.present?
+ implementation.call(*args, &block)
elsif parent_stub
parent_stub.invoke(nil, *args, &block)
end
@@ -284,7 +288,7 @@ def raise_out_of_order_error
# cart.add(Book.new(:isbn => 1934356379))
# # => passes
def with(*args, &block)
- @implementation = block if block_given? unless args.empty?
+ self.inner_implementation_action = block if block_given? unless args.empty?
@argument_list_matcher = ArgumentListMatcher.new(*args, &block)
self
end
@@ -296,7 +300,7 @@ def with(*args, &block)
#
# dealer.should_receive(:deal_card).exactly(10).times
def exactly(n, &block)
- @implementation = block if block
+ self.inner_implementation_action = block
set_expected_received_count :exactly, n
self
end
@@ -308,7 +312,7 @@ def exactly(n, &block)
#
# dealer.should_receive(:deal_card).at_least(9).times
def at_least(n, &block)
- @implementation = block if block
+ self.inner_implementation_action = block
set_expected_received_count :at_least, n
self
end
@@ -320,7 +324,7 @@ def at_least(n, &block)
#
# dealer.should_receive(:deal_card).at_most(10).times
def at_most(n, &block)
- @implementation = block if block
+ self.inner_implementation_action = block
set_expected_received_count :at_most, n
self
end
@@ -333,14 +337,14 @@ def at_most(n, &block)
# dealer.should_receive(:deal_card).at_least(10).times
# dealer.should_receive(:deal_card).at_most(10).times
def times(&block)
- @implementation = block if block
+ self.inner_implementation_action = block
self
end
# Allows an expected message to be received any number of times.
def any_number_of_times(&block)
- @implementation = block if block
+ self.inner_implementation_action = block
@expected_received_count = :any
self
end
@@ -361,7 +365,7 @@ def never
#
# car.should_receive(:go).once
def once(&block)
- @implementation = block if block
+ self.inner_implementation_action = block
set_expected_received_count :exactly, 1
self
end
@@ -372,7 +376,7 @@ def once(&block)
#
# car.should_receive(:go).twice
def twice(&block)
- @implementation = block if block
+ self.inner_implementation_action = block
set_expected_received_count :exactly, 2
self
end
@@ -385,7 +389,7 @@ def twice(&block)
# api.should_receive(:run).ordered
# api.should_receive(:finish).ordered
def ordered(&block)
- @implementation = block if block
+ self.inner_implementation_action = block
@order_group.register(self)
@ordered = true
self
@@ -406,7 +410,7 @@ def increase_actual_received_count!
@actual_received_count += 1
end
- protected
+ private
def failed_fast?
@failed_fast
@@ -423,13 +427,16 @@ def set_expected_received_count(relativity, n)
end
end
- private
+ def initial_implementation_action=(action)
+ implementation.initial_action = action
+ end
+
+ def inner_implementation_action=(action)
+ implementation.inner_action = action if action
+ end
- def build_implementation
- Implementation.new(
- @values_to_return, @args_to_yield,
- @eval_context, @error_generator
- ).method(:call)
+ def terminal_implementation_action=(action)
+ implementation.terminal_action = action
end
end
@@ -455,29 +462,16 @@ def negative_expectation_for?(message)
end
end
- # Represents a configured implementation. Takes into account
- # `and_return` and `and_yield` instructions.
+ # Handles the implementation of an `and_yield` declaration.
# @private
- class Implementation
- def initialize(values_to_return, args_to_yield, eval_context, error_generator)
- @values_to_return = values_to_return
+ class AndYieldImplementation
+ def initialize(args_to_yield, eval_context, error_generator)
@args_to_yield = args_to_yield
@eval_context = eval_context
@error_generator = error_generator
end
def call(*args_to_ignore, &block)
- default_return_value = perform_yield(&block)
- return default_return_value unless @values_to_return
-
- if @values_to_return.size > 1
- @values_to_return.shift
- else
- @values_to_return.first
- end
- end
-
- def perform_yield(&block)
return if @args_to_yield.empty? && @eval_context.nil?
@error_generator.raise_missing_block_error @args_to_yield unless block
@@ -491,5 +485,81 @@ def perform_yield(&block)
value
end
end
+
+ # Handles the implementation of an `and_return` implementation.
+ # @private
+ class AndReturnImplementation
+ def initialize(values_to_return)
+ @values_to_return = values_to_return
+ end
+
+ def call(*args_to_ignore, &block)
+ if @values_to_return.size > 1
+ @values_to_return.shift
+ else
+ @values_to_return.first
+ end
+ end
+ end
+
+ # Represents a configured implementation. Takes into account
+ # any number of sub-implementations.
+ # @private
+ class Implementation
+ attr_accessor :initial_action, :inner_action, :terminal_action
+
+ def call(*args, &block)
+ actions.map do |action|
+ action.call(*args, &block)
+ end.last
+ end
+
+ def present?
+ actions.any?
+ end
+
+ private
+
+ def actions
+ [initial_action, inner_action, terminal_action].compact
+ end
+ end
+
+ # Represents an `and_call_original` implementation.
+ # @private
+ class AndCallOriginalImplementation
+ def initialize(method)
+ @method = method
+ end
+
+ CannotModifyFurtherError = Class.new(StandardError)
+
+ def initial_action=(value)
+ raise cannot_modify_further_error
+ end
+
+ def inner_action=(value)
+ raise cannot_modify_further_error
+ end
+
+ def terminal_action=(value)
+ raise cannot_modify_further_error
+ end
+
+ def present?
+ true
+ end
+
+ def call(*args, &block)
+ @method.call(*args, &block)
+ end
+
+ private
+
+ def cannot_modify_further_error
+ CannotModifyFurtherError.new "This method has already been configured " +
+ "to call the original implementation, and cannot be modified further."
+ end
+ end
end
end
197 spec/rspec/mocks/combining_implementation_instructions_spec.rb
View
@@ -0,0 +1,197 @@
+require 'spec_helper'
+
+module RSpec
+ module Mocks
+ describe "Combining implementation instructions" do
+ it 'can combine and_yield and and_return' do
+ dbl = double
+ dbl.stub(:foo).and_yield(5).and_return(3)
+
+ expect { |b|
+ expect(dbl.foo(&b)).to eq(3)
+ }.to yield_with_args(5)
+ end
+
+ describe "combining and_yield, a block implementation and and_return" do
+ def verify_combined_implementation
+ dbl = double
+ (yield dbl).and_yield(5).and_return(3)
+
+ expect { |b|
+ expect(dbl.foo(:arg, &b)).to eq(3)
+ }.to yield_with_args(5)
+
+ expect(@block_called).to be_true
+ end
+
+ it 'works when passing a block to `stub`' do
+ verify_combined_implementation do |dbl|
+ dbl.stub(:foo) { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `with`' do
+ verify_combined_implementation do |dbl|
+ dbl.stub(:foo).with(:arg) { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `exactly`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).exactly(:once) { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `at_least`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).at_least(:once) { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `at_most`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).at_most(:once) { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `times`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).exactly(1).times { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `any_number_of_times`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).any_number_of_times { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `once`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).once { @block_called = true }
+ end
+ end
+
+ it 'works when passing a block to `twice`' do
+ the_double = nil
+
+ verify_combined_implementation do |dbl|
+ the_double = dbl
+ dbl.should_receive(:foo).twice { @block_called = true }
+ end
+
+ the_double.foo { |a| } # to ensure it is called twice
+ end
+
+ it 'works when passing a block to `ordered`' do
+ verify_combined_implementation do |dbl|
+ dbl.should_receive(:foo).ordered { @block_called = true }
+ end
+ end
+ end
+
+ it 'can combine and_yield and and_return with a block' do
+ dbl = double
+ dbl.stub(:foo).and_yield(5).and_return { :return }
+
+ expect { |b|
+ expect(dbl.foo(&b)).to eq(:return)
+ }.to yield_with_args(5)
+ end
+
+ it 'can combine and_yield and and_raise' do
+ dbl = double
+ dbl.stub(:foo).and_yield(5).and_raise("boom")
+
+ expect { |b|
+ expect { dbl.foo(&b) }.to raise_error("boom")
+ }.to yield_with_args(5)
+ end
+
+ it 'can combine and_yield, a block implementation and and_raise' do
+ dbl = double
+ block_called = false
+ dbl.stub(:foo) { block_called = true }.and_yield(5).and_raise("boom")
+
+ expect { |b|
+ expect { dbl.foo(&b) }.to raise_error("boom")
+ }.to yield_with_args(5)
+
+ expect(block_called).to be_true
+ end
+
+ it 'can combine and_yield and and_throw' do
+ dbl = double
+ dbl.stub(:foo).and_yield(5).and_throw(:bar)
+
+ expect { |b|
+ expect { dbl.foo(&b) }.to throw_symbol(:bar)
+ }.to yield_with_args(5)
+ end
+
+ it 'can combine and_yield, a block implementation and and_throw' do
+ dbl = double
+ block_called = false
+ dbl.stub(:foo) { block_called = true }.and_yield(5).and_throw(:bar)
+
+ expect { |b|
+ expect { dbl.foo(&b) }.to throw_symbol(:bar)
+ }.to yield_with_args(5)
+
+ expect(block_called).to be_true
+ end
+
+ it 'returns `nil` from all terminal actions to discourage further configuration' do
+ expect(double.stub(:foo).and_return(1)).to be_nil
+ expect(double.stub(:foo).and_raise("boom")).to be_nil
+ expect(double.stub(:foo).and_throw(:foo)).to be_nil
+ end
+
+ it 'allows the terminal action to be overriden' do
+ dbl = double
+ stubbed_double = dbl.stub(:foo)
+
+ stubbed_double.and_return(1)
+ expect(dbl.foo).to eq(1)
+
+ stubbed_double.and_return(3)
+ expect(dbl.foo).to eq(3)
+
+ stubbed_double.and_raise("boom")
+ expect { dbl.foo }.to raise_error("boom")
+
+ stubbed_double.and_throw(:bar)
+ expect { dbl.foo }.to throw_symbol(:bar)
+ end
+
+ it 'allows the inner implementation block to be overriden' do
+ dbl = double
+ stubbed_double = dbl.stub(:foo)
+
+ stubbed_double.with(:arg) { :with_block }
+ expect(dbl.foo(:arg)).to eq(:with_block)
+
+ stubbed_double.at_least(:once) { :at_least_block }
+ expect(dbl.foo(:arg)).to eq(:at_least_block)
+ end
+
+ it 'raises an error if `and_call_original` is followed by any other instructions' do
+ dbl = [1, 2, 3]
+ stubbed = dbl.stub(:size)
+ stubbed.and_call_original
+
+ msg_fragment = /cannot be modified further/
+
+ expect { stubbed.and_yield }.to raise_error(msg_fragment)
+ expect { stubbed.and_return(1) }.to raise_error(msg_fragment)
+ expect { stubbed.and_raise("a") }.to raise_error(msg_fragment)
+ expect { stubbed.and_throw(:bar) }.to raise_error(msg_fragment)
+
+ expect { stubbed.once { } }.to raise_error(msg_fragment)
+
+ expect(dbl.size).to eq(3)
+ end
+ end
+ end
+end
+
Something went wrong with that request. Please try again.