Skip to content

Commit

Permalink
Require exact match for top-level Hash parameter
Browse files Browse the repository at this point in the history
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]: f94e250
[2]: https://www.rubydoc.info/github/rspec/rspec-expectations/RSpec/Matchers#match-instance_method
  • Loading branch information
floehopper committed Jul 22, 2024
1 parent 18448d1 commit 5e6a07b
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 3 deletions.
7 changes: 5 additions & 2 deletions lib/mocha/parameter_matchers/has_entries.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/mocha/parameter_matchers/positional_or_keyword_hash.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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?
Expand Down
45 changes: 45 additions & 0 deletions test/acceptance/parameter_matcher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
16 changes: 16 additions & 0 deletions test/unit/parameter_matchers/positional_or_keyword_hash_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 5e6a07b

Please sign in to comment.