Adds failing spec demonstrating #210 #211

Merged
merged 3 commits into from Jan 7, 2013

2 participants

@alindeman

No description provided.

@alindeman

@myronmarston, @rafaelfranca: this is actually not a Ruby 2.0.0-specific issue, but I think it has surfaced since FileUtils's implementation changed there.

It's possible to define a module whose methods are accessible publicly on the module, but do not become a part of a class's public interface when mixed in:

module MyModule
  extend self

  def hello
    :hello
  end

  private :hello
  class << self; public :hello; end
end

class MyClass
  include MyModule
end

MyModule.hello # => :hello
MyClass.new.hello # => NoMethodError: private method `hello' called

There is only one hello method, but its visibility differs on the module vs. on the module's singleton class.

If you attempt to stub MyModule.hello with RSpec, here's roughly what happens now:

  • RSpec sees that hello is not defined directly on the module's singleton class so it adds a stubbed version without stashing the old one
  • Upon rspec_reset, RSpec calls remove_method :hello on MyModule's singleton class. The original method is restored, but its visibility is now private

I'm imagining that we need to stash and restore a method's visibility regardless of whether we stash the implementation. It's a tad tricky though, because you can stub methods that don't exist at all on the original object (in those cases, there is no visibility to restore).

I will keep toying around with solutions, but any thoughts are welcomed, esp. from @myronmarston :)

@alindeman

763dda3 adds a possible fix. Feedback welcomed.

@myronmarston myronmarston and 1 other commented on an outdated diff Jan 4, 2013
lib/rspec/mocks/method_double.rb
@@ -173,6 +174,12 @@ def restore_original_method
object_singleton_class.__send__(:remove_method, @method_name)
@method_stasher.restore
+ object_singleton_class.class_eval <<-EOF, __FILE__, __LINE__ + 1
+ if method_defined?(:#{@method_name}) || private_method_defined?(:#{@method_name})
+ #{@original_visibility}
+ end
+ EOF
@myronmarston
RSpec member

Generally, I try to avoid eval'ing strings unless it's absolutely needed (e.g. for dynamically defining a method that accepts a block on 1.8.6).

Can this be done without the string eval?

BTW, is the method_defined? || private_method_defined? check in place because an error (or some other problem) would occur if we set the visibility here when that expression evaluates to false? Or is it there simply because it defines the only case where you think we need to set the visibility?

Just wondering if we could simplify by always setting the visibility here rather than doing so conditionally.

BTW, is the method_defined? || private_method_defined? check in place because an error (or some other problem) would occur if we set the visibility here when that expression evaluates to false? Or is it there simply because it defines the only case where you think we need to set the visibility?

It's to avoid an error where we're stubbing a method that didn't originally exist.

e.g.,

foo = double
foo.stub(:bar) { :baz }

foo.rspec_reset # calling `public :bar` after removing the method would be an error

I'll go about removing the string interpolation as much as possible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston
RSpec member

Nice sleuthing as usual, @alindeman. Looks great in general, but I did have 2 small comments.

@rafaelfranca rafaelfranca referenced this pull request in erikhuda/thor Jan 4, 2013
Merged

Make directory action work with Ruby 2.0 #289

@myronmarston myronmarston merged commit a0c8217 into master Jan 7, 2013

1 check passed

Details default The Travis build passed
@myronmarston myronmarston deleted the issue_210 branch Jan 7, 2013
@myronmarston myronmarston referenced this pull request Jan 7, 2013
Closed

Issue with Ruby 2.0 #210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment