rspec / rspec-mocks

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

3.11.1 breaks argument matching in Ruby 2.7 #1466

Open drcapulet opened 2 years ago

drcapulet commented 2 years ago

Subject of the issue

When delegating using **kwargs, the spec fails when passed no keyword arguments in 2.7 (no issues with 3.0).

Your environment

Steps to reproduce

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  gem "rspec", "3.11.0"
  gem "rspec-mocks", "3.11.1"
end

puts "Ruby version is: #{RUBY_VERSION}"
require 'rspec/autorun'

def call(client, name, *args, **kwargs)
  client.public_send(name, *args, **kwargs)
end

RSpec.describe 'args broken' do
  let(:client) { double('@client') }

  it 'works' do
    expect(client).to receive(:foo).with('a', 1)

    call(client, :foo, 'a', 1)
  end

  it 'works with keyword args' do
    expect(client).to receive(:foo).with('a', 1, baz: 5)

    call(client, :foo, 'a', 1, baz: 5)
  end
end

Expected behavior

Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
Using bundler 2.3.4
Using rspec-support 3.11.0
Using diff-lcs 1.5.0
Using rspec-core 3.11.0
Using rspec-expectations 3.11.0
Using rspec-mocks 3.11.0
Using rspec 3.11.0
Ruby version is: 2.7.5
..

Finished in 0.00934 seconds (files took 0.06848 seconds to load)
2 examples, 0 failures

Actual behavior

Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
Using bundler 2.3.4
Using diff-lcs 1.5.0
Using rspec-support 3.11.0
Using rspec-expectations 3.11.0
Using rspec-core 3.11.0
Using rspec-mocks 3.11.1
Using rspec 3.11.0
Ruby version is: 2.7.5
F.

Failures:

  1) args broken works
     Failure/Error: client.public_send(name, *args, **kwargs)
       #<Double "@client"> received unexpected message :foo with ("a", 1)
     # new.rb:21:in `public_send'
     # new.rb:21:in `call'
     # new.rb:30:in `block (2 levels) in <main>'

Finished in 0.00692 seconds (files took 0.07046 seconds to load)
2 examples, 1 failure

Failed examples:

rspec new.rb:27 # args broken works
pirj commented 2 years ago

Are you certain that you correctly delegate kwargs in your implementation of def call? See this and this.

drcapulet commented 2 years ago

Unless I'm missing something, the issue with *args, **kwargs is only if the delegated method does not take keyword arguments?

class A
  def foo(a1, a2, bang: 5, baz: nil)
    puts [a1, a2, bang, baz].inspect
  end
end

def call(client, name, *args, **kwargs)
  client.public_send(name, *args, **kwargs)
end

call(A.new, :foo, 'a', 1) # => ["a", 1, 5, nil]
call(A.new, :foo, 'a', 1, baz: 5) # => ["a", 1, 5, 5]

Interestingly, if I switch client to instance_double(A) things resolve but in my case that's not an option.

pirj commented 2 years ago

If you debug it, args passed to the double are ["a", 1, {}]. You can confirm this with the following green example:

  it 'works' do
    expect(client).to receive(:foo).with('a', 1, {})

    call(client, :foo, 'a', 1)
  end
drcapulet commented 2 years ago

That's what RSpec is catching, but as I noted before using an instance_double doesn't have this issue - is there a difference in behavior in argument matching there (either expected or unexpected)?

pirj commented 2 years ago

It is not specific to RSpec, it is how keyword delegation with *args, **kwargs works in Ruby 2.7. In your example, the call method definition is to blame. If you write in an established way that supports correct delegation across Ruby versions:

ef call(client, name, *args)
  client.public_send(name, *args)
end
ruby2_keywords :call

RSpec.describe 'args broken' do
  let(:client) { double('@client') }

  it 'works' do
    expect(client).to receive(:foo).with('a', 1)

    call(client, :foo, 'a', 1)
  end
end

the example will pass just fine.

pirj commented 2 years ago

Speaking of instance_double, I don't really understand how exactly you use it in this case.

If you turn on doubled constant verification like instance_double docs suggest:

RSpec.configure do |config|
  config.mock_with :rspec do |mocks|
    mocks.verify_doubled_constant_names = true
  end
end

def call(client, name, *args, **kwargs)
  client.public_send(name, *args, **kwargs)
end

RSpec.describe 'args broken' do
  let(:client) { instance_double('@client') }

  it 'works' do
    expect(client).to receive(:foo).with('a', 1)

    call(client, :foo, 'a', 1)
  end
end

the example will fail with:

  1) args broken works
     Failure/Error: let(:client) { instance_double('@client') }
       "@client" is not a defined constant. Perhaps you misspelt it? Disable check with `verify_doubled_constant_names` configuration option.

Which is an expected outcome:

instance_double ... takes a class name or object as its first argument, then verifies that any methods being stubbed would be present on an instance of that class

I'd guess that the fact that the example with instance_double is working if verify_doubled_constant_names is switched off is not a reference behaviour for double. And basing on the information we have at hand, it should probably fail the example, too.

carlosantoniodasilva commented 2 years ago

Hey, I came across a similar issue when upgrading rspec-mocks on a Ruby 2.7 app, using method delegation with warts like this fails with a weird NoMethodError:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rspec', '= 3.11'
end

require 'rspec/autorun'

class Foo
  def bar(zomg, lol: nil)
    puts [zomg, lol].inspect
  end

  def delegate_bar(zomg, **kwargs)
    bar(zomg, **kwargs)
  end
end

RSpec.describe do
  it do
    expect_any_instance_of(Foo).to receive(:bar).with(:oops).and_call_original

    foo = Foo.new
    foo.delegate_bar(:oops)
  end

  it do
    expect_any_instance_of(Foo).to receive(:bar).with(:oops, lol: 'boom').and_call_original

    foo = Foo.new
    foo.delegate_bar(:oops, lol: 'boom')
  end
end
F[:oops, "boom"]
.

Failures:

  1)
     Failure/Error: bar(zomg, **kwargs)

     NoMethodError:
       undefined method `bar' for #<Foo:0x0000000122d74858>
     # test-rspec.rb:16:in `delegate_bar'
     # test-rspec.rb:25:in `block (2 levels) in <main>'

Finished in 0.00444 seconds (files took 0.03611 seconds to load)
2 examples, 1 failure

As you can see, the first method that doesn't pass any kwargs fail. My understanding is that the delegation should be changed to use *args and ruby2_keywords, which is understandable, however the failure here is really counter-intuitive I think. I could also just change the specs to expect on {} in place of the kwargs and they should pass, then change back once we upgrade to Ruby 3...

Even though we have a number of places using **kwargs for delegation, which I'll probably have to review, only a handful of specs really failed... I'm guessing just the handful that are testing specifically with but not passing kwargs, but the NoMethodError was really throwing me off there.

Thanks!

pirj commented 2 years ago

@carlosantoniodasilva All valid points. Thanks for reporting.

I believe the problem hides here and here. When args are still [:oops, {}] in find_almost_matching_expectation, when it delegates to expectation.matches_name_but_not_args, Ruby 2.7 crops the trailing {} and args become just [:oops] in matches_name_but_not_args. I believe this is done because Hash.ruby2_keywords_hash?(args.last) evaluates to true.

This is causing args_match? here to return a deceptive true, and this is causing method_missing to be called while this is not what was intended.

I don't have a solution to how to make the error message less confusing in this case.