From 42da8346614ea1bb25d3e8f961a788ad1d589267 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 15 Aug 2022 15:53:02 +0100 Subject: [PATCH 1/2] Implicitly set responder on partial mocks We now automatically set a responder on mock object which are used for partial mocks. Having made the change above, I had to set include_all to true for the call to Object#respond_to? in Mock#check_responder_responds_to in order to fix a load of broken tests. The legacy_behaviour_for_array_flatten condition in Mock#check_responder_responds_to is needed to avoid a regression of #580 in Ruby < v2.3. Hopefully this is a small step towards having Configuration.prevent(:stubbing_non_existent_method) check Method#arity and/or Method#parameters (#149) and rationalising Configuration.stubbing_non_existent_method= & Mock#responds_like (#531). --- lib/mocha/class_methods.rb | 2 +- lib/mocha/mock.rb | 11 ++++++++--- lib/mocha/object_methods.rb | 2 +- lib/mocha/ruby_version.rb | 1 + test/acceptance/responds_like_test.rb | 16 ++++++++-------- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/lib/mocha/class_methods.rb b/lib/mocha/class_methods.rb index 2ccd58fd3..7b2fc32fe 100644 --- a/lib/mocha/class_methods.rb +++ b/lib/mocha/class_methods.rb @@ -12,7 +12,7 @@ def initialize(klass) def mocha(instantiate = true) if instantiate - @mocha ||= Mocha::Mockery.instance.mock_impersonating_any_instance_of(@stubba_object) + @mocha ||= Mocha::Mockery.instance.mock_impersonating_any_instance_of(@stubba_object).responds_like_instance_of(@stubba_object) else defined?(@mocha) ? @mocha : nil end diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index b448cfe00..e6e7e8738 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -8,6 +8,7 @@ require 'mocha/parameters_matcher' require 'mocha/argument_iterator' require 'mocha/expectation_error_factory' +require 'mocha/ruby_version' module Mocha # Traditional mock object. @@ -381,9 +382,13 @@ def raise_unexpected_invocation_error(invocation, matching_expectation) end def check_responder_responds_to(symbol) - if @responder && !@responder.respond_to?(symbol) # rubocop:disable Style/GuardClause - raise NoMethodError, "undefined method `#{symbol}' for #{mocha_inspect} which responds like #{@responder.mocha_inspect}" - end + return unless @responder + + legacy_behaviour_for_array_flatten = !RUBY_V23_PLUS && !@responder.respond_to?(symbol) && (symbol == :to_ary) + + return if @responder.respond_to?(symbol, true) && !legacy_behaviour_for_array_flatten + + raise NoMethodError, "undefined method `#{symbol}' for #{mocha_inspect} which responds like #{@responder.mocha_inspect}" end def check_expiry diff --git a/lib/mocha/object_methods.rb b/lib/mocha/object_methods.rb index 4e4e59673..045a33174 100644 --- a/lib/mocha/object_methods.rb +++ b/lib/mocha/object_methods.rb @@ -14,7 +14,7 @@ module ObjectMethods # @private def mocha(instantiate = true) if instantiate - @mocha ||= Mocha::Mockery.instance.mock_impersonating(self) + @mocha ||= Mocha::Mockery.instance.mock_impersonating(self).responds_like(self) else defined?(@mocha) ? @mocha : nil end diff --git a/lib/mocha/ruby_version.rb b/lib/mocha/ruby_version.rb index 124eb4729..3d8e857cf 100644 --- a/lib/mocha/ruby_version.rb +++ b/lib/mocha/ruby_version.rb @@ -1,3 +1,4 @@ module Mocha + RUBY_V23_PLUS = Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('2.3') RUBY_V27_PLUS = Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('2.7') end diff --git a/test/acceptance/responds_like_test.rb b/test/acceptance/responds_like_test.rb index 35355d3a7..1bc85d7ca 100644 --- a/test/acceptance/responds_like_test.rb +++ b/test/acceptance/responds_like_test.rb @@ -130,14 +130,14 @@ def foo; end assert_passed(test_result) end - def test_mock_which_responds_like_object_with_protected_method_raises_no_method_error_when_method_is_not_stubbed + def test_mock_which_responds_like_object_with_protected_method_raises_unexpected_invocation_exception_when_method_is_not_stubbed object = Class.new do def foo; end protected :foo end.new test_result = run_as_test do m = mock.responds_like(object) - assert_raises(NoMethodError) { m.foo } # vs Minitest::Assertion for public method + assert_raises(Minitest::Assertion) { m.foo } end assert_passed(test_result) end @@ -168,7 +168,7 @@ def foo; end assert_passed(test_result) end - def test_mock_which_responds_like_object_with_protected_method_raises_no_method_error_when_method_is_stubbed + def test_mock_which_responds_like_object_with_protected_method_does_not_raise_exception_when_method_is_stubbed object = Class.new do def foo; end protected :foo @@ -176,7 +176,7 @@ def foo; end test_result = run_as_test do m = mock.responds_like(object) m.stubs(:foo) - assert_raises(NoMethodError) { m.foo } # vs no exception for public method + assert_nil m.foo end assert_passed(test_result) end @@ -196,14 +196,14 @@ def foo; end assert_passed(test_result) end - def test_mock_which_responds_like_object_with_private_method_raises_no_method_error_when_method_is_not_stubbed + def test_mock_which_responds_like_object_with_private_method_raises_unexpected_invocation_exception_when_method_is_not_stubbed object = Class.new do def foo; end private :foo end.new test_result = run_as_test do m = mock.responds_like(object) - assert_raises(NoMethodError) { m.foo } # vs Minitest::Assertion for public method + assert_raises(Minitest::Assertion) { m.foo } end assert_passed(test_result) end @@ -234,7 +234,7 @@ def foo; end assert_passed(test_result) end - def test_mock_which_responds_like_object_with_private_method_raises_no_method_error_when_method_is_stubbed + def test_mock_which_responds_like_object_with_private_method_does_not_raise_exception_when_method_is_stubbed object = Class.new do def foo; end private :foo @@ -242,7 +242,7 @@ def foo; end test_result = run_as_test do m = mock.responds_like(object) m.stubs(:foo) - assert_raises(NoMethodError) { m.foo } # vs no exception for public method + assert_nil m.foo end assert_passed(test_result) end From 906e106734c381ffbf50c28f2931aaefda1824e1 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 31 Dec 2022 16:18:21 +0000 Subject: [PATCH 2/2] 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 240747c76..95ef5883d 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 a45dcaa75..864bbc921 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 e6e7e8738..ed80f0233 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 8b41464d4..43df37ecc 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 6cd434294..df2f38153 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