Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
honigc opened this issue Sep 4, 2022 · 4 comments

Comments

@honigc
Copy link

honigc commented Sep 4, 2022

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
Copy link
Author

honigc commented Sep 4, 2022

This is at least partially fixed by #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

@mmercurio
Copy link

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.

@nevinera
Copy link
Contributor

nevinera commented May 7, 2024

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.

@JonRowe
Copy link
Member

JonRowe commented May 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants