From 19b27d1cd988d5efab38b8d7b19ea511013bdf24 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 10:39:07 +0000 Subject: [PATCH 1/6] Extract local vars outside if/else in Mock#handle_method_call I'm about to make some changes in this area and this will make it easier to see what's going on. --- lib/mocha/mock.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index 03c9c1c13..858e69628 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -321,10 +321,14 @@ def handle_method_call(symbol, arguments, block) check_expiry check_responder_responds_to(symbol) invocation = Invocation.new(self, symbol, arguments, block) - if (matching_expectation_allowing_invocation = all_expectations.match_allowing_invocation(invocation)) + + matching_expectation_allowing_invocation = all_expectations.match_allowing_invocation(invocation) + matching_expectation_ignoring_order = all_expectations.match(invocation, ignoring_order: true) + + if matching_expectation_allowing_invocation matching_expectation_allowing_invocation.invoke(invocation) - elsif (matching_expectation = all_expectations.match(invocation, ignoring_order: true)) || (!matching_expectation && !@everything_stubbed) - raise_unexpected_invocation_error(invocation, matching_expectation) + elsif matching_expectation_ignoring_order || (!matching_expectation_ignoring_order && !@everything_stubbed) + raise_unexpected_invocation_error(invocation, matching_expectation_ignoring_order) end end From b24bd4e971324fb62b4815c143afe870d806b945 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 10:47:27 +0000 Subject: [PATCH 2/6] Add Cardinality#invocations_never_allowed? I'm planning to use this in a subsequent commit. --- lib/mocha/cardinality.rb | 4 ++++ test/unit/cardinality_test.rb | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/lib/mocha/cardinality.rb b/lib/mocha/cardinality.rb index ab9b4f38e..fa8fb99dd 100644 --- a/lib/mocha/cardinality.rb +++ b/lib/mocha/cardinality.rb @@ -34,6 +34,10 @@ def invocations_allowed? @invocations.size < maximum end + def invocations_never_allowed? + maximum.zero? + end + def satisfied? @invocations.size >= required end diff --git a/test/unit/cardinality_test.rb b/test/unit/cardinality_test.rb index 1c265e97a..167d8e9a4 100644 --- a/test/unit/cardinality_test.rb +++ b/test/unit/cardinality_test.rb @@ -22,6 +22,13 @@ def test_should_allow_invocations_if_invocation_count_has_not_yet_reached_maximu assert !cardinality.invocations_allowed? end + def test_should_never_allow_invocations + cardinality = Cardinality.new.exactly(0) + assert cardinality.invocations_never_allowed? + cardinality << new_invocation + assert cardinality.invocations_never_allowed? + end + def test_should_be_satisfied_if_invocations_so_far_have_reached_required_threshold cardinality = Cardinality.new(2, 3) assert !cardinality.satisfied? From 471b99616ee696e0be643d0ee07a1040735ffd97 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 10:47:27 +0000 Subject: [PATCH 3/6] Add Expectation#invocations_never_allowed? I'm planning to use this in a subsequent commit. --- lib/mocha/expectation.rb | 5 +++++ test/unit/expectation_test.rb | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/lib/mocha/expectation.rb b/lib/mocha/expectation.rb index 5e6f95d4f..f55918bdc 100644 --- a/lib/mocha/expectation.rb +++ b/lib/mocha/expectation.rb @@ -653,6 +653,11 @@ def invocations_allowed? @cardinality.invocations_allowed? end + # @private + def invocations_never_allowed? + @cardinality.invocations_never_allowed? + end + # @private def satisfied? @cardinality.satisfied? diff --git a/test/unit/expectation_test.rb b/test/unit/expectation_test.rb index 55716d894..04dfe78f7 100644 --- a/test/unit/expectation_test.rb +++ b/test/unit/expectation_test.rb @@ -88,6 +88,13 @@ def test_should_allow_invocations_until_expected_invocation_count_is_a_range_fro assert !expectation.invocations_allowed? end + def test_should_never_allow_invocations + expectation = new_expectation.never + assert expectation.invocations_never_allowed? + invoke(expectation) + assert expectation.invocations_never_allowed? + end + def test_should_store_provided_backtrace backtrace = Object.new assert_equal backtrace, Expectation.new(nil, :expected_method, backtrace).backtrace From 212ba0bff7bfd99324e4ab2f546b4e7b7b0036e2 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 10:47:27 +0000 Subject: [PATCH 4/6] Add ExpectationList#match_never_allowing_invocation I'm planning to use this in a subsequent commit. --- lib/mocha/expectation_list.rb | 4 ++++ test/unit/expectation_list_test.rb | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/mocha/expectation_list.rb b/lib/mocha/expectation_list.rb index ba521b629..ba43fdb86 100644 --- a/lib/mocha/expectation_list.rb +++ b/lib/mocha/expectation_list.rb @@ -25,6 +25,10 @@ def match_allowing_invocation(invocation) matching_expectations(invocation).detect(&:invocations_allowed?) end + def match_never_allowing_invocation(invocation) + matching_expectations(invocation).detect(&:invocations_never_allowed?) + end + def verified?(assertion_counter = nil) @expectations.all? { |expectation| expectation.verified?(assertion_counter) } end diff --git a/test/unit/expectation_list_test.rb b/test/unit/expectation_list_test.rb index e8c36f885..a0c345c53 100644 --- a/test/unit/expectation_list_test.rb +++ b/test/unit/expectation_list_test.rb @@ -69,6 +69,17 @@ def test_should_find_most_recent_matching_expectation_allowing_invocation assert_same expectation1, expectation_list.match_allowing_invocation(Invocation.new(:irrelevant, :my_method)) end + def test_should_find_matching_expectation_never_allowing_invocation + expectation_list = ExpectationList.new + expectation1 = Expectation.new(nil, :my_method) + expectation2 = Expectation.new(nil, :my_method) + define_instance_method(expectation1, :invocations_never_allowed?) { true } + define_instance_method(expectation2, :invocations_never_allowed?) { false } + expectation_list.add(expectation1) + expectation_list.add(expectation2) + assert_same expectation1, expectation_list.match_never_allowing_invocation(Invocation.new(:irrelevant, :my_method)) + end + def test_should_combine_two_expectation_lists_into_one expectation_list1 = ExpectationList.new expectation_list2 = ExpectationList.new From 49b99ff2729beb0cab3bc9025b477700d086b59e Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 13:46:05 +0000 Subject: [PATCH 5/6] Warning if invocation matches never expectation Currently when an invocation matches an expectation which does not allow invocations (i.e. `Expectation#never` has been called on it), but the invocation also matches another expectation which does allow invocations, then the test does not fail with an unexpected invocation error. In #679 I'm planning to change this behaviour so the test fails fast with an unexpected invocation error. This commit displays a deprecation warning instead to give users a chance to update their code before the actual change is made. The new behaviour will bring the behaviour into line with what is already the case for mocks where `Mock#stub_everything` has been called as per this acceptance test [1] that was introduced in this commit [2]. [1]: https://github.com/freerange/mocha/blob/f2fa99197f35e2d2ce9554db63f6964057e29ce0/test/acceptance/expected_invocation_count_test.rb#L202-L214 [2]: https://github.com/freerange/mocha/commit/d3583772c3fd878a567a0b04750d98ff7d079358 --- lib/mocha/mock.rb | 13 +++++++++++ .../mocked_methods_dispatch_test.rb | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index 858e69628..a0c73d663 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/deprecation' module Mocha # Traditional mock object. @@ -323,9 +324,13 @@ def handle_method_call(symbol, arguments, block) invocation = Invocation.new(self, symbol, arguments, block) matching_expectation_allowing_invocation = all_expectations.match_allowing_invocation(invocation) + matching_expectation_never_allowing_invocation = all_expectations.match_never_allowing_invocation(invocation) matching_expectation_ignoring_order = all_expectations.match(invocation, ignoring_order: true) if matching_expectation_allowing_invocation + if matching_expectation_never_allowing_invocation + invocation_not_allowed_warning(invocation, matching_expectation_never_allowing_invocation) + end matching_expectation_allowing_invocation.invoke(invocation) elsif matching_expectation_ignoring_order || (!matching_expectation_ignoring_order && !@everything_stubbed) raise_unexpected_invocation_error(invocation, matching_expectation_ignoring_order) @@ -373,6 +378,14 @@ def any_expectations? private + def invocation_not_allowed_warning(invocation, expectation) + messages = [ + "The expectation defined at #{expectation.definition_location} does not allow invocations, but #{invocation.call_description} was invoked.", + 'This invocation will cause the test to fail fast in a future version of Mocha.' + ] + Deprecation.warning(messages.join(' ')) + end + def raise_unexpected_invocation_error(invocation, matching_expectation) if @unexpected_invocation.nil? @unexpected_invocation = invocation diff --git a/test/acceptance/mocked_methods_dispatch_test.rb b/test/acceptance/mocked_methods_dispatch_test.rb index 9b53aa992..168cfe408 100644 --- a/test/acceptance/mocked_methods_dispatch_test.rb +++ b/test/acceptance/mocked_methods_dispatch_test.rb @@ -1,4 +1,6 @@ require File.expand_path('../acceptance_test_helper', __FILE__) +require 'deprecation_disabler' +require 'execution_point' class MockedMethodDispatchTest < Mocha::TestCase include AcceptanceTest @@ -72,4 +74,24 @@ def test_should_find_latest_expectation_with_range_of_expected_invocation_count_ end assert_passed(test_result) end + + def test_should_display_deprecation_warning_if_invocation_matches_expectation_with_never_cardinality + execution_point = nil + test_result = run_as_test do + mock = mock('mock') + mock.stubs(:method) + mock.expects(:method).never; execution_point = ExecutionPoint.current + DeprecationDisabler.disable_deprecations do + mock.method + end + end + assert_passed(test_result) + message = Mocha::Deprecation.messages.last + location = execution_point.location + expected = [ + "The expectation defined at #{location} does not allow invocations, but #.method() was invoked.", + 'This invocation will cause the test to fail fast in a future version of Mocha.' + ] + assert_equal expected.join(' '), message + end end From e0b6dae06a01f69f50e5e0feb3a0512bdf537d36 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 13 Nov 2024 13:47:17 +0000 Subject: [PATCH 6/6] Avoid invoking matcher block multiple times In the previous commit, the changes meant that a block provided to `Expectation#with` was being invoked three times instead of once. The original behaviour was not explicitly documented and there is code in the wild [1] that relies on `Expectation#with` only being called once per invocation. I'm not convinced that test code should be relying on this behaviour, but from a performance point-of-view, it probably makes sense to avoid calling the matching methods on `ExpectationList` multiple times unnecessarily. This commit changes the code in `Mock#handle_method_call` to avoid unnecessary calls to these methods. I've created #682 to suggest improving the docs for `Expectation#with` when it takes a block argument to address the ambiguity that has become apparent. [1]: https://github.com/freerange/mocha/pull/681#issuecomment-2472059445 --- lib/mocha/expectation_list.rb | 2 -- lib/mocha/mock.rb | 13 ++++++++----- test/acceptance/parameter_matcher_test.rb | 11 +++++++++++ 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/mocha/expectation_list.rb b/lib/mocha/expectation_list.rb index ba43fdb86..2bd45dc27 100644 --- a/lib/mocha/expectation_list.rb +++ b/lib/mocha/expectation_list.rb @@ -53,8 +53,6 @@ def +(other) self.class.new(to_a + other.to_a) end - private - def matching_expectations(invocation, ignoring_order: false) @expectations.select { |e| e.match?(invocation, ignoring_order: ignoring_order) } end diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index a0c73d663..7a5ef92ed 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -323,17 +323,20 @@ def handle_method_call(symbol, arguments, block) check_responder_responds_to(symbol) invocation = Invocation.new(self, symbol, arguments, block) - matching_expectation_allowing_invocation = all_expectations.match_allowing_invocation(invocation) - matching_expectation_never_allowing_invocation = all_expectations.match_never_allowing_invocation(invocation) - matching_expectation_ignoring_order = all_expectations.match(invocation, ignoring_order: true) + matching_expectations = all_expectations.matching_expectations(invocation) + matching_expectation_allowing_invocation = matching_expectations.detect(&:invocations_allowed?) + matching_expectation_never_allowing_invocation = matching_expectations.detect(&:invocations_never_allowed?) if matching_expectation_allowing_invocation if matching_expectation_never_allowing_invocation invocation_not_allowed_warning(invocation, matching_expectation_never_allowing_invocation) end matching_expectation_allowing_invocation.invoke(invocation) - elsif matching_expectation_ignoring_order || (!matching_expectation_ignoring_order && !@everything_stubbed) - raise_unexpected_invocation_error(invocation, matching_expectation_ignoring_order) + else + matching_expectation_ignoring_order = all_expectations.match(invocation, ignoring_order: true) + if matching_expectation_ignoring_order || (!matching_expectation_ignoring_order && !@everything_stubbed) + raise_unexpected_invocation_error(invocation, matching_expectation_ignoring_order) + end end end diff --git a/test/acceptance/parameter_matcher_test.rb b/test/acceptance/parameter_matcher_test.rb index 88530151e..2bbbc588c 100644 --- a/test/acceptance/parameter_matcher_test.rb +++ b/test/acceptance/parameter_matcher_test.rb @@ -373,4 +373,15 @@ def test_should_not_match_parameters_when_values_do_not_add_up_to_ten end assert_failed(test_result) end + + def test_should_only_call_matcher_block_once + test_result = run_as_test do + number_of_invocations = 0 + mock = mock() + mock.stubs(:method).with { number_of_invocations += 1; true } + mock.method + assert_equal 1, number_of_invocations + end + assert_passed(test_result) + end end