Log a warning when assertions are incorrectly nested and errors are raised

Follow up to https://github.com/rails/rails/pull/37313

- Adds regression tests
- Logs a warning in cases where assertions are nested in a way that's likely to be confusing

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)
 

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

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

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