rspec-mocks icon indicating copy to clipboard operation
rspec-mocks copied to clipboard

Verifying doubles do not differentiate between a Hash and keyword args in Ruby 3

Open honigc opened this issue 1 year ago • 4 comments

Subject of the issue

When using an instance_double of a class that defines an instance method that takes keyword arguments, then using allow or expect to stub that method in the verifying double, the method can be called on the verifying double with either keywords or a Hash object passed. This happens whether or not with is used when setting up the stub.

When using live objects, passing a Hash without a double-splat raises ArgumentError in Ruby 3 while it worked fine in Ruby 2.7. The fact that tests don't fail in this situation in Ruby 3 has made upgrading Ruby in a large project more complicated.

Your environment

  • Ruby version: 3.1.2
  • rspec-mocks version: 3.11.1

Steps to reproduce

# frozen_string_literal: true

require "rubygems/source"
require "bundler/inline"

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

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

require "rspec/autorun"

RSpec.describe "args broken" do
  let(:klass) do
    Class.new do
      def m(keyword:); end
    end
  end

  it "works as expected in Ruby 3" do
    # Pass
    expect { klass.new.m(keyword: "value") }.to_not raise_error
    expect { klass.new.m(**{ keyword: "value" }) }.to_not raise_error
    expect { klass.new.m({ keyword: "value" }) }.to raise_error(ArgumentError)
  end

  it "works as expected with instance_double in Ruby 3" do
    instance = instance_double(klass)
    allow(instance).to receive(:m).with(keyword: "value")

    # Pass
    expect { instance.m(keyword: "value") }.to_not raise_error
    expect { instance.m(**{ keyword: "value" }) }.to_not raise_error
    # Fails
    expect { instance.m({ keyword: "value" }) }.to raise_error(ArgumentError)
  end
end

Expected behavior

Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
Using bundler 2.3.10
Using diff-lcs 1.5.0
Using rspec-support 3.11.0
Using rspec-core 3.11.0
Using rspec-expectations 3.11.0
Using rspec-mocks 3.11.1
Using rspec 3.11.0
..

Finished in 0.00919 seconds (files took 0.0839 seconds to load)
2 examples, 0 failures

Actual behavior

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

Failures:

  1) args broken works as expected with instance_double in Ruby 3
     Failure/Error: expect { instance.m({ keyword: "value" }) }.to raise_error(ArgumentError)
       expected ArgumentError but nothing was raised
     # rspec-mocks-bug.rb:37:in `block (2 levels) in <main>'

Finished in 0.01756 seconds (files took 0.07066 seconds to load)
2 examples, 1 failure

Failed examples:

rspec rspec-mocks-bug.rb:29 # args broken works as expected with instance_double in Ruby 3

honigc avatar Sep 04 '22 20:09 honigc

This is at least partially fixed by https://github.com/rspec/rspec-mocks/pull/1473 (when with is used to set up the stub, ArgumentError isn't raised but RSpec::Mocks::MockExpectationError is) so if the answer is to just wait for that to be released, I'd understand. When with is not used, though, there's still something I find confusing even with 1473 applied:

  it do
    instance = instance_double(klass)
    # .with is not used for the stub
    allow(instance).to receive(:m)

    # These pass, showing that arguments are verified at least a little bit
    expect { instance.m(:totally_wrong) }.to raise_error(ArgumentError)
    expect { instance.m(wrong: "value") }.to raise_error(ArgumentError)
    expect { instance.m({ wrong: "value" }) }.to raise_error(ArgumentError)
    # Fails
    expect { instance.m({ keyword: "value" }) }.to raise_error(StandardError)
  end

honigc avatar Sep 04 '22 20:09 honigc

I'm running into this with the latest rspec-mocks (version 3.12.5). It's making the upgrade from Ruby 2.7 to 3.x, extra painful.

mmercurio avatar May 15 '23 18:05 mmercurio

When with is not used, though, there's still something I find confusing even with 1473 applied:

@honigc - could you elaborate here? Is the confusing bit that RSpec::Mocks::MockExpectationError is not a subclass of StandardError? (I assume that's necessary for the broad error-handling rspec has to do to properly catch and expose the exceptions produced by the code under test as being separate from the exceptions produced by the mocks/matchers)

I'm curious if there's still any change needed here.

nevinera avatar May 07 '24 15:05 nevinera

I assume that's necessary for the broad error-handling rspec has to do to properly catch and expose the exceptions produced by the code under test as being separate from the exceptions produced by the mocks/matchers

Correct, it is because rescue by default catches StandardError, so we raise errors inherited from Exception to try to avoid your code swallowing spec failures.

JonRowe avatar May 08 '24 07:05 JonRowe