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

richardboehme · 2024-06-06T20:47:05Z

Motivation / Background

In our team we often discuss if it's better to pass a string or a proc to assert_difference. In my opinion it's better to pass a proc because linters and IDEs can analyze the ruby code in the proc. However one disadvantage is that the error message is less clear when using a proc, because the message just prints the inspect-output of the Proc object.

To fix this, I propose this PR which prints the source of the proc instead.

Detail

This Pull Request changes the error message of assert_difference, assert_no_difference, assert_changes and assert_no_changes when using MRI and when passing a proc to those assertions. It leverages the experimental RubyVM::AbstractSyntaxTree api to print the source code of the proc which makes it easier to spot why the test failed.

For example consider the following test:

test "see proc output" do
  assert_difference -> { 1 } do
  end
end

Previous output:

#<Proc:0x000074357846eae8 /home/richard/my-test-app/test/models/test.rb:21 (lambda)> didn't change by 1, but by 0.
Expected: 2
  Actual: 1

Output with this change:

"-> { 1 }" didn't change by 1, but by 0.
Expected: 2
  Actual: 1

Note that this only works in MRI because other ruby implementations do not implement the RubyVM::AbstractSyntaxTree api.

Additional information

I'm not sure if Rails wants to "depend" on an experimental api like RubyVM::AbstractSyntaxTree because it might not be stable and change in the future. I think this change brings value to Rails but I understand if there is a policy about not using experimental ruby apis. In this case we can close this PR.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

MatheusRich · 2024-06-07T17:45:40Z

I wonder if it's that valuable to print the expression inside the proc. Maybe the error message could be just Expression didn't change by 1, but by 0. That would work for any Ruby implementation.

MatheusRich · 2024-06-07T17:46:57Z

activesupport/lib/active_support/testing/assertions.rb

+            ast = RubyVM::AbstractSyntaxTree.of(callable, keep_script_lines: true)
+            "-> #{ast.source.strip}"


Could we use Prism here? I wonder if that would help with other Ruby implementations

Hm I think Prism does not have an API to get the source of a method. The only thing I can think of is parsing the file with Prism and trying to find the location of the assertion (probably by using the line number from caller?).

Maybe there is another way I do not know of, but right know I feel like it would be way too complicated to achieve this using Prism.

richardboehme · 2024-06-07T18:48:30Z

I wonder if it's that valuable to print the expression inside the proc. Maybe the error message could be just Expression didn't change by 1, but by 0. That would work for any Ruby implementation.

I think that might be valuable as well. By not printing Proc#inspect it might be easier to understand for folks that are new to the framework or language. However, for experienced developers this would mean we print even less information than before.

My goal here was to have the same information that I would get when using a code string instead of a proc. In this case we get the whole code string printed, so I thought why shouldn't we do this for procs as well.

activesupport/lib/active_support/testing/assertions.rb

activesupport/test/test_case_test.rb

byroot · 2024-06-08T07:29:19Z

I like this change, my only worry is that RubyVM::AbstractSyntaxTree is not stable, so we might get bit hard if it changes in the future. But perhaps this ship has sailed because we already use it in Action View

byroot · 2024-06-08T07:30:37Z

activesupport/test/test_case_test.rb

+      end
+    end
+
+    assert_equal "\"-> { @object.num }\" didn't change. It was already 0.\nExpected 0 to not be equal to 0.", error.message


I think I'd prefer something like:

Suggested change

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

Yeah I like that better as well, it's just a bit harder to implement since we do not get location information about the actual content of the proc. I tried implementing it in the second commit (I'll squash those before merging) using a regex, but I'm not sure how robust this solution is. Let me know what you think!

I made a pass on it. Using regexp to parse Ruby code is tricky, so I instead went to a route were we only include the source if we match the happy path (single line, defined with {}, no arguments).

The idea being that the overwhelming majority of procs used for assert_changes match these criteria, and those which don't would be a pain to render properly, so we just fallback to the old behavior.

Sounds like a good compromise to me. Thanks for merging!

richardboehme · 2024-06-08T20:42:38Z

I like this change, my only worry is that RubyVM::AbstractSyntaxTree is not stable, so we might get bit hard if it changes in the future. But perhaps this ship has sailed because we already use it in Action View

Yeah it's definitely a risk to evaluate. From my personal point of view I think the improvement it brings is worth it.

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`.

byroot · 2024-06-09T08:53:02Z

activesupport/lib/active_support/testing/assertions.rb

+            rescue SystemCallError
+              # Failed to get the source somehow
+              return callable


Also had to harden it a bit, as evidenced by railties test, it can happen that the source is not accessible.

…erence-output" This reverts commit e3ea4c7, reversing changes made to fdbe363. With prism as the default parser in Ruby 3.4, the method of getting a procs source does not work anymore. Also see https://bugs.ruby-lang.org/issues/20761

rails-bot bot added the activesupport label Jun 6, 2024

MatheusRich reviewed Jun 7, 2024

View reviewed changes

byroot reviewed Jun 8, 2024

View reviewed changes

activesupport/lib/active_support/testing/assertions.rb Outdated Show resolved Hide resolved

byroot reviewed Jun 8, 2024

View reviewed changes

activesupport/test/test_case_test.rb Outdated Show resolved Hide resolved

byroot reviewed Jun 8, 2024

View reviewed changes

richardboehme force-pushed the assert-difference-output branch from 36927a3 to 08874a7 Compare June 8, 2024 20:37

byroot force-pushed the assert-difference-output branch 3 times, most recently from 2fba0f7 to bd97b56 Compare June 9, 2024 08:36

byroot force-pushed the assert-difference-output branch from bd97b56 to 38e9695 Compare June 9, 2024 08:46

byroot reviewed Jun 9, 2024

View reviewed changes

byroot merged commit e3ea4c7 into rails:main Jun 9, 2024
2 of 3 checks passed

richardboehme deleted the assert-difference-output branch June 9, 2024 09:35

byroot mentioned this pull request Jun 19, 2024

Lazily generate assertion failure messages #52157

Merged

Earlopain mentioned this pull request Sep 16, 2024

Skip tests that raises "cannot get AST for ISEQ compiled by prism" if Ruby parse is Prism #52937

Closed

4 tasks

Earlopain mentioned this pull request Sep 26, 2024

Revert changes to assert_difference and friends from #52036 #53054

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

richardboehme commented Jun 6, 2024

MatheusRich commented Jun 7, 2024

MatheusRich Jun 7, 2024

richardboehme Jun 7, 2024

richardboehme commented Jun 7, 2024

byroot commented Jun 8, 2024

byroot Jun 8, 2024

richardboehme Jun 8, 2024

byroot Jun 9, 2024

byroot Jun 9, 2024

richardboehme Jun 9, 2024

richardboehme commented Jun 8, 2024

byroot Jun 9, 2024

		ast = RubyVM::AbstractSyntaxTree.of(callable, keep_script_lines: true)
		"-> #{ast.source.strip}"

	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

Improve error message when passing a proc to assert_difference or assert_changes #52036

Improve error message when passing a proc to assert_difference or assert_changes #52036

Conversation

richardboehme commented Jun 6, 2024

Motivation / Background

Detail

Additional information

Checklist

MatheusRich commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardboehme commented Jun 7, 2024

byroot commented Jun 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardboehme commented Jun 8, 2024

Choose a reason for hiding this comment

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

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