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

Ruby 3.2.0 and keyword arguments vs options hash #1513

Closed schinery closed 1 year ago

schinery commented 1 year ago

Subject of the issue

I'm currently in the process of upgrading some Rails apps from Ruby 3.1.3 to 3.2.0 and am now getting 'expected keyword arguments, received options hash' failures in a bunch of specs that pass in 3.1.3.

Wasn't sure if it is related to https://github.com/rspec/rspec-expectations/issues/1370 or https://github.com/rspec/rspec-expectations/issues/1350.

Your environment

Steps to reproduce

Rough basic example pulled from one of the Rails apps where the spec is now failing after the Ruby upgrade.

# app/models/app.rb

class App
  def initialize(app_name, do_something: false)
    # ...
  end

  def things
    # ...
  end
end

# app/controllers/things_controller.rb

class ThingsController < ApplicationController
  def index
    @app = App.new(app_name, do_something: true)
    @things = @app.things

    render partial: "index"
  end

  private

  def app_name
    @app_name ||= params[:app_name].to_sym
  end
end

# spec/requests/things_spec.rb

RSpec.describe "/things" do
  describe "GET /" do
    before do
      app = instance_double(App)
      allow(App).to receive(:new).with(:jimmy, do_something: true).and_return(app)
      allow(app).to receive(:things).and_return([])

      get "/things", params: { app_name: "jimmy" }
    end

    it "returns status 200 OK" do
      expect(response).to have_http_status(:ok)
    end
  end
end

Expected behavior

The spec passes with the expectations met.

Actual behavior

Failures:

  1) /things GET / returns status 200 OK
     Failure/Error: @app = App.new(app_name, do_something: true)

       #<App (class)> received :new with unexpected arguments
         expected: (:jimmy, {:do_something=>true}) (keyword arguments)
              got: (:jimmy, {:do_something=>true}) (options hash)
        Please stub a default value first if message might be received with other args as well.
schinery commented 1 year ago

Hmmm... if I do the following as an inline example then it works as expected.

# frozen_string_literal: true

begin
  require 'bundler/inline'
rescue LoadError => e
  warn '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.12.0' # Activate the gem and version you are reporting the issue against.
end

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

class App
  def things
    Thing.new(:jimmy, do_something: true)
  end
end

class Thing
  def initialize(name, do_something: false)
    # ...
  end
end

RSpec.describe 'keyword args' do
  before do
    thing = instance_double(Thing)
    allow(Thing).to receive(:new).with(:jimmy, do_something: true).and_return(thing)
    App.new.things
  end

  it { expect(Thing).to have_received(:new).with(:jimmy, do_something: true) }
end

Then it passes...

Fetching gem metadata from https://rubygems.org/...
Resolving dependencies...
Using bundler 2.4.1
Using diff-lcs 1.5.0
Using rspec-support 3.12.0
Using rspec-mocks 3.12.1
Using rspec-core 3.12.0
Using rspec-expectations 3.12.1
Using rspec 3.12.0
Ruby version is: 3.2.0
.

Finished in 0.00938 seconds (files took 0.07401 seconds to load)
1 example, 0 failures

Tried this with using both rspec-core and rspec-rails just to see if it made any difference. Also tried creating an inline Rails app to try but so far failing at producing that.

Also worth noting, in my original example if I change allow(App).to receive(:new).with(:jimmy, do_something: true) to allow(App).to receive(:new).with(:jimmy, { do_something: true }) then the spec passes but that doesn't seem like the fix.

pirj commented 1 year ago

Can you please try

allow(App).to receive(:new).with(:jimmy, {do_something: true})

And it’s not under our control how Rails calls App.new

schinery commented 1 year ago

Can you please try allow(App).to receive(:new).with(:jimmy, {do_something: true})

@pirj yeah if I do that the spec passes, but am trying to understand why it doesn’t need to be changed when run in Ruby 3.1.3.

