Skip to content

Commit

Permalink
Change AssertEmptyLiteral cop to check for assert_equal
Browse files Browse the repository at this point in the history
While this cop is correct that we should be using `assert_empty(list)`
over `assert([], list)`, it's right for the wrong reasons. That example
is bad because it's using `assert` and not `assert_equal`. An empty
list/hash is truthy so the assertion will always succeed. That is,
`assert([], [1, 2, 3])` will succeed even though at a glance it looks
like it shouldn't. I believe that what this cop is intending to check
for is `assert_equal([], list)` instead so I've updated the references
to `assert` to `assert_equal`.

Issue #117 proposes that we try to detect when `assert` is used where
the user likely wanted `assert_equal` so while this cop will no longer
register a violation for `assert([], list)`, if that proposal is
accepted then a future cop *will* register a violation.
  • Loading branch information
cstyles committed Feb 21, 2021
1 parent 09c018d commit 70f02e0
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 18 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### Bug fixes

* [#118](https://github.com/rubocop-hq/rubocop-minitest/pull/118): **(BREAKING)** Fix `Minitest/AssertEmptyLiteral` by making it check for `assert_equal([], array)` instead of `assert([], array)`. ([@cstyles][])

## 0.10.3 (2021-01-12)

### Bug fixes
Expand Down Expand Up @@ -177,3 +181,4 @@
[@andrykonchin]: https://github.com/andrykonchin
[@fatkodima]: https://github.com/fatkodima
[@tsmmark]: https://github.com/tsmmark
[@cstyles]: https://github.com/cstyles
6 changes: 3 additions & 3 deletions docs/modules/ROOT/pages/cops_minitest.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ assert_empty(object, 'message')
|===

This cop enforces the test to use `assert_empty`
instead of using `assert([], object)`.
instead of using `assert_equal([], object)`.

=== Examples

[source,ruby]
----
# bad
assert([], object)
assert({}, object)
assert_equal([], object)
assert_equal({}, object)
# good
assert_empty(object)
Expand Down
16 changes: 8 additions & 8 deletions lib/rubocop/cop/minitest/assert_empty_literal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module RuboCop
module Cop
module Minitest
# This cop enforces the test to use `assert_empty`
# instead of using `assert([], object)`.
# instead of using `assert_equal([], object)`.
#
# @example
# # bad
# assert([], object)
# assert({}, object)
# assert_equal([], object)
# assert_equal({}, object)
#
# # good
# assert_empty(object)
Expand All @@ -18,14 +18,14 @@ class AssertEmptyLiteral < Cop
include ArgumentRangeHelper

MSG = 'Prefer using `assert_empty(%<arguments>s)` over ' \
'`assert(%<literal>s, %<arguments>s)`.'
'`assert_equal(%<literal>s, %<arguments>s)`.'

def_node_matcher :assert_with_empty_literal, <<~PATTERN
(send nil? :assert ${hash array} $...)
def_node_matcher :assert_equal_with_empty_literal, <<~PATTERN
(send nil? :assert_equal ${hash array} $...)
PATTERN

def on_send(node)
assert_with_empty_literal(node) do |literal, matchers|
assert_equal_with_empty_literal(node) do |literal, matchers|
return unless literal.values.empty?

args = matchers.map(&:source).join(', ')
Expand All @@ -36,7 +36,7 @@ def on_send(node)
end

def autocorrect(node)
assert_with_empty_literal(node) do |_literal, matchers|
assert_equal_with_empty_literal(node) do |_literal, matchers|
object = matchers.first

lambda do |corrector|
Expand Down
14 changes: 7 additions & 7 deletions test/rubocop/cop/minitest/assert_empty_literal_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ def test_registers_offense_when_asserting_empty_array
assert_offense(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert([], somestuff)
^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert([], somestuff)`.
assert_equal([], somestuff)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert_equal([], somestuff)`.
end
end
RUBY
Expand All @@ -26,8 +26,8 @@ def test_registers_offense_when_asserting_empty_hash
assert_offense(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert({}, somestuff)
^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert({}, somestuff)`.
assert_equal({}, somestuff)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_empty(somestuff)` over `assert_equal({}, somestuff)`.
end
end
RUBY
Expand All @@ -41,7 +41,7 @@ def test_do_something
RUBY
end

def test_does_not_register_offense_when_using_assert_equal
def test_does_not_register_offense_when_asserting_non_empty_array
assert_no_offenses(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
Expand All @@ -51,11 +51,11 @@ def test_do_something
RUBY
end

def test_does_not_register_offense_when_using_assert_with_single_parameter
def test_does_not_register_offense_when_asserting_two_parameters
assert_no_offenses(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert(somestuff)
assert_equal(somestuff, someotherstuff)
end
end
RUBY
Expand Down

0 comments on commit 70f02e0

Please sign in to comment.