From 906e106734c381ffbf50c28f2931aaefda1824e1 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 +- lib/mocha/parameter_matchers/instance_methods.rb | 10 +++++++--- lib/mocha/parameters_matcher.rb | 13 +++++-------- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/lib/mocha/expectation.rb b/lib/mocha/expectation.rb index 240747c7..95ef5883 100644 --- a/lib/mocha/expectation.rb +++ b/lib/mocha/expectation.rb @@ -639,7 +639,7 @@ def matches_method?(method_name) # @private def match?(invocation) - @method_matcher.match?(invocation.method_name) && @parameters_matcher.match?(invocation.arguments) && @block_matcher.match?(invocation.block) && in_correct_order? + @method_matcher.match?(invocation.method_name) && @parameters_matcher.match?(invocation) && @block_matcher.match?(invocation.block) && in_correct_order? end # @private diff --git a/lib/mocha/invocation.rb b/lib/mocha/invocation.rb index a45dcaa7..864bbc92 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 @@ -62,6 +63,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 e6e7e873..ed80f023 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -321,7 +321,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) if (matching_expectation_allowing_invocation = all_expectations.match_allowing_invocation(invocation)) matching_expectation_allowing_invocation.invoke(invocation) elsif (matching_expectation = all_expectations.match(invocation)) || (!matching_expectation && !@everything_stubbed) diff --git a/lib/mocha/parameter_matchers/instance_methods.rb b/lib/mocha/parameter_matchers/instance_methods.rb index 8b41464d..43df37ec 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, _method_accepts_keyword_arguments = true) 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, method_accepts_keyword_arguments = true) + if method_accepts_keyword_arguments + 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 6cd43429..df2f3815 100644 --- a/lib/mocha/parameters_matcher.rb +++ b/lib/mocha/parameters_matcher.rb @@ -9,26 +9,23 @@ 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 signature = matchers.mocha_inspect signature = signature.gsub(/^\[|\]$/, '') "(#{signature})" end - def matchers - @expected_parameters.map { |p| p.to_matcher(@expectation) } + def matchers(invocation) + @expected_parameters.map { |p| p.to_matcher(@expectation, invocation.method_accepts_keyword_arguments?) } end end end