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

Expecting kwargs does not work in Ruby 3.2. #1523

Closed wuarmin closed 7 months ago

wuarmin commented 1 year ago

Subject of the issue

Your environment

Steps to reproduce

# set up double
repo_double = instance_double(MyRepo)
expect(repo_double).to receive(:filter).with(first_name: "Michael", last_name: "Knight").and_return([]) 

# call filter of double in implementation
filter = { first_name: "Michael", last_name: "Knight" }
repo.filter(**filter)

fails with

#<InstanceDouble(Repositories::MyRepo) (anonymous)> received :filter with unexpected arguments
         expected: ({:first_name=>"Michael", :last_name=>"Knight"}) (keyword arguments)
              got: ({:first_name=>"Michael", :last_name=>"Knight"}) (options hash)

Expected behavior

the spec should not fail. This code worked in Ruby 3.1.2

Actual behavior

the spec fails

Thanks

JonRowe commented 1 year ago

Whats the implementation of filter? As you can see from other issues we're aware of some differences in Ruby 3.2 so this is probably a duplicate.

pirj commented 1 year ago

In general, there should be no difference in 3.2?

a = {a: 1, b: 2}

def x(**opt)
  puts opt.inspect
end
def y(a:, b:)
  puts a + b
end
def x(opt)
  puts opt.inspect
end

x(**a) # => {:a=>1, :b=>2}
y(**a) # => 3
z(**a) # => {:a=>1, :b=>2}
JonRowe commented 1 year ago

@pirj, in the case of def x(opt) RSpecs output above would be correct, it doesn't take keyword arguments, and in general using **kwargs to an argument as a hash is a duplicate of #1492 and there is something that changed there, but honestly people shouldn't be de-splating keyword arguments to hashes

wuarmin commented 1 year ago

Thanks for the response @JonRowe. Here's the implementation of filter

def filter(**filter)
  get("customers", filter)
end

private

def get(path, params = {})
  conn.get(path, params)
end

and the method is called like this:

repo.filter(first_name: "Toni", last_name: "Hawk")
gregg-platogo commented 1 year ago

That's what we came up with to fix this issue https://github.com/platogo/rspec-mocks/commit/6cdc4867952c4c5f8f47fdfea112ca785a9e3cd3

pirj commented 1 year ago

This is interesting, @gregg-platogo ! Wondering if it would work like we do e.g. here:

        @original_implementation_callable ||= original_method ||
          Proc.new do |*args, &block|
            @object.__send__(:method_missing, @method_name, *args, &block)
          end.tap do |proc|
            proc.ruby2_keywords if proc.respond_to?(:ruby2_keywords)
          end

Would you be so kind to check if this would work for your cases?

ngouy commented 1 year ago

Digging internet late at night, I think the issue is here no ?

image

We should capture args and kwargs, I might be wrong of course

ngouy commented 1 year ago

yea. When I monkey patch to image

My pre-ruby 3 working, now not working test is finally green again

ngouy commented 1 year ago

I don't have time nor faith to write the spec for that, hence make a PR. But in my case it fixed it

jonfreeland commented 1 year ago

This is working for me as well, code for convenience:

module RSpec
  module Mocks
    class MessageExpectation
      def and_call_original
        block = lambda do |original, *args, **kwargs, &b|
          original.call(*args, **kwargs, &b)
        end
        block = block.ruby2_keywords if block.respond_to?(:ruby2_keywords)

        wrap_original(__method__, &block)
      end
    end
  end
end

It still results in a warning: Skipping set of ruby2_keywords flag for proc (proc accepts keywords or proc does not accept argument splat)

But tests are green.

igor-drozdov commented 1 year ago

@jonfreeland I wonder if the issue is fixed in 3.12.6 by this PR: https://github.com/rspec/rspec-mocks/pull/1552

@gregg-platogo 3.12.6 now contains the fix that is very similar to yours: https://github.com/rspec/rspec-mocks/pull/1552. Could you please verify that it works for you?

gregg-platogo commented 1 year ago

Yes, we did update to 3.12.6 last week and it does work like a charm ❤️ Thank you guys!

igor-drozdov commented 1 year ago

@JonRowe I wonder if this issue can be closed now, WDYT?

DylanEtris commented 7 months ago

I noticed that this issue hasn't had any updates in a while and the issue has been resolved, can it be closed?

pirj commented 7 months ago

Seems fixed. Thanks everyone involved.