Support strict keyword argument matching

Closes #562. This introduces a new `strict_keyword_argument_matching` configuration option. This option is only available in Ruby >= v2.7 and is disabled by default to enable gradual adoption. When the strict keyword argument option is enabled, an expectation expecting keyword arguments (via `Expectation#with`) will no longer match an invocation passing a positional Hash argument. Without this option, positional hash and keyword arguments are treated the same during comparison, which can lead to false negatives in Ruby >= v3.0 (see examples below). * 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 * 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 For more details on keyword arguments in Ruby v3, refer to this article [1]. 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. Closes #446. See also #535 & #544 for discussions relating to this change. [1]: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0 Co-authored-by: Nicholas Koh <[email protected]>
freerange · Oct 12, 2022 · 2e72eb5 · 2e72eb5
2 parents eec7200 + ef0fa0c
commit 2e72eb5
Show file tree

Hide file tree

Showing 18 changed files with 522 additions and 19 deletions.
diff --git a/Gemfile b/Gemfile
@@ -28,3 +28,5 @@ if ENV['MOCHA_GENERATE_DOCS']
   gem 'redcarpet'
   gem 'yard'
 end
+
+gem 'ruby2_keywords', '~> 0.0.5'
diff --git a/lib/mocha/configuration.rb b/lib/mocha/configuration.rb
@@ -1,3 +1,5 @@
+require 'mocha/ruby_version'
+
 module Mocha
   # Allows setting of configuration options. See {Configuration} for the available options.
   #
@@ -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,
+      :strict_keyword_argument_matching => false
     }.freeze
 
     attr_reader :options
@@ -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 >= v2.7.
+    #
+    # Without this option, positional hash and keyword arguments are treated the same during comparison, which can lead to false
+    # negatives in Ruby >= v3.0 (see examples below). For more details on keyword arguments in Ruby v3, refer to
+    # {https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0 this article}.
+    #
+    # 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 configuration option is turned off by default to enable gradual adoption, but 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+.
       #

diff --git a/lib/mocha/expectation.rb b/lib/mocha/expectation.rb
@@ -1,3 +1,4 @@
+require 'ruby2_keywords'
 require 'mocha/method_matcher'
 require 'mocha/parameters_matcher'
 require 'mocha/expectation_error'
@@ -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.
     #

diff --git a/lib/mocha/inspect.rb b/lib/mocha/inspect.rb
@@ -20,7 +20,7 @@ def mocha_inspect(wrapped = true)
     module HashMethods
       def mocha_inspect
         unwrapped = collect { |key, value| "#{key.mocha_inspect} => #{value.mocha_inspect}" }.join(', ')
-        "{#{unwrapped}}"
+        Hash.ruby2_keywords_hash?(self) ? unwrapped : "{#{unwrapped}}"
       end
     end
 

diff --git a/lib/mocha/invocation.rb b/lib/mocha/invocation.rb
@@ -79,7 +79,6 @@ def full_description
     def argument_description
       signature = arguments.mocha_inspect
       signature = signature.gsub(/^\[|\]$/, '')
-      signature = signature.gsub(/^\{|\}$/, '') if arguments.length == 1
       "(#{signature})"
     end
   end

diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb
@@ -1,3 +1,4 @@
+require 'ruby2_keywords'
 require 'mocha/expectation'
 require 'mocha/expectation_list'
 require 'mocha/invocation'
@@ -312,6 +313,7 @@ def all_expectations
     def method_missing(symbol, *arguments, &block)
       handle_method_call(symbol, arguments, block)
     end
+    ruby2_keywords(:method_missing)
     # rubocop:enable Style/MethodMissingSuper
 
     # @private

diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb
@@ -1,4 +1,5 @@
 require 'mocha/parameter_matchers/equals'
+require 'mocha/parameter_matchers/positional_or_keyword_hash'
 
 module Mocha
   module ParameterMatchers
@@ -16,3 +17,11 @@ def to_matcher
 class Object
   include Mocha::ParameterMatchers::InstanceMethods
 end
+
+# @private
+class Hash
+  # @private
+  def to_matcher
+    Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self)
+  end
+end
diff --git a/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb b/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb
@@ -0,0 +1,33 @@
+require 'mocha/configuration'
+require 'mocha/parameter_matchers/base'
+
+module Mocha
+  module ParameterMatchers
+    # @private
+    class PositionalOrKeywordHash < Base
+      def initialize(value)
+        @value = value
+      end
+
+      def matches?(available_parameters)
+        parameter, is_last_parameter = extract_parameter(available_parameters)
+        return false unless parameter.is_a?(Hash)
+
+        if is_last_parameter && Mocha.configuration.strict_keyword_argument_matching?
+          return false unless ::Hash.ruby2_keywords_hash?(parameter) == ::Hash.ruby2_keywords_hash?(@value)
+        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
diff --git a/lib/mocha/parameters_matcher.rb b/lib/mocha/parameters_matcher.rb
@@ -23,7 +23,6 @@ def parameters_match?(actual_parameters)
     def mocha_inspect
       signature = matchers.mocha_inspect
       signature = signature.gsub(/^\[|\]$/, '')
-      signature = signature.gsub(/^\{|\}$/, '') if matchers.length == 1
       "(#{signature})"
     end
 

diff --git a/lib/mocha/ruby_version.rb b/lib/mocha/ruby_version.rb
@@ -1,4 +1,5 @@
 require 'mocha/deprecation'
 
 module Mocha
+  RUBY_V27_PLUS = Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('2.7')
 end
diff --git a/lib/mocha/stubbed_method.rb b/lib/mocha/stubbed_method.rb
@@ -1,3 +1,4 @@
+require 'ruby2_keywords'
 require 'mocha/ruby_version'
 
 module Mocha
@@ -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
 

diff --git a/test/acceptance/keyword_argument_matching_test.rb b/test/acceptance/keyword_argument_matching_test.rb
@@ -0,0 +1,177 @@
+require File.expand_path('../acceptance_test_helper', __FILE__)
+
+class KeywordArgumentMatchingTest < 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_should_not_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
+
+  if Mocha::RUBY_V27_PLUS
+    def test_should_not_match_non_hash_args_with_keyword_args
+      test_result = run_as_test do
+        mock = mock()
+        mock.expects(:method).with(**{ :key => 1 })
+        Mocha::Configuration.override(:strict_keyword_argument_matching => true) do
+          mock.method([2])
+        end
+      end
+      assert_failed(test_result)
+    end
+  end
+end