Add new `Minitest/AssertWithExpectedArgument` cop #120

cstyles · 2021-02-21T04:56:33Z

Fixes #117

It is a common mistake to use assert instead of assert_equal. This
is dangerous because minitest will treat the second argument to assert
as the msg parameter and, as long as the first argument is truthy, the
test will always pass. This false negative gives the user a sense of
security even though they're not actually testing what they think they
are.

This PR introduces a new cop which will register a violation if
assert is called with two arguments and the second argument isn't a
String. If the second argument is a String, then the user is
likely using assert as intended. If it's not, most of the time it
means a mistake has been made.

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

koic · 2021-02-21T07:41:58Z

test/rubocop/cop/minitest/assert_with_two_arguments_test.rb

+        end
+      end
+    RUBY
+  end


Can you add the following test cases?

def test_does_not_register_offense_when_second_argument_is_an_interpolated_string assert_no_offenses(<<~'RUBY') class FooTest < Minitest::Test def test_do_something assert([], "empty array should be truthy #{additional_message}") end end RUBY end def test_does_not_register_offense_when_using_assert_equal_with_two_arguments assert_no_offenses(<<~'RUBY') class FooTest < Minitest::Test def test_do_something assert_equal([], actual) end end RUBY end

Fixed in 912fd40

I changed the second test a bit since assert_equal([], actual) should suggest assert_empty? (as of #118 being merged). It looks like assert_no_offenses won't fail if a violation for a different cop is registered but I'd feel better if the example had no violations whatsoever.

assert_equal([], actual) should suggest assert_empty?

The role of each cop force should be simple, so let Minitest/AssertEmptyLiteral cop make suggestions for assert_empty?. So the cop is up to the suggestion to assert_equal :-)

koic · 2021-02-21T07:42:14Z

lib/rubocop/cop/minitest/assert_with_two_arguments.rb

+
+        def on_send(node)
+          assert_with_two_arguments?(node) do |_expected, message|
+            return if message.str_type?


Can you add the condition?

Suggested change

return if message.str_type?

return if message.str_type? || message.dstr_type?

Fixed in 6917350

koic · 2021-02-21T07:42:16Z

lib/rubocop/cop/minitest/assert_with_two_arguments.rb

+      #
+      class AssertWithTwoArguments < Cop
+        MSG = 'Did you mean to use `assert_equal(%<arguments>s)`?'
+


Can you add RESTRICT_ON_SEND?

diff --git a/lib/rubocop/cop/minitest/assert_with_two_arguments.rb b/lib/rubocop/cop/minitest/assert_with_two_arguments.rb index 4ba7bd9..a3b81ff 100644 --- a/lib/rubocop/cop/minitest/assert_with_two_arguments.rb +++ b/lib/rubocop/cop/minitest/assert_with_two_arguments.rb @@ -19,6 +19,8 @@ module RuboCop class AssertWithTwoArguments < Cop MSG = 'Did you mean to use `assert_equal(%<arguments>s)`?' + RESTRICT_ON_SEND = %i[assert].freeze + def_node_matcher :assert_with_two_arguments?, <<~PATTERN (send nil? :assert $_ $_) PATTERN

Fixed in 0db8efb

andyw8 · 2021-02-21T16:17:22Z

I think the name of this cop could be clearer, how about AssetWithInvalidArguments, or AssertWithInvalidMessageArgument?

koic · 2021-02-21T17:11:33Z

lib/rubocop/cop/minitest/assert_with_two_arguments.rb

+  module Cop
+    module Minitest
+      # This cop tries to detect when a user accidentally used
+      # `assert` when they meant to use `assert_equal`


Can you add the period at the end of the sentence?

Suggested change

# `assert` when they meant to use `assert_equal`

# `assert` when they meant to use `assert_equal`.

Fixed in 4b0e9ea

koic · 2021-02-21T17:14:33Z

I'm worried that the term Invalid may be a bit abstract for cop name.

andyw8 · 2021-02-21T17:21:32Z

AssertWithNonStringMessageArgument or AssertWithWrongMessageType ?

cstyles · 2021-02-21T17:41:42Z

I pushed a bunch of fixup commits in response to code review. Let me know when things look good and I'll squash them (along with the changelog commit), rebase onto master, and resolve the merge conflict.

I think the name of this cop could be clearer, how about AssetWithInvalidArguments, or AssertWithInvalidMessageArgument?

AssertWithNonStringMessageArgument or AssertWithWrongMessageType ?

Yeah, I'm not in love with the current name either. Naming things is hard. Personally I think AssertWithNonStringMessageArgument is the clearest.

koic · 2021-02-21T17:48:24Z

Thank you for the idea of the cop name. I think this cop is essentially checking that assert method is passed expected argument that it never receives, so I think of the name AssertWithExpectedArgument.

# bad
assert(expected, actual) # `assert` method never accepts `expected` argument.

