From 9d2b88ef018cdd988a0d97507615af79b925924c Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 8 Apr 2024 09:26:19 +0200 Subject: [PATCH] Include the original test name in expired stub error messages It is currently very hard to track down such leaked stubs, when they happen, as they might be created in one test and cause an exception dozens of tests later. By including the name of the test that invalidated them, it makes the debugging process a bit easier. Note, I made `setup` also accept the current test instance purely for consistency with `teardown` but it's not strictly necessary. --- lib/mocha/hooks.rb | 10 +++++-- lib/mocha/integration/minitest/adapter.rb | 15 ++++++++++ lib/mocha/integration/test_unit/adapter.rb | 5 ++++ lib/mocha/mock.rb | 8 ++++-- lib/mocha/mockery.rb | 8 +++--- test/acceptance/mock_test.rb | 2 +- test/integration/shared_tests.rb | 32 ++++++++++++++++++++++ test/test_runner.rb | 8 ++++-- test/unit/hooks_test.rb | 2 +- 9 files changed, 77 insertions(+), 13 deletions(-) diff --git a/lib/mocha/hooks.rb b/lib/mocha/hooks.rb index cf19cf81e..745116db1 100644 --- a/lib/mocha/hooks.rb +++ b/lib/mocha/hooks.rb @@ -35,8 +35,14 @@ def mocha_verify(assertion_counter = nil) # Resets Mocha after a test (only for use by authors of test libraries). # # This method should be called after each individual test has finished (including after any "teardown" code). - def mocha_teardown - Mockery.teardown + def mocha_teardown(origin = mocha_test_name) + Mockery.teardown(origin) + end + + # Returns a string representing the unit test name, to be included in some Mocha + # to help track down potential bugs. + def mocha_test_name + nil end end end diff --git a/lib/mocha/integration/minitest/adapter.rb b/lib/mocha/integration/minitest/adapter.rb index 0efbc7ee4..ee000a353 100644 --- a/lib/mocha/integration/minitest/adapter.rb +++ b/lib/mocha/integration/minitest/adapter.rb @@ -46,6 +46,21 @@ def after_teardown super mocha_teardown end + + # @private + def mocha_test_name + if respond_to?(:name) + test_name = name + elsif respond_to?(:__name__) # Older minitest + test_name = __name__ + end + + if test_name + "#{self.class.name}##{test_name}" + else + self.class.name + end + end end end end diff --git a/lib/mocha/integration/test_unit/adapter.rb b/lib/mocha/integration/test_unit/adapter.rb index 83c06b3a9..38da47742 100644 --- a/lib/mocha/integration/test_unit/adapter.rb +++ b/lib/mocha/integration/test_unit/adapter.rb @@ -37,6 +37,11 @@ def self.included(mod) private + # @private + def mocha_test_name + name + end + # @private def handle_mocha_expectation_error(exception) return false unless exception.is_a?(Mocha::ExpectationError) diff --git a/lib/mocha/mock.rb b/lib/mocha/mock.rb index 6b2b9737d..03c9c1c13 100644 --- a/lib/mocha/mock.rb +++ b/lib/mocha/mock.rb @@ -343,8 +343,8 @@ def __verified__?(assertion_counter = nil) end # @private - def __expire__ - @expired = true + def __expire__(origin) + @expired = origin || true end # @private @@ -393,8 +393,10 @@ def check_responder_responds_to(symbol) def check_expiry return unless @expired + origin = @expired == true ? 'one test' : @expired + sentences = [ - "#{mocha_inspect} was instantiated in one test but it is receiving invocations within another test.", + "#{mocha_inspect} was instantiated in #{origin} but it is receiving invocations within another test.", 'This can lead to unintended interactions between tests and hence unexpected test failures.', 'Ensure that every test correctly cleans up any state that it introduces.' ] diff --git a/lib/mocha/mockery.rb b/lib/mocha/mockery.rb index c08ed60ca..26aae88b7 100644 --- a/lib/mocha/mockery.rb +++ b/lib/mocha/mockery.rb @@ -48,8 +48,8 @@ def verify(*args) instance.verify(*args) end - def teardown - instance.teardown + def teardown(origin = nil) + instance.teardown(origin) ensure @instances.pop end @@ -91,9 +91,9 @@ def verify(assertion_counter = nil) end end - def teardown + def teardown(origin = nil) stubba.unstub_all - mocks.each(&:__expire__) + mocks.each { |m| m.__expire__(origin) } reset end diff --git a/test/acceptance/mock_test.rb b/test/acceptance/mock_test.rb index 855748345..7b5faa0d6 100644 --- a/test/acceptance/mock_test.rb +++ b/test/acceptance/mock_test.rb @@ -159,7 +159,7 @@ def test_should_raise_stubbing_error_if_mock_receives_invocations_in_another_tes reuse_mock_test_result = run_as_test do Foo.logger.expects(:log).with('Foo was here') e = assert_raises(Mocha::StubbingError) { Foo.new.use_the_mock } - assert e.message.include?('# was instantiated in one test but it is receiving invocations within another test.') + assert e.message.include?('# was instantiated in FakeTest#test_me but it is receiving invocations within another test.') assert e.message.include?('This can lead to unintended interactions between tests and hence unexpected test failures.') assert e.message.include?('Ensure that every test correctly cleans up any state that it introduces.') end diff --git a/test/integration/shared_tests.rb b/test/integration/shared_tests.rb index 657ccb8c6..5107c3b82 100644 --- a/test/integration/shared_tests.rb +++ b/test/integration/shared_tests.rb @@ -172,5 +172,37 @@ def test_real_object_expectation_does_not_leak_into_subsequent_test assert_equal execution_point, ExecutionPoint.new(exception.backtrace) assert_match(/undefined method `foo'/, exception.message) end + + def test_leaky_mock + origin = nil + leaky_mock = nil + execution_point = nil + test_result = run_as_tests( + test_1: lambda { + origin = mocha_test_name + leaky_mock ||= begin + bad_mock = mock(:leaky) + bad_mock.expects(:call) + bad_mock + end + + leaky_mock.call + }, + test_2: lambda { + leaky_mock ||= begin + bad_mock = mock(:leaky) + bad_mock.expects(:call) + bad_mock + end + + execution_point = ExecutionPoint.current; leaky_mock.call + } + ) + assert_errored(test_result) + exception = test_result.errors.first.exception + assert_equal execution_point, ExecutionPoint.new(exception.backtrace) + expected = /# was instantiated in #{Regexp.escape(origin)} but it is receiving invocations within another test/ + assert_match(expected, exception.message) + end end # rubocop:enable Metrics/ModuleLength diff --git a/test/test_runner.rb b/test/test_runner.rb index b2a6acd19..9745088cd 100644 --- a/test/test_runner.rb +++ b/test/test_runner.rb @@ -7,10 +7,14 @@ def run_as_test(&block) run_as_tests(test_me: block) end - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def run_as_tests(methods = {}) base_class = Mocha::TestCase test_class = Class.new(base_class) do + def self.name; + 'FakeTest' + end + include Assertions methods.each do |(method_name, proc)| @@ -45,7 +49,7 @@ def run_as_tests(methods = {}) test_result end - # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength def assert_passed(test_result) flunk "Test errored unexpectedly with message: #{test_result.errors.map(&:exception)}" if test_result.error_count > 0 diff --git a/test/unit/hooks_test.rb b/test/unit/hooks_test.rb index 60f45310c..d39a55d61 100644 --- a/test/unit/hooks_test.rb +++ b/test/unit/hooks_test.rb @@ -14,7 +14,7 @@ class << self class FakeMockery def verify(*args); end - def teardown + def teardown(_origin = nil) raise 'exception within Mockery#teardown' end end