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

Fix argument forwarding for "receive" with Ruby 3.2.0 #1514

Merged
merged 4 commits into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ jobs:
strategy:
matrix:
ruby:
- '3.2'
- '3.1'
- '3.0'
- 2.7
Expand Down
2 changes: 2 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ Bug Fixes:
* Prevent a misleading error message when using `allow(...).not_to` with
unsupported matchers. (Phil Pirozhkov, #1503)
* Fix mocking keyword args for `have_received` with a block. (Adam Steel, #1508)
* Fix mocking keyword args for `received` with Ruby 3.2.0. (Slava Kardakov,
Benoit Tigeot, Phil Pirozhkov, Benoit Daloze, #1514)

### 3.12.0 / 2022-10-26
[Full Changelog](http://github.com/rspec/rspec-mocks/compare/v3.11.2...v3.12.0)
Expand Down
4 changes: 3 additions & 1 deletion lib/rspec/mocks/argument_list_matcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ def args_match?(*actual_args)
return false if expected_args.size != actual_args.size

if RUBY_VERSION >= "3"
# if both arguments end with Hashes, and if one is a keyword hash and the other is not, they don't match
# If the expectation was set with keywords, while the actual method was called with a positional hash argument, they don't match.
# If the expectation was set without keywords, e.g., with({a: 1}), then it fine to call it with either foo(a: 1) or foo({a: 1}).
# This corresponds to Ruby semantics, as if the method was def foo(options).
if Hash === expected_args.last && Hash === actual_args.last
if !Hash.ruby2_keywords_hash?(actual_args.last) && Hash.ruby2_keywords_hash?(expected_args.last)
return false
Expand Down
1 change: 1 addition & 0 deletions lib/rspec/mocks/verifying_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ def proxy_method_invoked(obj, *args, &block)
validate_arguments!(args)
super
end
ruby2_keywords :proxy_method_invoked if respond_to?(:ruby2_keywords, true)

def validate_arguments!(actual_args)
@method_reference.with_signature do |signature|
Expand Down
2 changes: 1 addition & 1 deletion spec/rspec/mocks/argument_matchers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ def ==(other)
end

if RUBY_VERSION >= "3"
it "fails to matches against a hash submitted as a positional argument and received as keyword arguments in Ruby 3.0 or later", :reset => true do
it "fails to match against a hash submitted as a positional argument and received as keyword arguments in Ruby 3.0 or later", :reset => true do
opts = {:a => "a", :b => "b"}
expect(a_double).to receive(:random_call).with(:a => "a", :b => "b")
expect do
Expand Down
53 changes: 53 additions & 0 deletions spec/rspec/mocks/matchers/receive_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ module Mocks
end
end

# FIXME: this is defined here to prevent
# "warning: method redefined; discarding old kw_args_method"
# because shared examples are evaluated several times.
# When we flatten those shared examples in RSpec 4 because
# of no "should" syntax, it will become possible to put this
# class definition closer to examples that use it.
if RSpec::Support::RubyFeatures.required_kw_args_supported?
binding.eval(<<-RUBY, __FILE__, __LINE__)
class TestObject
def kw_args_method(a:, b:); end
end
RUBY
end

shared_examples "a receive matcher" do |*options|
it 'allows the caller to configure how the subject responds' do
wrapped.to receive(:foo).and_return(5)
Expand Down Expand Up @@ -109,6 +123,45 @@ module Mocks

expect(receiver.foo(kw: :arg)).to eq(:arg)
end

it "expects to receive keyword args" do
dbl = instance_double(TestObject)
expect(dbl).to receive(:kw_args_method).with(a: 1, b: 2)
dbl.kw_args_method(a: 1, b: 2)
end

if RUBY_VERSION >= '3.0'
it "fails to expect to receive hash with keyword args" do
expect {
dbl = instance_double(TestObject)
expect(dbl).to receive(:kw_args_method).with(a: 1, b: 2)
dbl.kw_args_method({a: 1, b: 2})
}.to fail_with do |failure|
reset_all
expect(failure.message)
.to include('expected: ({:a=>1, :b=>2}) (keyword arguments)')
.and include('got: ({:a=>1, :b=>2}) (options hash)')
end
end
else
it "expects to receive hash with keyword args" do
dbl = instance_double(TestObject)
expect(dbl).to receive(:kw_args_method).with(a: 1, b: 2)
dbl.kw_args_method({a: 1, b: 2})
end
end

it "expects to receive hash with a hash" do
dbl = instance_double(TestObject)
expect(dbl).to receive(:kw_args_method).with({a: 1, b: 2})
dbl.kw_args_method({a: 1, b: 2})
end

it "expects to receive keyword args with a hash" do
dbl = instance_double(TestObject)
expect(dbl).to receive(:kw_args_method).with({a: 1, b: 2})
dbl.kw_args_method(a: 1, b: 2)
Comment on lines +162 to +163
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fail, shouldn't it? We expect an options hash, and pass kwargs.

Copy link
Member Author

@benoittgt benoittgt Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I am wondering if it is a side effect of changes in 3.2. Like on release note. @eregon any idea?

Screen Shot 2023-01-03 at 10 09 41

Copy link
Member

@pirj pirj Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to be in the opposite situation when an option hash passed as a positional argument to an expectation suddenly matches with a kwargs hash.

I suppose that the problem is here:

            if Hash === expected_args.last && Hash === actual_args.last
              if !Hash.ruby2_keywords_hash?(actual_args.last) && Hash.ruby2_keywords_hash?(expected_args.last)
                return false

In our scenario, Hash.ruby2_keywords_hash?(actual_args.last) is true, while the other one is false.
They don't match, but we don't return false like we do e.g. here:

          if Hash === expected_hash && Hash === actual_hash &&
            (Hash.ruby2_keywords_hash?(expected_hash) != Hash.ruby2_keywords_hash?(actual_hash))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing this:

-              if !Hash.ruby2_keywords_hash?(actual_args.last) && Hash.ruby2_keywords_hash?(expected_args.last)
+              if Hash.ruby2_keywords_hash?(actual_args.last) != Hash.ruby2_keywords_hash?(expected_args.last)

This is causing two more specs to fail, at least on Ruby 3.2:

            it "matches against a hash submitted as keyword arguments and received as positional argument (in both Ruby 2 and Ruby 3)" do
              expect(dbl).to receive(:kw_args_method).with(1, {:required_arg => 2, :optional_arg => 3})
              dbl.kw_args_method(1, :required_arg => 2, :optional_arg => 3)
            end
        it "matches against a hash submitted as keyword arguments a and received as a positional argument (in both Ruby 2 and Ruby 3)" do
          opts = {:a => "a", :b => "b"}
          expect(a_double).to receive(:random_call).with(opts)
          a_double.random_call(:a => "a", :b => "b")
        end

But if we'd make this comparison strict, I agree that it should fail.

Do you guys agree to make such a change, and see if it affects other Rubies?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure message is suboptimal:

received unexpected message :kw_args_method with ({:a=>1, :b=>2})

while the method was indeed called, and arguments were alike, it's just the Hash.ruby2_keywords_hash? that didn't match.
We do it better here:

                "  expected: (#{expected_input.inspect}, {:one=>1}) (keyword arguments)\n" \                                                  │
                "       got: (#{actual_input.inspect}, {:one=>1}) (options hash)\n" \

but this differ doesn't seem to come into play.

Might be related: rspec/rspec-support#537

Copy link
Contributor

@eregon eregon Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the comment above:

# if both arguments end with Hashes, and if one is a keyword hash and the other is not, they don't match

is super confusing. That should be updated to something like:

# if both arguments end with Hashes, and if #with was called with keywords but the method was called without keywords, they don't match
# if #with is called without keywords, e.g., with({a: 1}), then it's fine to call it with either foo(a: 1) or foo({a: 1}) because it's as if the method was: def foo(options).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is only true for methods that accept positional parameters, right? E.g. for:

class A
  def foo(bar:)
  end
end

a = A.new
expect(a).to receive(:foo).with({bar: 1})
a.foo({bar: 1})

If we follow

if #with is called without keywords, e.g., with({a: 1}), then it's fine to call it with either foo(a: 1) or foo({a: 1}) because it's as if the method was: def foo(options).

The spec will pass, but the real-life code:

a = A.new
a.foo({bar: 1})

would blow up with:

ArgumentError: wrong number of arguments (given 1, expected 0; required keyword: bar)

that means we would be concealing the error.

Is my understanding correct, @eregon ?

Copy link
Contributor

@eregon eregon Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a mock, so the method might not exist, right?
From with({a: 1}) that doesn't indicate the method takes keyword arguments, hence it can be called with whatever (passing a: 1 or {a: 1} shouldn't make any difference).

If the method does exist like in that case, I think maybe RSpec could warn that you call with without kwargs when the method accepts kwargs.
Or, you could actually check whether the existing method accepts kwargs or not for the kwargs-vs-positional check, but then what happens if the method does not exist?

Wouldn't this specific example fail anyway because it actually calls the original method?
And if it doesn't call the original method, isn't it wrong to look at the original method?

EDIT: it indeed does not call the original for expect(a).to receive(:foo).with({bar: 1}).
That's super confusing to me, it just returns nil instead.
MSpec would always call the original, unless one uses .and_return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this example wouldn't be caught by your change, right? (no ruby2_keyword flag on both sides)
So it's a bug of writing .with({bar: 1}) which means "expect no kwargs" when the real method only accepts kwargs. Hence maybe that's worth a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this example wouldn't be caught by your change

Fair enough. The rest makes total sense, too.

end
RUBY
end
end
Expand Down
14 changes: 14 additions & 0 deletions spec/rspec/mocks/stub_implementation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,20 @@ def obj.foo; :original; end
include_context "with isolated configuration"
before { RSpec::Mocks.configuration.verify_partial_doubles = true }
include_examples "stubbing `new` on class objects"

if RSpec::Support::RubyFeatures.required_kw_args_supported?
binding.eval(<<-RUBY, __FILE__, __LINE__)
it "handles keyword arguments correctly" do
klass = Class.new do
def initialize(kw:)
end
end

allow(klass).to receive(:new).and_call_original
klass.new(kw: 42)
end
RUBY
end
end
end
end
Expand Down