Correctly unstubs methods stubbed with `#any_instance` #182

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

alindeman commented Sep 6, 2012

  • 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
@alindeman alindeman 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
8dee2ec

I can confirm this commit works.

@myronmarston myronmarston and 1 other commented on an outdated diff Sep 8, 2012

lib/rspec/mocks/method_double.rb
- 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.class_eval <<-EOF, __FILE__, __LINE__ + 1
+ remove_method :#{@method_name}
+ EOF
@myronmarston

myronmarston Sep 8, 2012

Owner

Do you need to eval a string here for this? I would think you could just do object_singleton_class.send(:remove_method, @method_name).

In general, I try to avoid eval'ing strings unless it's really needed.

@alindeman

alindeman Sep 8, 2012

Contributor

Yah, doh, that makes a lot more sense. I think the original code was class_evaled so I just copied it over without considering simplifications.

@myronmarston myronmarston commented on an outdated diff Sep 8, 2012

lib/rspec/mocks/stashed_instance_method.rb
+# @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.class_eval <<-EOF, __FILE__, __LINE__ + 1
+ alias_method :#{stashed_method_name}, :#{@method}
+ EOF
@myronmarston

myronmarston Sep 8, 2012

Owner

Again, I think you can get by here with @klass.send(:alias_method, stashed_method_name, @method)--no string eval needed (unless I'm missing something).

@myronmarston myronmarston and 1 other commented on an outdated diff Sep 8, 2012

lib/rspec/mocks/stashed_instance_method.rb
+ private
+
+ # @private
+ def method_defined_directly_on_klass?
+ if @klass.method_defined?(@method) || @klass.private_method_defined?(@method)
+ method_obj = @klass.instance_method(@method)
+ if method_obj && method_obj.respond_to?(:owner)
+ method_obj.owner == @klass
+ else
+ # 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
+ end
@myronmarston

myronmarston Sep 8, 2012

Owner

In a case like this, where a conditional exists to handle a specific ruby version, I like to move it out into the class body where I conditionally define a helper method. In your case, it could be like this:

def method_defined_directly_on_klass?
  if @klass.method_defined?(@method) || @klass.private_method_defined?(@method)
    method_owned_by_klass?(@klass.instance_method(@method))
  end
end

if method(:class).respond_to?(:owner)
  def method_owned_by_klass?(method)
    method.owner == @klass
  end
else
  def method_owned_by_klass?(method)
    # 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

I like this better for a few reasons:

  • The conditional is going to take the exact same branch at runtime every single time. Doing it this way makes that clear by resolving the conditional up front when the class is defined.
  • It's kinda polymorphic (at runtime at least), even if this isn't the typical way people think of polymorphism.
  • I think the code is cleaner this way.

Thoughts?

@alindeman

alindeman Sep 8, 2012

Contributor

Ah ha! I like it.

@myronmarston myronmarston commented on an outdated diff Sep 8, 2012

lib/rspec/mocks/stashed_instance_method.rb
+
+ # @private
+ def stashed_method_name
+ "obfuscated_by_rspec_mocks__#{@method}"
+ end
+
+ public
+
+ # @private
+ def restore
+ return unless @method_is_stashed
+
+ @klass.class_eval <<-EOF, __FILE__, __LINE__ + 1
+ alias_method :#{@method}, :#{stashed_method_name}
+ remove_method :#{stashed_method_name}
+ EOF
@myronmarston

myronmarston Sep 8, 2012

Owner

Could this use the non-string form of class eval like so?

@klass.class_eval do
  alias_method @method, stashed_method_name
  remove_method stashed_method_name
end
Owner

myronmarston commented Sep 8, 2012

Awesome work, Andy! I have a couple thoughts on slightly simpler implementations of a few things (see my comments above) but otherwise this looks great.

@dchelimsky dchelimsky commented on the diff Sep 8, 2012

lib/rspec/mocks/method_double.rb
@@ -44,16 +46,6 @@ class << @object; self; end
end
# @private
- def obfuscate(method_name)
@dchelimsky

dchelimsky Sep 8, 2012

Owner

There's still one call to obfuscate in this class - must not be tested.

@alindeman

alindeman Sep 8, 2012

Contributor

Ouch. I'll definitely fix this and also try to cover it with a spec.

@alindeman

alindeman Sep 8, 2012

Contributor

Good news everyone! I think that object_responds_to? (which uses obfuscate) is unused as of b599a42.

There's no reference to it in any of the rspec repos currently (except its definition):

[rspec-dev] grep -R object_responds_to\? repos
repos/rspec-mocks/lib/rspec/mocks/method_double.rb:      def object_responds_to?(method_name)

I'm going to bias towards removing it unless someone lets me know that I'm missing something.

Owner

dchelimsky commented Sep 8, 2012

Besides obfuscate, I think this looks good overall and I agree w/ @myronmarston's suggestions.

Contributor

alindeman commented Sep 8, 2012

Pushed up some new commits that address the code review comments. If I get a 👍 from @myronmarston or @dchelimsky, I'll squash-merge.

Contributor

alindeman commented Sep 8, 2012

Not sure what's up with Travis. There was no output for the failed 1.8.7 build http://travis-ci.org/#!/rspec/rspec-mocks/jobs/2382370 and it runs green locally.

@myronmarston myronmarston commented on an outdated diff Sep 9, 2012

lib/rspec/mocks/stashed_instance_method.rb
+ @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 ::Method.method_defined?(:owner)
@myronmarston

myronmarston Sep 9, 2012

Owner

This is a bit pedantic, but shouldn't this be ::UnboundMethod.method_defined?(:owner)? @klass.instance_method returns an UnboundMethod.

I think that owner is either present or absent on both ::Method and ::UnboundMethod but it still makes sense to check against the class that the object below is actually an instance of.

Owner

myronmarston commented Sep 9, 2012

👍

@alindeman alindeman Checks against UnboundMethod instead of the parent Method
* Class#instance_method returns an UnboundMethod
70b0818
Contributor

alindeman commented Sep 9, 2012

Will change Method to UnboundMethod and merge. Thanks for the reviews.

alindeman closed this in a727464 Sep 9, 2012

@alindeman alindeman added a commit that referenced this pull request Sep 9, 2012

@alindeman alindeman Changelog for #167 and #182
[ci skip]
f890472
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment