rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 357 forks source link

and_wrap_original doesn't work with kwargs in Ruby 3.2 #1524

Closed swrobel closed 1 year ago

swrobel commented 1 year ago

and_wrap_original doesn't work with kwargs in Ruby 3.2

Your environment

Steps to reproduce

Add the following spec to the "on a partial double" context of and_wrap_original_spec.rb, which is based on a similar spec from and_call_original_spec.rb:

if RSpec::Support::RubyFeatures.kw_args_supported?
  binding.eval(<<-CODE, __FILE__, __LINE__)
  it "works for methods that accept keyword arguments" do
    def instance.foo(bar: nil); bar; end
    allow(instance).to receive(:foo).and_wrap_original { |m, *args| m.call(*args) }
    expect(instance.foo(bar: "baz")).to eq("baz")
  end
  CODE
end

It will fail on the current version of rspec-mocks with wrong number of arguments (given 1, expected 0). Work was done to fix this for and_call_original in #1464 but a similar workaround was not added for and_wrap_original. If you modify the definition of and_wrap_original like so:

def and_wrap_original(&block)
  block = block.ruby2_keywords if block.respond_to?(:ruby2_keywords) # New

  wrap_original(__method__, &block)
end

It will pass the additional spec above, but other specs will fail with:

Failure/Error: raise "Warnings were generated: #{output}" if has_output?

      RuntimeError:
        Warnings were generated: rspec-mocks/lib/rspec/mocks/message_expectation.rb:163: warning: Skipping set of ruby2_keywords flag for proc (proc accepts keywords or proc does not accept argument splat)

I can't say that I understand enough about the inner workings of ruby2_keywords and its use with different types of procs/blocks to figure this out, otherwise I would've opened up a PR for the change. You can see my WiP on my branch here.

Expected behavior

and_wrap_original works with methods that expect kwargs

Actual behavior

and_wrap_original fails with methods that expect kwargs

pirj commented 1 year ago

The following crutch worked, but I have serious doubts that it would work correctly for all cases of user-provided blocks:

        if block && block.parameters.first.first == :rest
          block.ruby2_keywords if block.respond_to?(:ruby2_keywords)
        end
JonRowe commented 1 year ago

So I ran your spec and it passed without any code changes on 3.1, so it only fails on 3.2 and using **kwargs here in the block works correctly for both 3.1 and 3.2, so I'm honestly leaning towards saying "you should manage this block yourself here", I think any improvement we should make here should be more directed at it failing correctly...

JonRowe commented 1 year ago

Closing this but going to increase documentation over in #1525 this spec works as:

it "works for methods that accept keyword arguments" do
  def instance.foo(bar: nil); bar; end
  allow(instance).to receive(:foo).and_wrap_original { |m, **kwargs| m.call(**kwargs) }
  expect(instance.foo(bar: "baz")).to eq("baz")
end