From 96a85ad7ea5bf433d80575103b281dc21afc8f96 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 18 Jul 2024 16:25:57 +0100 Subject: [PATCH 1/3] Set top_level to true in PositionalOrKeywordHashTest These tests were all originally written with the `top_level: true` behaviour in mind and so I think it's better to make this explicit. --- .../positional_or_keyword_hash_test.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) 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 005246a3..18951db4 100644 --- a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb +++ b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb @@ -11,27 +11,27 @@ class PositionalOrKeywordHashTest < Mocha::TestCase include Mocha::ParameterMatchers def test_should_describe_matcher - matcher = { key_1: 1, key_2: 2 }.to_matcher + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) assert_equal '{:key_1 => 1, :key_2 => 2}', matcher.mocha_inspect end def test_should_match_non_last_hash_arg_with_hash_arg - matcher = { key_1: 1, key_2: 2 }.to_matcher + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) 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 + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) assert !matcher.matches?([%w[a b]]) end def test_should_match_hash_arg_with_hash_arg - matcher = { key_1: 1, key_2: 2 }.to_matcher + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) assert matcher.matches?([{ key_1: 1, key_2: 2 }]) end def test_should_match_keyword_args_with_keyword_args - 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(top_level: true) # rubocop:disable Style/BracesAroundHashParameters assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end @@ -74,28 +74,28 @@ def test_should_match_keyword_args_with_hash_arg_but_display_deprecation_warning 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 + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) 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 + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) 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 + matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) 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 + 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?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end From 18448d19c80d70bca657612d3ea5ac9c662c144d Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 18 Jul 2024 16:51:48 +0100 Subject: [PATCH 2/3] Build matcher directly in PositionalOrKeywordHashTest Previously we were relying on the `#to_matcher` method to implicitly build the matcher, but since this is a *unit* test for the `PositionalOrKeywordHash` class, it seems better to build the matcher explicitly build the matcher. --- .../positional_or_keyword_hash_test.rb | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) 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 18951db4..2db66c75 100644 --- a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb +++ b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb @@ -11,38 +11,42 @@ class PositionalOrKeywordHashTest < Mocha::TestCase include Mocha::ParameterMatchers def test_should_describe_matcher - matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) assert_equal '{:key_1 => 1, :key_2 => 2}', matcher.mocha_inspect end def test_should_match_non_last_hash_arg_with_hash_arg - matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) 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(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) assert !matcher.matches?([%w[a b]]) end def test_should_match_hash_arg_with_hash_arg - matcher = { key_1: 1, key_2: 2 }.to_matcher(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) assert matcher.matches?([{ key_1: 1, key_2: 2 }]) end def test_should_match_keyword_args_with_keyword_args - matcher = Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }).to_matcher(top_level: true) # rubocop:disable Style/BracesAroundHashParameters + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })) # rubocop:disable Style/BracesAroundHashParameters assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters 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(top_level: true) # rubocop:disable Style/BracesAroundHashParameters + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: is_a(String), key_2: is_a(Integer) })) # 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: expectation, top_level: true) # rubocop:disable Style/BracesAroundHashParameters + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }), expectation) # rubocop:disable Style/BracesAroundHashParameters DeprecationDisabler.disable_deprecations do assert matcher.matches?([{ key_1: 1, key_2: 2 }]) end @@ -58,7 +62,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: expectation, top_level: true) + matcher = build_matcher({ key_1: 1, key_2: 2 }, expectation) DeprecationDisabler.disable_deprecations do assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end @@ -74,42 +78,46 @@ def test_should_match_keyword_args_with_hash_arg_but_display_deprecation_warning 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(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) 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(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) 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(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) 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(top_level: true) # rubocop:disable Style/BracesAroundHashParameters + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })) # 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(top_level: true) # rubocop:disable Style/BracesAroundHashParameters + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })) # 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(top_level: true) + hash = { key_1: 1, key_2: 2 } + matcher = build_matcher(hash) 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 +125,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: expectation, top_level: true) + matcher = build_matcher({ key_1: 1, key_2: 2 }, expectation) DeprecationDisabler.disable_deprecations do matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end @@ -127,4 +135,10 @@ def test_should_display_deprecation_warning_even_if_parent_expectation_is_nil assert_includes message, 'but received keyword arguments (key_1: 1, key_2: 2)' end end + + private + + def build_matcher(hash, expectation = nil) + Mocha::ParameterMatchers::PositionalOrKeywordHash.new(hash, expectation) + end end From 5e6a07b2710dac76e9346def561ca0d44765bf86 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 22 Jul 2024 12:30:08 +0100 Subject: [PATCH 3/3] Require exact match for top-level Hash parameter In this commit [1] which fixed #647 and which was released in v2.3.0, a top-level `Hash` argument passed into `Expectation#with` was implicitly converted into a `HasEntries` matcher in order to handle "nested" matchers used within the keys & values of the `Hash`. This _new_ behaviour makes particular sense for keyword arguments where the requirement to wrap such a `Hash` in a call to `#has_entries` was a bit surprising as described in #647. However, that change inadvertently broke basic `Hash` parameter matching as described in #657, because the `HasEntries` matcher only needs to match each of the entries specified; it will still match if there are *extra* entries in the `Hash` it is matching against. In order to fix this without breaking the "nested" matching behaviour added in [1], I've added an optional `exact` keyword argument to the `HasEntries` constructor and set this to `true` when it's called from `PositionalOrKeywordHash#matches?`. I haven't bothered to document the new `exact` argument, because currently it's only used internally. I did consider introducing a new matcher more like `RSpec::Matchers#match` matcher [2], but I decided that wouldn't provide as elegant a solution to #647 as the change in this commit. I can imagine introducing something like `#has_exact_entries` to the public API, but it would probably make sense to add something similar for exact matching of array items at the same time, so I'm going to leave that for now. I've added a few acceptance tests in this commit which would have caught the problem that I inadvertently introduced in [1] and a few that cover the "nested" matching behaviour that was introduced in that commit (previously I had only added a unit test). [1]: https://github.com/freerange/mocha/commit/f94e250414335d822ec8d0a1331cb25cf90bf837 [2]: https://www.rubydoc.info/github/rspec/rspec-expectations/RSpec/Matchers#match-instance_method --- lib/mocha/parameter_matchers/has_entries.rb | 7 ++- .../positional_or_keyword_hash.rb | 3 +- test/acceptance/parameter_matcher_test.rb | 45 +++++++++++++++++++ .../positional_or_keyword_hash_test.rb | 16 +++++++ 4 files changed, 68 insertions(+), 3 deletions(-) diff --git a/lib/mocha/parameter_matchers/has_entries.rb b/lib/mocha/parameter_matchers/has_entries.rb index 1b6d18d8..2b90f34b 100644 --- a/lib/mocha/parameter_matchers/has_entries.rb +++ b/lib/mocha/parameter_matchers/has_entries.rb @@ -30,20 +30,23 @@ def has_entries(entries) # rubocop:disable Naming/PredicateName # Parameter matcher which matches when actual parameter contains all expected +Hash+ entries. class HasEntries < Base # @private - def initialize(entries) + def initialize(entries, exact: false) @entries = entries + @exact = exact end # @private def matches?(available_parameters) parameter = available_parameters.shift + return false if @exact && @entries.length != parameter.length + has_entry_matchers = @entries.map { |key, value| HasEntry.new(key, value) } AllOf.new(*has_entry_matchers).matches?([parameter]) end # @private def mocha_inspect - "has_entries(#{@entries.mocha_inspect})" + @exact ? @entries.mocha_inspect : "has_entries(#{@entries.mocha_inspect})" end end end diff --git a/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb b/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb index d9b77c42..341d314b 100644 --- a/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb +++ b/lib/mocha/parameter_matchers/positional_or_keyword_hash.rb @@ -1,6 +1,7 @@ require 'mocha/configuration' require 'mocha/deprecation' require 'mocha/parameter_matchers/base' +require 'mocha/parameter_matchers/has_entries' module Mocha module ParameterMatchers @@ -14,7 +15,7 @@ def initialize(value, expectation) def matches?(available_parameters) parameter, is_last_parameter = extract_parameter(available_parameters) - return false unless HasEntries.new(@value).matches?([parameter]) + return false unless HasEntries.new(@value, exact: true).matches?([parameter]) if is_last_parameter && !same_type_of_hash?(parameter, @value) return false if Mocha.configuration.strict_keyword_argument_matching? diff --git a/test/acceptance/parameter_matcher_test.rb b/test/acceptance/parameter_matcher_test.rb index d982539f..49cb5822 100644 --- a/test/acceptance/parameter_matcher_test.rb +++ b/test/acceptance/parameter_matcher_test.rb @@ -11,6 +11,24 @@ def teardown teardown_acceptance_test end + def test_should_match_hash_parameter_which_is_exactly_the_same + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(key_1: 'value_1') + mock.method(key_1: 'value_1') + end + assert_passed(test_result) + end + + def test_should_not_match_hash_parameter_which_is_not_exactly_the_same + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(key_1: 'value_1') + mock.method(key_1: 'value_1', key_2: 'value_2') + end + assert_failed(test_result) + end + def test_should_match_hash_parameter_with_specified_key test_result = run_as_test do mock = mock() @@ -137,6 +155,33 @@ def test_should_not_match_hash_parameter_with_specified_entries_using_nested_mat assert_failed(test_result) end + def test_should_match_hash_parameter_that_is_exactly_a_key_that_is_a_string_with_a_value_that_is_an_integer + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(is_a(String) => is_a(Integer)) + mock.method('key_1' => 123) + end + assert_passed(test_result) + end + + def test_should_not_match_hash_parameter_that_is_exactly_a_key_that_is_a_string_with_a_value_that_is_an_integer_because_value_not_integer + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(is_a(String) => is_a(Integer)) + mock.method('key_1' => '123') + end + assert_failed(test_result) + end + + def test_should_not_match_hash_parameter_that_is_exactly_a_key_that_is_a_string_with_a_value_that_is_an_integer_because_of_extra_entry + test_result = run_as_test do + mock = mock() + mock.expects(:method).with(is_a(String) => is_a(Integer)) + mock.method('key_1' => 123, 'key_2' => 'doesntmatter') + end + assert_failed(test_result) + end + def test_should_match_parameter_that_matches_any_value 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 2db66c75..591a01b9 100644 --- a/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb +++ b/test/unit/parameter_matchers/positional_or_keyword_hash_test.rb @@ -34,16 +34,32 @@ def test_should_match_hash_arg_with_hash_arg assert matcher.matches?([{ key_1: 1, key_2: 2 }]) end + def test_should_not_match_hash_arg_with_different_hash_arg + hash = { key_1: 1 } + matcher = build_matcher(hash) + assert !matcher.matches?([{ key_1: 1, key_2: 2 }]) + end + def test_should_match_keyword_args_with_keyword_args matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })) # rubocop:disable Style/BracesAroundHashParameters assert matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters end + def test_should_not_match_keyword_args_with_different_keyword_args + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1 })) # rubocop:disable Style/BracesAroundHashParameters + assert !matcher.matches?([Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 })]) # rubocop:disable Style/BracesAroundHashParameters + end + def test_should_match_keyword_args_with_matchers_using_keyword_args matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: is_a(String), key_2: is_a(Integer) })) # 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_not_match_keyword_args_with_matchers_using_keyword_args_when_not_all_entries_are_matched + matcher = build_matcher(Hash.ruby2_keywords_hash({ key_1: is_a(String) })) # 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 = build_matcher(Hash.ruby2_keywords_hash({ key_1: 1, key_2: 2 }), expectation) # rubocop:disable Style/BracesAroundHashParameters