Fix some false negatives for Minitest/GlobalExpectations

.rubocop_todo.yml

@@ -20,4 +20,4 @@ Metrics/MethodLength:
 # Configuration parameters: AutoCorrect, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
 # URISchemes: http, https
 Layout/LineLength:
-  Max: 100
+  Max: 120

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## master (unreleased)
 
+### Bug fixes
+
+* [#72](https://github.com/rubocop-hq/rubocop-minitest/pull/72): Fix some false negatives for `Minitest/GlobalExpectations`. ([@andrykonchin][])
+
 ## 0.8.0 (2020-03-24)
 
 ### New features
@@ -119,3 +123,4 @@
 [@abhaynikam]: https://github.com/abhaynikam
 [@herwinw]: https://github.com/herwinw
 [@fsateler]: https://github.com/fsateler
+[@andrykonchin]: https://github.com/andrykonchin

lib/rubocop/cop/minitest/global_expectations.rb

@@ -30,27 +30,43 @@ class GlobalExpectations < Cop
 
         BLOCK_MATCHERS = %i[must_output must_raise must_be_silent must_throw].freeze
 
-        MATCHERS_STR = (VALUE_MATCHERS + BLOCK_MATCHERS).map do |m|
+        VALUE_MATCHERS_STR = VALUE_MATCHERS.map do |m|
           ":#{m}"
         end.join(' ').freeze
 
-        def_node_matcher :global_expectation?, <<~PATTERN
-          (send {
-            (send _ _)
-            ({lvar ivar cvar gvar} _)
-            (send {(send _ _) ({lvar ivar cvar gvar} _)} _ _)
-          } {#{MATCHERS_STR}} ...)
+        BLOCK_MATCHERS_STR = BLOCK_MATCHERS.map do |m|
+          ":#{m}"
+        end.join(' ').freeze
+
+        # There are aliases for the `_` method - `expect` and `value`
+        DSL_METHODS_LIST = %w[_ value expect].map do |n|
+          ":#{n}"
+        end.join(' ').freeze
+
+        def_node_matcher :value_global_expectation?, <<~PATTERN
+          (send !(send nil? {#{DSL_METHODS_LIST}} _) {#{VALUE_MATCHERS_STR}} _)
+        PATTERN
+
+        def_node_matcher :block_global_expectation?, <<~PATTERN
+          (send
+            [
+              !(send nil? {#{DSL_METHODS_LIST}} _)
+              !(block (send nil? {#{DSL_METHODS_LIST}}) _ _)
+            ]
+            {#{BLOCK_MATCHERS_STR}}
+            _
+          )
         PATTERN
 
         def on_send(node)
-          return unless global_expectation?(node)
+          return unless value_global_expectation?(node) || block_global_expectation?(node)
 
           message = format(MSG, preferred: preferred_receiver(node))
           add_offense(node, location: node.receiver.source_range, message: message)
         end
 
         def autocorrect(node)
-          return unless global_expectation?(node)
+          return unless value_global_expectation?(node) || block_global_expectation?(node)
 
           lambda do |corrector|
             receiver = node.receiver.source_range

lib/rubocop/cop/mixin/minitest_cop_rule.rb

@@ -22,9 +22,7 @@ module MinitestCopRule
       # @param inverse [Boolean] An optional param. Order of arguments replaced by auto-correction.
       #
       def define_rule(assertion_method, target_method:, preferred_method: nil, inverse: false)
-        if preferred_method.nil?
-          preferred_method = "#{assertion_method}_#{target_method.to_s.delete('?')}"
-        end
+        preferred_method = "#{assertion_method}_#{target_method.to_s.delete('?')}" if preferred_method.nil?
 
         class_eval(<<~RUBY, __FILE__, __LINE__ + 1)
           include ArgumentRangeHelper

test/assertion_helper.rb

@@ -29,9 +29,7 @@ def assert_offense(source, file = nil)
     @cop.instance_variable_get(:@options)[:auto_correct] = true
 
     expected_annotations = RuboCop::RSpec::ExpectOffense::AnnotatedSource.parse(source)
-    if expected_annotations.plain_source == source
-      raise 'Use `assert_no_offenses` to assert that no offenses are found'
-    end
+    raise 'Use `assert_no_offenses` to assert that no offenses are found' if expected_annotations.plain_source == source
 
     @processed_source = inspect_source(expected_annotations.plain_source, @cop, file)
 

test/rubocop/cop/minitest/global_expectations_test.rb

@@ -162,6 +162,67 @@ class GlobalExpectationsTest < Minitest::Test
         end
       RUBY
     end
+
+    define_method(:"test_no_offense_when_using_expect_form_of_#{matcher}_with_value_method") do
+      assert_no_offenses(<<~RUBY)
+        it 'does something' do
+          value(n).#{matcher} 42
+        end
+      RUBY
+    end
+
+    define_method(:"test_no_offense_when_using_expect_form_of_#{matcher}_with_expect_method") do
+      assert_no_offenses(<<~RUBY)
+        it 'does something' do
+          expect(n).#{matcher} 42
+        end
+      RUBY
+    end
+
+    define_method(:"test_registers_offense_when_using_global_#{matcher}_for_chained_hash_reference") do
+      assert_offense(<<~RUBY)
+        it 'does something' do
+          options[:a][:b].#{matcher} 0
+          ^^^^^^^^^^^^^^^ Use `_(options[:a][:b])` instead.
+        end
+      RUBY
+
+      assert_correction(<<~RUBY)
+        it 'does something' do
+          _(options[:a][:b]).#{matcher} 0
+        end
+      RUBY
+    end
+
+    define_method(:"test_registers_offense_when_using_global_#{matcher}_for_method_call_with_params") do
+      assert_offense(<<~RUBY)
+        it 'does something' do
+          foo(a).#{matcher} 0
+          ^^^^^^ Use `_(foo(a))` instead.
+        end
+      RUBY
+
+      assert_correction(<<~RUBY)
+        it 'does something' do
+          _(foo(a)).#{matcher} 0
+        end
+      RUBY
+    end
+
+    define_method(:"test_registers_offense_when_using_global_#{matcher}_for_constant") do
+      assert_offense(<<~RUBY)
+        it 'does something' do
+          C.#{matcher}(:a)
+          ^ Use `_(C)` instead.
+        end
+      RUBY
+
+      assert_correction(<<~RUBY)
+        it 'does something' do
+          _(C).#{matcher}(:a)
+        end
+      RUBY
+    end
   end
 
   def test_works_with_chained_method_calls
@@ -202,5 +263,37 @@ def test_works_with_chained_method_calls
         end
       RUBY
     end
+
+    define_method(:"test_no_offense_when_using_expect_form_of_#{matcher}_with_value_method") do
+      assert_no_offenses(<<~RUBY)
+        it 'does something' do
+          value(n).#{matcher} 42
+        end
+      RUBY
+    end
+
+    define_method(:"test_no_offense_when_using_expect_form_of_#{matcher}_with_expect_method") do
+      assert_no_offenses(<<~RUBY)
+        it 'does something' do
+          expect(n).#{matcher} 42
+        end
+      RUBY
+    end
+
+    define_method(:"test_no_offense_when_using_expect_form_of_#{matcher}_with_value_method_and_block") do
+      assert_no_offenses(<<~RUBY)
+        it 'does something' do
+          value { n }.#{matcher} 42
+        end
+      RUBY
+    end
+
+    define_method(:"test_no_offense_when_using_expect_form_of_#{matcher}_with_expect_method_and_block") do
+      assert_no_offenses(<<~RUBY)
+        it 'does something' do
+          expect { n }.#{matcher} 42
+        end
+      RUBY
+    end
   end
 end