Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warning when delegating to mock in Ruby 2.4 #321

Closed
tjvc opened this issue Feb 9, 2018 · 4 comments
Closed

Warning when delegating to mock in Ruby 2.4 #321

tjvc opened this issue Feb 9, 2018 · 4 comments
Assignees

Comments

@tjvc
Copy link

tjvc commented Feb 9, 2018

Ruby 2.4 warns when delegating to a private method with Forwardable: https://bugs.ruby-lang.org/projects/ruby-trunk/repository/revisions/56210.

When delegating to a Mocha mock object with Forwardable, a "forwarding to private method" warning is shown.

For example, given:

require "forwardable"

class Foo
  extend Forwardable

  attr_reader :baz

  def_delegator :baz, :bar

  def initialize(baz)
    @baz = baz
  end
end
require "minitest/autorun"
require "mocha/mini_test"

require_relative "foo"

class FooTest < Minitest::Test
  def test_delegates_bar_to_baz
    baz = mock
    baz.expects(:bar)

    foo = Foo.new(baz)
    foo.bar
  end
end

The test will output:

foo_test.rb:12:in `test_delegates_bar_to_baz': Foo#bar at /Users/thomcarter/code/mocha/foo.rb:8 forwarding to private method Mocha::Mock#bar

Ideally these warnings would not be shown, because in most cases the expectation is set on a public method.

@floehopper
Copy link
Member

floehopper commented Feb 11, 2018

@tjvc:

Thanks for reporting this with such a clear example.

As far as I can see this is a problem with Ruby and not Mocha, because I see the same behaviour when I replace the mock object with a simple Ruby class using method_missing:

  • baz.rb
 class Baz
   def method_missing(symbol, *args)
     if symbol == :bar
       123
     else
       super
     end
   end
 
   def respond_to_missing?(symbol, include_private = false)
     (symbol == :bar) || super
   end 
 end
  • foo_test.rb
require "minitest/autorun"

require_relative "foo"
require_relative "baz"

class FooTest < Minitest::Test
  def test_delegates_bar_to_baz
    baz = Baz.new

    foo = Foo.new(baz)
    foo.bar
  end
end
foo_test.rb:11:in `test_delegates_bar_to_baz': Foo#bar at foo.rb:8 forwarding to private method Baz#bar

Does that make sense? I think the next step would be to report the issue to Ruby - are you happy to do that or would you like me to do it?

@floehopper
Copy link
Member

FWIW I think the problem lies on this line of forwardable.rb - I'm not familiar with this use of defined?, but I assume it doesn't take methods defined by method_missing into account.

@floehopper
Copy link
Member

Just to clarify, I would consider Baz#bar to be a "public" method, because:

baz = Baz.new
p baz.respond_to?(:bar, include_private = false) # => true

tjvc added a commit to tjvc/mocha that referenced this issue Feb 12, 2018
Fixes freerange#321.

As of Ruby 2.4, Forwardable warns when delegating to a private method.

Warnings are shown when delegating methods to a Mocha::Mock, which uses
method_missing, a private method, to respond to calls to mocked methods.

Defining respond_to_missing? on the receiver, in place of respond_to?,
prevents warnings from being shown.
@tjvc
Copy link
Author

tjvc commented Feb 12, 2018

Thanks for getting back to me, @floehopper.

I did a bit more digging: it looks like an issue was already reported for Ruby, with the suggested fix being to implement respond_to_missing? where method_missing is used. I noticed that you did this in your example, but that respond_to_missing? is not implemented in Mocha::Mock. Once I implemented it, the warnings were no longer shown.

I opened a pull request with the change (#323), but I can see that this has broken compatibility with Ruby 1.8.7, where respond_to_missing? is not available. Implementing both respond_to? and respond_to_missing? does not remove the warnings, so I'm not sure how best to proceed. I hope this is of some help, all the same!

tjvc added a commit to tjvc/mocha that referenced this issue Feb 13, 2018
Fixes freerange#321.

As of Ruby 2.4, Forwardable warns when delegating to a private method.
Warnings are shown when delegating methods to a Mocha::Mock.

To determine whether to show a warning, Forwardable tests the delegated
method with the defined? operator. Mocha uses method_missing to respond
to mocked methods. defined? uses respond_to_missing? to determine
whether or not a missing method is defined. Defining respond_to_missing?
on Mocha::Mock, in place of respond_to?, prevents warnings from being
shown.

To preserve compatibility with Ruby 1.8.7, we continue to conditionally
define respond_to?. Prior to Ruby 1.9.3, defined? returns nil for method
calls handled via method_missing, so we also only test that mocked
methods are defined? in later Ruby versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants