Mocks dont raise keyword argument errors

pedrocarmona commented 3 years ago

Subject of the issue

When preparing for ruby 3, I want the following deprecation warnings to be thrown when using mocks, so that I can update the code:

Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

Your environment

Ruby version: 2.7.2
rspec-mocks version: 3.10.2

configure application to show deprecations

via env variable:

RUBYOPT='-W:deprecated'

or add to config/environments/test.rb

Warning[:deprecated] = true

Steps to reproduce

describe "#thing" do
  let(:something) { instance_double(Something) } 
  before do
    allow(Something).to receive(:new).and_return(something)
  end
  it "initializes something" do
    subject

    expect(Something).to have_received(:new).with(
        a: :b
     )
  end
end

Expected behavior

Moreover, I would expect it to fail in ruby 3 (as the code would not be executed in production).

and the block version can detect the kwargs problem, so it should work like it:

describe "#thing" do
  let(:something) { instance_double(Something) } 
  before do
    allow(Something).to receive(:new).and_return(something)
  end
  it "initializes something" do
    subject

    expect(Something).to have_received(:new) do |*args, **kwargs|
      expect(kwargs).to match({a: :b})
    end
  end
end

When using this approach we can correctly detect deprecated version ^

Actual behavior

All tests pass

Fix

monkey patching allows to throw errors

module RSpec
  module Mocks
    module Matchers
      class HaveReceived
        alias_method :original_with, :with
        def with(*args, **kwargs)
          @block ||= proc do |*w_args, **w_kwargs|
            # enforce keyword args
          end

          original_with(*args, **kwargs)
        end
      end
    end
  end
end

JonRowe commented 3 years ago

Can you add to your reproduction the missing subject and the definition of Something required to make it work?

The problem with your fix is it forces use of keyword arguments, we tried a similar setup for fixing the warnings generated internally but it caused issues on Ruby < 3 where hashes were used and were meant to be used as a last placed positional argument. This is still valid in Ruby 2.x even if you are warned on 2.7, but we can't change our implementation as both uses are valid. The solution elsewhere was to use the ruby2_keywords setup, but I know we've had some improvements to with for Ruby 3 that are unreleased, does your spec correctly fail on Ruby 3 using main?

pedrocarmona commented 3 years ago

Hi @JonRowe, thank you for your fast response.

I was on vacation, away from keyboard. I used main branch (had to tweak it to look like 3.10.3 otherwise it would not bundle in my project). After that, and using ruby 3, the test did not break (I was expecting it to break). My next step will be to prepare a gist that exposes the problem. Moreover, for this case I was using dry-initializer (could be an edge case).

Will get back to you soon. Thank you very much.

pedrocarmona commented 3 years ago

Hi @JonRowe, created a small project exemplifying the problem.

The code is deployed in heroku, and for ruby 2 it works, but in ruby 3 it does not, however in the CI the tests pass. (It would be expectable that :

test_ruby_3_0_1 rspec_mocks_next : should fail (does not fail)
test_ruby_3_0_1 rspec_mocks_3_10_2: should fail (does not fail)
test_ruby_2_7_3 rspec_mocks_next - does not fail - its ok :)
test_ruby_2_7_3 rspec_mocks_3_10_2 - does not fail - its ok :)

where next is a version that was in master at the time of bundling.

source code: https://github.com/pedrocarmona/rspec_mocks_sample/blob/main/app/controllers/checks_controller.rb test: https://github.com/pedrocarmona/rspec_mocks_sample/blob/main/spec/requests/checks_spec.rb

code deployed with ruby 2: https://rspec-mocks-sample-2.herokuapp.com/checks/new code deployed with ruby 3: https://rspec-mocks-sample-3.herokuapp.com/checks/new

tests:

https://app.circleci.com/pipelines/github/pedrocarmona/rspec_mocks_sample/15/workflows/840e3aaf-a9e5-4c79-9578-eba27d502630/jobs/57/parallel-runs/0/steps/0-107

https://app.circleci.com/pipelines/github/pedrocarmona/rspec_mocks_sample/15/workflows/840e3aaf-a9e5-4c79-9578-eba27d502630/jobs/55/parallel-runs/0/steps/0-107

https://app.circleci.com/pipelines/github/pedrocarmona/rspec_mocks_sample/15/workflows/840e3aaf-a9e5-4c79-9578-eba27d502630/jobs/54/parallel-runs/0/steps/0-107

https://app.circleci.com/pipelines/github/pedrocarmona/rspec_mocks_sample/15/workflows/840e3aaf-a9e5-4c79-9578-eba27d502630/jobs/56/parallel-runs/0/steps/0-107

JonRowe commented 3 years ago

Can you reproduce it in a small snippet rather than a cloneable repo, and definietly without Rails?

pedrocarmona commented 3 years ago

In the following script there are two types of test, one with mocks, the other without mocks.

executing this code in ruby 3:

(without mocks) 'returns the result of multipling by two' raises an error unless the patch **multiplier_params is applied
(with mocks/spies) 'calls the multiplier' does not raise error.

Currently, throughout our codebase, we use the version with mocks/spies. In my option, would be nice if those spies would raise the same error as executing the code.

require 'rspec/core'

class Multiplier
  def initialize(quotient:, data:)
    @quotient = quotient
    @data = data
  end

  def call
    @data.map { |element| element * @quotient }
  end
end

class Engine
  attr_reader :multiplier

  def initialize(data:)
    @data = data
  end

  def call
    # to work in ruby 3: Multiplier.new(**multiplier_params).call
    Multiplier.new(multiplier_params).call
  end

  def multiplier_params
    {
      data: @data,
      quotient: 2
    }
  end
end

RSpec.describe Engine do
  let(:data) { [1, 2, 3] }
  subject { Engine.new(data: data) }

  describe "#call" do
    it "returns the result of multipling by two" do
      expect(subject.call).to match_array([2, 4, 6])
    end

    context "with mocks" do
      let(:multiplier) { instance_double(Multiplier) }
      let(:multiplier_result) { 'multiplier_result' }

      before do
        allow(Multiplier).to receive(:new).and_return(multiplier)
        allow(multiplier).to receive(:call).and_return(multiplier_result)
      end

      it "calls multiplier" do
        subject.call

        expect(Multiplier).to have_received(:new).with(
          data: data,
          quotient: 2
        )

        expect(multiplier).to have_received(:call)
      end

      it "returns the multiplier result" do
        expect(subject.call).to eq(multiplier_result)
      end
    end
  end
end

Gemfile:

source 'https://rubygems.org'

ruby '3.0.1'

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

pedrocarmona commented 3 years ago

In order to fetch the required params for the initialize method, its possible to use this:

>> Multiplier.instance_method(:initialize).parameters
=> [[:keyreq, :quotient], [:keyreq, :data]]

source: https://bugs.ruby-lang.org/issues/17596

I think rspec-mocks could leverage from matching the initializer arity (as it currently does for doubles instance methods)

JonRowe commented 3 years ago

Apologies for the delay here, I've been meaning to look into this more, we do in fact already validate arguments for new using initialize, the issue here is that the allow is overly unspecific, meaning anything can happen, and because we maintain compatibility with ruby2 the later spy matches because its a hash. A normal mock expect(..).to receive(...).with fails as expected with some changes that were made in #1394

We don't raise the original error because of implementation details (there isn't an original error to raise, we are checking captured arguments vs expected arguments) and changing that behaviour is out of scope really, as an aside I note our error diff could be improved to indicate that kw args and hashes are different

vovimayhem commented 2 years ago

I got bitten by something similar on keypup-io/cloudtasker#32 comment... I couldn't fix a "false positive" test that should been failing on Ruby 3... I've got this gist exposing the problem... Even main branch is not working the way I was expecting to... :/ RSpec with is converting keywords to hashes somehow.

JonRowe commented 2 years ago

RSpec with is converting keywords to hashes somehow.

The problem is that keyword arguments are hashes everywhere in Ruby except "strict keyword" ruby method definitions. Even on 3.1 a keyword splat is a hash.

irb(main):001:0> def display_keywords(**kwargs) = puts kwargs.inspect
# => :a
irb(main):002:0> def display_keywords_class(**kwargs) = puts kwargs.class
# => :b
irb(main):003:0> display_keywords(ac: :dc)
# {:ac=>:dc}
# => nil
irb(main):004:0> display_keywords_class(ac: :dc)
# Hash
# => nil

pkondzior commented 1 year ago

They do not support it because all information about kwargs are not being preserved on method_missing calls.

Proof: https://github.com/rspec/rspec-mocks/blob/main/lib/rspec/mocks/test_double.rb#L74

Ruby 3.0 and higher is not going to be able to determine if Hash.ruby2_keywords_hash? if it does not know that it should expect them there. Flag will be trimmed and keyword arguments will be treated as positional arguments.

Same story goes to the HaveRecived Matcher which is doing neither

https://github.com/rspec/rspec-mocks/blob/main/lib/rspec/mocks/matchers/have_received.rb#L53-L56

You need to wrap your arguments into Hash.ruby2_keywords_hash to make that working and even with this, you still cannot have proper match on the called method because method_missing is skipping that flag too.

rspec / rspec-mocks