NoMethodError when stubbing any instance of class whose respond_to? depends on object state

[floehopper]

This follows on from #432.

If either stubbing_non_existent_method or stubbing_non_public_method configuration options are set to :warn or :prevent (i.e. not the default value of :allow) and you try to stub a method on any instance of a class like the one defined below using something like Foo.any_instance.stubs(:foo), then I think you will see the NoMethodError mentioned in #432.

class Foo
  def initialize(attrs = {})
    @attributes = attrs
  end
  def respond_to?(method, *args)
    @attributes.key?(method) ? @attributes[method] : super
  end
end

[floehopper]

I might have got the wrong end of the stick, but I think I did some work on something similar in the past. Rather than calling allocate it's possible to call the constructor with the relevant number of required arguments (determined from Method#arity) where each argument is an instance of a class something along the lines of RespondToEverything (below):

class RespondToEverything
  def method_missing(symbol, *args)
    self.class.new
  end

  def respond_to?(string, include_all=false)
    true
  end
end

This means it is possible to call the constructor rather than allocate. However, if the object genuinely respond_to? actually depends on the object state as per the example in the description, then things get harder. I might be wrong, but I think we'd need the any instance stubbing to somehow describe the state or the respond_to? behaviour of the instance(s) being stubbed...?

Maybe it's too much of an edge case. I've never been keen on using the any instance approach anyway except in legacy code where I can't easily change the code I want to test.

[nitishr]

From what I can tell, calling the constructor with an object that RespondsToEverything will help matters, because it won't allow/enable respond_to? to respond "truthfully", which means it's useless for the stubbing_non_existent_method and stubbing_non_public_method checks. So, the outcome won't be any different from either always passing or always failing those checks.

An alternative that I haven't properly thought through would be to allow .any_instance to accept some parameters (presumably constructor arguments or something else that can allow us to call .new in a way that'd meaningfully initialize the object).

However, I'm inclined to agree with the last paragraph of your comments. Too much of an edge case, and not recommended anyway. Not sure it's worth adding complexity to the code. I'd much rather add a warning to the documentation to inform users of the current behavior and the reasoning/limitation driving it.

Clients who can't live with that always have the option to do something like:

Foo.stubs(:new).returns(mock_foo)
mock_foo.expects(:bar)

[floehopper]

@nitishr

From what I can tell, calling the constructor with an object that RespondsToEverything will help matters, because it won't allow/enable respond_to? to respond "truthfully", which means it's useless for the stubbing_non_existent_method and stubbing_non_public_method checks. So, the outcome won't be any different from either always passing or always failing those checks.

I assume you mean "calling the constructor with an object that RespondsToEverything won't help matters"...?

Anyway, I understand the problem better now. Thanks!

An alternative that I haven't properly thought through would be to allow .any_instance to accept some parameters (presumably constructor arguments or something else that can allow us to call .new in a way that'd meaningfully initialize the object).

Hmm. That sounds awkward. I'm not convinced!

However, I'm inclined to agree with the last paragraph of your comments. Too much of an edge case, and not recommended anyway. Not sure it's worth adding complexity to the code. I'd much rather add a warning to the documentation to inform users of the current behavior and the reasoning/limitation driving it.

Clients who can't live with that always have the option to do something like:

Foo.stubs(:new).returns(mock_foo)
mock_foo.expects(:bar)

That seems sensible. I like the idea of adding something like that to the documentation.

One last thought. I haven't thought through whether this would be possible to implement, but I wonder if we could take an approach along the lines of Mock#responds_like or Mock#responds_like_instance_of, e.g.

Foo.any_instance.responds_like(Foo.new(:arg1, :arg2)).expects(:bar)

or

Foo.any_instance.responds_to(:bar).expects(:bar)

What do you think?

[nitishr]

Both of those options seem sensible. The first one has the downside of needing to know and being able to construct a Foo (which may be complicated). So, I'd prefer the second one.

It's almost like type declaration, except that instead of a compiler, you're providing mocha some 'type' information to allow the stub to pass the :stubbing_{non_existent,public}_method checks.

However, amongst all of them, I'd prefer just skipping the respond_to? check for mocks/stubs of any_instance, in order to avoid complexity for making a discouraged feature more powerful/useful for what I think would be very small minority of cases.

