From 910849d9912f8418a0d0c00ebb1a92f9205b6285 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 31 Dec 2022 16:18:21 +0000 Subject: [PATCH] WIP: Relax keyword argument matching When a method doesn't accept keyword arguments. In this scenario keyword or Hash-type arguments are assigned as a single Hash to the last argument without any warnings and strict keyword matching should not have any effect. This is an exploratory spike on fixing #593. * This has highlighted a significant problem with partial mocks in #532. The method obtained from the responder is the stub method defined by Mocha and not the original. This effectively makes it useless! * I'm not sure the method_accepts_keyword_arguments? belongs on Invocation, but that's the most convenient place for now. It feels as if we need to have a bit of a sort out of where various things live and perhaps introduce some new classes to make things clearer. * We might want to think ahead a bit at what we want to do in #149 to decide the best way to go about this. * I'm not sure it's sensible to re-use the Equals matcher; we could instead parameterize PositionalOrKeywordHash, although the logic in there is already quite complex. Conversely if this is a good approach, it might make more sense to do something similar when creating a hash matcher for a non-last parameter to further simplify the code. * I haven't yet introduced any acceptance tests for this and I suspect there might be some edge cases yet to come out of the woodwork. In particular, I think it's worth exhaustively working through the various references mentioned in this comment [1]. [1]: https://github.com/freerange/mocha/issues/593#issuecomment-1365967966 --- lib/mocha/expectation.rb | 2 +- lib/mocha/invocation.rb | 9 ++++- lib/mocha/mock.rb | 2 +- .../parameter_matchers/instance_methods.rb | 4 +- lib/mocha/parameters_matcher.rb | 15 ++++---- test/unit/parameters_matcher_test.rb | 37 +++++++++++-------- 6 files changed, 41 insertions(+), 28 deletions(-) diff --git a/lib/mocha/expectation.rb b/lib/mocha/expectation.rb index edf7d0b9..0355afd6 100644 --- a/lib/mocha/expectation.rb +++ b/lib/mocha/expectation.rb @@ -694,7 +694,7 @@ def matches_method?(method_name) # @private def match?(invocation, ignoring_order: false) - order_independent_match = @method_matcher.match?(invocation.method_name) && @parameters_matcher.match?(invocation.arguments) && @block_matcher.match?(invocation.block) + order_independent_match = @method_matcher.match?(invocation.method_name) && @parameters_matcher.match?(invocation) && @block_matcher.match?(invocation.block) ignoring_order ? order_independent_match : order_independent_match && in_correct_order? end diff --git a/lib/mocha/invocation.rb b/lib/mocha/invocation.rb index cc15c8ba..55f9b70c 100644 --- a/lib/mocha/invocation.rb +++ b/lib/mocha/invocation.rb @@ -8,11 +8,12 @@ module Mocha class Invocation attr_reader :method_name, :block - def initialize(mock, method_name, arguments = [], block = nil) + def initialize(mock, method_name, arguments = [], block = nil, responder = nil) @mock = mock @method_name = method_name @arguments = arguments @block = block + @responder = responder @yields = [] @result = nil end @@ -63,6 +64,12 @@ def full_description "\n - #{call_description} #{result_description}" end + def method_accepts_keyword_arguments? + return true unless @responder + + @responder.method(@method_name).parameters.any? { |k, _v| %i[keyreq key keyrest].include?(k) } + end + private def argument_description diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index 02e45bc8..27d76a60 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -322,7 +322,7 @@ def method_missing(symbol, *arguments, &block) def handle_method_call(symbol, arguments, block) check_expiry check_responder_responds_to(symbol) - invocation = Invocation.new(self, symbol, arguments, block) + invocation = Invocation.new(self, symbol, arguments, block, @responder) matching_expectations = all_expectations.matching_expectations(invocation) diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index 20a8e537..2db7762d 100644 --- a/lib/mocha/parameter_matchers/instance_methods.rb +++ b/lib/mocha/parameter_matchers/instance_methods.rb @@ -7,10 +7,10 @@ module ParameterMatchers # @private module InstanceMethods # @private - def to_matcher(expectation: nil, top_level: false) + def to_matcher(expectation: nil, top_level: false, method_accepts_keyword_arguments: true) if Base === self self - elsif Hash === self && top_level + elsif Hash === self && (top_level || method_accepts_keyword_arguments) Mocha::ParameterMatchers::PositionalOrKeywordHash.new(self, expectation) else Mocha::ParameterMatchers::Equals.new(self) diff --git a/lib/mocha/parameters_matcher.rb b/lib/mocha/parameters_matcher.rb index da1abb54..3e8d7532 100644 --- a/lib/mocha/parameters_matcher.rb +++ b/lib/mocha/parameters_matcher.rb @@ -9,18 +9,15 @@ def initialize(expected_parameters = [ParameterMatchers::AnyParameters.new], exp @matching_block = matching_block end - def match?(actual_parameters = []) + def match?(invocation) + actual_parameters = invocation.arguments || [] if @matching_block @matching_block.call(*actual_parameters) else - parameters_match?(actual_parameters) + matchers(invocation).all? { |matcher| matcher.matches?(actual_parameters) } && actual_parameters.empty? end end - def parameters_match?(actual_parameters) - matchers.all? { |matcher| matcher.matches?(actual_parameters) } && actual_parameters.empty? - end - def mocha_inspect if @matching_block '(arguments_accepted_by_custom_matching_block)' @@ -31,8 +28,10 @@ def mocha_inspect end end - def matchers - @expected_parameters.map { |p| p.to_matcher(expectation: @expectation, top_level: true) } + def matchers(invocation = nil) + @expected_parameters.map do |p| + p.to_matcher(expectation: @expectation, top_level: true, method_accepts_keyword_arguments: invocation ? invocation.method_accepts_keyword_arguments? : true) + end end end end diff --git a/test/unit/parameters_matcher_test.rb b/test/unit/parameters_matcher_test.rb index a97d2d4a..27db6db2 100644 --- a/test/unit/parameters_matcher_test.rb +++ b/test/unit/parameters_matcher_test.rb @@ -1,90 +1,91 @@ require File.expand_path('../../test_helper', __FILE__) require 'mocha/parameters_matcher' +require 'mocha/invocation' class ParametersMatcherTest < Mocha::TestCase include Mocha def test_should_match_any_actual_parameters_if_no_expected_parameters_specified parameters_matcher = ParametersMatcher.new - assert parameters_matcher.match?([1, 2, 3]) + assert parameters_matcher.match?(invocation_with_arguments([1, 2, 3])) end def test_should_match_if_actual_parameters_are_same_as_expected_parameters parameters_matcher = ParametersMatcher.new([4, 5, 6]) - assert parameters_matcher.match?([4, 5, 6]) + assert parameters_matcher.match?(invocation_with_arguments([4, 5, 6])) end def test_should_not_match_if_actual_parameters_are_different_from_expected_parameters parameters_matcher = ParametersMatcher.new([4, 5, 6]) - assert !parameters_matcher.match?([1, 2, 3]) + assert !parameters_matcher.match?(invocation_with_arguments([1, 2, 3])) end def test_should_not_match_if_there_are_less_actual_parameters_than_expected_parameters parameters_matcher = ParametersMatcher.new([4, 5, 6]) - assert !parameters_matcher.match?([4, 5]) + assert !parameters_matcher.match?(invocation_with_arguments([4, 5])) end def test_should_not_match_if_there_are_more_actual_parameters_than_expected_parameters parameters_matcher = ParametersMatcher.new([4, 5]) - assert !parameters_matcher.match?([4, 5, 6]) + assert !parameters_matcher.match?(invocation_with_arguments([4, 5, 6])) end def test_should_not_match_if_not_all_required_parameters_are_supplied optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert !parameters_matcher.match?([4]) + assert !parameters_matcher.match?(invocation_with_arguments([4])) end def test_should_match_if_all_required_parameters_match_and_no_optional_parameters_are_supplied optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert parameters_matcher.match?([4, 5]) + assert parameters_matcher.match?(invocation_with_arguments([4, 5])) end def test_should_match_if_all_required_and_optional_parameters_match_and_some_optional_parameters_are_supplied optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert parameters_matcher.match?([4, 5, 6]) + assert parameters_matcher.match?(invocation_with_arguments([4, 5, 6])) end def test_should_match_if_all_required_and_optional_parameters_match_and_all_optional_parameters_are_supplied optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert parameters_matcher.match?([4, 5, 6, 7]) + assert parameters_matcher.match?(invocation_with_arguments([4, 5, 6, 7])) end def test_should_not_match_if_all_required_and_optional_parameters_match_but_too_many_optional_parameters_are_supplied optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert !parameters_matcher.match?([4, 5, 6, 7, 8]) + assert !parameters_matcher.match?(invocation_with_arguments([4, 5, 6, 7, 8])) end def test_should_not_match_if_all_required_parameters_match_but_some_optional_parameters_do_not_match optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert !parameters_matcher.match?([4, 5, 6, 0]) + assert !parameters_matcher.match?(invocation_with_arguments([4, 5, 6, 0])) end def test_should_not_match_if_some_required_parameters_do_not_match_although_all_optional_parameters_do_match optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert !parameters_matcher.match?([4, 0, 6]) + assert !parameters_matcher.match?(invocation_with_arguments([4, 0, 6])) end def test_should_not_match_if_all_required_parameters_match_but_no_optional_parameters_match optionals = ParameterMatchers::Optionally.new(6, 7) parameters_matcher = ParametersMatcher.new([4, 5, optionals]) - assert !parameters_matcher.match?([4, 5, 0, 0]) + assert !parameters_matcher.match?(invocation_with_arguments([4, 5, 0, 0])) end def test_should_match_if_actual_parameters_satisfy_matching_block parameters_matcher = ParametersMatcher.new { |x, y| x + y == 3 } - assert parameters_matcher.match?([1, 2]) + assert parameters_matcher.match?(invocation_with_arguments([1, 2])) end def test_should_not_match_if_actual_parameters_do_not_satisfy_matching_block parameters_matcher = ParametersMatcher.new { |x, y| x + y == 3 } - assert !parameters_matcher.match?([2, 3]) + assert !parameters_matcher.match?(invocation_with_arguments([2, 3])) end def test_should_remove_outer_array_braces @@ -108,4 +109,10 @@ def test_should_indicate_that_matcher_logic_is_defined_by_custom_block parameters_matcher = ParametersMatcher.new { true } assert_equal '(arguments_accepted_by_custom_matching_block)', parameters_matcher.mocha_inspect end + + private + + def invocation_with_arguments(arguments) + Invocation.new(nil, nil, arguments, nil, nil) + end end