From 6f9e43d2091620ce04e2da456c7b727994485e19 Mon Sep 17 00:00:00 2001 From: wasabigeek Date: Sun, 25 Sep 2022 21:57:25 +0800 Subject: [PATCH 1/7] Add acceptance tests for keyword arg matching This is to better characterize the existing behaviour. It should also make it clearer what changes when strict keyword argument matching is enabled in a subsequent commit. --- .../keyword_argument_matching_test.rb | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 test/acceptance/keyword_argument_matching_test.rb diff --git a/test/acceptance/keyword_argument_matching_test.rb b/test/acceptance/keyword_argument_matching_test.rb new file mode 100644 index 000000000..50e1b400a --- /dev/null +++ b/test/acceptance/keyword_argument_matching_test.rb @@ -0,0 +1,112 @@ +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 + + 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 + + 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 + + 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 + + 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 From a3defd5229e47f3c27554c3b061e38b5aafcd8f3 Mon Sep 17 00:00:00 2001 From: Nicholas Koh Date: Thu, 29 Sep 2022 23:05:14 +0800 Subject: [PATCH 2/7] Add tests to characterize inspection of Hashes We're planning to change how this works once we're explicitly distinguishing between keyword and positional Hashes. This should make it easier to see what changes. Co-authored-by: Nicholas Koh --- ...positional_and_keyword_has_inspect_test.rb | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 test/acceptance/positional_and_keyword_has_inspect_test.rb diff --git a/test/acceptance/positional_and_keyword_has_inspect_test.rb b/test/acceptance/positional_and_keyword_has_inspect_test.rb new file mode 100644 index 000000000..f98e3dc36 --- /dev/null +++ b/test/acceptance/positional_and_keyword_has_inspect_test.rb @@ -0,0 +1,63 @@ +class PositionalAndKeywordHashInspectTest < Mocha::TestCase + include AcceptanceTest + + def setup + setup_acceptance_test + end + + def teardown + teardown_acceptance_test + end + + def test_single_hash_parameters_in_invocation_and_expectation_print_correctly + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with({ :foo => 42 }) # rubocop:disable Style/BracesAroundHashParameters + mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters + end + assert_equal [ + 'unexpected invocation: #.method(:key => 42)', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(:foo => 42)' + ], test_result.failure_message_lines + end + + def test_unexpected_keyword_arguments_in_invocation_and_expectation_print_correctly + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with(:foo => 42) + mock.method(:key => 42) + end + assert_equal [ + 'unexpected invocation: #.method(:key => 42)', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(:foo => 42)' + ], test_result.failure_message_lines + end + + def test_last_hash_parameters_in_invocation_and_expectation_print_correctly + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with(1, { :foo => 42 }) # rubocop:disable Style/BracesAroundHashParameters + mock.method(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters + end + assert_equal [ + 'unexpected invocation: #.method(1, {:key => 42})', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(1, {:foo => 42})' + ], test_result.failure_message_lines + end + + def test_unexpected_keyword_arguments_with_other_positionals_in_invocation_and_expectation_print_correctly + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with(1, :foo => 42) + mock.method(1, :key => 42) + end + assert_equal [ + 'unexpected invocation: #.method(1, {:key => 42})', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(1, {:foo => 42})' + ], test_result.failure_message_lines + end +end From 7d534d0c0eb4fc0685639c834109b7e18cd37ba2 Mon Sep 17 00:00:00 2001 From: wasabigeek Date: Tue, 20 Sep 2022 22:40:01 +0800 Subject: [PATCH 3/7] Add strict keyword arg matching config option This option doesn't currently do anything, but in a subsequent commit it will determine whether to perform strict keyword argument comparision. This option is turned off by default to enable gradual adoption, but may be turned on by default in the future. This option is only available in Ruby >= v2.7. --- lib/mocha/configuration.rb | 56 ++++++++++++++++++++++++++++++++- lib/mocha/ruby_version.rb | 1 + test/unit/configuration_test.rb | 22 +++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/lib/mocha/configuration.rb b/lib/mocha/configuration.rb index 352959af6..1e8cfb618 100644 --- 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 >= 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 + # {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+. # diff --git a/lib/mocha/ruby_version.rb b/lib/mocha/ruby_version.rb index 989a46fb5..a752a63c9 100644 --- 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/test/unit/configuration_test.rb b/test/unit/configuration_test.rb index 016bc164f..b3c998e2e 100644 --- a/test/unit/configuration_test.rb +++ b/test/unit/configuration_test.rb @@ -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 + def test_allow_temporarily_changes_config_when_given_block Mocha.configure { |c| c.stubbing_method_unnecessarily = :warn } yielded = false @@ -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 !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 + Mocha.configure { |c| c.strict_keyword_argument_matching = true } + end + end + end end From 636d4633d2f91a02450f653deb4241d7712a0532 Mon Sep 17 00:00:00 2001 From: wasabigeek Date: Sun, 25 Sep 2022 22:14:19 +0800 Subject: [PATCH 4/7] Mark methods passing keyword args via args splat This marks Expectation#with, Mock#method_missing, and the method defined by StubbedMethod#define_new_method. See [1,2] for more information about how Module#ruby2_keywords works. The ruby2_keywords shim gem [3] provides an empty Module#ruby2_keywords method, for forward source-level compatibility against Ruby v2.7 and Ruby v3. [1]: https://ruby-doc.org/core-3.1.2/Module.html#method-i-ruby2_keywords [2]: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/ [3]: https://github.com/ruby/ruby2_keywords --- Gemfile | 2 ++ lib/mocha/expectation.rb | 2 ++ lib/mocha/mock.rb | 2 ++ lib/mocha/stubbed_method.rb | 2 ++ test/unit/any_instance_method_test.rb | 2 +- test/unit/instance_method_test.rb | 2 +- 6 files changed, 10 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 7b7b8ca87..0113a7f7a 100644 --- 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/expectation.rb b/lib/mocha/expectation.rb index 4e71b555a..915003800 100644 --- 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/mock.rb b/lib/mocha/mock.rb index 80c895d88..8894a58e9 100644 --- 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/stubbed_method.rb b/lib/mocha/stubbed_method.rb index fa6a7cadb..75f773ff3 100644 --- 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/unit/any_instance_method_test.rb b/test/unit/any_instance_method_test.rb index 15f5dd791..d23e0057d 100644 --- a/test/unit/any_instance_method_test.rb +++ b/test/unit/any_instance_method_test.rb @@ -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| diff --git a/test/unit/instance_method_test.rb b/test/unit/instance_method_test.rb index 36a2a7e31..0845d3d36 100644 --- a/test/unit/instance_method_test.rb +++ b/test/unit/instance_method_test.rb @@ -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| From 336ab3bd860cc7583cc8eec9ea57e465c39b8ebf Mon Sep 17 00:00:00 2001 From: wasabigeek Date: Wed, 28 Sep 2022 22:49:07 +0800 Subject: [PATCH 5/7] Implement optional strict keyword arg matching 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). 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 --- lib/mocha/configuration.rb | 8 +- .../parameter_matchers/instance_methods.rb | 9 +++ .../positional_or_keyword_hash.rb | 33 +++++++++ .../keyword_argument_matching_test.rb | 65 +++++++++++++++++ .../positional_or_keyword_hash_test.rb | 73 +++++++++++++++++++ 5 files changed, 184 insertions(+), 4 deletions(-) create mode 100644 lib/mocha/parameter_matchers/positional_or_keyword_hash.rb create mode 100644 test/unit/parameter_matchers/positional_or_keyword_hash_test.rb diff --git a/lib/mocha/configuration.rb b/lib/mocha/configuration.rb index 1e8cfb618..6fdbd7779 100644 --- a/lib/mocha/configuration.rb +++ b/lib/mocha/configuration.rb @@ -306,16 +306,16 @@ 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. + # 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 >= 3.0 (see examples below). For more details on keyword arguments in Ruby 3, refer to the relevant - # {https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0 blog post}. + # 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 is turned off by default to enable gradual adoption, and may be turned on by default in the future. + # 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. diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index 5bb229cd4..e4430d675 100644 --- 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 new file mode 100644 index 000000000..f8e9f8fba --- /dev/null +++ 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/test/acceptance/keyword_argument_matching_test.rb b/test/acceptance/keyword_argument_matching_test.rb index 50e1b400a..0adaf59e7 100644 --- a/test/acceptance/keyword_argument_matching_test.rb +++ b/test/acceptance/keyword_argument_matching_test.rb @@ -20,6 +20,19 @@ def test_should_match_hash_parameter_with_keyword_args 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() @@ -29,6 +42,19 @@ def test_should_match_hash_parameter_with_splatted_keyword_args 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() @@ -56,6 +82,19 @@ def test_should_match_positional_and_keyword_args_with_last_positional_hash 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() @@ -65,6 +104,19 @@ def test_should_match_last_positional_hash_with_keyword_args 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() @@ -109,4 +161,17 @@ def test_should_match_last_positional_hash_with_hash_matcher 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 diff --git a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb new file mode 100644 index 000000000..c810ff582 --- /dev/null +++ b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb @@ -0,0 +1,73 @@ +require File.expand_path('../../../test_helper', __FILE__) + +require 'mocha/parameter_matchers/positional_or_keyword_hash' +require 'mocha/parameter_matchers/instance_methods' +require 'mocha/inspect' + +class PositionalOrKeywordHashTest < Mocha::TestCase + include Mocha::ParameterMatchers + + def test_should_describe_matcher + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + assert_equal '{:key_1 => 1, :key_2 => 2}', matcher.mocha_inspect + end + + def test_should_match_hash_arg_with_hash_arg + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + assert matcher.matches?([{ :key_1 => 1, :key_2 => 2 }]) + end + + def test_should_match_non_last_hash_arg_with_hash_arg + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + assert matcher.matches?([{ :key_1 => 1, :key_2 => 2 }, %w[a b]]) + end + + def test_should_not_match_non_hash_arg_with_hash_arg + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + assert !matcher.matches?([%w[a b]]) + end + + if Mocha::RUBY_V27_PLUS + def test_should_match_non_last_hash_arg_with_hash_arg_when_strict_keyword_args_is_enabled + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + assert matcher.matches?([{ :key_1 => 1, :key_2 => 2 }, %w[a b]]) + end + end + + def test_should_not_match_non_hash_arg_with_hash_arg_when_strict_keyword_args_is_enabled + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + assert !matcher.matches?([%w[a b]]) + end + end + + def test_should_match_hash_arg_with_hash_arg_when_strict_keyword_args_is_enabled + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + assert matcher.matches?([{ :key_1 => 1, :key_2 => 2 }]) + end + end + + def test_should_match_keyword_args_with_keyword_args_when_strict_keyword_args_is_enabled + matcher = Hash.ruby2_keywords_hash({ :key_1 => 1, :key_2 => 2 }).to_matcher # rubocop:disable Style/BracesAroundHashParameters + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + assert matcher.matches?([Hash.ruby2_keywords_hash({ :key_1 => 1, :key_2 => 2 })]) # rubocop:disable Style/BracesAroundHashParameters + end + end + + def test_should_not_match_hash_arg_with_keyword_args_when_strict_keyword_args_is_enabled + matcher = Hash.ruby2_keywords_hash({ :key_1 => 1, :key_2 => 2 }).to_matcher # rubocop:disable Style/BracesAroundHashParameters + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + assert !matcher.matches?([{ :key_1 => 1, :key_2 => 2 }]) + end + end + + def test_should_not_match_keyword_args_with_hash_arg_when_strict_keyword_args_is_enabled + matcher = { :key_1 => 1, :key_2 => 2 }.to_matcher + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + assert !matcher.matches?([Hash.ruby2_keywords_hash({ :key_1 => 1, :key_2 => 2 })]) # rubocop:disable Style/BracesAroundHashParameters + end + end + end +end From e6c8c27ae79970f15c4c298e0cb8d2e7d07ee13a Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 9 Oct 2022 15:09:42 +0100 Subject: [PATCH 6/7] Always wrap Hash inspection in braces Previously in the description of an invocation or of a parameter matcher, a single Hash argument was displayed without wrapping braces. The reasoning behind this seems to be lost in the mists of time, but I suspect it pre-dated keyword arguments and was an attempt at reflecting the way you don't/didn't need to wrap some Hash arguments with braces when invoking a method. However, even if it did make sense at one point, it no longer does now that we have keyword arguments, so it feels simpler to always wrap a Hash argument in braces. I'm hoping this will also make it easier to see how adding strict keyword matching will change the behaviour. Co-authored-by: Nicholas Koh --- lib/mocha/invocation.rb | 1 - lib/mocha/parameters_matcher.rb | 1 - .../positional_and_keyword_has_inspect_test.rb | 8 ++++---- test/unit/parameters_matcher_test.rb | 13 ------------- 4 files changed, 4 insertions(+), 19 deletions(-) diff --git a/lib/mocha/invocation.rb b/lib/mocha/invocation.rb index 2ac911792..6740fac59 100644 --- 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/parameters_matcher.rb b/lib/mocha/parameters_matcher.rb index 2b6ed8481..7bc9579aa 100644 --- 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/test/acceptance/positional_and_keyword_has_inspect_test.rb b/test/acceptance/positional_and_keyword_has_inspect_test.rb index f98e3dc36..57180cf5a 100644 --- a/test/acceptance/positional_and_keyword_has_inspect_test.rb +++ b/test/acceptance/positional_and_keyword_has_inspect_test.rb @@ -16,9 +16,9 @@ def test_single_hash_parameters_in_invocation_and_expectation_print_correctly mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters end assert_equal [ - 'unexpected invocation: #.method(:key => 42)', + 'unexpected invocation: #.method({:key => 42})', 'unsatisfied expectations:', - '- expected exactly once, invoked never: #.method(:foo => 42)' + '- expected exactly once, invoked never: #.method({:foo => 42})' ], test_result.failure_message_lines end @@ -29,9 +29,9 @@ def test_unexpected_keyword_arguments_in_invocation_and_expectation_print_correc mock.method(:key => 42) end assert_equal [ - 'unexpected invocation: #.method(:key => 42)', + 'unexpected invocation: #.method({:key => 42})', 'unsatisfied expectations:', - '- expected exactly once, invoked never: #.method(:foo => 42)' + '- expected exactly once, invoked never: #.method({:foo => 42})' ], test_result.failure_message_lines end diff --git a/test/unit/parameters_matcher_test.rb b/test/unit/parameters_matcher_test.rb index 91959dde6..53ffafa7d 100644 --- a/test/unit/parameters_matcher_test.rb +++ b/test/unit/parameters_matcher_test.rb @@ -99,19 +99,6 @@ def test_should_display_numeric_arguments_as_is assert_equal '(1, 2, 3)', parameters_matcher.mocha_inspect end - def test_should_remove_curly_braces_if_hash_is_only_argument - params = [{ :a => 1, :z => 2 }] - parameters_matcher = ParametersMatcher.new(params) - assert_nil parameters_matcher.mocha_inspect.index('{') - assert_nil parameters_matcher.mocha_inspect.index('}') - end - - def test_should_not_remove_curly_braces_if_hash_is_not_the_only_argument - params = [1, { :a => 1 }] - parameters_matcher = ParametersMatcher.new(params) - assert_equal '(1, {:a => 1})', parameters_matcher.mocha_inspect - end - def test_should_indicate_that_matcher_will_match_any_actual_parameters parameters_matcher = ParametersMatcher.new assert_equal '(any_parameters)', parameters_matcher.mocha_inspect From ef0fa0c9894f31faae260d4656796a2940bb09e5 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 9 Oct 2022 15:24:09 +0100 Subject: [PATCH 7/7] Improve inspection of positional & keyword Hashes With the introduction of strict keyword matching, we're explicitly distinguishing between positional and keyword Hashes. However, this meant the descriptions of Hash arguments in invocations and parameter matchers had become confusing. This attempts to make that less confusing by always wrapping a Hash in braces unless we *know* its a keyword Hash, in which case don't wrap it in braces. This means that in earlier versions of Ruby where we can't distinguish between a positional and keyword Hash, it will always be displayed wrapped in braces. Co-authored-by: Nicholas Koh --- lib/mocha/inspect.rb | 2 +- ...positional_and_keyword_has_inspect_test.rb | 98 +++++++++++++++++-- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/lib/mocha/inspect.rb b/lib/mocha/inspect.rb index 8ad4e26ac..fe34d6487 100755 --- 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/test/acceptance/positional_and_keyword_has_inspect_test.rb b/test/acceptance/positional_and_keyword_has_inspect_test.rb index 57180cf5a..5e131c8d8 100644 --- a/test/acceptance/positional_and_keyword_has_inspect_test.rb +++ b/test/acceptance/positional_and_keyword_has_inspect_test.rb @@ -28,11 +28,19 @@ def test_unexpected_keyword_arguments_in_invocation_and_expectation_print_correc mock.expects(:method).with(:foo => 42) mock.method(:key => 42) end - assert_equal [ - 'unexpected invocation: #.method({:key => 42})', - 'unsatisfied expectations:', - '- expected exactly once, invoked never: #.method({:foo => 42})' - ], test_result.failure_message_lines + if Mocha::RUBY_V27_PLUS + assert_equal [ + 'unexpected invocation: #.method(:key => 42)', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(:foo => 42)' + ], test_result.failure_message_lines + else + assert_equal [ + 'unexpected invocation: #.method({:key => 42})', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method({:foo => 42})' + ], test_result.failure_message_lines + end end def test_last_hash_parameters_in_invocation_and_expectation_print_correctly @@ -54,10 +62,80 @@ def test_unexpected_keyword_arguments_with_other_positionals_in_invocation_and_e mock.expects(:method).with(1, :foo => 42) mock.method(1, :key => 42) end - assert_equal [ - 'unexpected invocation: #.method(1, {:key => 42})', - 'unsatisfied expectations:', - '- expected exactly once, invoked never: #.method(1, {:foo => 42})' - ], test_result.failure_message_lines + if Mocha::RUBY_V27_PLUS + assert_equal [ + 'unexpected invocation: #.method(1, :key => 42)', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(1, :foo => 42)' + ], test_result.failure_message_lines + else + assert_equal [ + 'unexpected invocation: #.method(1, {:key => 42})', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(1, {:foo => 42})' + ], test_result.failure_message_lines + end + end + + if Mocha::RUBY_V27_PLUS + def test_single_hash_parameters_in_invocation_and_expectation_print_correctly_when_strict_keyword_args_is_enabled + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with({ :foo => 42 }) # rubocop:disable Style/BracesAroundHashParameters + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + mock.method({ :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters + end + end + assert_equal [ + 'unexpected invocation: #.method({:key => 42})', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method({:foo => 42})' + ], test_result.failure_message_lines + end + + def test_unexpected_keyword_arguments_in_invocation_and_expectation_print_correctly_when_strict_keyword_args_is_enabled + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with(:foo => 42) + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + mock.method(:key => 42) + end + end + assert_equal [ + 'unexpected invocation: #.method(:key => 42)', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(:foo => 42)' + ], test_result.failure_message_lines + end + + def test_last_hash_parameters_in_invocation_and_expectation_print_correctly_when_strict_keyword_args_is_enabled + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with(1, { :foo => 42 }) # rubocop:disable Style/BracesAroundHashParameters + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + mock.method(1, { :key => 42 }) # rubocop:disable Style/BracesAroundHashParameters + end + end + assert_equal [ + 'unexpected invocation: #.method(1, {:key => 42})', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(1, {:foo => 42})' + ], test_result.failure_message_lines + end + + def test_unexpected_keyword_arguments_with_other_positionals_in_invocation_and_expectation_print_correctly_when_strict_keyword_args_is_enabled + test_result = run_as_test do + mock = mock('mock') + mock.expects(:method).with(1, :foo => 42) + Mocha::Configuration.override(:strict_keyword_argument_matching => true) do + mock.method(1, :key => 42) + end + end + assert_equal [ + 'unexpected invocation: #.method(1, :key => 42)', + 'unsatisfied expectations:', + '- expected exactly once, invoked never: #.method(1, :foo => 42)' + ], test_result.failure_message_lines + end end end