Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Correctly unstubs methods stubbed with `#any_instance`

* Instances stubbed with `#any_instance` would not be usable after the
  test finished because `MethodDouble` would stash the implementation of
  the method already overridden by `AnyInstance::Recorder`. When the
  test finished, that implementation would be restored on the object's
  singleton class, and any future calls to it would blow up with a stack
  overflow.
* This fix only stashes methods if they are defined on the object's
  singleton class to begin with; `AnyInstance::Recorder` defines a
  method on the object's class so that method will not be stashed.
* If there is no method on the object's singleton class, RSpec can
  safely define one there without stashing the original implementation.
  At the end of the test, the method is simply removed entirely from the
  singleton class. Any original implementation defined in the object's
  ancestor chain will show through again.
* This issue cannot be fixed on MRI 1.8.6 because it does not support
  `Method#owner`. However, `#any_instance` itself is not supported on
  1.8.6 for the same reason. The fix should not negatively affect 1.8.6,
  though, because the fallback behavior is to stash the method in all
  cases (which was the original behavior).

* This commit also refactors the stashing behavior out into its own
  object. While not explicitly necessary, it helped me reason about the
  fix much easier than when all the responsibility was in `MethodDouble`
  (which also has other responsibilities).

* Fixes #167
* Closes #182
  • Loading branch information...
