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

any_instance_of does not restore method on JRuby #1338

Open
marcotc opened this issue Aug 20, 2020 · 7 comments
Open

any_instance_of does not restore method on JRuby #1338

marcotc opened this issue Aug 20, 2020 · 7 comments

Comments

@marcotc
Copy link

marcotc commented Aug 20, 2020

Subject of the issue

A special combination of any_instance_of calls can prevent rspec-mocks from properly restoring the originally mocked method method during it cleanup step. I'm only able to observe this on JRuby.

Your environment

  • Ruby version: jruby 9.2.11.1
  • rspec-mocks version: 3.9.1

Steps to reproduce

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

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

  gem 'rspec', '3.9.0'
  gem 'rspec-mocks', '3.9.1'
end

require 'rspec/autorun'

puts "Ruby version is: #{RUBY_VERSION}"
STDERR.puts 'You need JRuby to reproduce this issue!' unless RSpec::Support::Ruby.jruby?

RSpec.describe do
  it do
    clazz = Class.new do
      prepend Module.new

      def foo
      end
    end

    allow_any_instance_of(clazz).to receive(:foo)

    RSpec::Mocks.space.reset_all

    expect_any_instance_of(clazz).to_not receive(:foo)

    RSpec::Mocks.space.reset_all

    clazz.new.foo
  end
end

Expected behavior

No failures:

1 example, 0 failures

Actual behavior

Failures:

  1)
     Failure/Error: clazz.new.foo

     NoMethodError:
       undefined method `foo' for #<#<Class:0x1d008e61>:0x29db008c>
       Did you mean?  fork
     # rspec-mock-issue.rb:39:in `block in rspec-mock-issue.rb'

Finished in 0.02769 seconds (files took 0.15631 seconds to load)
1 example, 1 failure

Preliminary investigation

My semi-educated guess is that the issue lies in this code block: https://github.com/rspec/rspec-mocks/blob/v3.9.1/lib/rspec/mocks/any_instance/recorder.rb#L202-L222
Forcing alias_method method_name, alias_method_name to when running on JRuby in that code block "solves" the problem from my example. I unfortunately wasn't able process how the comment included in that snippet linked is applying to our case, so I'm not 100% confident that this is the correct solution.

@pirj
Copy link
Member

pirj commented Aug 20, 2020

Nice find, thanks for reporting.

Would it make sense to add JRuby check to RUBY_VERSION < "2.3"?
Is there a more real-life spec that we could add to prevent regression?
Does it make sense to open a JRuby ticket to eventually address this?

@pirj
Copy link
Member

pirj commented Aug 24, 2020

@marcotc ping

@marcotc
Copy link
Author

marcotc commented Aug 24, 2020

For context, I spent around 2 tries trying to fix this issue, but I wasn't able to figure out how the internals of rspec-mocks should work around the involved the area. My goal was to open a PR to fix the issue, and the only reason I'm opening an issue instead because I wasn't able to come up with a solution.

Would it make sense to add JRuby check to RUBY_VERSION < "2.3"?

It would this specific issue, but I don't have enough confidence to say if that's the right solution.

Is there a more real-life spec that we could add to prevent regression?

I tried my best to reduce the test to its minimal form, and I wasn't able to simplify it any further than the reported version above.

In our use-case it boiled down to:

  1. A test calls allow_any_instance_of(clazz).to receive(:foo).
  2. Subsequent test calls expect_any_instance_of(clazz).to_not receive(:foo).
  3. After that, clazz does not respond to :foo anymore.

Trying to simplify/remove any of these steps did not allow me to reproduce the issue anymore.

Does it make sense to open a JRuby ticket to eventually address this?

I don't have the confidence to know if the issue lies in this repo or JRuby. I noticed there's an issue linked from rspec-mocks about an MRI but that I thought was affecting JRuby, but JRuby actually behaves as expected in that scenario, so it's not that same issue.

@pirj
Copy link
Member

pirj commented Aug 25, 2020

- if RUBY_VERSION < "2.3" || backed_up_method_owner[method_name.to_sym] == self
+ if RUBY_VERSION < "2.3" || RSpec::Support::Ruby.jruby? || backed_up_method_owner[method_name.to_sym] == self

made your spec green on jruby-head (jruby 9.3.0.0-SNAPSHOT (2.6.5) 2020-08-25 2f0c49000a OpenJDK 64-Bit Server VM 14.0.1+14 on 14.0.1+14 +jit [darwin-x86_64])

I propose the following:

  1. test on earlier rubies available in RVM, check if the above change works on all
  2. open a ticket for JRuby
  3. add a spec and a note to the above change mentioning JRuby ticket
  4. send a pull request with the change

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

Ah JRuby, I haven't missed your quirks, is this something thats happening only on the current head? Why haven't our builds caught it @pirj?

@pirj
Copy link
Member

pirj commented Aug 26, 2020

The example

it 'tracks aliased method calls' do
is a bit different. Here, there's no even a parent class involved.
But there is:

  • prepend
  • double stubbing
  • double reset

without any one of those the issue doesn't reproduce.

@JonRowe
Copy link
Member

JonRowe commented Aug 26, 2020

Care to add an example to our build that fails as a draft as a first step?

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

No branches or pull requests

3 participants