error: methods spied on in module_function style are not properly reverted

[EbokianKnight]

When mocking module_functions / extend self modules, Spy does not recognize the ownership of the original method and won't re-apply it to the module during teardown. Here is a quick Minitest that shows this happening:

require 'test_helper'

class SpyTesting < ActiveSupport::TestCase
  module Example
    extend self

    def hello
      'hello world'
    end
  end

  test 'one' do
    Spy.on(Example, :hello).and_return('yo')
    assert_equal Example.hello, 'yo'
  end

  test 'two' do
    Spy.on(Example, :hello).and_return('yo')
    assert_equal Example.hello, 'yo'
  end

end

Running this code will always have one of the two tests fail.

Error:
SpyTesting#test_two:
NameError: undefined method `hello' for class `#<Class:SpyTesting::Example>'
    test/spy_testing.rb:18:in `block in <class:SpyTesting>'

2 tests, 1 assertions, 0 failures, 1 errors, 0 skips

This is due to the fact that in lib/spy/subroutine.rb:87 the method_owner does not equal the original_method.owner.

method_owner
=> #<Class:SpyTesting::Example>
method_owner.class
=> Class

original_method.owner
=> SpyTesting::Example
original_method.owner.class
=> Module

I'm still looking for the easiest fix, but. I figure I should share the issue either way. This works without issue in spy 1.0.0, which has been our solution for a while, but we will need to go to ruby 3 eventually. Removing the second conditional to the if statement will cause the tests to pass, but I assume it's there for an excellent reason. I'm still learning the internals.

[ryanong]

Interesting. So this is broken in 1.0.1?

[EbokianKnight]

Hey Ryan, Yeah, this is broken in Spy 1.0.1. Here’s my test suite I was using in the forked gem to see how everything is performing. I just re-confirmed the fact using this, so found it came in handy. require 'test_helper' module Spy class TestUnhook < Minitest::Test module ModuleFunctionStyle extend self def hello 'hello world' end end module Injected def hello 'hello world' end end class ModuleInjectedStyle include Injected end class ModuleExtendedStyle extend Injected end class SingletonMethodStyle def self.hello 'hello world' end end class InstanceMethodStyle def hello 'hello world' end end def test_ModuleFunctionStyle spy = Spy.on(ModuleFunctionStyle, :hello).and_return('yo') assert_equal ModuleFunctionStyle.hello, 'yo' spy.unhook assert_equal ModuleFunctionStyle.hello, 'hello world' end def test_ModuleInjectedStyle instance = ModuleInjectedStyle.new spy = Spy.on(instance, :hello).and_return('yo') assert_equal instance.hello, 'yo' spy.unhook assert_equal instance.hello, 'hello world' end def test_ModuleExtendedStyle spy = Spy.on(ModuleExtendedStyle, :hello).and_return('yo') assert_equal ModuleExtendedStyle.hello, 'yo' spy.unhook assert_equal ModuleExtendedStyle.hello, 'hello world' end def test_SingletonMethodStyle spy = Spy.on(SingletonMethodStyle, :hello).and_return('yo') assert_equal SingletonMethodStyle.hello, 'yo' spy.unhook assert_equal SingletonMethodStyle.hello, 'hello world' end def test_InstanceMethodStyle instance = InstanceMethodStyle.new spy = Spy.on(instance, :hello).and_return('yo') assert_equal instance.hello, 'yo' spy.unhook assert_equal instance.hello, 'hello world' end end end With the following change in 1.0.2 everything passes nicely. Running it in my full app with 6 years of tests (many Spy cases) the only thing that happened was some of my instance method block tests using args failed because the first argument there seems to be a reflection (self) of the spy instance. I didn’t bother testing to see if the arguments changed from 1.0.0 to 1.0.2, I just saw a comment in the fork talking about it being intentional somewhere and just fixed the issue by adding the |_, args|. Spy.on_instance_method(PageText, :push!).and_return { |_, args| @page_text.update!(args) } # unhooks method from object # @return [self] def unhook raise NeverHookedError, "'#{method_name}' method has not been hooked" unless hooked? method_owner.send(:remove_method, method_name) if original_method #&& method_owner == original_method.owner original_method.owner.send(:define_method, method_name, original_method) original_method.owner.send(original_method_visibility, method_name) if original_method_visibility end clear_method! Agency.instance.retire(self) self end So far, I’ve not been able to find any negative situations with this as the fix. ~ Adam

…

On Jul 13, 2022, at 8:41 AM, Ryan Ong ***@***.***> wrote: Interesting. So this is broken in 1.0.1? — Reply to this email directly, view it on GitHub <#25 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACPUVIMBWISYOAMRLMF7THDVT22RDANCNFSM53IOJEBQ>. You are receiving this because you authored the thread.