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

Support strict keyword argument matching #554

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@ if ENV['MOCHA_GENERATE_DOCS']
gem 'redcarpet'
gem 'yard'
end

gem 'ruby2_keywords', '~> 0.0.5'
56 changes: 55 additions & 1 deletion lib/mocha/configuration.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'mocha/ruby_version'

module Mocha
# Allows setting of configuration options. See {Configuration} for the available options.
#
Expand Down Expand Up @@ -43,7 +45,8 @@ class Configuration
:stubbing_non_public_method => :allow,
:stubbing_method_on_nil => :prevent,
:display_matching_invocations_on_failure => false,
:reinstate_undocumented_behaviour_from_v1_9 => true
:reinstate_undocumented_behaviour_from_v1_9 => true,
floehopper marked this conversation as resolved.
Show resolved Hide resolved
:strict_keyword_argument_matching => false
}.freeze

attr_reader :options
Expand Down Expand Up @@ -303,6 +306,57 @@ def reinstate_undocumented_behaviour_from_v1_9?
@options[:reinstate_undocumented_behaviour_from_v1_9]
end

# Configure whether to perform strict keyword argument comparision. Only supported in Ruby >= 2.7.
#
# Without this option, positional hash and keyword arguments are treated the same during comparison, which can lead to false
# negatives in Ruby >= 3.0 (see examples below). For more details on keyword arguments in Ruby 3, refer to the relevant
floehopper marked this conversation as resolved.
Show resolved Hide resolved
# {https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0 blog post}.
#
# Note that Hash matchers such as +has_value+ or +has_key+ will still treat positional hash and keyword arguments the same,
# so false negatives are still possible when they are used.
#
# This is turned off by default to enable gradual adoption, and may be turned on by default in the future.
#
# When +value+ is +true+, strict keyword argument matching will be enabled.
# When +value+ is +false+, strict keyword argument matching is disabled. This is the default.
#
# @param [Symbol] value one of +true+, +false+.
#
# @example Loose keyword argument matching (default)
#
# class Example
# def foo(a, bar:); end
# end
#
# example = Example.new
# example.expects(:foo).with('a', bar: 'b')
# example.foo('a', { bar: 'b' })
# # This passes the test, but would result in an ArgumentError in practice
#
# @example Strict keyword argument matching
#
# Mocha.configure do |c|
# c.strict_keyword_argument_matching = true
# end
#
# class Example
# def foo(a, bar:); end
# end
#
# example = Example.new
# example.expects(:foo).with('a', bar: 'b')
# example.foo('a', { bar: 'b' })
# # This now fails as expected
def strict_keyword_argument_matching=(value)
raise 'Strict keyword argument matching requires Ruby 2.7 and above.' unless Mocha::RUBY_V27_PLUS
@options[:strict_keyword_argument_matching] = value
end

# @private
def strict_keyword_argument_matching?
@options[:strict_keyword_argument_matching]
end

class << self
# Allow the specified +action+.
#
Expand Down
2 changes: 2 additions & 0 deletions lib/mocha/expectation.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'ruby2_keywords'
require 'mocha/method_matcher'
require 'mocha/parameters_matcher'
require 'mocha/expectation_error'
Expand Down Expand Up @@ -222,6 +223,7 @@ def with(*expected_parameters, &matching_block)
@parameters_matcher = ParametersMatcher.new(expected_parameters, &matching_block)
self
end
ruby2_keywords(:with)

# Modifies expectation so that the expected method must be called with a block.
#
Expand Down
6 changes: 5 additions & 1 deletion lib/mocha/mock.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'ruby2_keywords'
require 'mocha/expectation'
require 'mocha/expectation_list'
require 'mocha/invocation'
Expand Down Expand Up @@ -308,9 +309,12 @@ def all_expectations
end

# @private
def method_missing(symbol, *arguments, &block) # rubocop:disable Style/MethodMissingSuper
# rubocop:disable Style/MethodMissingSuper
def method_missing(symbol, *arguments, &block)
handle_method_call(symbol, arguments, block)
end
ruby2_keywords(:method_missing)
# rubocop:enable Style/MethodMissingSuper

# @private
def handle_method_call(symbol, arguments, block)
Expand Down
9 changes: 9 additions & 0 deletions lib/mocha/parameter_matchers/instance_methods.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'mocha/parameter_matchers/equals'
require 'mocha/parameter_matchers/positional_or_keyword_hash'

module Mocha
module ParameterMatchers
Expand All @@ -16,3 +17,11 @@ def to_matcher
class Object
include Mocha::ParameterMatchers::InstanceMethods
end

# @private
class Hash
floehopper marked this conversation as resolved.
Show resolved Hide resolved
# @private
def to_matcher
Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self)
end
end
31 changes: 31 additions & 0 deletions lib/mocha/parameter_matchers/positional_or_keyword_hash.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
require 'mocha/configuration'
require 'mocha/parameter_matchers/base'

module Mocha
module ParameterMatchers
# @private
class PositionalOrKeywordHash < Base
Copy link
Contributor Author

@wasabigeek wasabigeek Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to a better name. Note that naming this Hash caused some constant lookup failures due to namespacing e.g. in HasEntry#parse_option, so I thought it'd be safer to name this something else (instead of checking and changing all Hashes in the codebase to explicit reference the top-level Hash).

Also, should this be # @private?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to a better name. Note that naming this Hash caused some constant lookup failures due to namespacing e.g. in HasEntry#parse_option, so I thought it'd be safer to name this something else (instead of checking and changing all Hashes in the codebase to explicit reference the top-level Hash).

That makes sense to me and I can't immediately think of a better name. 👍

Also, should this be # @private?

Yes, I think the whole class could be marked as private for documentation purposes since I think it's only used internally. That way you could remove the method-level YARD annotations.

def initialize(value)
@value = value
end

def matches?(available_parameters)
parameter, is_last_parameter = extract_parameter(available_parameters)
if is_last_parameter && Mocha.configuration.strict_keyword_argument_matching?
return false unless ::Hash.ruby2_keywords_hash?(parameter) == ::Hash.ruby2_keywords_hash?(@value)
Copy link
Contributor Author

@wasabigeek wasabigeek Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually prefer that we don't couple to ruby2_keywords here, but couldn't think of a significantly better alternative yet:

  • The Hash is frozen, so we can't change any instance variable on it, e.g. to assign a different attribute. An alternative might be to wrap the Hash, but I'm not sure if I'd break other assumptions in the code.
  • Explicitly separating keyword arguments is tricky for now because we don't want to introduce a breaking change (e.g. hash matchers like has_value will still do "loose matching"). A fair amount of matching logic also relies on iterating and shifting an array of params (e.g. any_parameters).

Would suggest to revisit if/when we can turn on strict keyword matching by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually prefer that we don't couple to ruby2_keywords here, but couldn't think of a significantly better alternative yet:

Sorry, my brain's a bit fried - what are the downsides of coupling to ruby2_keywords?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly this statement:

This method will probably be removed at some point, as it exists only for backwards compatibility

I'd expect that it wouldn't be removed till Ruby 2 is end-of-life, but it still gives me a bit of pause, and would be nice if we could limit it to the "edges" of the code (i.e. invocation / expectation)

end
parameter == @value
end

def mocha_inspect
@value.mocha_inspect
end

private

def extract_parameter(available_parameters)
[available_parameters.shift, available_parameters.empty?]
end
end
end
end
1 change: 1 addition & 0 deletions lib/mocha/ruby_version.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'mocha/deprecation'

module Mocha
RUBY_V27_PLUS = Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('2.7')
end
2 changes: 2 additions & 0 deletions lib/mocha/stubbed_method.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'ruby2_keywords'
require 'mocha/ruby_version'

module Mocha
Expand Down Expand Up @@ -45,6 +46,7 @@ def define_new_method
stub_method_owner.send(:define_method, method_name) do |*args, &block|
self_in_scope.mock.handle_method_call(method_name_in_scope, args, block)
end
stub_method_owner.send(:ruby2_keywords, method_name)
retain_original_visibility(stub_method_owner)
end

Expand Down
164 changes: 164 additions & 0 deletions test/acceptance/keyword_arguments_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
require File.expand_path('../acceptance_test_helper', __FILE__)

class KeywordArgumentsTest < Mocha::TestCase
include AcceptanceTest

def setup
setup_acceptance_test
end

def teardown
teardown_acceptance_test
end

def test_should_match_hash_parameter_with_keyword_args
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(:key => 42)
mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
assert_passed(test_result)
end

if Mocha::RUBY_V27_PLUS
def test_should_not_match_hash_parameter_with_keyword_args_when_strict_keyword_matching_is_enabled
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(:key => 42)
Mocha::Configuration.override(:strict_keyword_argument_matching => true) do
mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
end
assert_failed(test_result)
end
end

def test_should_match_hash_parameter_with_splatted_keyword_args
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(**{ :key => 42 })
mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
assert_passed(test_result)
end

if Mocha::RUBY_V27_PLUS
def test_not_should_match_hash_parameter_with_splatted_keyword_args_when_strict_keyword_matching_is_enabled
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(**{ :key => 42 })
Mocha::Configuration.override(:strict_keyword_argument_matching => true) do
mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
end
assert_failed(test_result)
end
end

def test_should_match_splatted_hash_parameter_with_keyword_args
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(:key => 42)
mock.method(**{ :key => 42 })
end
assert_passed(test_result)
end

def test_should_match_splatted_hash_parameter_with_splatted_hash
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(**{ :key => 42 })
mock.method(**{ :key => 42 })
end
assert_passed(test_result)
end

def test_should_match_positional_and_keyword_args_with_last_positional_hash
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
mock.method(1, :key => 42)
end
assert_passed(test_result)
end

if Mocha::RUBY_V27_PLUS
def test_should_not_match_positional_and_keyword_args_with_last_positional_hash_when_strict_keyword_args_is_enabled
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
Mocha::Configuration.override(:strict_keyword_argument_matching => true) do
mock.method(1, :key => 42)
end
end
assert_failed(test_result)
end
end

def test_should_match_last_positional_hash_with_keyword_args
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, :key => 42)
mock.method(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
assert_passed(test_result)
end

if Mocha::RUBY_V27_PLUS
def test_should_not_match_last_positional_hash_with_keyword_args_when_strict_keyword_args_is_enabled
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, :key => 42)
Mocha::Configuration.override(:strict_keyword_argument_matching => true) do
mock.method(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
end
assert_failed(test_result)
end
end

def test_should_match_positional_and_keyword_args_with_keyword_args
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, :key => 42)
mock.method(1, :key => 42)
end
assert_passed(test_result)
end

def test_should_match_hash_parameter_with_hash_matcher
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(has_key(:key))
mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
assert_passed(test_result)
end

def test_should_match_splatted_hash_parameter_with_hash_matcher
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(has_key(:key))
mock.method(**{ :key => 42 })
end
assert_passed(test_result)
end

def test_should_match_positional_and_keyword_args_with_hash_matcher
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, has_key(:key))
mock.method(1, :key => 42)
end
assert_passed(test_result)
end

def test_should_match_last_positional_hash_with_hash_matcher
test_result = run_as_test do
mock = mock()
mock.expects(:method).with(1, has_key(:key))
mock.method(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters
end
assert_passed(test_result)
end
end
2 changes: 1 addition & 1 deletion test/unit/any_instance_method_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def test_should_include_the_filename_and_line_number_in_exceptions
method.define_new_method

expected_filename = 'stubbed_method.rb'
expected_line_number = 46
expected_line_number = 47

exception = assert_raises(Exception) { klass.new.method_x }
matching_line = exception.backtrace.find do |line|
Expand Down
22 changes: 22 additions & 0 deletions test/unit/configuration_test.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
require File.expand_path('../../test_helper', __FILE__)
require 'mocha/configuration'
require 'mocha/ruby_version'

class ConfigurationTest < Mocha::TestCase
def teardown
Mocha::Configuration.reset_configuration
end

floehopper marked this conversation as resolved.
Show resolved Hide resolved
def test_allow_temporarily_changes_config_when_given_block
Mocha.configure { |c| c.stubbing_method_unnecessarily = :warn }
yielded = false
Expand Down Expand Up @@ -34,4 +39,21 @@ def test_warn_when_temporarily_changes_config_when_given_block
assert yielded
assert_equal :allow, Mocha.configuration.stubbing_method_unnecessarily
end

def test_strict_keyword_argument_matching_works_is_false_by_default
assert_equal false, Mocha.configuration.strict_keyword_argument_matching?
end

if Mocha::RUBY_V27_PLUS
def test_enabling_strict_keyword_argument_matching_works_in_ruby_2_7_and_above
Mocha.configure { |c| c.strict_keyword_argument_matching = true }
assert Mocha.configuration.strict_keyword_argument_matching?
end
else
def test_enabling_strict_keyword_argument_matching_raises_error_if_below_ruby_2_7
assert_raises(RuntimeError, 'Strict keyword argument matching requires Ruby 2.7 and above.') do
floehopper marked this conversation as resolved.
Show resolved Hide resolved
Mocha.configure { |c| c.strict_keyword_argument_matching = true }
end
end
end
end
2 changes: 1 addition & 1 deletion test/unit/instance_method_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def test_should_include_the_filename_and_line_number_in_exceptions
method.define_new_method

expected_filename = 'stubbed_method.rb'
expected_line_number = 46
expected_line_number = 47

exception = assert_raises(Exception) { klass.method_x }
matching_line = exception.backtrace.find do |line|
Expand Down