From f05ab4bdee1872935f563420fcaf210f29210928 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 9 Nov 2024 10:50:07 +0000 Subject: [PATCH] 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. After #681 a deprecation warning was displayed in this scenario, but now an unexpected invocation error is reported. 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. Also 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]. 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) This commit also takes into account the fix in this commit [3]: In #681 I added logic to display a deprecation warning about a change I planned to make in this PR. However, it turns out the logic in both was faulty as demonstrated in this example [4] where the deprecation warning was incorrectly displayed for the 2nd call to `Foo.create_if`: class Foo def self.create_if(condition) new if condition end end test "it creates only if condition is true" do Foo.expects(:new).never Foo.create_if(false) Foo.expects(:new).once Foo.create_if(true) end This commit adds a new acceptance test to cover an example where the logic was incorrect and updates the logic in `Mock#handle_method_call` to fix the logic so this new test passes and none of the existing tests fail. Unfortunately the implementation is now rather complicated and hard to follow, but I think it will do for now. I have a couple of ideas for ways to simplify it, but I don't have time to work on that now. Closes #678. Also addresses #490, #131 & #44. [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 [3]: https://github.com/freerange/mocha/pull/686/commits/01874f198c389baf07a3a03fc03d0cec16d75b54 [4]: https://github.com/freerange/mocha/pull/679#issuecomment-2503272428 --- README.md | 2 +- lib/mocha/mock.rb | 17 +++------ .../mocked_methods_dispatch_test.rb | 35 +++++++------------ 3 files changed, 18 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index c63cd508..0ee35884 100644 --- a/README.md +++ b/README.md @@ -276,7 +276,7 @@ Maybe, but probably not. Partial mocking changes the state of objects in the `Ob Stubs and expectations are basically the same thing. A stub is just an expectation of zero or more invocations. The `Expectation#stubs` method is syntactic sugar to make the intent of the test more explicit. -When a method is invoked on a mock object, the mock object searches through its expectations from newest to oldest to find one that matches the invocation. After the invocation, the matching expectation might stop matching further invocations. +When a method is invoked on a mock object, the mock object searches through its expectations from newest to oldest to find one that matches the invocation. After the invocation, the matching expectation might stop matching further invocations. If the expectation that matches the invocation has a cardinality of "never", then an unexpected invocation error is reported. See the [documentation](https://mocha.jamesmead.org/Mocha/Mock.html) for `Mocha::Mock` for further details. diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index 4cf5839c..059dc24d 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -35,6 +35,9 @@ module Mocha # while an +expects(:foo).at_least_once+ expectation will always be matched # against invocations. # + # However, note that if the expectation that matches the invocation has a + # cardinality of "never", then an unexpected invocation error is reported. + # # This scheme allows you to: # # - Set up default stubs in your the +setup+ method of your test class and @@ -326,15 +329,11 @@ def handle_method_call(symbol, arguments, block) # rubocop:disable Metrics/Cyclo matching_expectations = all_expectations.matching_expectations(invocation) index = 0 - never_allowed_expectation = nil while index < matching_expectations.length matching_expectation = matching_expectations[index] if matching_expectation.invocations_never_allowed? - never_allowed_expectation = matching_expectation + raise_unexpected_invocation_error(invocation, matching_expectation) elsif matching_expectation.invocations_allowed? - if never_allowed_expectation - invocation_not_allowed_warning(invocation, never_allowed_expectation) - end return matching_expectation.invoke(invocation) end index += 1 @@ -387,14 +386,6 @@ 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 594c962e..af89da12 100644 --- a/test/acceptance/mocked_methods_dispatch_test.rb +++ b/test/acceptance/mocked_methods_dispatch_test.rb @@ -1,6 +1,4 @@ require File.expand_path('../acceptance_test_helper', __FILE__) -require 'deprecation_disabler' -require 'execution_point' class MockedMethodDispatchTest < Mocha::TestCase include AcceptanceTest @@ -75,37 +73,30 @@ def test_should_find_latest_expectation_with_range_of_expected_invocation_count_ assert_passed(test_result) end - def test_should_display_deprecation_warning_if_invocation_matches_expectation_with_never_cardinality - execution_point = nil + 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; execution_point = ExecutionPoint.current - DeprecationDisabler.disable_deprecations do - mock.method - end + mock.expects(:method).never + mock.method 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 + 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 - def test_should_not_display_deprecation_warning_if_invocation_matches_expectation_allowing_invocation_before_matching_expectation_with_never_cardinality + def test_should_not_fail_fast_if_invocation_matches_expectation_allowing_invocation_before_matching_expectation_with_never_cardinality test_result = run_as_test do mock = mock('mock') mock.expects(:method).never mock.expects(:method).once - Mocha::Deprecation.messages = [] - DeprecationDisabler.disable_deprecations do - mock.method - end + mock.method end assert_passed(test_result) - assert Mocha::Deprecation.messages.empty? end end