Merge pull request #141 from koic/make_assert_nil_and_refute_nil_awar…

…e_of_assert_method

[Fix #140] Make `AssertNil` and `RefuteNil` aware of `assert(obj.nil?)`

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## master (unreleased)
 
+### New features
+
+* [#140](https://github.com/rubocop/rubocop-minitest/issues/140): Make `Minitest/AssertNil` and `Minitest/RefuteNil` aware of `assert(obj.nil?)` and `refute(obj.nil?)`. ([@koic][])
+
 ## 0.14.0 (2021-07-03)
 
 ### New features

config/default.yml

@@ -59,7 +59,7 @@ Minitest/AssertKindOf:
   VersionAdded: '0.10'
 
 Minitest/AssertNil:
-  Description: 'This cop enforces the test to use `assert_nil` instead of using `assert_equal(nil, something)`.'
+  Description: 'This cop enforces the test to use `assert_nil` instead of using `assert_equal(nil, something)` or `assert(something.nil?)`.'
   StyleGuide: 'https://minitest.rubystyle.guide#assert-nil'
   Enabled: true
   VersionAdded: '0.1'
@@ -172,7 +172,7 @@ Minitest/RefuteKindOf:
   VersionAdded: '0.10'
 
 Minitest/RefuteNil:
-  Description: 'This cop enforces the test to use `refute_nil` instead of using `refute_equal(nil, something)`.'
+  Description: 'This cop enforces the test to use `refute_nil` instead of using `refute_equal(nil, something)` or `refute(something.nil?)`.'
   StyleGuide: 'https://minitest.rubystyle.guide#refute-nil'
   Enabled: true
   VersionAdded: '0.2'

docs/modules/ROOT/pages/cops_minitest.adoc

@@ -261,8 +261,8 @@ assert_match(matcher, string, 'message')
 | -
 |===
 
-This cop enforces the test to use `assert_nil`
-instead of using `assert_equal(nil, something)`.
+This cop enforces the test to use `assert_nil` instead of using
+`assert_equal(nil, something)` or `assert(something.nil?)`.
 
 === Examples
 
@@ -271,6 +271,8 @@ instead of using `assert_equal(nil, something)`.
 # bad
 assert_equal(nil, actual)
 assert_equal(nil, actual, 'message')
+assert(object.nil?)
+assert(object.nil?, 'message')
 
 # good
 assert_nil(actual)
@@ -927,8 +929,8 @@ refute_match(matcher, string, 'message')
 | -
 |===
 
-This cop enforces the test to use `refute_nil`
-instead of using `refute_equal(nil, something)`.
+This cop enforces the test to use `refute_nil` instead of using
+`refute_equal(nil, something)` or `refute(something.nil?)`.
 
 === Examples
 
@@ -937,6 +939,8 @@ instead of using `refute_equal(nil, something)`.
 # bad
 refute_equal(nil, actual)
 refute_equal(nil, actual, 'message')
+refute(actual.nil?)
+refute(actual.nil?, 'message')
 
 # good
 refute_nil(actual)

lib/rubocop/cop/minitest/assert_nil.rb

@@ -3,41 +3,45 @@
 module RuboCop
   module Cop
     module Minitest
-      # This cop enforces the test to use `assert_nil`
-      # instead of using `assert_equal(nil, something)`.
+      # This cop enforces the test to use `assert_nil` instead of using
+      # `assert_equal(nil, something)` or `assert(something.nil?)`.
       #
       # @example
       #   # bad
       #   assert_equal(nil, actual)
       #   assert_equal(nil, actual, 'message')
+      #   assert(object.nil?)
+      #   assert(object.nil?, 'message')
       #
       #   # good
       #   assert_nil(actual)
       #   assert_nil(actual, 'message')
       #
       class AssertNil < Base
         include ArgumentRangeHelper
+        include NilAssertionHandleable
         extend AutoCorrector
 
-        MSG = 'Prefer using `assert_nil(%<arguments>s)` over ' \
-              '`assert_equal(nil, %<arguments>s)`.'
-        RESTRICT_ON_SEND = %i[assert_equal].freeze
+        ASSERTION_TYPE = 'assert'
+        RESTRICT_ON_SEND = %i[assert_equal assert].freeze
 
-        def_node_matcher :assert_equal_with_nil, <<~PATTERN
-          (send nil? :assert_equal nil $_ $...)
+        def_node_matcher :nil_assertion, <<~PATTERN
+          {
+            (send nil? :assert_equal nil $_ $...)
+            (send nil? :assert (send $_ :nil?) $...)
+          }
         PATTERN
 
         def on_send(node)
-          assert_equal_with_nil(node) do |actual, message|
-            message = message.first
+          nil_assertion(node) do |actual, message|
+            register_offense(node, actual, message)
+          end
+        end
 
-            arguments = [actual.source, message&.source].compact.join(', ')
+        private
 
-            add_offense(node, message: format(MSG, arguments: arguments)) do |corrector|
-              corrector.replace(node.loc.selector, 'assert_nil')
-              corrector.replace(first_and_second_arguments_range(node), actual.source)
-            end
-          end
+        def assertion_type
+          ASSERTION_TYPE
         end
       end
     end

lib/rubocop/cop/minitest/refute_nil.rb

@@ -3,43 +3,45 @@
 module RuboCop
   module Cop
     module Minitest
-      # This cop enforces the test to use `refute_nil`
-      # instead of using `refute_equal(nil, something)`.
+      # This cop enforces the test to use `refute_nil` instead of using
+      # `refute_equal(nil, something)` or `refute(something.nil?)`.
       #
       # @example
       #   # bad
       #   refute_equal(nil, actual)
       #   refute_equal(nil, actual, 'message')
+      #   refute(actual.nil?)
+      #   refute(actual.nil?, 'message')
       #
       #   # good
       #   refute_nil(actual)
       #   refute_nil(actual, 'message')
       #
       class RefuteNil < Base
         include ArgumentRangeHelper
+        include NilAssertionHandleable
         extend AutoCorrector
 
-        MSG = 'Prefer using `refute_nil(%<arguments>s)` over ' \
-              '`refute_equal(nil, %<arguments>s)`.'
-        RESTRICT_ON_SEND = %i[refute_equal].freeze
+        ASSERTION_TYPE = 'refute'
+        RESTRICT_ON_SEND = %i[refute_equal refute].freeze
 
-        def_node_matcher :refute_equal_with_nil, <<~PATTERN
-          (send nil? :refute_equal nil $_ $...)
+        def_node_matcher :nil_refutation, <<~PATTERN
+          {
+            (send nil? :refute_equal nil $_ $...)
+            (send nil? :refute (send $_ :nil?) $...)
+          }
         PATTERN
 
         def on_send(node)
-          refute_equal_with_nil(node) do |actual, message|
-            message = message.first
+          nil_refutation(node) do |actual, message|
+            register_offense(node, actual, message)
+          end
+        end
 
-            arguments = [actual.source, message&.source].compact.join(', ')
+        private
 
-            add_offense(node, message: format(MSG, arguments: arguments)) do |corrector|
-              corrector.replace(node.loc.selector, 'refute_nil')
-              corrector.replace(
-                first_and_second_arguments_range(node), actual.source
-              )
-            end
-          end
+        def assertion_type
+          ASSERTION_TYPE
         end
       end
     end

lib/rubocop/cop/minitest_cops.rb

@@ -4,6 +4,7 @@
 require_relative 'mixin/in_delta_mixin'
 require_relative 'mixin/minitest_cop_rule'
 require_relative 'mixin/minitest_exploration_helpers'
+require_relative 'mixin/nil_assertion_handleable'
 require_relative 'minitest/assert_empty'
 require_relative 'minitest/assert_empty_literal'
 require_relative 'minitest/assert_equal'

lib/rubocop/cop/mixin/nil_assertion_handleable.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Minitest
+      # Common functionality for `AssertNil` and `RefuteNil` cops.
+      module NilAssertionHandleable
+        MSG = 'Prefer using `%<assertion_type>s_nil(%<preferred_args>s)` over `%<method>s(%<current_args>s)`.'
+
+        private
+
+        def register_offense(node, actual, message)
+          message = build_message(node, actual, message)
+
+          add_offense(node, message: message) do |corrector|
+            autocorrect(corrector, node, actual)
+          end
+        end
+
+        def build_message(node, actual, message)
+          message = message.first
+          message_source = message&.source
+
+          preferred_args = [actual.source, message_source].compact
+          current_args = if comparison_assertion_method?(node)
+                           ['nil', preferred_args].join(', ')
+                         else
+                           ["#{actual.source}.nil?", message_source].compact.join(', ')
+                         end
+
+          format(
+            MSG,
+            assertion_type: assertion_type,
+            preferred_args: preferred_args.join(', '),
+            method: node.method_name, current_args: current_args
+          )
+        end
+
+        def autocorrect(corrector, node, actual)
+          corrector.replace(node.loc.selector, :"#{assertion_type}_nil")
+          if comparison_assertion_method?(node)
+            corrector.replace(first_and_second_arguments_range(node), actual.source)
+          else
+            corrector.remove(node.first_argument.loc.dot)
+            corrector.remove(node.first_argument.loc.selector)
+          end
+        end
+
+        def comparison_assertion_method?(node)
+          node.method?(:"#{assertion_type}_equal")
+        end
+      end
+    end
+  end
+end

test/rubocop/cop/minitest/assert_nil_test.rb

@@ -85,6 +85,44 @@ def test_do_something
     RUBY
   end
 
+  def test_registers_offense_when_using_assert_with_nil_predicate_method_call
+    assert_offense(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          assert(somestuff.nil?)
+          ^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff)` over `assert(somestuff.nil?)`.
+        end
+      end
+    RUBY
+
+    assert_correction(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          assert_nil(somestuff)
+        end
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_using_assert_with_nil_predicate_method_call_and_message
+    assert_offense(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          assert(somestuff.nil?, 'message')
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_nil(somestuff, 'message')` over `assert(somestuff.nil?, 'message')`.
+        end
+      end
+    RUBY
+
+    assert_correction(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          assert_nil(somestuff, 'message')
+        end
+      end
+    RUBY
+  end
+
   def test_does_not_register_offense_when_using_assert_nil_method
     assert_no_offenses(<<~RUBY)
       class FooTest < Minitest::Test

test/rubocop/cop/minitest/refute_nil_test.rb

@@ -85,6 +85,44 @@ def test_do_something
     RUBY
   end
 
+  def test_registers_offense_when_using_refute_with_nil_predicate_method_call
+    assert_offense(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          refute(somestuff.nil?)
+          ^^^^^^^^^^^^^^^^^^^^^^ Prefer using `refute_nil(somestuff)` over `refute(somestuff.nil?)`.
+        end
+      end
+    RUBY
+
+    assert_correction(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          refute_nil(somestuff)
+        end
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_using_refute_with_nil_predicate_method_call_and_message
+    assert_offense(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          refute(somestuff.nil?, 'message')
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `refute_nil(somestuff, 'message')` over `refute(somestuff.nil?, 'message')`.
+        end
+      end
+    RUBY
+
+    assert_correction(<<~RUBY)
+      class FooTest < Minitest::Test
+        def test_do_something
+          refute_nil(somestuff, 'message')
+        end
+      end
+    RUBY
+  end
+
   def test_does_not_register_offense_when_using_refute_nil_method
     assert_no_offenses(<<~RUBY)
       class FooTest < Minitest::Test