-
Notifications
You must be signed in to change notification settings - Fork 22.1k
Prepping rails for minitest 6 #56202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d987d90 to
ee31f01
Compare
978c7f4 to
5c51421
Compare
f9b0ff2 to
0e2427a
Compare
MT6 changes the way assertion messages work. Now, if a proc is passed in for the message, it wins untouched. So for the rails assertions that want to have diffs shown while calling assert_equal with a message proc, the proc needs to call diff itself. This feels redundant to me, but not my call. And since the procs win now, they need to provide their own periods at the end of the text.
Mostly minor and mostly centered around whether there are diffs.
0e2427a to
b58ab2a
Compare
tenderlove
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes seem fine, but I have a couple questions. If some of these changes are necessary, I would like to understand why. If they aren't necessary, then maybe remove them from the PR? (I don't actually mind whether or not those changes are there, I just want to know if they are there because of the upgrade to MT6)
|
|
||
| test "not being empty when route is added" do | ||
| assert_predicate self, :empty? | ||
| assert_empty @set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these need to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because y'all made empty? private and assert_predicate does a send on self. Breaks. assert_empty on @set passes.
| rich_message = -> do | ||
| code_string = expression.respond_to?(:call) ? _callable_to_source_string(expression) : expression | ||
| error = "`#{code_string}` changed" | ||
| error = "`#{code_string}` changed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these periods load bearing? Why do they need to be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message helper has gotten a lot simpler/cleaner for MT6 and doesn't end sentences for you anymore. changing the error message to include the period passes on both MT5 and MT6. Otherwise I have to fight versioning on the assert_equal against the message output.
This branch contains all the fixes needed for minitest 6 to work smoothly with rails. I've now tested extensively on MT5 as well as MT6.
I have another branch testing out the actual MT6 bump and it still has some sort of flaky going on that I'm investigating.