Permalink
Browse files

Externalize methods and state that was previously held on all objects.

- Remove `@mock_proxy` ivar
- Remove `rspec_verify`, `rspec_reset`, `__mock_proxy` and `__remove_mock_proxy` methods.

This is a refactoring that I'm doing to pave the way for the new expect
syntax (#153). As that syntax "wraps" objects in order to mock or stub
them, we need to externalize these methods and state. The methods directly
on the object (e.g. `stub` and `should_receive`) will simply delegate to
this external logic.

This refactoring had some nice side benefits:

- No need for the hacky YAML fix. It was only needed because we were storing
  `@mock_proxy` on the mocked object.
- AnyInstance no longer needs to much with `dup`; again, this was only needed
  because it dup'd the `@mock_proxy` ivar.
- The Marshal extension gets significantly simpler (again, due to not
  storing `@mock_proxy` on the object anymore).

Rather than storing the mock proxies on the objects, we are now storing
them in a hash, keyed by object_id.  This is pretty simple and consistent,
but could be problematic for objects that muck with `object_id`. That method
seems a bit sacred, though, so I'm not too worried about it.
1 parent 73fb798 commit 210c9f02872b2afcea67c6fcb992a13535cb94d7 @myronmarston myronmarston committed Mar 18, 2013
@@ -26,57 +26,37 @@ module AnyInstance
#
# @return [Recorder]
def any_instance
- RSpec::Mocks::space.add(self)
- modify_dup_to_remove_mock_proxy_when_invoked
+ AnyInstance.tracked_klasses << self
__recorder
end
# @private
def rspec_verify
__recorder.verify
- super
ensure
__recorder.stop_all_observation!
- restore_dup
@__recorder = nil
end
# @private
- def rspec_reset
- restore_dup
- __mock_proxy.reset
- end
-
- # @private
def __recorder
@__recorder ||= AnyInstance::Recorder.new(self)
end
- private
- def modify_dup_to_remove_mock_proxy_when_invoked
- if method_defined?(:dup) and !method_defined?(:__rspec_original_dup)
- class_eval do
- def __rspec_dup(*arguments, &block)
- clone = __rspec_original_dup(*arguments, &block)
- clone.send :__remove_mock_proxy
- clone
- end
-
- alias_method :__rspec_original_dup, :dup
- alias_method :dup, :__rspec_dup
- end
+ def self.verify_all
+ tracked_klasses.each do |klass|
+ klass.rspec_verify
end
end
- def restore_dup
- if method_defined?(:__rspec_original_dup)
- class_eval do
- alias_method :dup, :__rspec_original_dup
- remove_method :__rspec_original_dup
- remove_method :__rspec_dup
- end
- end
+ def self.reset_all
+ tracked_klasses.clear
+ end
+
+ def self.tracked_klasses
+ @tracked_klasses ||= []
end
end
end
end
+
@@ -103,7 +103,7 @@ def stop_all_observation!
# @private
def playback!(instance, method_name)
- RSpec::Mocks::space.add(instance)
+ RSpec::Mocks.space.ensure_registered(instance)
message_chains.playback!(instance, method_name)
@played_methods[method_name] = instance
received_expected_message!(method_name) if message_chains.has_expectation?(method_name)
@@ -2,18 +2,12 @@ module Marshal
class << self
def dump_with_mocks(*args)
object = args.shift
- return dump_without_mocks(*args.unshift(object)) unless object.instance_variable_defined?(:@mock_proxy)
- mp = object.instance_variable_get(:@mock_proxy)
- return dump_without_mocks(*args.unshift(object)) unless mp.is_a?(::RSpec::Mocks::Proxy)
-
- object.__send__(:remove_instance_variable, :@mock_proxy)
-
- begin
- dump_without_mocks(*args.unshift(object.dup))
- ensure
- object.instance_variable_set(:@mock_proxy,mp)
+ unless ::RSpec::Mocks.space.registered?(object)
+ return dump_without_mocks(*args.unshift(object))
end
+
+ dump_without_mocks(*args.unshift(object.dup))
end
alias_method :dump_without_mocks, :dump
@@ -1,23 +0,0 @@
-if defined?(Psych) && Psych.respond_to?(:dump)
- module Psych
- class << self
- def dump_with_mocks(object, *args)
- return dump_without_mocks(object, *args) unless object.instance_variable_defined?(:@mock_proxy)
-
- mp = object.instance_variable_get(:@mock_proxy)
- return dump_without_mocks(object, *args) unless mp.is_a?(::RSpec::Mocks::Proxy)
-
- object.__send__(:remove_instance_variable, :@mock_proxy)
-
- begin
- dump_without_mocks(object, *args)
- ensure
- object.instance_variable_set(:@mock_proxy, mp)
- end
- end
-
- alias_method :dump_without_mocks, :dump
- alias_method :dump, :dump_with_mocks
- end
- end
-end
@@ -17,7 +17,7 @@
require 'rspec/mocks/errors'
require 'rspec/mocks/error_generator'
require 'rspec/mocks/space'
-require 'rspec/mocks/serialization'
+require 'rspec/mocks/extensions/marshal'
require 'rspec/mocks/any_instance'
require 'rspec/mocks/mutate_const'
require 'rspec/mocks/stub_chain'
@@ -142,7 +142,6 @@ class << @object; self; end
# @private
def configure_method
- RSpec::Mocks::space.add(@object) if RSpec::Mocks::space
warn_if_nil_class
@original_visibility = visibility_for_method
@method_stasher.stash unless @method_is_proxied
@@ -155,7 +154,7 @@ def define_proxy_method
object_singleton_class.class_eval <<-EOF, __FILE__, __LINE__ + 1
def #{@method_name}(*args, &block)
- __mock_proxy.message_received :#{@method_name}, *args, &block
+ ::RSpec::Mocks.space.mock_proxy_for(self).message_received :#{@method_name}, *args, &block
end
#{visibility_for_method}
EOF
View
@@ -12,13 +12,15 @@ module Methods
# logger.should_receive(:log)
# thing_that_logs.do_something_that_logs_a_message
def should_receive(message, opts={}, &block)
- __mock_proxy.add_message_expectation(opts[:expected_from] || caller(1)[0], message.to_sym, opts, &block)
+ mock_proxy = ::RSpec::Mocks.space.mock_proxy_for(self)
+ mock_proxy.add_message_expectation(opts[:expected_from] || caller(1)[0], message.to_sym, opts, &block)
end
# Sets and expectation that this object should _not_ receive a message
# during this example.
def should_not_receive(message, &block)
- __mock_proxy.add_negative_message_expectation(caller(1)[0], message.to_sym, &block)
+ mock_proxy = ::RSpec::Mocks.space.mock_proxy_for(self)
+ mock_proxy.add_negative_message_expectation(caller(1)[0], message.to_sym, &block)
end
# Tells the object to respond to the message with the specified value.
@@ -32,7 +34,7 @@ def stub(message_or_hash, opts={}, &block)
if Hash === message_or_hash
message_or_hash.each {|message, value| stub(message).and_return value }
else
- __mock_proxy.add_stub(caller(1)[0], message_or_hash.to_sym, opts, &block)
+ ::RSpec::Mocks.space.mock_proxy_for(self).add_stub(caller(1)[0], message_or_hash.to_sym, opts, &block)
end
end
@@ -44,7 +46,7 @@ def stub(message_or_hash, opts={}, &block)
# shared `before` hook for the common case, but you want to replace it
# for a special case.
def unstub(message)
- __mock_proxy.remove_stub(message)
+ ::RSpec::Mocks.space.mock_proxy_for(self).remove_stub(message)
end
def stub!(message_or_hash, opts={}, &block)
@@ -93,7 +95,7 @@ def stub_chain(*chain, &blk)
# returned.
def as_null_object
@_null_object = true
- __mock_proxy.as_null_object
+ ::RSpec::Mocks.space.mock_proxy_for(self).as_null_object
end
# Returns true if this object has received `as_null_object`
@@ -103,37 +105,9 @@ def null_object?
# @private
def received_message?(message, *args, &block)
- __mock_proxy.received_message?(message, *args, &block)
- end
-
- # @private
- def rspec_verify
- __mock_proxy.verify
- end
-
- # @private
- def rspec_reset
- __mock_proxy.reset
- end
-
- private
-
- def __mock_proxy
- @mock_proxy ||= begin
- mp = if TestDouble === self
- Proxy.new(self, @name, @options)
- else
- Proxy.new(self)
- end
-
- Serialization.fix_for(self)
- mp
- end
- end
-
- def __remove_mock_proxy
- @mock_proxy = nil
+ ::RSpec::Mocks.space.mock_proxy_for(self).received_message?(message, *args, &block)
end
end
end
end
+
@@ -345,27 +345,13 @@ def rspec_reset
def self.mutate(mutator)
register_mutator(mutator)
mutator.mutate
- ensure_registered_with_mocks_space
- end
-
- # Ensures the constant stubbing is registered with
- # rspec-mocks space so that stubbed constants can
- # be restored when examples finish.
- #
- # @api private
- def self.ensure_registered_with_mocks_space
- return if defined?(@registered_with_mocks_space) && @registered_with_mocks_space
- ::RSpec::Mocks.space.add(self)
- @registered_with_mocks_space = true
end
# Resets all stubbed constants. This is called automatically
# by rspec-mocks when an example finishes.
#
# @api private
- def self.rspec_reset
- @registered_with_mocks_space = false
-
+ def self.reset_all
# We use reverse order so that if the same constant
# was stubbed multiple times, the original value gets
# properly restored.
@@ -411,4 +397,4 @@ def self.raise_on_invalid_const
# of both stubbing and hiding.
ConstantStubber = ConstantMutator
end
-end
+end
View
@@ -17,9 +17,9 @@ def warn_about_expectations_on_nil=(new_value)
def allow_message_expectations_on_nil
@warn_about_expectations_on_nil = false
- # ensure nil.rspec_verify is called even if an expectation is not set in the example
+ # ensure nil is verified even if an expectation is not set in the example
# otherwise the allowance would effect subsequent examples
- RSpec::Mocks::space.add(nil) unless RSpec::Mocks::space.nil?
+ RSpec::Mocks.space.ensure_registered(nil) unless RSpec::Mocks.space.nil?
end
# @private
@@ -1,34 +0,0 @@
-require 'rspec/mocks/extensions/marshal'
-require 'rspec/mocks/extensions/psych' if defined?(::Psych)
-
-module RSpec
- module Mocks
- # @private
- module Serialization
- # @private
- def self.fix_for(object)
- object.extend(YAML) if defined?(::YAML)
- rescue TypeError
- # Can't extend Fixnums, Symbols, true, false, or nil
- end
-
- # @private
- module YAML
- # @private
- def to_yaml(options = {})
- return nil if defined?(::Psych) && options.respond_to?(:[]) && options[:nodump]
- return super(options) unless instance_variable_defined?(:@mock_proxy)
-
- mp = @mock_proxy
- remove_instance_variable(:@mock_proxy)
-
- begin
- super(options)
- ensure
- @mock_proxy = mp
- end
- end
- end
- end
- end
-end
View
@@ -2,32 +2,50 @@ module RSpec
module Mocks
# @api private
class Space
- def add(obj)
- receivers << obj unless receivers.detect {|m| m.equal? obj}
+ attr_reader :mock_proxies
+
+ def initialize
+ @mock_proxies = {}
end
def verify_all
- receivers.each do |mock|
- mock.rspec_verify
+ mock_proxies.values.each do |object|
+ object.verify
end
+
+ AnyInstance.verify_all
end
def reset_all
- receivers.each do |mock|
- mock.rspec_reset
+ ConstantMutator.reset_all
+ AnyInstance.reset_all
+
+ mock_proxies.values.each do |object|
+ object.reset
end
- receivers.clear
+
+ mock_proxies.clear
expectation_ordering.clear
end
def expectation_ordering
@expectation_ordering ||= OrderGroup.new
end
- private
+ def mock_proxy_for(object)
+ mock_proxies.fetch(object.object_id) do
+ mock_proxies[object.object_id] = if TestDouble === object
+ object.__build_mock_proxy
+ else
+ Proxy.new(object)
+ end
+ end
+ end
+
+ alias ensure_registered mock_proxy_for
- def receivers
- @receivers ||= []
+ def registered?(object)
+ mock_proxies.has_key?(object.object_id)
end
end
end
@@ -42,8 +42,8 @@ def format_chain(*chain, &blk)
end
def find_matching_stub
- object.__send__(:__mock_proxy).
- __send__(:find_matching_method_stub, chain.first.to_sym)
+ ::RSpec::Mocks.space.mock_proxy_for(object).
+ __send__(:find_matching_method_stub, chain.first.to_sym)
end
end
end
Oops, something went wrong.

7 comments on commit 210c9f0

Contributor

alindeman commented on 210c9f0 Mar 30, 2013

Flyby review: could we use __id__ instead? Would that address your concern about objects that muck with object_id?

Owner

myronmarston replied Mar 30, 2013

Flyby review: could we use id instead? Would that address your concern about objects that muck with object_id?

I didn't even know about __id__! Not sure how I missed that...

Anyhow, I was playing around in IRB and noticed that BasicObject provides __id__ but not object_id:

➜  ~  irb
1.9.3p327 :001 > k = BasicObject.new
(Object doesn't support #inspect)
 =>
1.9.3p327 :002 > k.__id__
 => 70139647345580
1.9.3p327 :003 > k.object_id
NoMethodError: undefined method `object_id' for #<BasicObject:0x007f9551878758>
    from (irb):3
    from /Users/myron/.rvm/rubies/ruby-1.9.3-p327/bin/irb:16:in `<main>'
1.9.3p327 :004 >

So it looks like we should use __id__. I checked in IRB on 1.8.6 and noticed that method works there, as well. There's still the possibility of someone mucking with __id__ but I feel like the underscores communicate "this is off limits; don't mess with this" and an object that redefines __id__ is a poorly behaved object, IMO.

Contributor

alindeman replied Mar 31, 2013

Awesome! Exactly, what I meant (but didn't exactly say) is that I consider any object that messes with __ methods to be completely insane and unsupported.

I forgot that __id__ was on BasicObject too. Good find.

Owner

myronmarston replied Mar 31, 2013

I just pushed a PR (#259) for this :). Will merge when travis is green. (But feel free to review it if you want).

Owner

dchelimsky replied Jun 5, 2013

@myronmarston can we eliminate the serialization_spec entirely after this commit? Asking because I'd like to get rid of the warning Ruby 2.0 emits saying "syck has been removed" triggered by https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/serialization_spec.rb#L63

Owner

myronmarston replied Jun 5, 2013

@myronmarston can we eliminate the serialization_spec entirely after this commit? Asking because I'd like to get rid of the warning Ruby 2.0 emits saying "syck has been removed" triggered by https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/serialization_spec.rb#L63

I don't think we can remove it entirely. We still have our marshal fix that it is testing. We don't have any YAML-specific code in rspec-mocks, anymore, but given that it was a bug in the past I'd like to keep the YAML specs to prevent future regressions.

What do you think about putting ruby version conditionals in place so that on 2.0 the syck one is skipped?

Owner

dchelimsky replied Jun 5, 2013

I had done that locally and felt bad about it :) But if you think that's fine I'll make it so.

Please sign in to comment.