Fix stubbing prepended only methods

[godfat]

Previously, we're assuming the method must be defined in the
singleton class. However this is not always true. Whenever the
method was only defined in the prepended module, then it's not
defined in the singleton class. We need to find the owner of
the method instead, which is the prepended module.

Closes #1213

[xaviershay]

Thank you! This looks like a neat fix to me. Will wait for the build to go green before merging.

[godfat]

@xaviershay Thanks for the support. I realized that there's still a failing test:

spec/rspec/mocks/partial_double_spec.rb:604

Which is:

it 'uses the method signature from `#initialize` for arg verification' do
  pending "Failing on JRuby due to https://github.com/jruby/jruby/issues/2565" if RSpec::Support::Ruby.jruby?

  subclass = Class.new(klass) do
    private_class_method :new
  end

  prevents(/arguments/) { allow(subclass).to receive(:new).with(1) }
  allow(subclass).to receive(:new).with(1, 2)
end

It's actually not failing, but it would mark the new method as private for the other class, causing issues. I am still figuring out how to deal with this edge case.

[myronmarston]

Thanks @godfat!

[myronmarston]

A couple things...

• It looks like this is only used from restore_original_visibility, below. Can it be actually private instead of just being hidden from the YARD docs via @private? We use @private only for methods that need to be publicly accessible for other parts of rspec-mocks to be able to call, but that we consider to not be part of our public API.
• What's the reason for the instance_method(...).bind(...).call(...) business? (It'd probably be obvious if I had worked in rspec-mocks recently, but I haven't). Alternately, we do have RSpec::Support.method_handle_for(object, method_name).call(*args) that you could use that does something similar--it might be worth using here?

[godfat]

Yes, good calls, thanks.

As for instance_method(...).bind(...).call(...), it's needed because the method method and respond_to? might be stubbed, and when that happened obviously it's not working.

I tried RSpec::Support.method_handle_for but then spec/rspec/mocks/and_call_original_spec.rb:257 would fail with:

  2) and_call_original on a partial double that overrides #method works for a singleton method
     Failure/Error: RSpec::Support.method_handle_for(object, :method).call(@method_name).owner

     ArgumentError:
       wrong number of arguments (given 1, expected 0)
     # /Users/godfat/project/fork/rspec-mocks/lib/rspec/mocks/method_double.rb:56:in `method'

All the other tests except the still failing one passed. Any idea? Maybe it's a bug in RSpec::Support.method_handle_for?

[myronmarston]

Woops, with the slow wi-fi on this bus I didn't see your comment there @xaviershay as I was reviewing :).

[godfat]

I pushed the current progress. Now the only failing test is:

it "correctly restores the visibility of methods whose visibility has been tweaked on the singleton class" do
  # hello is a private method when mixed in, but public on the module
  # itself
  mod = Module.new {
    extend self
    def hello; :hello; end

    private :hello
    class << self; public :hello; end;
  }

  expect(mod.hello).to eq(:hello)

  allow(mod).to receive(:hello) { :stub }
  reset mod

  expect(mod.hello).to eq(:hello)
end

Due to:

  1) A method stub #rspec_reset correctly restores the visibility of methods whose visibility has been tweaked on the singleton class
     Failure/Error: expect(mod.hello).to eq(:hello)

     NoMethodError:
       private method `hello' called for #<Module:0x00007f832828b560>
     # ./spec/rspec/mocks/stub_spec.rb:352:in `block (3 levels) in <module:Mocks>'

It's not passing because InstanceMethodStasher#method_owned_by_klass? wasn't properly detecting that the method was defined by its own. This is an edge case due to extend self, which makes it think that it's not defined on its own, but it actually is...

That method was a bit complex and I am still thinking a general way which could cover this edge case.

[godfat]

Actually, we could remove all the checks in restore_original_visibility when this is only called when @method_stasher.method_is_stashed?

[godfat]

Oh, Ruby 2.3.7+ passed but all the other older Rubies broke. We're really supporting very old rubies. I'll revisit this later as I don't have older rubies installed locally.

Any help would be appreciated though, as I am personally not too interested in older rubies. singleton_class could be easy to fix, but method_owned_by_klass? and Object.instance_method(:method).bind(object) could be tough.

I'll not force push too much from now as I don't want to pollute the related issue anymore.

[godfat]

Is there any plan to drop support for older Rubies?

[JonRowe]

In RSpec 4 we will drop some but not necessarily all older Rubies.

[godfat]

@JonRowe Thanks. What about 2.2.10? According to the test, this works on 2.3.7+

[JonRowe]

Given the ruby 2 branch is mostly trouble free it is unlikely to be dropped

[godfat]

