[Bug] Recent changes to Mock respond_to? behaviour causing NoMethodError

[adrianna-chang-shopify]

Hi!

In trying to upgrade Shopify's primary Rails application from mocha v1.14 to v1.15, a test started to fail unexpectedly. I've set up a repro to mimic what the test was doing (Ruby v3.1.2, but shouldn't matter):

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "~> 7.0.4"
  gem "sqlite3"
  gem "mocha"
end

require "active_record"
require "minitest/autorun"
require 'mocha/minitest'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :posts, force: true
end

class Post < ActiveRecord::Base; end

class MockingTest < Minitest::Test
  def test_mocking_active_records_flatten
    m1 = mock()
    m1.responds_like(Post.new)

    m2 = mock()
    m2.responds_like(Post.new)

    arr = [m1, m2].flatten
  end
end

=> # Running:

E

Error:
MockingTest#test_mocking_active_records_flatten:
NoMethodError: undefined method `to_ary' for #<Mock:0x2530> which responds like #<Post:0x2558>

        raise NoMethodError, "undefined method `#{symbol}' for #{mocha_inspect} which responds like #{@responder.mocha_inspect}"
        ^^^^^
    /Users/adriannachang/.gem/ruby/3.1.2/gems/mocha-2.0.0/lib/mocha/mock.rb:385:in `check_responder_responds_to'
    /Users/adriannachang/.gem/ruby/3.1.2/gems/mocha-2.0.0/lib/mocha/mock.rb:322:in `handle_method_call'
    /Users/adriannachang/.gem/ruby/3.1.2/gems/mocha-2.0.0/lib/mocha/mock.rb:314:in `method_missing'
    scripts/mocha_experiment.rb:36:in `flatten'
    scripts/mocha_experiment.rb:36:in `test_mocking_active_records_flatten'

rails test scripts/mocha_experiment.rb:29

I traced it back to this change. To sum:

• Array#flatten calls respond_to?(:to_ary, true) on each element in the array. With include_all being true, it looks at private methods.
• The mock's respond_to_missing? delegates to the mock's @responder, forwarding the include_all argument, so respond_to? returns true -- Active Record objects do respond to #to_ary, but note that this method is private
  . respond_to? returns true here.
• When method missing steps in to actually handle #to_ary, however, we check again whether the @responder responds to the method in question, but this time we don't include private methods.
• We raise NoMethodError.

This passed prior to v1.15 because we were only forwarding the include_all argument when calling @responder.respond_to?(symbol) if the method's arity was > 1, which was not the case for ActiveRecord#to_ary. Consequently, respond_to?(:to_ary, true) would return false, Array.flatten would not attempt to delegate #to_ary to Mock#method_missing, and we wouldn't raise the NoMethodError.

I'll admit that I'm not sure what the fix is -- is there a way we could have check_responder_responds_to handle private methods properly so that method_missing can kick in properly for a method that has private visibility on the responder? Given that #respond_to_missing? will return true for private methods when include_all is true...

Thanks! ❤️

[floehopper]

@adrianna-chang-shopify

Many thanks for the bug report and for taking the time to reproduce and dig into the problem.

I've had a quick look and I've found the commit where the change was originally introduced and the commit note sounds suspiciously closely related to your issue!

As soon as I have time, I'll do some more investigation and get back to you.

[floehopper]

Note to self: this might relate to #531 & #532.

[floehopper]

@adrianna-chang-shopify

This passed prior to v1.15 because we were only forwarding the include_all argument when calling @responder.respond_to?(symbol) if the method's arity was > 1, which was not the case for ActiveRecord#to_ary.

I don't think the way you've described the condition in what you've said is quite correct. The code that was removed from Mock#respond_to_missing? was checking the arity of the #respond_to? method on the responder; not the arity of the #to_ary method.

Object#respond_to? returns an arity of -1 (at least for recent versions of Ruby). And as far as I can see ActiveRecord does not define its own version of #respond_to? on model classes (at least not for recent versions of Rails). This implies that pre-v1.15 we were never passing the include_private parameter to the #respond_to? method on the responder; whereas in v1.15 we always pass the include_private parameter. This in turn is why pre-v1.15 we were never considering private methods and always reported that #to_ary did not exist.

The commit note in 6416b74 suggests the if/else statement in Mock#respond_to_missing? was not added for this reason and so I suspect your tests were effectively passing by accident. However, the problem definitely lies with Mocha and I will keep working on this.

[adrianna-chang-shopify]

Hi @floehopper ! Thanks for the speedy response and for digging into this, it's much appreciated! ❤️

I don't think the way you've described the condition in what you've said is quite correct. The code that was removed from Mock#respond_to_missing? was checking the arity of the #respond_to? method on the responder; not the arity of the #to_ary method.

Oh, yes you're absolutely right! Sorry, I glanced through that code too quickly 🤦‍♀️

Object#respond_to? returns an arity of -1 (at least for recent versions of Ruby). And as far as I can see ActiveRecord does not define its own version of #respond_to? on model classes (at least not for recent versions of Rails).

I believe it actually does so that it can handle attribute methods -- aka here. The arity is -2 though, so the same logic applies. My assessment was the same as yours -- prior to v1.15, we were ignoring the include_private parameter, so #flatten was in turn ignoring the arity part and the tests still managed to pass.

6416b74 is an interesting find! Funny that it also had to do with #flatten and #to_ary. Adding the arity check there is odd for sure -- definitely seems to make sense to drop it. The main issue is that ultimately we're expecting method_missing to hook in for a method that is private, and most of the time we wouldn't necessarily want a mock to stub methods private to the responder. #to_ary being private on ActiveRecord complicates things 😛

[floehopper]

Some more useful references:

• Mocha Lighthouse ticket: Mock#to_ary and Array#flatten problem
• RSpec issue referencing Mocha Lighthouse ticket
• RSpec commit addressing the issue
• Mocha commit fixing a problem with tests relying on behaviour of Array#flatten which seems to have changed between Ruby v1.8 & v1.9
• ActiveRecord::Core#to_ary private method definition with explanatory comment
• Tenderlove blog post explaining a bit about #to_ary

[floehopper]

@adrianna-chang-shopify

And as far as I can see ActiveRecord does not define its own version of #respond_to? on model classes (at least not for recent versions of Rails).

I believe it actually does so that it can handle attribute methods -- aka here.

Doh! Yes, you're quite right - I don't know how I missed that! 🤦‍♂️

[floehopper]

@adrianna-chang-shopify

Sorry for the delay in getting back to you. I have continued to investigate this issue, but it's opened a bit of a can of worms! 😂

I've opened #583 as a first stab at a fix, but I still have a bunch of questions I need to resolve.

However, in the meantime it would be great if you could try changing your Gemfile to point at the fix-responds-with-regression branch and see if it fixes your issue:

gem 'mocha', git: 'https://github.com/freerange/mocha.git', branch: 'fix-responds-with-regression'

[adrianna-chang-shopify]

Hey @floehopper, no worries at all! That fixes the test indeed 👍 I know that deciding on the API for protected / private methods with Mock#responds_like in general is a bit nuanced, but at least for Array#flatten and the private #to_ary deal, seems reasonable to have responds_to? only consider public methods on the responder and return false. I'm not sure there's a use case for overriding #to_ary beyond having it return nil.

Anyways, appreciate you digging into this! I'll keep my eyes out for subsequent patch releases 👀

[floehopper]

That fixes the test indeed 👍

Awesome - thanks for the quick response! ❤️

[floehopper]

@adrianna-chang-shopify Sorry for the delay - the fix has been released in v1.15.1, v1.16.1 & v2.0.2. I'm going to close this for now, but let me know if you run into any issues. 🤞