# good
assert(test, message)
assert_equal(expected, actual)

cstyles · 2021-02-21T17:58:56Z

I think this cop is essentially checking that assert method is passed expected argument that it never receives

I'm not sure what you mean by this. As I understand it, assert does receive both the expected and message arguments. The issue is that the user passes in something that they think will be treated as the act (or "actual") parameter to assert_equal (source) but in reality it's treated as the failure message parameter to assert.

koic · 2021-02-21T19:19:16Z

test/rubocop/cop/minitest/assert_with_two_arguments_test.rb

+        end
+      end
+    RUBY
+  end


Can you add the testcase?

def test_registers_offense_when_second_argument_is_a_variable assert_offense(<<~RUBY) class FooTest < Minitest::Test def test_do_something my_list = [1, 2, 3].length assert(3, my_list_length) ^^^^^^^^^^^^^^^^^^^^^^^^^ Did you mean to use `assert_equal(3, my_list_length)`? end end RUBY end

Added in d8e98bf

koic · 2021-02-21T19:27:02Z

Let's take assert([], items) as an example of a user misusing assert. The problem with this is that it is given expected argument that assert does not receive.

The user should write assert_equal([], items), but it is giving expect argument by mistake in using assert method.
So I think this cop will check that assert method should not be given an expected value.

The following is an updated offense message with this comment added.

MSG = 'assert method does not take expected value argument. Did you mean to use assert_equal(%<arguments>s)?''

This is the reason for the naming that meant the violation AssertWithExpectedArgument cop.

And I'm worried about AssertWithNonStringMessageArgument because variable can be used in message argument by nature. So I'm worried that the implementation will affect the cop name.

Of course, I think there are more better names I haven't come up with. Naming is difficult 😅

cstyles · 2021-02-22T00:34:23Z

OK, I think I understand what you meant. assert doesn't actually take an "expected" (exp) parameter; it takes an "object" (obj) parameter and then checks if it's truthy.

I guess my issue with calling the cop AssertWithExpectedArgument is that it assumes that the user wanted to treat the first argument as the expected value and the second argument as the actual value. And yeah, from my experience most of the time that's true. But, as I pointed out in #117, there are occasionally times when that's not true, when the user did want to pass a variable as the msg parameter, and when they aren't thinking of the first argument as "expected". This cop is just a best-effort attempt to catch the first case but there will be false positives. In such a case, the user might be confused by the message warning them about an "expected" argument.

The reason why I personally prefer AssertWithNonStringMessageArgument is that it's literally telling you what the cop does: it registers a violation when the second argument (the message) isn't a String. So in the false positive case, the user will see that and know that they can safely ignore the violation.

I guess a better MSG might be something like:

MSG = 'The `msg` argument to `assert` is not a `String`. Did you mean to use assert_equal(%<arguments>s)?'

It's worth mentioning that I'm not fundamentally opposed to either. I just thought I'd explain my preference. Ultimately it's your call to make.

koic · 2021-03-07T06:03:23Z

Sorry for my late reply.
I'm concern that the naming AssertWithNonStringMessageArgument includes string literal type dependency.
For example, Ruby 3.0 introduced Typing (Static Analysis), and I'm seeing experiments in which types are integrated with RuboCop. pocke/rubocop-typed#2

For example, in future it may be possible to detect cases that do not use string literals in messages, such as:

# I hope that these variable message and constant message can be detected as strings in future :-)
message ="#{variable} phrase"
assert(test, message)

MESSAGE ='fixed phrase'
assert(test, MESSAGE)

Unfortunately, that experimentation haven't caught up yet, so current implementation have to rely on literal types 😅
But that's why I prefer the name AssertWithExpectedArgument.

cstyles · 2021-03-20T20:34:12Z

That's a good point. I went ahead and changed the cop name to AssertWithExpectedArgument, squashed all the fixup commits, and rebased onto master. Let me know if I missed anything or if there are any additional changes you'd like to see!

andyw8 · 2021-03-20T20:39:16Z

Perhaps change the PR title to match the new name to avoid confusion, and make it easier to search for?

cstyles · 2021-03-20T20:40:10Z

Done! Good catch!

koic · 2021-03-20T20:43:00Z

CHANGELOG.md

