Handle keyword arguments separately for and_call_original in supported rubies

[JonRowe]

Fixes #1306

[pirj]

I'm still getting a warning running this:

class Foo
  def initialize(bar: nil); end
end

RSpec.describe Foo do
  it 'works' do
    expect(Foo).to receive(:new).with(bar: :qux).and_call_original
    Foo.new(bar: :qux)
  end
end

[JonRowe]

Ah thats a slightly difference scenario, I think we need more tests around this.

[pirj]

That's from #1306 🤷‍♂

[JonRowe]

Yeah I had written a similar spec assuming it was and_call_original to blame, but seems like we'll need more specs considering we have so little surrounding keyword argument usage in these places.

[benoittgt]

Does #1324 (comment) will be handle in this PR too?

[JonRowe]

Yep I'll be continuing some work over in rspec support later, and then using that with more scenarios to fix this.

[imanel]

Any update on this?

[pirj]

Looks good.
Do you mind for another CI run just in case?

Shame on me. There's still this receive(:new) case left to fix.

[imanel]

@pirj rspec/rspec-support#419 should solve the case you mentioned, although it's a bit hacky so I'm not sure if it will get accepted.

[pirj]

Pushed a spec to cover the original request.
It passes nicely with rspec/rspec-support#419.
Do you want me to move the conditional that transforms new to initialize from rspec-support here?

[pirj]

@JonRowe It seems we either have to merge rspec/rspec-support#419, or to close it and pull the method_or_class_initialize to rspec-mocks.
What would you prefer?

[benoittgt]

Quick win while waiting for the patch. Bump to Ruby 2.7.2 released this week. No more warnings.

[pirj]

Bump to Ruby 2.7.2 released this week. No more warnings.

That would be just perfect if we didn't have to do anything tricky with RSpec to be kwargs-compliant and no warnings were issued!

Wondering if this means there's a performance impact in Ruby 3.
Do you think it's possible to take one of our previous kwargs-related fixes and benchmark the difference?

I'm more inclined to let postpone those fixes if the performance gain is marginal rather than maintain this burden up to the point when we drop support for Ruby 2.x (for three more years to come or so).

[JonRowe]

The warnings just become opt-in in 2.7.2, they don't change the fact we need to change these things before Ruby 3.0 is released.

[benoittgt]

The warnings just become opt-in in 2.7.2, they don't change the fact we need to change these things before Ruby 3.0 is released.

Yes of course. :)

[benoittgt]

I spend already 5 hours trying to implement rspec/rspec-support#419 (comment) but I'm stuck. I am not able to fix the failing spec here.

@pirj or @JonRowe will you be available to tackle this in pairing with me ? I understand we need to find the right signature for the new => initialize but I am no able to properly refer to the method we want to call and I do not see a clear path to reuse the existing code made in rspec-mocks.

[pirj]

@benoittgt I'm available tomorrow in the afternoon. Lost track of what's going on, so you lead, me

[godfat]

I am providing two more failures for and_wrap_original and block implementation. This is just a demonstration so I omitted binding.eval and we might want to put those in the corresponding tests.

diff --git a/spec/rspec/mocks/and_call_original_spec.rb b/spec/rspec/mocks/and_call_original_spec.rb
index 25c5a85c..4459142a 100644
--- a/spec/rspec/mocks/and_call_original_spec.rb
+++ b/spec/rspec/mocks/and_call_original_spec.rb
@@ -88,6 +88,18 @@ RSpec.describe "and_call_original" do
         expect(instance.initialize_arg).to eq :initialize_arg
         CODE
       end
+
+      it 'works with keyword arguments for and_wrap_original' do
+        expect(instance).to receive(:meth_3).and_wrap_original { |m, keyword_arg:| m.call(keyword_arg: keyword_arg.to_s) }
+
+        expect(instance.meth_3(keyword_arg: :original_arg)).to eq 'original_arg'
+      end
+
+      it 'works with keyword arguments for block implementation' do
+        expect(instance).to receive(:meth_4) { |keyword_arg:| keyword_arg }
+
+        expect(instance.meth_4(keyword_arg: :original_arg)).to eq :original_arg
+      end
     end
 
     it 'errors when you pass through the wrong number of args' do

Or in Ruby:

it 'works with keyword arguments for and_wrap_original' do
  expect(instance).to receive(:meth_3).and_wrap_original { |m, keyword_arg:| m.call(keyword_arg: keyword_arg.to_s) }

  expect(instance.meth_3(keyword_arg: :original_arg)).to eq 'original_arg'
end

it 'works with keyword arguments for block implementation' do
  expect(instance).to receive(:meth_4) { |keyword_arg:| keyword_arg }

  expect(instance.meth_4(keyword_arg: :original_arg)).to eq :original_arg
end

[godfat]

This fixed and_wrap_original

diff --git a/lib/rspec/mocks/message_expectation.rb b/lib/rspec/mocks/message_expectation.rb
index d999dc0f..643b7ca5 100644
--- a/lib/rspec/mocks/message_expectation.rb
+++ b/lib/rspec/mocks/message_expectation.rb
@@ -115,7 +115,9 @@ module RSpec
       #     original_method.call(*args, &block).first(10)
       #   end
       def and_wrap_original(&block)
-        wrap_original(__method__, &block)
+        wrap_original(__method__) do |original, *args, &supplied_block|
+          __call_original(block, original, *args, &supplied_block)
+        end
       end
 
       # @overload and_raise

[bryanp]

@JonRowe What's the status on getting this finished up and merged? and_call_original is currently broken on Ruby 3 when the original method accepts keyword arguments. Willing to help, but not sure what all is left here.

[pirj]

@bryanp According to the plan, quite a lot of method have to be marked with ruby2_keywords.

I took a stab at a similar rspec-core issue today, but could barely handle half. There's an uncertainty if the whole chain should be marked as such or something is not right with the approach yet. On a synthetic example, ruby2_keywords do help, but marking just a few methods down the call chain doesn't.

You can certainly help. There is this #1306 (comment) that you can turn into a failing spec like this https://github.com/rspec/rspec-mocks/pull/1383/files#diff-ee4bec83cefa8d926951324b0059b58f31c016628981f29a0c391cd9b8a51315R15. After that, apply the approach described here rspec/rspec-expectations#1221 (comment) and (in brief) here rspec/rspec-expectations#1241 (comment).

If you decide to do so, please open a draft PR so we can avoid double work and intervene early in the process.

[bryanp]

@pirj Here's an initial PR that resolves the issue I hit today: #1385.