Thanks. I could probably see if I could fix one more older ruby, but if that won't fix even older rubies, I think I am not looking into them then...

[godfat]

jruby 9.2.13.0 can pass this so removing it, otherwise RSpec is complaining this is actually passing.

[godfat]

This test can catch the need for !Mocks.space.any_instance_recorder_for(owner, true) in the other condition. It was discovered in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44417#note_429658296 thanks to @engwan !

[benoittgt]

I am in favor of merging this to 4-0-dev and move on.

[fledman]

@benoittgt @godfat @JonRowe @pirj
so is this ready to merge into 4-0-dev then?

[JonRowe]

If it was rebased from that branch and build passed, yes.

[godfat]

I rebased against 4-0-dev but there's no CI running.

[pirj]

Push-forced to trigger CI. Running.

[godfat]

It doesn't seem like CI is running unless some maintainers approve this. Let me know what else needs to be done because I am not clear what we're missing.

[pirj]

Approved, running.

[godfat]

Thanks. Looks like it's all green. Do we need anything else?

Review notes addressed

[pirj]

Thank you for all the hard work!

@JonRowe Should we merge to 4-0-dev, or wait until we release 4.0, and release this in 4.0.1?

[pirj]

Does it affect the performance?

[godfat]

It's memoized so I don't think we'll see anything noticeable in practice. If we disregard memoization and run simple benchmark, object.method(method_name).owner is about 3 times faster:

Warming up --------------------------------------
 object.method.owner   261.517k i/100ms
       bind and call    91.219k i/100ms
Calculating -------------------------------------
 object.method.owner      2.661M (± 0.9%) i/s -     13.337M in   5.011938s
       bind and call    914.184k (± 1.0%) i/s -      4.652M in   5.089434s

From running this:

require 'benchmark/ips'

Benchmark.ips do |x|
  object = Object.new

  x.report('object.method.owner') do
    object.method(:to_s).owner
  end

  x.report('bind and call') do
    Object.instance_method(:method).bind(object).call(:to_s).owner
  end
end

Well, or we can remove the test and calling out that do not override method, which I fully support :P

[pirj]

I'm clearly on your side with you that one shouldn't override Kernel's internals with unrelated application-level implementations. However, someone's code may break with a weird message, and they will open a ticket, and we'll have to address it, since even though this might not be a good style to override method, it's not our call to enforce that in any way.

[godfat]

Yeah, of course, that's why I was half joking there :)

