Merge pull request #199 from fatkodima/skip_without_comment-cop

Add new `Minitest/SkipWithoutComment` cop

changelog/new_minitest_skip_without_reason_cop.md

@@ -0,0 +1 @@
+* [#198](https://github.com/rubocop/rubocop-minitest/issues/198): Add new `Minitest/SkipWithoutReason` cop. ([@fatkodima][])

config/default.yml

@@ -239,6 +239,11 @@ Minitest/SkipEnsure:
   Enabled: pending
   VersionAdded: '0.20'
 
+Minitest/SkipWithoutReason:
+  Description: 'Checks for skipped tests missing the skipping reason.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Minitest/TestMethodName:
   Description: 'This cop enforces that test method names start with `test_` prefix.'
   Enabled: 'pending'

lib/rubocop/cop/minitest/skip_without_reason.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Minitest
+      # Checks for skipped tests missing the skipping reason.
+      #
+      # @example
+      #   # bad
+      #   skip
+      #   skip('')
+      #
+      #   # bad
+      #   if condition?
+      #     skip
+      #   else
+      #     skip
+      #   end
+      #
+      #   # good
+      #   skip("Reason why the test was skipped")
+      #
+      #   # good
+      #   skip if condition?
+      #
+      class SkipWithoutReason < Base
+        MSG = 'Add a reason explaining why the test is skipped.'
+
+        RESTRICT_ON_SEND = %i[skip].freeze
+
+        def on_send(node)
+          return if node.receiver || !blank_argument?(node)
+
+          conditional_node = conditional_parent(node)
+          return if conditional_node && !only_skip_branches?(conditional_node)
+
+          return if node.parent&.resbody_type?
+
+          add_offense(node)
+        end
+
+        private
+
+        def blank_argument?(node)
+          message = node.first_argument
+          message.nil? || (message.str_type? && message.value == '')
+        end
+
+        def conditional_parent(node)
+          return unless (parent = node.parent)
+
+          if parent.if_type? || parent.case_type?
+            parent
+          elsif parent.when_type?
+            parent.parent
+          end
+        end
+
+        def only_skip_branches?(node)
+          branches = node.branches.compact
+          branches.size > 1 && branches.all? { |branch| branch.send_type? && branch.method?(:skip) }
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/minitest_cops.rb

@@ -44,6 +44,7 @@
 require_relative 'minitest/refute_predicate'
 require_relative 'minitest/refute_respond_to'
 require_relative 'minitest/skip_ensure'
+require_relative 'minitest/skip_without_reason'
 require_relative 'minitest/test_method_name'
 require_relative 'minitest/unreachable_assertion'
 require_relative 'minitest/unspecified_exception'

test/rubocop/cop/minitest/skip_without_reason_test.rb

@@ -0,0 +1,120 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class SkipWithoutReasonTest < Minitest::Test
+  def test_registers_offense_when_skip_without_reason
+    assert_offense(<<~RUBY)
+      skip
+      ^^^^ Add a reason explaining why the test is skipped.
+    RUBY
+  end
+
+  def test_registers_offense_when_skip_with_empty_reason
+    assert_offense(<<~RUBY)
+      skip('')
+      ^^^^^^^^ Add a reason explaining why the test is skipped.
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_skip_with_string_reason
+    assert_no_offenses(<<~RUBY)
+      skip('Reason')
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_skip_with_dynamic_reason
+    assert_no_offenses(<<~RUBY)
+      skip(reason)
+    RUBY
+  end
+
+  def test_registers_offense_when_skip_within_loop
+    assert_offense(<<~RUBY)
+      skip while condition?
+      ^^^^ Add a reason explaining why the test is skipped.
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_skip_within_conditional
+    assert_no_offenses(<<~RUBY)
+      skip if condition?
+
+      case
+      when condition?
+        skip
+      else
+        do_something
+      end
+
+      case
+      when condition?
+        do_something
+      else
+        skip
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_skip_within_complex_body_of_conditional
+    assert_offense(<<~RUBY)
+      if condition?
+        do_something
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      end
+
+      case
+      when condition?
+        do_something
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      end
+
+      case
+      when condition?
+      else
+        do_something
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_conditional_branches_contains_only_skip
+    assert_offense(<<~RUBY)
+      if condition?
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      else
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      end
+
+      case
+      when condition?
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      else
+        skip
+        ^^^^ Add a reason explaining why the test is skipped.
+      end
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_skip_within_rescue
+    assert_no_offenses(<<~RUBY)
+      begin
+        do_something
+      rescue ArgumentError
+        skip
+      end
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_calling_skip_on_object
+    assert_no_offenses(<<~RUBY)
+      object.skip
+    RUBY
+  end
+end