Been looking up the Ruby 3.2 keyword argument changes and stumbled on https://github.com/rspec/rspec-mocks/issues/1495 and https://github.com/rspec/rspec-mocks/pull/1502, so assume it’s the same thing here?

pirj commented 1 year ago

Actually you're right, I've missed this line in your code.

@app = App.new(app_name, do_something: true)
marcqualie commented 1 year ago

This is also happening will keywords args only when uprading from ruby 3.1 to 3.2.

MyClass.call(content: content, url: url)
expect(MyClass).to receive(:call).with(content: content, url: url).and_return mock_result

# failure
#<MyClass(class)> received :call with unexpected arguments
  expected: ({:content=>"somestring", :url=>"https://example.com"}) (keyword arguments)
       got: ({:content=>"somestring", :url=>"https://example.com"}) (options hash)

As suggested above, converting the test expectation to a hash makes the test pass:

expect(MyClass).to receive(:call).with({ content: content, url: url }).and_return mock_result

This is asserting incorrect behaviour though.. if actually changing the underlying code to call with a hash, the test still passes, but runtime error occurs:

wrong number of arguments (given 1, expected 0; required keywords: content, url)

I hope this helps narorw down the issue.

mblumtritt commented 1 year ago

I can confirm the issue described above. It prevents me from updating to Ruby v3.2.0, which is important for me :/

ric2b commented 1 year ago

This might be related to https://github.com/rspec/rspec-mocks/issues/1512, .withshares the argument customization implementation with .and_call_original and other methods: https://github.com/rspec/rspec-mocks/blob/ad4f5a1c6e3bf0e6ba135e4fef82153ce8972d84/lib/rspec/mocks/matchers/receive_message_chain.rb#L16-L21

schinery commented 1 year ago

@pirj I've just tried applying the ruby2_keywords :proxy_method_invoked if respond_to?(:ruby2_keywords, true) change from https://github.com/rspec/rspec-mocks/pull/1502 to my locally installed rspec-mocks and the tests that were failing now pass as expected.

benoittgt commented 1 year ago

@schinery could you try this https://github.com/rspec/rspec-mocks/pull/1514 ?

schinery commented 1 year ago

@benoittgt just tried but am getting the following error when adding gem "rspec-mocks", git: "https://github.com/rspec/rspec-mocks", branch: "ruby-3.2" to the Gemfile:

bundle update
Fetching https://github.com/rspec/rspec-mocks
Fetching gem metadata from https://rubygems.org/..........
Resolving dependencies...
Could not find compatible versions

Because every version of rspec-mocks depends on rspec-support = 3.13.0.pre
  and rspec-support = 3.13.0.pre could not be found in rubygems repository https://rubygems.org/ or installed locally,
  every version of rspec-mocks is forbidden.
So, because Gemfile depends on rspec-mocks >= 0,
  version solving has failed.

However this was the change I applied locally which resolved the particular issue I was having.

benoittgt commented 1 year ago

@schinery Do you have better result with this

gem 'rspec-mocks', git: 'https://github.com/rspec/rspec-mocks.git', branch: 'ruby-3.2'
%w[rspec-core rspec-expectations rspec-rails rspec-support].each do |lib|
  gem lib, git: "https://github.com/rspec/#{lib}.git", branch: 'main'
end

Thanks for the feedback.

schinery commented 1 year ago

@benoittgt that did the trick, and can confirm that the failing specs are passing again as expected locally.

I've got some more Rails apps that also have failing specs in, will try in some of those too.

schinery commented 1 year ago

I've got some more Rails apps that also have failing specs in, will try in some of those too.

Having some more dependency issues in the other projects (with rspec-html-matchers for example) so might be awhile confirming in them...

adsteel commented 1 year ago

Been looking up the Ruby 3.2 keyword argument changes and stumbled on https://github.com/rspec/rspec-mocks/issues/1495 and https://github.com/rspec/rspec-mocks/pull/1502, so assume it’s the same thing here?

@schinery I believe so. I think we could merge this issue and that issue together.