commit a727464658e4c3f08dcff9c62814ade3c5852d73 1 parent 645b158
@alindeman alindeman authored
View
1  lib/rspec/mocks/framework.rb
@@ -3,6 +3,7 @@
# object in the system.
require 'rspec/mocks/extensions/instance_exec'
+require 'rspec/mocks/stashed_instance_method'
require 'rspec/mocks/method_double'
require 'rspec/mocks/methods'
require 'rspec/mocks/argument_matchers'
View
76 lib/rspec/mocks/method_double.rb
@@ -10,7 +10,9 @@ def initialize(object, method_name, proxy)
@method_name = method_name
@object = object
@proxy = proxy
- @stashed = false
+
+ @stashed_method = StashedInstanceMethod.new(object_singleton_class, @method_name)
+ @method_is_proxied = false
store(:expectations, [])
store(:stubs, [])
end
@@ -44,72 +46,38 @@ class << @object; self; end
end
# @private
- def obfuscate(method_name)
- "obfuscated_by_rspec_mocks__#{method_name}"
- end
-
- # @private
- def stashed_method_name
- obfuscate(method_name)
- end
-
- # @private
- def object_responds_to?(method_name)
- if @proxy.already_proxied_respond_to?
- @object.__send__(obfuscate(:respond_to?), method_name)
- elsif method_name == :respond_to?
- @proxy.already_proxied_respond_to
- else
- @object.respond_to?(method_name, true)
- end
- end
-
- # @private
def configure_method
RSpec::Mocks::space.add(@object) if RSpec::Mocks::space
warn_if_nil_class
- unless @stashed
- stash_original_method
- define_proxy_method
- end
- end
-
- # @private
- def stash_original_method
- stashed = stashed_method_name
- orig = @method_name
- object_singleton_class.class_eval do
- alias_method(stashed, orig) if method_defined?(orig) || private_method_defined?(orig)
- end
- @stashed = true
+ @stashed_method.stash
+ define_proxy_method
end
# @private
def define_proxy_method
- method_name = @method_name
- visibility_for_method = "#{visibility} :#{method_name}"
- object_singleton_class.class_eval(<<-EOF, __FILE__, __LINE__)
- def #{method_name}(*args, &block)
- __mock_proxy.message_received :#{method_name}, *args, &block
+ return if @method_is_proxied
+
+ object_singleton_class.class_eval <<-EOF, __FILE__, __LINE__ + 1
+ def #{@method_name}(*args, &block)
+ __mock_proxy.message_received :#{@method_name}, *args, &block
end
- #{visibility_for_method}
+ #{visibility_for_method}
EOF
+ @method_is_proxied = true
+ end
+
+ # @private
+ def visibility_for_method
+ "#{visibility} :#{method_name}"
end
# @private
def restore_original_method
- if @stashed
- method_name = @method_name
- stashed_method_name = self.stashed_method_name
- object_singleton_class.instance_eval do
- remove_method method_name
- if method_defined?(stashed_method_name) || private_method_defined?(stashed_method_name)
- alias_method method_name, stashed_method_name
- remove_method stashed_method_name
- end
- end
- @stashed = false
- end
+ return unless @method_is_proxied
+
+ object_singleton_class.__send__(:remove_method, @method_name)
+ @stashed_method.restore
+ @method_is_proxied = false
end
# @private
View
60 lib/rspec/mocks/stashed_instance_method.rb
@@ -0,0 +1,60 @@
+# @private
+class StashedInstanceMethod
+ def initialize(klass, method)
+ @klass = klass
+ @method = method
+
+ @method_is_stashed = false
+ end
+
+ # @private
+ def stash
+ return if !method_defined_directly_on_klass? || @method_is_stashed
+
+ @klass.__send__(:alias_method, stashed_method_name, @method)
+ @method_is_stashed = true
+ end
+
+ private
+
+ # @private
+ def method_defined_directly_on_klass?
+ method_defined_on_klass? && method_owned_by_klass?
+ end
+
+ # @private
+ def method_defined_on_klass?
+ @klass.method_defined?(@method) || @klass.private_method_defined?(@method)
+ end
+
+ if ::UnboundMethod.method_defined?(:owner)
+ # @private
+ def method_owned_by_klass?
+ @klass.instance_method(@method).owner == @klass
+ end
+ else
+ # @private
+ def method_owned_by_klass?
+ # On 1.8.6, which does not support Method#owner, we have no choice but
+ # to assume it's defined on the klass even if it may be defined on
+ # a superclass.
+ true
+ end
+ end
+
+ # @private
+ def stashed_method_name
+ "obfuscated_by_rspec_mocks__#{@method}"
+ end
+
+ public
+
+ # @private
+ def restore
+ return unless @method_is_stashed
+
+ @klass.__send__(:alias_method, @method, stashed_method_name)
+ @klass.__send__(:remove_method, stashed_method_name)
+ @method_is_stashed = false
+ end
+end
View
13 spec/rspec/mocks/any_instance_spec.rb
@@ -859,6 +859,19 @@ class RSpec::SampleRspecTestClass;end
http_request_class.new.existing_method.should == "foo"
end
end
+
+ context "when used after the test has finished" do
+ it "restores the original behavior of a stubbed method" do
+ klass.any_instance.stub(:existing_method).and_return(:stubbed_return_value)
+
+ instance = klass.new
+ instance.existing_method.should == :stubbed_return_value
+
+ RSpec::Mocks.verify
+
+ instance.existing_method.should == :existing_method_return_value
+ end
+ end
end
end
end
View
53 spec/rspec/mocks/stashed_instance_method_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe StashedInstanceMethod do
+ class ExampleClass
+ def hello
+ :hello_defined_on_class
+ end
+ end
+
+ def singleton_class_for(obj)
+ class << obj; self; end
+ end
+
+ it "stashes the current implementation of an instance method so it can be temporarily replaced" do
+ obj = Object.new
+ def obj.hello; :hello_defined_on_singleton_class; end;
+
+ stashed_method = StashedInstanceMethod.new(singleton_class_for(obj), :hello)
+ stashed_method.stash
+
+ def obj.hello; :overridden_hello; end
+ expect(obj.hello).to eql :overridden_hello
+
+ stashed_method.restore
+ expect(obj.hello).to eql :hello_defined_on_singleton_class
+ end
+
+ it "stashes private instance methods" do
+ obj = Object.new
+ def obj.hello; :hello_defined_on_singleton_class; end;
+ singleton_class_for(obj).__send__(:private, :hello)
+
+ stashed_method = StashedInstanceMethod.new(singleton_class_for(obj), :hello)
+ stashed_method.stash
+
+ def obj.hello; :overridden_hello; end
+ stashed_method.restore
+ expect(obj.send(:hello)).to eql :hello_defined_on_singleton_class
+ end
+
+ it "only stashes methods directly defined on the given class, not its ancestors" do
+ obj = ExampleClass.new
+
+ stashed_method = StashedInstanceMethod.new(singleton_class_for(obj), :hello)
+ stashed_method.stash
+
+ def obj.hello; :overridden_hello; end;
+ expect(obj.hello).to eql :overridden_hello
+
+ stashed_method.restore
+ expect(obj.hello).to eql :overridden_hello
+ end
+end

