diff --git a/actioncable/lib/action_cable/test_helper.rb b/actioncable/lib/action_cable/test_helper.rb index 3d709f6519..d235d20d2d 100644 --- a/actioncable/lib/action_cable/test_helper.rb +++ b/actioncable/lib/action_cable/test_helper.rb @@ -45,7 +45,7 @@ module ActionCable def assert_broadcasts(stream, number, &block) if block_given? original_count = broadcasts_size(stream) - assert_nothing_raised(&block) + _assert_nothing_raised_or_warn("assert_broadcasts", &block) new_count = broadcasts_size(stream) actual_count = new_count - original_count else @@ -106,7 +106,7 @@ module ActionCable old_messages = new_messages clear_messages(stream) - assert_nothing_raised(&block) + _assert_nothing_raised_or_warn("assert_broadcast_on", &block) new_messages = broadcasts(stream) clear_messages(stream) diff --git a/activejob/lib/active_job/test_helper.rb b/activejob/lib/active_job/test_helper.rb index 2d71ac96b6..876bf85390 100644 --- a/activejob/lib/active_job/test_helper.rb +++ b/activejob/lib/active_job/test_helper.rb @@ -124,7 +124,7 @@ module ActiveJob if block_given? original_jobs = enqueued_jobs_with(only: only, except: except, queue: queue) - assert_nothing_raised(&block) + _assert_nothing_raised_or_warn("assert_enqueued_jobs", &block) new_jobs = enqueued_jobs_with(only: only, except: except, queue: queue) @@ -397,7 +397,7 @@ module ActiveJob if block_given? original_enqueued_jobs = enqueued_jobs.dup - assert_nothing_raised(&block) + _assert_nothing_raised_or_warn("assert_enqueued_with", &block) jobs = enqueued_jobs - original_enqueued_jobs else @@ -591,7 +591,7 @@ module ActiveJob queue_adapter.queue = queue queue_adapter.at = at - assert_nothing_raised(&block) + _assert_nothing_raised_or_warn("perform_enqueued_jobs", &block) ensure queue_adapter.perform_enqueued_jobs = old_perform_enqueued_jobs queue_adapter.perform_enqueued_at_jobs = old_perform_enqueued_at_jobs diff --git a/activesupport/lib/active_support/testing/assertions.rb b/activesupport/lib/active_support/testing/assertions.rb index 437fc07909..4af8d90885 100644 --- a/activesupport/lib/active_support/testing/assertions.rb +++ b/activesupport/lib/active_support/testing/assertions.rb @@ -99,7 +99,7 @@ module ActiveSupport } before = exps.map(&:call) - retval = assert_nothing_raised(&block) + retval = _assert_nothing_raised_or_warn("assert_difference", &block) expressions.zip(exps, before) do |(code, diff), exp, before_value| error = "#{code.inspect} didn't change by #{diff}" @@ -176,7 +176,7 @@ module ActiveSupport exp = expression.respond_to?(:call) ? expression : -> { eval(expression.to_s, block.binding) } before = exp.call - retval = assert_nothing_raised(&block) + retval = _assert_nothing_raised_or_warn("assert_changes", &block) unless from == UNTRACKED error = "Expected change from #{from.inspect}" @@ -223,7 +223,7 @@ module ActiveSupport exp = expression.respond_to?(:call) ? expression : -> { eval(expression.to_s, block.binding) } before = exp.call - retval = assert_nothing_raised(&block) + retval = _assert_nothing_raised_or_warn("assert_no_changes", &block) unless from == UNTRACKED error = "Expected initial value of #{from.inspect}" @@ -244,6 +244,22 @@ module ActiveSupport retval end + + private + def _assert_nothing_raised_or_warn(assertion, &block) + assert_nothing_raised(&block) + rescue Minitest::UnexpectedError => e + if tagged_logger && tagged_logger.warn? + warning = <<~MSG + #{self.class} - #{name}: #{e.error.class} raised. + If you expected this exception, use `assert_raises` as near to the code that raises as possible. + Other block based assertions (eg. `#{assertion}`) can be used, as long as `assert_raises` is inside their block. + MSG + tagged_logger.warn warning + end + + raise + end end end end diff --git a/activesupport/test/test_case_test.rb b/activesupport/test/test_case_test.rb index 6339792ecf..f900093e4b 100644 --- a/activesupport/test/test_case_test.rb +++ b/activesupport/test/test_case_test.rb @@ -350,6 +350,85 @@ class AssertionsTest < ActiveSupport::TestCase end end +class ExceptionsInsideAssertionsTest < ActiveSupport::TestCase + def before_setup + require "stringio" + @out = StringIO.new + self.tagged_logger = ActiveSupport::TaggedLogging.new(Logger.new(@out)) + super + end + + def test_warning_is_logged_if_caught_internally + run_test_that_should_pass_and_log_a_warning + expected = <<~MSG + ExceptionsInsideAssertionsTest - test_warning_is_logged_if_caught_internally: ArgumentError raised. + If you expected this exception, use `assert_raises` as near to the code that raises as possible. + Other block based assertions (eg. `assert_no_changes`) can be used, as long as `assert_raises` is inside their block. + MSG + assert @out.string.include?(expected), @out.string + end + + def test_warning_is_not_logged_if_caught_correctly_by_user + run_test_that_should_pass_and_not_log_a_warning + assert_not @out.string.include?("assert_nothing_raised") + end + + def test_warning_is_not_logged_if_assertions_are_nested_correctly + error = assert_raises(Minitest::Assertion) do + run_test_that_should_fail_but_not_log_a_warning + end + assert_not @out.string.include?("assert_nothing_raised") + assert error.message.include?("(lambda)> changed") + end + + def test_fails_and_warning_is_logged_if_wrong_error_caught + error = assert_raises(Minitest::Assertion) do + run_test_that_should_fail_confusingly + end + expected = <<~MSG + ExceptionsInsideAssertionsTest - test_fails_and_warning_is_logged_if_wrong_error_caught: ArgumentError raised. + If you expected this exception, use `assert_raises` as near to the code that raises as possible. + Other block based assertions (eg. `assert_no_changes`) can be used, as long as `assert_raises` is inside their block. + MSG + assert @out.string.include?(expected), @out.string + assert error.message.include?("ArgumentError: ArgumentError") + assert error.message.include?("in `block (2 levels) in run_test_that_should_fail_confusingly'") + end + + private + def run_test_that_should_pass_and_log_a_warning + assert_raises(Minitest::UnexpectedError) do # this assertion passes, but it's unlikely to be how anyone writes a test + assert_no_changes -> { 1 } do # this assertion doesn't run. the error below is caught and the warning logged. + raise ArgumentError.new + end + end + end + + def run_test_that_should_fail_confusingly + assert_raises(ArgumentError) do # this assertion fails (confusingly) because it catches a Minitest::UnexpectedError. + assert_no_changes -> { 1 } do # this assertion doesn't run. the error below is caught and the warning logged. + raise ArgumentError.new + end + end + end + + def run_test_that_should_pass_and_not_log_a_warning + assert_no_changes -> { 1 } do # this assertion passes + assert_raises(ArgumentError) do # this assertion passes + raise ArgumentError.new + end + end + end + + def run_test_that_should_fail_but_not_log_a_warning + assert_no_changes -> { rand } do # this assertion fails + assert_raises(ArgumentError) do # this assertion passes + raise ArgumentError.new + end + end + end +end + # Setup and teardown callbacks. class SetupAndTeardownTest < ActiveSupport::TestCase setup :reset_callback_record, :foo