BTW, you probably realize this already, but I should still clarify that the workaround of stubbing .new to return a mock as I suggested above isn't the same as stubbing/mocking any_instance, since with any_instance you'll always get a partial double, but with a stubbed constructor you'll get whatever you make the stub return.

[wpliao1989]

Just want to report that a similar issue was introduced by a Rails 6 change: https://github.com/rails/rails/pull/33227.

After this change, any_instance.stubs(:attr_method) will throw Mocha::StubbingError: stubbing non-existent method. if stubbing_non_existent_method: :prevent is on.

# foo  :string(255)
class Foo < ActiveRecord::Base
end

# Rails 5

Foo.any_instance.stubs(:foo).returns('Hello') # works

# Rails 6

Foo.any_instance.stubs(:foo).returns('Hello') # Mocha::StubbingError: stubbing non-existent method...

The workaround I can think of currently is to call Foo.new beforehand or wrap the stubbing

Mocha::Configuration.override(stubbing_non_existent_method: :allow) do
  Foo.any_instance.stubs(:foo).returns('Hello')
end

[floehopper]

@wpliao1989 Thanks for reporting. 👍

[h-lame]

We just turned on stubbing_non_existent_method :prevent in our app and started getting random failures on CI with some of our tests that us respond_like_instance_of with an ActiveRecord::Base model. Looking at the implementation of respond_like_instance_of and the linked rails issue from above it seems like the problem is that ar objects won't have db attributes defined by a bare allocate anymore. Our random failures were caused by sometimes the test in question running after tests that used real instances of the model in question, and sometimes running before.

We have a helper called mock_like_instance_of defined as follows:

def mock_like_instance_of(klass, mock_name: klass.name, **extra_mock_args)
  mock(mock_name, extra_mock_args).tap { |m| m.responds_like_instance_of(klass) }
end

and changing it to:

 def mock_like_instance_of(klass, mock_name: klass.name, **extra_mock_args)
+  klass.define_attribute_methods if klass < ActiveRecord::Base
   mock(mock_name, extra_mock_args).tap { |m| m.responds_like_instance_of(klass) }
 end

helped. It seems like we could introduce a similar change to mocha's own implementation of responds_like_instance_of but this would be rails specific. Something like:

 def responds_like_instance_of(responder_class)
+  responder_class.define_attribute_methods if defined? ActiveRecord::Base && responder_class < ActiveRecord::Base
   responds_like(responder_class.allocate)
 end

Might work. Maybe that change should be in a special mocha/rails file to be included in rails apps only?

[floehopper]

@h-lame Thanks for reporting this and for suggesting a fix. I haven't had as much time as I would like to stay on top of this thread. Am I right in thinking that your fix wouldn't address the problem @wpliao1989 is seeing. To fix that we'd need to change the code in Mocha::ClassMethods::AnyInstance#respond_to?...? i.e. ideally we'd have a generic way to properly "initialize" sub-classes of ActiveRecord::Base.

[floehopper]

If I get the chance I'll try to investigate whether RSpec is doing anything about this, but that might not be for a day or two, so anyone on this thread should feel free to investigate! 😉

[h-lame]

@floehopper - you're right. Seems like anywhere that mocha calls allocate on a class it would need to also call define_attribute_methods if that class is an ActiveRecord::Base subclass. Seems like there's two classes of problems here:

1. A heavily dynamic object whose public API surface is dictated by internal state of the instance. Something like a slightly stricter version of OpenStruct, such as you might get from Hashie
2. A somewhat dynamic object whose public API surface is dictated by internal state of the class. Something like ActiveRecord::Base.

I don't know that mocha can do much about the former, feels like users should be driven towards using bare mock or stub objects in this case because the objects are so unknowable in terms of their respond_to?. For the latter, perhaps some kind of mechanism for "booting up" the class could be provided by mocha and instead of calling allocate directly on these classes mocha uses this mechanism (default of which is to call allocate). A mocha/rails file could ship with mocha to add klass.define_attribute_methods for ActiveRecord::Base into this mechanism.

FWIW: this seems to be what rspec-rails does: https://github.com/rspec/rspec-rails/blob/01704c50c146f720db914724c25681781ecefb23/lib/rspec/rails/active_record.rb which looks like it has exactly the mechanism I'm describing. The rspec-mock code that this hooks into is significantly more complex, but it also doesn't appear to use allocate, but neither does it appear to instantiate the objects used. I didn't dig too deep though.