Half joking, half serious, because there are a lot of other methods we're relying on, and of course we won't want to be so defensive to do it this way for most of the built-in methods? (or a rule of thumb is, whenever someone is filing a ticket 🤷 This is also relying on someone isn't trying to break RSpec intentionally)

I see method or send is more commonly overridden with a completely different behaviour (if it's the same behaviour like some people will override method_missing also with respond_to_missing? properly, that would be mostly alright, but issues might still happen especially for mocks implementation which often talks about proxies), and that's why we have __send__ we can use, sadly not for method and it may be confusing that __method__ is actually a completely different method! Not like __send__ is an alias of send.

Sorry that this is probably falling down to a rant. To be more on the topic, since the test is already there, I don't think a regression is acceptable. However if we never added that test, I would think it's fine to just say "don't override method". Maybe RSpec 5, or maybe it doesn't matter after all. It's just one method right now. We don't have an army of Object.instance_method(method_name).bind(object) yet. Or do we? Maybe it's inside one of the support libraries already? 😅 To be honest I won't be surprised!

[pirj]

We defend against a number of quite odd things:

# This class protects us against Mutex.new stubbed out within tests

rspec/rspec-core#2886, rspec/rspec-support#431 and in a few other places.

Maybe it's inside one of the support libraries already? 😅

Good point, it is! 😄
https://github.com/rspec/rspec-support/blob/d63133f478408c1d965e673b96ad10ef5a5d183f/lib/rspec/support.rb#L53

[godfat]

We defend against a number of quite odd things:

Oh my, I was guilty here. I am sure I was once lazy and stubbed File.read without explicit about the arguments, and it caused some issues. However being explicit about a specific argument did the trick for me.

I wonder if this kinds of things will need a more general approach. For example, will this work?

RSpec::Support::Source::File = ::File.dup

Good point, it is! 😄

Ok, thank you for spotting that. I updated the code to use it instead.

[pirj]

kinds of things

I can think of three cases we've touched:

1. Ruby core method stubbed (File.read, File.expand_path, Mutex.new)
2. Ruby core method overridden in a subclass (Kernel.method)
3. Attempt to define a method with memoized helpers that would break Example (to_s, initialize)

Not sure if there could be a common solution to all of this.

[godfat]

Ruby core method stubbed (File.read, File.expand_path, Mutex.new)

I just tried this does work:

PreservedFile = File.dup

def File.read
  'break you'
end

PreservedFile.read('README.md')

We can have a list of preserved classes.

module RSpec
  module Preserved
    File = ::File.dup
    Mutex = ::Mutex.dup
  end
end

Ruby core method overridden in a subclass (Kernel.method)

We can always do it via the same trick.

module RSpec
  module Preserved
    module_function
    def send_kernel(object, message, *args, **kargs)
      ::Kernel.instance_method(message).bind(object).call(*args, **kargs)
    end
  end
end

method_owner = RSpec::Preserved.send_kernel(object, :method).call(method_name).owner

Edited: Fixed above send_kernel usage.

Attempt to define a method with memoized helpers that would break Example (to_s, initialize)

For to_s and initialize I think we should just raise an error like now, indeed. It would be too much work for those to be overridden. Even if we can stick with __to_s__ or __initialize__ and play around with allocate, it's easily broken with other integration.

Not sure if there could be a common solution to all of this.

I didn't mean a common solution to all of above. I meant a common solution for the same issue. For example, we use the same way to resolve File.read or Mutex.new (a module method) got overridden. We use the same way to resolve a particular instance method got overridden.

[JonRowe]

Our policy is to do the method work around for cases as they occur

[JonRowe]

@JonRowe Should we merge to 4-0-dev, or wait until we release 4.0, and release this in 4.0.1?

@pirj I'm happy for you to make that decision, personally I'd lean towards merging into 4-0-dev if you're happy with it.

[pirj]

@godfat, thanks a lot for getting this to the finish line, and for an interesting conversation.

[fledman]

@JonRowe @pirj
is there somewhere to follow along how rspec 4 development is going?
(besides just scrolling through the lists of open PRs)

[pirj]

@fledman There's this ticket https://github.com/rspec/rspec/issues/61.
In a nutshell, RSpec 4 is pretty much done. There are 2 open PRs that introduce deprecation warnings for the future RSpec 3.99, and one is WIP.
You can also look at RSpec 4 milestones in repos (e.g. https://github.com/rspec/rspec-mocks/milestone/11).

This PR was not exactly planned for the release, but it turned out to be easier to target it for 4.0 rather than support all the older Ruby versions.

If you wish to give RSpec 4 a try, you can do it right away:

# Gemfile
%w[rspec rspec-core rspec-expectations rspec-mocks rspec-support].each do |lib|
  gem "#{lib}", github: "rspec/#{lib}", branch: '4-0-dev'
end

Any pre-release bug report from a real-world project is highly appreciated.

[fledman]

@pirj I got a relatively large codebase running successfully

the most troublesome adjustment was the change to strict_predicate_matchers

it was very unexpected that to be_XXX and not_to be_XXX are not inverses: both can fail
particularly when the underlying value is nil e.g. not_to (be|have)_XXX is effectively an eql(false) check

also some Ruby built-ins no longer being assertable this way e.g. be_infinite on positive and negative infinity

otherwise it was just getting the Gemfile to resolve all the conflicts due to it being a pre-release:

• I had to set ENV['RSPEC_CI'] = '1' and ENV['RSPEC_VERSION'] = '4.0.0.pre' in my Gemfile itself
• I had to pull rspec-rails directly from github so those vars would get picked up by its gemspec
• I had to fork a few other gems that had restrictive gemspec dependencies (rspec-sidekiq and rspec_junit_formatter)

[fledman]

I suppose it would have been helpful if there had been deprecation warnings for the switch fromif:/unless: to skip:

[pirj]

@fledman Thanks a lot for putting your time into this!

We'll add deprecation warnings in 3.99, e.g. for :if/:unless`.

I really hope strict predicate matchers are for the good.

some Ruby built-ins no longer being assertable this way e.g. be_infinite on positive and negative infinity

Float::INFINITY.infinite? # => 1

Oh, that is true. Well, truthy. There were a couple of similar ones, and it's mostly surprising that they don't return true/false.

[fledman]

@pirj maybe you can have both strict (true/false) and loose (truthy/falsey) forms of the be/have matchers?

that way, regardless of which is the "default", it is easy to do inline adjustment

some syntax possibilities:

1. picking a symbol to append e.g. be_infinite vs be_infinite? or have_foo vs have_foo!
2. adding an option to the matcher e.g. be_ready(strict: false)
3. coming up with another prefix (couldn't think of one)

alternatively there could be an explicitly truthy/falsey matcher
expect(number).to reply(:infinite?)
expect(number).to respond_truthy(:infinite?)