Suggest assert_equal when using assert with multiple arguments

[cstyles]

Is your feature request related to a problem? Please describe.

We recently caught an issue where we accidentally used the following code...

assert 1, search_table_rows.size

...when we really wanted this:

assert_equal 1, search_table_rows.size

The first example will always pass, effectively making the test useless. This has happened a number of times before (someone even previously reported it in #32) and the only way I know of to prevent it is to hope that someone catches it in code review.

Describe the solution you'd like

The most obvious solution is to register a violation when two arguments are passed to assert and to suggest assert_equal in that case. However, assert does accept a second argument – a custom message to be displayed if the test fails:

https://github.com/seattlerb/minitest/blob/b8fb1014ad1932eddefb5107c0f6504ba45e0cbc/lib/minitest/assertions.rb#L178

It would be annoying to get a violation every time the user actually wanted to use msg. A simple improvement would be to not register a violation if the second argument is a literal String.

I poked around using grep.app and found that most usages of the msg argument were using a literal String:

https://github.com/thoughtbot/paperclip/blob/e3f7493b4cc954405a0209d26933830333ae2869/spec/paperclip/validators/attachment_file_name_validator_spec.rb#L37
https://github.com/theforeman/foreman/blob/46fcf9361e2d8c6cb89af2c3f8b9be60365fdd21/test/models/orchestration/dhcp_test.rb#L12
https://github.com/faker-ruby/faker/blob/664cbafe9d15bddb97843dcbe107204e6e7291cb/test/test_locale.rb#L75
https://github.com/Linuxbrew/legacy-linuxbrew/blob/532040391bb984e410b7ad73bdd0c518f945cc0b/Library/Formula/lcm.rb#L62
https://github.com/rails/rails/blob/da418dc2508eb8af94ba88b70034021cd17b1abd/activerecord/test/cases/validations/uniqueness_validation_test.rb#L80

Most of the instances I could find where msg wasn't a literal String were in custom assertion code like the following:

https://github.com/thoughtbot/shoulda-context/blob/970d3d57a584ecb2652f0bc7188761024de16c52/lib/shoulda/context/assertions.rb#L94
https://github.com/thoughtbot/json_matchers/blob/4644ca82adb01055528b5643df93feb1d35c8205/lib/json_matchers/minitest/assertions.rb#L14

These are legitimate usages of assert with two, non-literal arguments. So if the proposed solution was implemented, these cases would have to be amended with a rubocop:disable. From my experience, it's generally more likely that the user really meant to use assert_equal but ultimately it's not my call to make. At the very least, we could add the cop but have it disabled by default.

Describe alternatives you've considered

It would be nice if the msg parameter to assert was instead a named parameter but I think it's far too late for such a drastically breaking change at this point.

Additional context

This idea was previously suggested in #32. That issue was closed by #38 but while the change (namely, adding a cop to prefer assert_empty X to assert [], X) is desirable, it didn't address the actual issue as I understand it. I figured it was better to open a new issue than to revive a dead one.

[cstyles]

Another idea that just occurred to me would be to register a violation if assert is called with two arguments and the first argument to assert is a literal number/string. For example:

assert 100, list.length

This seems like a pretty obvious case where assert_equal was what the user intended.

[andyw8]

Probably relevant: https://gist.github.com/arthurnn/cbd4da8512e4994dd8fe719ffcbcb569

[cstyles]

I audited our largest repository at my company for accidental usages of assert and found more than 200 of them. Most of them were easily fixed by switching to assert_equal but a nontrivial number of tests are now failing because the underlying test logic was faulty and we didn't catch it. 😱

75% of them use a literal (Integer, Symbol, String, etc.) for the expected argument like I mentioned in this comment. So I think that's a decent place to start. I can try to throw together a PR this weekend but if someone more familiar with the codebase also wants to give it a go, feel free (but please let me know!).

Probably relevant: gist.github.com/arthurnn/cbd4da8512e4994dd8fe719ffcbcb569

Thanks, this looks super useful!