@@ -6,6 +6,9 @@

 * [#118](https://github.com/rubocop/rubocop-minitest/pull/118): **(BREAKING)** Fix `Minitest/AssertEmptyLiteral` by making it check for `assert_equal([], array)` instead of `assert([], array)`. ([@cstyles][])
 * [#122](https://github.com/rubocop/rubocop-minitest/pull/122): Fix `Minitest/TestMethodName` for tests with multiple assertions. ([@ghiculescu][])
+### New features
+
+* [#117](https://github.com/rubocop/rubocop-minitest/issues/117): Add new cop `Minitest/AssertWithExpectedArgument` to check for unintended usages of `assert` instead of `assert_equal`. ([@cstyles][])


Can you move this New features section to above the Bug fixes section and squash your commits into one?

## master (unreleased) + +### New features + +* [#117](https://github.com/rubocop/rubocop-minitest/issues/117): Add new cop `Minitest/AssertWithExpectedArgument` to check for unintended usages of `assert` instead of `assert_equal`. ([@cstyles][]) ### Bug fixes * [#118](https://github.com/rubocop/rubocop-minitest/pull/118): **(BREAKING)** Fix `Minitest/AssertEmptyLiteral` by making it check for `assert_equal([], array)` instead of `assert([], array)`. ([@cstyles][]) * [#122](https://github.com/rubocop/rubocop-minitest/pull/122): Fix `Minitest/TestMethodName` for tests with multiple assertions. ([@ghiculescu][]) -### New features - -* [#117](https://github.com/rubocop/rubocop-minitest/issues/117): Add new cop `Minitest/AssertWithExpectedArgument` to check for unintended usages of `assert` instead of `assert_equal`. ([@cstyles][]) ## 0.10.3 (2021-01-12)

It is a common mistake to use `assert` instead of `assert_equal`. This is dangerous because minitest will treat the second argument to `assert` as the `msg` parameter and, as long as the first argument is truthy, the test will always pass. This false negative gives the user a sense of security even though they're not actually testing what they think they are. This commit introduces a new cop which will register a violation if `assert` is called with two arguments and the second argument isn't a `String`. If the second argument *is* a `String`, then the user is likely using `assert` as intended. If it's not, most of the time it means a mistake has been made.

koic · 2021-03-20T20:47:40Z

Thanks!

cstyles mentioned this pull request Feb 21, 2021

Change AssertEmptyLiteral cop to check for assert_equal #118

Merged

8 tasks

koic reviewed Feb 21, 2021

View reviewed changes

cstyles force-pushed the assert-vs-assert_equal branch from 4b0e9ea to c11c324 Compare February 21, 2021 17:41

koic reviewed Feb 21, 2021

View reviewed changes

cstyles force-pushed the assert-vs-assert_equal branch from df48d62 to d8e98bf Compare February 21, 2021 23:37

cstyles force-pushed the assert-vs-assert_equal branch from d8e98bf to 2d5c3e7 Compare March 20, 2021 20:33

cstyles changed the title ~~Add new Minitest/AssertWithTwoArguments cop~~ Add new Minitest/AssertWithExpectedArgument cop Mar 20, 2021

koic reviewed Mar 20, 2021

View reviewed changes

cstyles force-pushed the assert-vs-assert_equal branch from 2d5c3e7 to 2dea4e6 Compare March 20, 2021 20:44

koic merged commit 81dacc0 into rubocop:master Mar 20, 2021

cstyles deleted the assert-vs-assert_equal branch March 20, 2021 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `Minitest/AssertWithExpectedArgument` cop #120

Add new `Minitest/AssertWithExpectedArgument` cop #120

cstyles commented Feb 21, 2021

koic Feb 21, 2021

cstyles Feb 21, 2021

koic Feb 21, 2021

koic Feb 21, 2021

cstyles Feb 21, 2021

koic Feb 21, 2021

cstyles Feb 21, 2021

andyw8 commented Feb 21, 2021

koic Feb 21, 2021

cstyles Feb 21, 2021

koic commented Feb 21, 2021

andyw8 commented Feb 21, 2021

cstyles commented Feb 21, 2021

koic commented Feb 21, 2021 •

edited

Loading

cstyles commented Feb 21, 2021

koic Feb 21, 2021

cstyles Feb 21, 2021

koic commented Feb 21, 2021

cstyles commented Feb 22, 2021

koic commented Mar 7, 2021

cstyles commented Mar 20, 2021

andyw8 commented Mar 20, 2021

cstyles commented Mar 20, 2021

koic Mar 20, 2021

koic commented Mar 20, 2021

	return if message.str_type?
	return if message.str_type? \|\| message.dstr_type?

	# `assert` when they meant to use `assert_equal`
	# `assert` when they meant to use `assert_equal`.

Add new Minitest/AssertWithExpectedArgument cop #120

Add new Minitest/AssertWithExpectedArgument cop #120

Conversation

cstyles commented Feb 21, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Feb 21, 2021

andyw8 commented Feb 21, 2021

cstyles commented Feb 21, 2021

koic commented Feb 21, 2021 • edited Loading

cstyles commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Feb 21, 2021

cstyles commented Feb 22, 2021

koic commented Mar 7, 2021

cstyles commented Mar 20, 2021

andyw8 commented Mar 20, 2021

cstyles commented Mar 20, 2021

Choose a reason for hiding this comment

koic commented Mar 20, 2021

Add new `Minitest/AssertWithExpectedArgument` cop #120

Add new `Minitest/AssertWithExpectedArgument` cop #120

koic commented Feb 21, 2021 •

edited

Loading