6 comments on commit a727464

@jonhyman

There's a regression here. Check out the behavior of doing:

User.stub(:where)
User.stub(:where)
User.unstub(:where)
User.where

Bad behavior is in 328bd5d
Good behavior is in 645b158


Bad behavior, note that #where becomes undefined:

1.9.3p194 :001 > RSpec::Mocks::setup(Object.new)
 => #<RSpec::Mocks::Space:0x007fbd13067bb0> 
1.9.3p194 :002 > User.stub(:where)
 => #<RSpec::Mocks::MessageExpectation:0x007fbd13072df8 @error_generator=#<RSpec::Mocks::ErrorGenerator:0x007fbd13069d20 @declared_as="Mock", @target=User, @name=nil, @opts={}>, @expected_from="(irb):2:in `irb_binding'", @message=:where, @actual_received_count=0, @expected_received_count=:any, @argument_list_matcher=#<RSpec::Mocks::ArgumentListMatcher:0x007fbd13072da8 @expected_args=[#<RSpec::Mocks::ArgumentMatchers::AnyArgsMatcher:0x007fbd13072dd0>], @block=nil, @match_any_args=true, @matchers=nil>, @consecutive=false, @exception_to_raise=nil, @args_to_throw=[], @order_group=#<RSpec::Mocks::OrderGroup:0x007fbd13069cd0 @ordering=[]>, @exactly=nil, @at_most=nil, @at_least=nil, @args_to_yield=[], @failed_fast=nil, @args_to_yield_were_cloned=false, @eval_context=nil, @implementation=nil> 
1.9.3p194 :003 > User.stub(:where)
 => #<RSpec::Mocks::MessageExpectation:0x007fbd1307fd78 @error_generator=#<RSpec::Mocks::ErrorGenerator:0x007fbd13069d20 @declared_as="Mock", @target=User, @name=nil, @opts={}>, @expected_from="(irb):3:in `irb_binding'", @message=:where, @actual_received_count=0, @expected_received_count=:any, @argument_list_matcher=#<RSpec::Mocks::ArgumentListMatcher:0x007fbd1307fd28 @expected_args=[#<RSpec::Mocks::ArgumentMatchers::AnyArgsMatcher:0x007fbd1307fd50>], @block=nil, @match_any_args=true, @matchers=nil>, @consecutive=false, @exception_to_raise=nil, @args_to_throw=[], @order_group=#<RSpec::Mocks::OrderGroup:0x007fbd13069cd0 @ordering=[]>, @exactly=nil, @at_most=nil, @at_least=nil, @args_to_yield=[], @failed_fast=nil, @args_to_yield_were_cloned=false, @eval_context=nil, @implementation=nil> 
1.9.3p194 :004 > User.where
 => nil 
1.9.3p194 :005 > User.unstub(:where)
 => [] 
