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

Define respond_to_missing? on Mocha::Mock #323

Merged
merged 1 commit into from Mar 20, 2018

Conversation

tjvc
Copy link

@tjvc tjvc commented Feb 12, 2018

Fixes #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.

@floehopper
Copy link
Member

@tjvc Thanks for digging into this a bit more. There are already places in the code which do something different for older Ruby versions. Can I suggest you add require 'mocha/ruby_version' to lib/mocha/mock.rb and use if PRE_RUBY_V19 to define respond_to? with an else which defines respond_to_missing?. Ideally I'd really like to see some kind of test for this - perhaps a test could check the same thing that Forwardable is checking to decide whether or not to issue a warning...?

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.
@tjvc
Copy link
Author

tjvc commented Feb 13, 2018

Thanks for the pointer @floehopper. I've updated the code to conditionally define respond_to? for Ruby versions < 1.9. I've also added a test that expectations are defined? on mocks, which is the check that Forwardable uses. Because defined? doesn't seem to like method calls handled via method_missing in version 1.8.7, we only run this test for later versions. That seemed reasonable, because the behaviour is only really required for versions >= 2.4, and there are similar examples elsewhere in the code. Let me know what you think!

@floehopper
Copy link
Member

@tjvc Thanks for updating the PR. I'll try to take a look as soon as I have time.

@tjvc
Copy link
Author

tjvc commented Mar 19, 2018

Hi @floehopper, was just wondering if you'd had a chance to take a look at this yet?

@floehopper
Copy link
Member

@tjvc Sorry for the delay - we've been very busy with client work. This looks great to me! I'll merge it now.

@floehopper floehopper merged commit 9835454 into freerange:master Mar 20, 2018
@floehopper
Copy link
Member

I'll try to get a release out in the next few days including this change.

@tjvc
Copy link
Author

tjvc commented Mar 20, 2018

No worries, @floehopper. Thanks for merging! 👍

@floehopper
Copy link
Member

I've just released v1.4.0 which includes this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants