Merge pull request #52036 from richardboehme/assert-difference-output

Improve error message when passing a proc to `assert_difference` or `assert_changes`

Author: byroot

activesupport/CHANGELOG.md

@@ -1,3 +1,6 @@
+*   Improve error message when using `assert_difference` or `assert_changes` with a
+    proc by printing the proc's source code (MRI only).
 
+    *Richard Böhme*, *Jean Boussier*
 
 Please check [7-2-stable](https://github.com/rails/rails/blob/7-2-stable/activesupport/CHANGELOG.md) for previous changes.

activesupport/lib/active_support/testing/assertions.rb

@@ -118,7 +118,8 @@ def assert_difference(expression, *args, &block)
 
         expressions.zip(exps, before) do |(code, diff), exp, before_value|
           actual = exp.call
-          error  = "#{code.inspect} didn't change by #{diff}, but by #{actual - before_value}"
+          code_string = code.respond_to?(:call) ? _callable_to_source_string(code) : code
+          error  = "`#{code_string}` didn't change by #{diff}, but by #{actual - before_value}"
           error  = "#{message}.\n#{error}" if message
           assert_equal(before_value + diff, actual, error)
         end
@@ -202,7 +203,8 @@ def assert_changes(expression, message = nil, from: UNTRACKED, to: UNTRACKED, &b
 
         after = exp.call
 
-        error = "#{expression.inspect} didn't change"
+        code_string = expression.respond_to?(:call) ? _callable_to_source_string(expression) : expression
+        error = "`#{code_string}` didn't change"
         error = "#{error}. It was already #{to.inspect}" if before == to
         error = "#{message}.\n#{error}" if message
         refute_equal before, after, error
@@ -249,7 +251,8 @@ def assert_no_changes(expression, message = nil, from: UNTRACKED, &block)
 
         after = exp.call
 
-        error = "#{expression.inspect} changed"
+        code_string = expression.respond_to?(:call) ? _callable_to_source_string(expression) : expression
+        error = "`#{code_string}` changed"
         error = "#{message}.\n#{error}" if message
 
         if before.nil?
@@ -276,6 +279,36 @@ def _assert_nothing_raised_or_warn(assertion, &block)
 
           raise
         end
+
+        def _callable_to_source_string(callable)
+          if defined?(RubyVM::AbstractSyntaxTree) && callable.is_a?(Proc)
+            ast = begin
+              RubyVM::AbstractSyntaxTree.of(callable, keep_script_lines: true)
+            rescue SystemCallError
+              # Failed to get the source somehow
+              return callable
+            end
+            return callable unless ast
+
+            source = ast.source
+            source.strip!
+
+            # We ignore procs defined with do/end as they are likely multi-line anyway.
+            if source.start_with?("{")
+              source.delete_suffix!("}")
+              source.delete_prefix!("{")
+              source.strip!
+              # It won't read nice if the callable contains multiple
+              # lines, and it should be a rare occurence anyway.
+              # Same if it takes arguments.
+              if !source.include?("\n") && !source.start_with?("|")
+                return source
+              end
+            end
+          end
+
+          callable
+        end
     end
   end
 end

activesupport/test/test_case_test.rb

@@ -54,7 +54,7 @@ def test_assert_no_difference_fail
         @object.increment
       end
     end
-    assert_equal "\"@object.num\" didn't change by 0, but by 1.\nExpected: 0\n  Actual: 1", error.message
+    assert_equal "`@object.num` didn't change by 0, but by 1.\nExpected: 0\n  Actual: 1", error.message
   end
 
   def test_assert_no_difference_with_message_fail
@@ -63,7 +63,7 @@ def test_assert_no_difference_with_message_fail
         @object.increment
       end
     end
-    assert_equal "Object Changed.\n\"@object.num\" didn't change by 0, but by 1.\nExpected: 0\n  Actual: 1", error.message
+    assert_equal "Object Changed.\n`@object.num` didn't change by 0, but by 1.\nExpected: 0\n  Actual: 1", error.message
   end
 
   def test_assert_no_difference_with_multiple_expressions_pass
@@ -157,7 +157,7 @@ def test_hash_of_expressions_with_message
         @object.increment
       end
     end
-    assert_equal "Object Changed.\n\"@object.num\" didn't change by 0, but by 1.\nExpected: 0\n  Actual: 1", error.message
+    assert_equal "Object Changed.\n`@object.num` didn't change by 0, but by 1.\nExpected: 0\n  Actual: 1", error.message
   end
 
   def test_assert_difference_message_includes_change
@@ -167,7 +167,17 @@ def test_assert_difference_message_includes_change
         @object.increment
       end
     end
-    assert_equal "\"@object.num\" didn't change by 5, but by 2.\nExpected: 5\n  Actual: 2", error.message
+    assert_equal "`@object.num` didn't change by 5, but by 2.\nExpected: 5\n  Actual: 2", error.message
+  end
+
+  def test_assert_difference_message_with_lambda
+    skip if !defined?(RubyVM::AbstractSyntaxTree)
+
+    error = assert_raises Minitest::Assertion do
+      assert_difference(-> { @object.num }, 1, "Object Changed") do
+      end
+    end
+    assert_equal "Object Changed.\n`@object.num` didn't change by 1, but by 0.\nExpected: 1\n  Actual: 0", error.message
   end
 
   def test_hash_of_lambda_expressions
@@ -233,7 +243,19 @@ def test_assert_changes_with_to_option_but_no_change_has_special_message
       end
     end
 
-    assert_equal "\"@object.num\" didn't change. It was already 0.\nExpected 0 to not be equal to 0.", error.message
+    assert_equal "`@object.num` didn't change. It was already 0.\nExpected 0 to not be equal to 0.", error.message
+  end
+
+  def test_assert_changes_message_with_lambda
+    skip if !defined?(RubyVM::AbstractSyntaxTree)
+
+    error = assert_raises Minitest::Assertion do
+      assert_changes -> { @object.num }, to: 0 do
+        # no changes
+      end
+    end
+
+    assert_equal "`@object.num` didn't change. It was already 0.\nExpected 0 to not be equal to 0.", error.message
   end
 
   def test_assert_changes_with_wrong_to_option
@@ -349,7 +371,91 @@ def test_assert_no_changes_with_message
       end
     end
 
-    assert_equal "@object.num should not change.\n\"@object.num\" changed.\nExpected: 0\n  Actual: 1", error.message
+    assert_equal "@object.num should not change.\n`@object.num` changed.\nExpected: 0\n  Actual: 1", error.message
+  end
+
+  def test_assert_no_changes_message_with_lambda
+    skip if !defined?(RubyVM::AbstractSyntaxTree)
+
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes -> { @object.num } do
+        @object.increment
+      end
+    end
+    assert_equal "`@object.num` changed.\nExpected: 0\n  Actual: 1", error.message
+
+    check = Proc.new {
+      @object.num
+    }
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes check do
+        @object.increment
+      end
+    end
+    assert_equal "`@object.num` changed.\nExpected: 1\n  Actual: 2", error.message
+
+    check = lambda {
+      @object.num
+    }
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes check do
+        @object.increment
+      end
+    end
+    assert_equal "`@object.num` changed.\nExpected: 2\n  Actual: 3", error.message
+
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes -> { @object.num } do
+        @object.increment
+      end
+    end
+    assert_equal "`@object.num` changed.\nExpected: 3\n  Actual: 4", error.message
+
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes ->(a = nil) { @object.num } do
+        @object.increment
+      end
+    end
+    assert_match(/#<Proc:0x.*changed/, error.message)
+  end
+
+  def test_assert_no_changes_message_with_multi_line_lambda
+    check = lambda {
+      "title".upcase
+      @object.num
+    }
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes check do
+        @object.increment
+      end
+    end
+    assert_match(/#<Proc:0x.*changed/, error.message)
+
+    check = lambda {
+      "title".upcase
+      @object.num
+    }
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes check do
+        @object.increment
+      end
+    end
+    assert_match(/#<Proc:0x.*changed/, error.message)
+  end
+
+  def test_assert_no_changes_message_with_not_real_callable
+    check = Object.new
+    def check.call
+      @object.num
+    end
+    check.instance_variable_set(:@object, @object)
+
+    error = assert_raises Minitest::Assertion do
+      assert_no_changes check do
+        @object.increment
+      end
+    end
+    assert_match(/#<Object:0x.*changed/, error.message)
   end
 
   def test_assert_no_changes_with_long_string_wont_output_everything
@@ -362,7 +468,7 @@ def test_assert_no_changes_with_long_string_wont_output_everything
     end
 
     assert_match <<~output, error.message
-      "lines" changed.
+      `lines` changed.
       --- expected
       +++ actual
       @@ -10,4 +10,5 @@
@@ -403,7 +509,7 @@ def test_warning_is_not_logged_if_assertions_are_nested_correctly
       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")
+    assert error.message.include?("`rand` changed")
   end
 
   def test_fails_and_warning_is_logged_if_wrong_error_caught