From 38e9695c10f822caaa54e399833c346249660005 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20B=C3=B6hme?= Date: Thu, 6 Jun 2024 22:09:28 +0200 Subject: [PATCH] Improve error message when passing a proc to `assert_difference` Previously if `assert_difference` called with a proc fails, the inspect output of the proc object was shown. This is not helpful to identify what went wrong. With this commit we leverage the experimental `RubyVM::AbstractSyntaxTree` api of MRI to print the source code of the proc that was passed to `assert_difference`. On all other platforms the behavior stays the same. The same applies to `assert_changes`. --- activesupport/CHANGELOG.md | 3 + .../lib/active_support/testing/assertions.rb | 39 +++++- activesupport/test/test_case_test.rb | 122 ++++++++++++++++-- 3 files changed, 153 insertions(+), 11 deletions(-) diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 943bc0018e..78acdbdd28 100644 --- a/activesupport/CHANGELOG.md +++ b/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. diff --git a/activesupport/lib/active_support/testing/assertions.rb b/activesupport/lib/active_support/testing/assertions.rb index 8d484a78f1..4de837d5df 100644 --- a/activesupport/lib/active_support/testing/assertions.rb +++ b/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 diff --git a/activesupport/test/test_case_test.rb b/activesupport/test/test_case_test.rb index 7ce9ba640f..fa8a5aab6a 100644 --- a/activesupport/test/test_case_test.rb +++ b/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(/# changed") + assert error.message.include?("`rand` changed") end def test_fails_and_warning_is_logged_if_wrong_error_caught