rspec / rspec-mocks

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

3.10.3 breaks hash argument matching in Ruby 3+ #1460

Closed alextwoods closed 1 year ago

alextwoods commented 2 years ago

Subject of the issue

With rspec-mocks 3.10.3 we started seeing test failures in our CI: Previously passing run - fails now with rspec mocks 3.10.3 for ruby 3+. (Note, passes with rspec-mocks 3.10.2).

I believe the change in behavior is related to #1394.

Note: This issue is similar to #1457 but applies to a slightly different case

Your environment

Steps to reproduce


class TestObject
  def initialize(object)
    @object = object
  end

  def foo(arg, options={})
    @object.foo(arg, options)
  end
end

describe 'rspec-mocks 3.10.3' do
  # This code passes in Ruby 2.7, but fails on Ruby 3.
  # Note: this test passes on Ruby 3 w/ rspec-mocks 3.10.2
  it 'breaks in ruby 3.0' do
    object = double('object')
    t = TestObject.new(object)

    expect(object).to receive(:foo).with('arg', opt: 'value')
    t.foo('arg', opt: 'value')
  end

  # This is the issue from: #1457 
  # this code passes in ruby 3
  it 'breaks in ruby 2.7' do
    obj = Object.new
    expect(obj).to receive(:call).with(:foo, **{})
    obj.call(:foo, **{})
  end
end

The above tests can be fixed by either:

  1. Changing the spec to: with('arg', {opt: 'value'})
  2. Or changing the code to use the double splat: @object.foo(arg, **options)

Expected behavior

Tests that pass in 3.10.2 should pass in 3.10.3.

Actual behavior

Some tests fail.

pirj commented 2 years ago

Thanks for reporting.

Do you want to give this patch a go to see if it resolves the problem?

alextwoods commented 2 years ago

The patch does not fix the issue, but it does at least provide better failure output, eg:

  expected: ("source", opt_name: "opt-value")
       got: ("source", {:opt_name=>"opt-value"})
JonRowe commented 2 years ago

What this issue is reporting is actually the correct behaviour. On Ruby 3:

expect(object).to receive(:foo).with('arg', opt: 'value')

This is setting an expectation of calling foo('arg', opt: 'value') with opt being a keyword argument. This is why passing in a proper hash is not matching, prior to 3.10.3 this was a bug and hashes would allowed to be silently be passed when keywords were required etc, and using your own snippet to demonstrate that hashes and keywords are not compatible:

class TestObject
  def initialize(object)
    @object = object
  end

  def foo(arg, options={})
    @object.foo(arg, options)
  end
end

class RealObject
  def foo(arg, opt:)
  end
end

TestObject.new(RealObject.new).foo('arg', opt: 'a')
# wrong number of arguments (given 2, expected 1; required keyword: opt) (ArgumentError)

In the naive sense, e.g. without verifying doubles, there is simply no way to do any other behaviour, we must fail a spec for not matching keywords vs hashes, as the default behaviour on Ruby3 matches this, so your example is unsolvable.

Now I expect prehaps in reality you are mocking a collaborator which has a defined behaviour, and indeed with instance doubles we could solve that scenario, where we have a real class we can see if the real object expects keywords or a hash, but we'd have to extend this existing behaviour in that scenario.

alextwoods commented 2 years ago

I agree this is generally correct Ruby 3 behavior - a proper hash is not the same as keyword args. My issue is that a patch version change introduced behavior that makes matchers less permissive and breaks existing, working code.

In your RealObject example above, if the RealObject#foo takes a hash, rather than keyword args, then the code is valid and works as expected, eg:

class RealObject
  def foo(arg, options={})
  end
end

TestObject.new(RealObject.new).foo('arg', opt: 'a') # RealObject#foo will be called with option={opt: 'a'}
JonRowe commented 2 years ago

The problem is a generic double has no real implementation, so we should assume Ruby 3 semantics on Ruby 3.

Previously keyword arguments were broken, and this change fixed them. That it unforuntately makes some previous specs invalid (which were incorrect on Ruby 3) is sad but there is no way for a generic double to fix it. I'd happily review a PR to make instance verifying doubles work (.e.g. instance_double("ClassName")).

The simple work around is to set with('arg', {opt: 'a'})

JonRowe commented 2 years ago

The TL;DR is we cannot make expect(double()).to receive(:foo).with('arg', opt: :a) work with both keywords and a hash correctly, so we must pick the Ruby 3 semantics as correct.

gingerlime commented 2 years ago

We're seeing a similar issue(?) after upgrading rspec-rails, which updated rspec-mocks as a dependency...

     Failure/Error: update_async(user.id, properties)

       ClassName received :method with unexpected arguments
         expected: (5, {:subscription_started_date=>1646810075})
              got: (5, {:subscription_started_date=>1646810075})
raldred commented 2 years ago

We have this issue during upgrade to ruby 3, the diff is misleading. It seems the diff can't distinguish between a hash being passed or keyword args.

In our case the hash being passed needed double splatting.

#<Double "check"> received :create with unexpected arguments
         expected: ("1111111-123123-123123", {:reports=>[{:name=>"identity"}, {:name=>"document"}], :tags=>["USA"], :type=>"express"})
              got: ("1111111-123123-123123", {:reports=>[{:name=>"identity"}, {:name=>"document"}], :tags=>["USA"], :type=>"express"})

Fix:

@api.check.create(applicant_id, **create_check_params(country_code: country_code))

pirj commented 2 years ago

Do you mind sharing your spec and the subject under test?

On 25 Aug 2022, at 14:06, Rob Aldred @.***> wrote:

 We have this issue during upgrade to ruby 3, the diff is misleading.

<Double "check"> received :create with unexpected arguments

     expected: ("1111111-123123-123123", {:reports=>[{:name=>"identity"}, {:name=>"document"}], :tags=>["USA"], :type=>"express"})
          got: ("1111111-123123-123123", {:reports=>[{:name=>"identity"}, {:name=>"document"}], :tags=>["USA"], :type=>"express"})

Fixed by changing the code calling this method to double splat the hash being passed @api.check.create(applicant_id, **create_check_params(country_code: country_code))

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

jpriollaud commented 1 year ago

Ruby 3: Wrapping in curly braces with('arg', {opt: 'value'}) fixed one test, thanks to @alextwoods

This doesn't seem to work in another test with a similar problem.

let(:payload) do { somekey: [ ] } end

...
.with('somestring', payload)

ArgumentError: Invalid keyword arguments provided: somekey

I tried all variations of moving somekey to the opt: position, wrapping in curly braces, double splat, etc.

Method works fine and I'd rather not change it, but instead just fix this test.

JonRowe commented 1 year ago

This issue was solved in 3.12.0 providing diff handling for this and 3.12.1 has some fixes to improve edge cases, for your issue @jpriollaud we'd need some more reproduction