BasicObjects do not have mocha ObjectMethods

[wasabigeek]

👋 hello! Came across the above, not sure if it's a bug or feature. An example:

require 'minitest/autorun'
require 'mocha/minitest'

class Example2 < BasicObject
end

class Test < Minitest::Test
  def test_mocha
    example = Example2.new
    example.expects(:foo).with({ a: 2})
    example.foo(a: 2)
  end
end

The above fails with:

Error:
Test#test_mocha:
NoMethodError: undefined method `expects' for #<Example2:0x000000010603f2f8>

    example.expects(:foo).with({ a: 2})
           ^^^^^^^^
    tmp.rb:22:in `test_mocha'

I'm curious if this is intentional - it seems to be because BasicObject isn't one of the patched classes:

mocha/lib/mocha/api.rb

Lines 39 to 42 in e32e9e5

	def self.included(_mod)
	Object.send(:include, Mocha::ObjectMethods)
	Class.send(:include, Mocha::ClassMethods)
	end

---

How we came across this was pretty funny - we saw strict kwarg deprecation warnings for the delegate.rb module e.g. deprecation warning at /opt/rubies/3.1.2/lib/ruby/3.1.0/delegate.rb:87:in 'method_missing': Expectation defined at /opt/rubies/3.1.2/lib/ruby/3.1.0/delegate.rb:87:in 'method_missing' expected positional hash ({:a => 2}), but received keyword arguments (:a => 2). These will stop matching when strict keyword argument matching is enabled. See the documentation for Mocha::Configuration#strict_keyword_argument_matching=.

Turns out some tests were using SimpleDelegator, which inherits from BasicObject and thus doesn't have .expect. However, it attempts to pass expect onto it's delegatee (?), which in our case inherited from Object and thus had the mocha object methods. Example to reproduce:

require 'minitest/autorun'
require 'mocha/minitest'

class Example < SimpleDelegator
end

class Test < Minitest::Test
  def test_mocha
    mocked = mock
    example = Example.new(mock)
    example.expects(:foo).with({ a: 2})
    example.foo(a: 2)
  end
end

[floehopper]

@wasabigeek

Thanks for reporting this.

I'm curious if this is intentional - it seems to be because BasicObject isn't one of the patched classes:

I think the simple answer to this is that when Mocha was first built, BasicObject didn't exist in the language!

I know that in the past we've considered inheriting Mocha::Mock from BasicObject instead of Object before (see #189 & #191), but you're talking about partial mocks rather than pure mock objects which is different.

In the abstract it makes some sense to me that we leave BasicObject alone in case someone wants an object that doesn't have Mocha methods mixed in. However, your concrete example with SimpleDelegator makes me less sure it's the right / least surprising approach. I'd be interested to hear what you think about that.

If we were to change it, we'd need to think through the impact it might have on users of Mocha. My initial thought is that it would effectively be adding new functionality and so we wouldn't need to provide deprecation warnings and it could go out in a minor version bump. However, there are probably some obscure edge cases where the change might break some existing code if it was somehow relying on a class inheriting from BasicObject not having the Mocha methods mixed in. What do you think?

I suppose another approach would be to make it a configuration option, but I'm less convinced about that.

[amomchilov]

What do you think about providing these Mocha-related methods in a module? Users who want them in their BasicObject-deriving classes or objects can opt-in with an include.

It seems like a reasonable compromise. You can get the behaviour if you need it, but you don't pollute the BasicObject class globally.

[wasabigeek]

@floehopper sorry for the delay! I too have mixed feelings about this.

In particular, no one has raised this before, and it's not blocking us - we only encountered it indirectly (the test still worked, albeit the expectation was defined on the underlying object instead of the delegator). I think the use case is pretty niche.

Having it user-definable (e.g. via documenting a module to include in any object - can we reuse ObjectMethods?) as @amomchilov suggested makes sense to me, but maybe we could also leave this issue up for a while to see if others chime in with their use cases.

[floehopper]

@amomchilov:

Thanks for the suggestion.

What do you think about providing these Mocha-related methods in a module? Users who want them in their BasicObject-deriving classes or objects can opt-in with an include.

It seems like a reasonable compromise. You can get the behaviour if you need it, but you don't pollute the BasicObject class globally.

My slight concern with that approach is that the methods would probably end up being included twice in most objects/classes, i.e. once by default via Object and again by the manual inclusion into BasicObject (from which all objects inherit). While this would probably be OK, because the latest method definitions would "overwrite" the earlier ones, it doesn't seem very elegant.

A configuration option would be better, because we could decide to include into one or the other. If we go that way, I have a nagging feeling there might be some trickiness with the order of requiring files, but I haven't thought that through in any detail and I doubt it's insurmountable.

[floehopper]

@floehopper sorry for the delay! I too have mixed feelings about this.

In particular, no one has raised this before, and it's not blocking us - we only encountered it indirectly (the test still worked, albeit the expectation was defined on the underlying object instead of the delegator). I think the use case is pretty niche.

Having it user-definable (e.g. via documenting a module to include in any object - can we reuse ObjectMethods?) as @amomchilov suggested makes sense to me, but maybe we could also leave this issue up for a while to see if others chime in with their use cases.

No worries. Thanks for your thoughts. As explained above, I'm leaning towards making it a configuration option. I'll give it some more thought and maybe spike a solution when I've got some spare time.

[amomchilov]

My slight concern with that approach is that the methods would probably end up being included twice in most objects/classes, i.e. once by default via Object and again by the manual inclusion into BasicObject (from which all objects inherit). While this would probably be OK, because the latest method definitions would "overwrite" the earlier ones, it doesn't seem very elegant.

That would be totally ok actually. The dynamic dispatch of expect (and friends) would just go looking up the ancestor chain for the first class/module that implements it. It'll always find it on Object, and stop there. It wouldn't know or care that BasicObject also happens to include it.

I think that works out much more simply than a configuration option, and it's a more standard/consistent mechanism that I would guess it's more lined up with user expectations. When faced with the question with "How do I get this method on this class?" I'm guessing most peoples first inclination is to look for the correct module, rather than some special-built configuration option.

What do you think?

[floehopper]

I'm closing this in favour of #622.