1.9.3p194 :006 > User.where
NoMethodError: undefined method `where' for Object:Class
  from /Users/jonathan/.rvm/gems/ruby-1.9.3-p194/gems/attr_encrypted-1.2.0/lib/attr_encrypted.rb:229:in `method_missing'
  from /Users/jonathan/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rspec-mocks-328bd5df9994/lib/rspec/mocks/proxy.rb:141:in `message_received'
  from /Users/jonathan/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rspec-mocks-328bd5df9994/lib/rspec/mocks/method_double.rb:62:in `where'
  from (irb):6
  from /Users/jonathan/.rvm/gems/ruby-1.9.3-p194/gems/railties-3.2.8/lib/rails/commands/console.rb:47:in `start'
  from /Users/jonathan/.rvm/gems/ruby-1.9.3-p194/gems/railties-3.2.8/lib/rails/commands/console.rb:8:in `start'
  from /Users/jonathan/.rvm/gems/ruby-1.9.3-p194/gems/railties-3.2.8/lib/rails/commands.rb:41:in `<top (required)>'
  from script/rails:6:in `require'
  from script/rails:6:in `<main>'

Good behavior:

1.9.3p194 :002 > RSpec::Mocks::setup(Object.new)
 => #<RSpec::Mocks::Space:0x007fdd62135ca8> 
1.9.3p194 :003 > User.stub(:where)
 => #<RSpec::Mocks::MessageExpectation:0x007fdd62142228 @error_generator=#<RSpec::Mocks::ErrorGenerator:0x007fdd62143e98 @declared_as="Mock", @target=User, @name=nil, @opts={}>, @expected_from="(irb):3:in `irb_binding'", @message=:where, @actual_received_count=0, @expected_received_count=:any, @argument_list_matcher=#<RSpec::Mocks::ArgumentListMatcher:0x007fdd62142110 @expected_args=[#<RSpec::Mocks::ArgumentMatchers::AnyArgsMatcher:0x007fdd62142138>], @block=nil, @match_any_args=true, @matchers=nil>, @consecutive=false, @exception_to_raise=nil, @args_to_throw=[], @order_group=#<RSpec::Mocks::OrderGroup:0x007fdd62143e20 @ordering=[]>, @exactly=nil, @at_most=nil, @at_least=nil, @args_to_yield=[], @failed_fast=nil, @args_to_yield_were_cloned=false, @eval_context=nil, @implementation=nil> 
1.9.3p194 :004 > User.stub(:where)
 => #<RSpec::Mocks::MessageExpectation:0x007fdd6214eaf0 @error_generator=#<RSpec::Mocks::ErrorGenerator:0x007fdd62143e98 @declared_as="Mock", @target=User, @name=nil, @opts={}>, @expected_from="(irb):4:in `irb_binding'", @message=:where, @actual_received_count=0, @expected_received_count=:any, @argument_list_matcher=#<RSpec::Mocks::ArgumentListMatcher:0x007fdd6214eaa0 @expected_args=[#<RSpec::Mocks::ArgumentMatchers::AnyArgsMatcher:0x007fdd6214eac8>], @block=nil, @match_any_args=true, @matchers=nil>, @consecutive=false, @exception_to_raise=nil, @args_to_throw=[], @order_group=#<RSpec::Mocks::OrderGroup:0x007fdd62143e20 @ordering=[]>, @exactly=nil, @at_most=nil, @at_least=nil, @args_to_yield=[], @failed_fast=nil, @args_to_yield_were_cloned=false, @eval_context=nil, @implementation=nil> 
1.9.3p194 :005 > User.unstub(:where)
 => [] 
1.9.3p194 :006 > User.where
 => #<Mongoid::Criteria
  selector: {},
  options:  {},
  class:    User,
  embedded: false>
@alindeman
Collaborator

Thank you. I'll dig into this ASAP.

@myronmarston

@alindeman -- I just ran into this in a project when I updated my bundle. I started to take a look at it and came up with a failing spec. It's in the method_wrongly_removed_when_stubbed_twice_bug branch:

93ef54b

Feel free to use this when you tackle this.

@alindeman
Collaborator

Thank you @myronmarston. That would have been my first step, so you've definitely saved me some time.

@alindeman
Collaborator

@jonhyman, @myronmarston: I pushed dcc92e9 which hopefully fixes this. Please let me know if it doesn't for you. Thanks again for the test, @myronmarston.

@jonhyman

Works for me. Thanks.

Please sign in to comment.
Something went wrong with that request. Please try again.