From 83987c7da2cada9abefa012883feed58ccf06310 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 03c9c1c1..858e6962 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 c4e6efb9a999e40cff6d3d0d164b457f1f1812d4 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 ab9b4f38..fa8fb99d 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 1c265e97..167d8e9a 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 f84a4a782eec0fa9b26928a80d767804dc2459a3 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 5e6f95d4..f55918bd 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 55716d89..04dfe78f 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 c5024717c1a0d2954a2fe2ffd615d6928acf46df 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 ba521b62..ba43fdb8 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 e8c36f88..a0c345c5 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 9ed84cd0d7a3be9bdfc2aaf498cf8badd33f6f44 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 10:50:07 +0000 Subject: [PATCH 5/6] Fail fast if invocation matches never expectation Previously when an invocation matched an expectation which did not allow invocations (i.e. `Expectation#never` had been called on it), but the invocation also matched another expectation which did allow invocations, then the test would not fail with an unexpected invocation error. This was happening because neither the `if` condition was `true`, because the "never" expectation was not returned by `ExpectationList#match_allowing_invocation`, but the other expectation allowing expectations was returned. Thus `Expectation#invoke` was called on the latter and `Mock#raise_unexpected_invocation_error` was not called. This behaviour was confusing and had led to a number of issues being raised over the years: #44, #131, #490 & most recently #678. Previously I had thought this was a symptom of the JMock v1 dispatch behaviour (which _might_ still be true) and thus would be best addressed by adopting the JMock v2 dispatch behaviour (#173). However, having considered this specific scenario involving a "never" expectation, I've decided to try to fix it with the changes in this commit. Now a test like this will fail with an unexpected invocation error: mock = mock('mock') mock.stubs(:method) mock.expects(:method).never mock.method unexpected invocation: #.method() unsatisfied expectations: - expected never, invoked once: #.method(any_parameters) satisfied expectations: - allowed any number of times, invoked never: #.method(any_parameters) Closes #678. Also addresses #490, #131 & #44. --- lib/mocha/mock.rb | 3 ++- test/acceptance/mocked_methods_dispatch_test.rb | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index 858e6962..f349297a 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -323,9 +323,10 @@ 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_allowing_invocation && !matching_expectation_never_allowing_invocation 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) diff --git a/test/acceptance/mocked_methods_dispatch_test.rb b/test/acceptance/mocked_methods_dispatch_test.rb index 9b53aa99..5789dc42 100644 --- a/test/acceptance/mocked_methods_dispatch_test.rb +++ b/test/acceptance/mocked_methods_dispatch_test.rb @@ -72,4 +72,21 @@ def test_should_find_latest_expectation_with_range_of_expected_invocation_count_ end assert_passed(test_result) end + + def test_should_fail_fast_if_invocation_matches_expectation_with_never_cardinality + test_result = run_as_test do + mock = mock('mock') + mock.stubs(:method) + mock.expects(:method).never + mock.method + end + assert_failed(test_result) + assert_equal [ + 'unexpected invocation: #.method()', + 'unsatisfied expectations:', + '- expected never, invoked once: #.method(any_parameters)', + 'satisfied expectations:', + '- allowed any number of times, invoked never: #.method(any_parameters)' + ], test_result.failure_message_lines + end end From 73c4ea6ef5def4f9203e7a0d6f3a460849d511bb 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 ba43fdb8..2bd45dc2 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 f349297a..e44ecee4 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -322,14 +322,17 @@ 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 && !matching_expectation_never_allowing_invocation 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 88530151..2bbbc588 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