From be24b41bf94757b36f536f690714611fbe94a73e Mon Sep 17 00:00:00 2001 From: James Mead Date: Fri, 12 Jul 2024 13:19:23 +0100 Subject: [PATCH 1/4] Fix regression in ParameterMatchers#has_entry, etc This regression was caused by the fix for nested parameter matching for keyword arguments in #648. The arguments passed in to `#has_entry` (and probably other matchers) were inadvertently triggering the behaviour in `ParameterMatchers::PositionalOrKeywordHash` when really that was only intended to be used for arguments passed directly to a stubbed method. In #648, this line [1], which is effectively the same as using `ParameterMatchers::Equal`, was replaced [2] by use of `ParameterMatchers::HasEntries`. While the latter was suitable for arguments passed directly to a stubbed method, it was not suitable for arguments passed to matcher methods like `#has_entry`. Now the behaviour in `ParameterMatchers::PositionalOrKeywordHash` is only triggered for arguments passed directly to a stubbed method. This is achieved using a new `top_level` argument for the `#to_matcher` method. This `top_level` is only set to `true` in `ParametersMatcher#matchers` which is deep in the call stack from `Mock#method_missing` as part of the expectation matching process. I'm not completely happy with the name of `top_level`, but I can't think of anything better at the moment. Fixes #654. [1]: https://github.com/freerange/mocha/blob/5324009b296939c0a0da9d05c13b823a6dceb643/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb#L16 [2]: https://github.com/freerange/mocha/blob/f94e250414335d822ec8d0a1331cb25cf90bf837/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb#L17 --- lib/mocha/parameter_matchers/base.rb | 2 +- .../parameter_matchers/instance_methods.rb | 10 +++++++--- lib/mocha/parameters_matcher.rb | 2 +- .../keyword_argument_matching_test.rb | 18 ++++++++++++++++++ .../positional_or_keyword_hash_test.rb | 12 ++++++------ 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/lib/mocha/parameter_matchers/base.rb b/lib/mocha/parameter_matchers/base.rb index 52725d70e..a8244106a 100644 --- a/lib/mocha/parameter_matchers/base.rb +++ b/lib/mocha/parameter_matchers/base.rb @@ -3,7 +3,7 @@ module ParameterMatchers # @abstract Subclass and implement +#matches?+ and +#mocha_inspect+ to define a custom matcher. Also add a suitably named instance method to {ParameterMatchers} to build an instance of the new matcher c.f. {#equals}. class Base # @private - def to_matcher(_expectation = nil) + def to_matcher(_expectation = nil, _top_level = false) self end diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index 8b41464d4..d52024cae 100644 --- a/lib/mocha/parameter_matchers/instance_methods.rb +++ b/lib/mocha/parameter_matchers/instance_methods.rb @@ -6,7 +6,7 @@ module ParameterMatchers # @private module InstanceMethods # @private - def to_matcher(_expectation = nil) + def to_matcher(_expectation = nil, _top_level = false) Mocha::ParameterMatchers::Equals.new(self) end end @@ -21,7 +21,11 @@ class Object # @private class Hash # @private - def to_matcher(expectation = nil) - Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self, expectation) + def to_matcher(expectation = nil, top_level = false) + if top_level + Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self, expectation) + else + Mocha::ParameterMatchers::Equals.new(self) + end end end diff --git a/lib/mocha/parameters_matcher.rb b/lib/mocha/parameters_matcher.rb index 6cd434294..3daac2e07 100644 --- a/lib/mocha/parameters_matcher.rb +++ b/lib/mocha/parameters_matcher.rb @@ -28,7 +28,7 @@ def mocha_inspect end def matchers - @expected_parameters.map { |p| p.to_matcher(@expectation) } + @expected_parameters.map { |p| p.to_matcher(@expectation, true) } end end end diff --git a/test/acceptance/keyword_argument_matching_test.rb b/test/acceptance/keyword_argument_matching_test.rb index be5861cdf..8988d3905 100644 --- a/test/acceptance/keyword_argument_matching_test.rb +++ b/test/acceptance/keyword_argument_matching_test.rb @@ -198,6 +198,24 @@ def test_should_match_keyword_args_with_nested_matcher assert_passed(test_result) end + def test_should_match_keyword_args_with_matcher_built_using_keyword_args + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(has_entry(:k1, k2: 'v2')) + mock.method(k1: { k2: 'v2' }) + end + assert_passed(test_result) + end + + def test_should_not_match_keyword_args_with_matcher_built_using_keyword_args + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(has_entry(:k1, k2: 'v2')) + mock.method(k1: { k2: 'v2', k3: 'v3' }) + end + assert_failed(test_result) + end + def test_should_match_last_positional_hash_with_hash_matcher test_result = run_as_test do mock = mock() diff --git a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb index 9e6c5e665..fe4c1c77f 100644 --- a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb +++ b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb @@ -36,13 +36,13 @@ def test_should_match_keyword_args_with_keyword_args end def test_should_match_keyword_args_with_matchers_using_keyword_args - matcher = Hash.ruby2_keywords_hash({ key_1: is_a(String), key_2: is_a(Integer) }).to_matcher # rubocop:disable Style/BracesAroundHashParameters + matcher = Hash.ruby2_keywords_hash({ key_1: is_a(String), key_2: is_a(Integer) }).to_matcher(nil, true) # rubocop:disable Style/BracesAroundHashParameters assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 'foo', key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end def test_should_match_hash_arg_with_keyword_args_but_display_deprecation_warning_if_appropriate expectation = Mocha::Expectation.new(self, :foo); execution_point = ExecutionPoint.current - matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(expectation) # rubocop:disable Style/BracesAroundHashParameters + matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(expectation, true) # rubocop:disable Style/BracesAroundHashParameters DeprecationDisabler.disable_deprecations do assert matcher.matches?([{ key_1: 1, key_2: 2 }]) end @@ -58,7 +58,7 @@ def test_should_match_hash_arg_with_keyword_args_but_display_deprecation_warning def test_should_match_keyword_args_with_hash_arg_but_display_deprecation_warning_if_appropriate expectation = Mocha::Expectation.new(self, :foo); execution_point = ExecutionPoint.current - matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation) + matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation, true) DeprecationDisabler.disable_deprecations do assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end @@ -102,14 +102,14 @@ def test_should_match_keyword_args_with_keyword_args_when_strict_keyword_args_is 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 + matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(nil, true) # 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 + matcher = { key_1: 1, key_2: 2 }.to_matcher(nil, true) 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 @@ -117,7 +117,7 @@ def test_should_not_match_keyword_args_with_hash_arg_when_strict_keyword_args_is def test_should_display_deprecation_warning_even_if_parent_expectation_is_nil expectation = nil - matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation) + matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation, true) DeprecationDisabler.disable_deprecations do matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end From 600ee2aaa769b489034bac070363dd4d8dd418b3 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 16 Jul 2024 11:41:30 +0100 Subject: [PATCH 2/4] Reduce duplication in to_matcher methods --- .../parameter_matchers/instance_methods.rb | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index d52024cae..8d7ea059d 100644 --- a/lib/mocha/parameter_matchers/instance_methods.rb +++ b/lib/mocha/parameter_matchers/instance_methods.rb @@ -6,8 +6,12 @@ module ParameterMatchers # @private module InstanceMethods # @private - def to_matcher(_expectation = nil, _top_level = false) - Mocha::ParameterMatchers::Equals.new(self) + def to_matcher(expectation = nil, top_level = false) + if is_a?(Hash) && top_level + Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self, expectation) + else + Mocha::ParameterMatchers::Equals.new(self) + end end end end @@ -17,15 +21,3 @@ def to_matcher(_expectation = nil, _top_level = false) class Object include Mocha::ParameterMatchers::InstanceMethods end - -# @private -class Hash - # @private - def to_matcher(expectation = nil, top_level = false) - if top_level - Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self, expectation) - else - Mocha::ParameterMatchers::Equals.new(self) - end - end -end From e9de64e493508457163a060220d3052d031af11f Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 16 Jul 2024 11:54:14 +0100 Subject: [PATCH 3/4] Consolidate to_matcher definitions in one place --- lib/mocha/parameter_matchers/base.rb | 5 ----- lib/mocha/parameter_matchers/instance_methods.rb | 5 ++++- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/mocha/parameter_matchers/base.rb b/lib/mocha/parameter_matchers/base.rb index a8244106a..8c1fa22d8 100644 --- a/lib/mocha/parameter_matchers/base.rb +++ b/lib/mocha/parameter_matchers/base.rb @@ -2,11 +2,6 @@ module Mocha module ParameterMatchers # @abstract Subclass and implement +#matches?+ and +#mocha_inspect+ to define a custom matcher. Also add a suitably named instance method to {ParameterMatchers} to build an instance of the new matcher c.f. {#equals}. class Base - # @private - def to_matcher(_expectation = nil, _top_level = false) - self - end - # A shorthand way of combining two matchers when both must match. # # Returns a new {AllOf} parameter matcher combining two matchers using a logical AND. diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index 8d7ea059d..a169bbb3d 100644 --- a/lib/mocha/parameter_matchers/instance_methods.rb +++ b/lib/mocha/parameter_matchers/instance_methods.rb @@ -1,3 +1,4 @@ +require 'mocha/parameter_matchers/base' require 'mocha/parameter_matchers/equals' require 'mocha/parameter_matchers/positional_or_keyword_hash' @@ -7,7 +8,9 @@ module ParameterMatchers module InstanceMethods # @private def to_matcher(expectation = nil, top_level = false) - if is_a?(Hash) && top_level + if is_a?(Base) + self + elsif is_a?(Hash) && top_level Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self, expectation) else Mocha::ParameterMatchers::Equals.new(self) From 3b60b7df390bce19b928099281c9af8bccbe8472 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 16 Jul 2024 12:01:38 +0100 Subject: [PATCH 4/4] Change to_matcher method to take keyword arguments --- lib/mocha/parameter_matchers/instance_methods.rb | 2 +- lib/mocha/parameters_matcher.rb | 2 +- .../positional_or_keyword_hash_test.rb | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index a169bbb3d..d86da04fd 100644 --- a/lib/mocha/parameter_matchers/instance_methods.rb +++ b/lib/mocha/parameter_matchers/instance_methods.rb @@ -7,7 +7,7 @@ module ParameterMatchers # @private module InstanceMethods # @private - def to_matcher(expectation = nil, top_level = false) + def to_matcher(expectation: nil, top_level: false) if is_a?(Base) self elsif is_a?(Hash) && top_level diff --git a/lib/mocha/parameters_matcher.rb b/lib/mocha/parameters_matcher.rb index 3daac2e07..72645fe61 100644 --- a/lib/mocha/parameters_matcher.rb +++ b/lib/mocha/parameters_matcher.rb @@ -28,7 +28,7 @@ def mocha_inspect end def matchers - @expected_parameters.map { |p| p.to_matcher(@expectation, true) } + @expected_parameters.map { |p| p.to_matcher(expectation: @expectation, top_level: true) } 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 index fe4c1c77f..005246a3d 100644 --- a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb +++ b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb @@ -36,13 +36,13 @@ def test_should_match_keyword_args_with_keyword_args end def test_should_match_keyword_args_with_matchers_using_keyword_args - matcher = Hash.ruby2_keywords_hash({ key_1: is_a(String), key_2: is_a(Integer) }).to_matcher(nil, true) # rubocop:disable Style/BracesAroundHashParameters + matcher = Hash.ruby2_keywords_hash({ key_1: is_a(String), key_2: is_a(Integer) }).to_matcher(top_level: true) # rubocop:disable Style/BracesAroundHashParameters assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 'foo', key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end def test_should_match_hash_arg_with_keyword_args_but_display_deprecation_warning_if_appropriate expectation = Mocha::Expectation.new(self, :foo); execution_point = ExecutionPoint.current - matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(expectation, true) # rubocop:disable Style/BracesAroundHashParameters + matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(expectation: expectation, top_level: true) # rubocop:disable Style/BracesAroundHashParameters DeprecationDisabler.disable_deprecations do assert matcher.matches?([{ key_1: 1, key_2: 2 }]) end @@ -58,7 +58,7 @@ def test_should_match_hash_arg_with_keyword_args_but_display_deprecation_warning def test_should_match_keyword_args_with_hash_arg_but_display_deprecation_warning_if_appropriate expectation = Mocha::Expectation.new(self, :foo); execution_point = ExecutionPoint.current - matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation, true) + matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation: expectation, top_level: true) DeprecationDisabler.disable_deprecations do assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end @@ -102,14 +102,14 @@ def test_should_match_keyword_args_with_keyword_args_when_strict_keyword_args_is 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(nil, true) # rubocop:disable Style/BracesAroundHashParameters + matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(top_level: true) # 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(nil, true) + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) 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 @@ -117,7 +117,7 @@ def test_should_not_match_keyword_args_with_hash_arg_when_strict_keyword_args_is def test_should_display_deprecation_warning_even_if_parent_expectation_is_nil expectation = nil - matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation, true) + matcher = { key_1: 1, key_2: 2 }.to_matcher(expectation: expectation, top_level: true) DeprecationDisabler.disable_deprecations do matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end