[Fix rubocop#169] Add new Minitest/EnsureCallEvenIfSkip cop

Closes rubocop#169.

This cop checks that `ensure` call even if `skip`. It is unexpected
that `ensure` will be called when skipping test.
If conditional `skip` is used, it checks that `ensure` is also called
conditionally.

On the other hand, it accepts `skip` used in `rescue` because `ensure`
may be teardown process to `begin` setup process.

```ruby
# bad
def test_skip
  skip 'This test is skipped.'

  assert 'foo'.present?
ensure
  do_something
end

# bad
def test_conditional_skip
  skip 'This test is skipped.' if condition

  assert do_something
ensure
  do_teardown
end

# good
def test_skip
  skip 'This test is skipped.'

  begin
    assert 'foo'.present?
  ensure
    do_something
  end
end

# good
def test_conditional_skip
  skip 'This test is skipped.' if condition

  assert do_something
ensure
  if condition
    do_teardown
  end
end

# good
def test_skip_is_used_in_rescue
  do_setup
  assert do_something
rescue
  skip 'This test is skipped.'
ensure
  do_teardown
end
```

changelog/new_add_new_ensure_call_even_if_skip_cop.md

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

config/default.yml

@@ -111,6 +111,11 @@ Minitest/DuplicateTestRun:
   Enabled: pending
   VersionAdded: '0.19'
 
+Minitest/EnsureCallEvenIfSkip:
+  Description: 'Checks that `ensure` call even if `skip`.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Minitest/GlobalExpectations:
   Description: 'This cop checks for deprecated global expectations.'
   StyleGuide: 'https://minitest.rubystyle.guide#global-expectations'

lib/rubocop/cop/minitest/ensure_call_even_if_skip.rb

@@ -0,0 +1,95 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Minitest
+      # Checks that `ensure` call even if `skip`. It is unexpected that `ensure` will be called when skipping test.
+      # If conditional `skip` is used, it checks that `ensure` is also called conditionally.
+      #
+      # On the other hand, it accepts `skip` used in `rescue` because `ensure` may be teardown process to `begin`
+      # setup process.
+      #
+      # @example
+      #
+      #   # bad
+      #   def test_skip
+      #     skip 'This test is skipped.'
+      #
+      #     assert 'foo'.present?
+      #   ensure
+      #     do_something
+      #   end
+      #
+      #   # bad
+      #   def test_conditional_skip
+      #     skip 'This test is skipped.' if condition
+      #
+      #     assert do_something
+      #   ensure
+      #     do_teardown
+      #   end
+      #
+      #   # good
+      #   def test_skip
+      #     skip 'This test is skipped.'
+      #
+      #     begin
+      #       assert 'foo'.present?
+      #     ensure
+      #       do_something
+      #     end
+      #   end
+      #
+      #   # good
+      #   def test_conditional_skip
+      #     skip 'This test is skipped.' if condition
+      #
+      #     assert do_something
+      #   ensure
+      #     if condition
+      #       do_teardown
+      #     end
+      #   end
+      #
+      #   # good
+      #   def test_skip_is_used_in_rescue
+      #     do_setup
+      #     assert do_something
+      #   rescue
+      #     skip 'This test is skipped.'
+      #   ensure
+      #     do_teardown
+      #   end
+      #
+      class EnsureCallEvenIfSkip < Base
+        MSG = '`ensure` is called even though the test is skipped.'
+
+        def on_ensure(node)
+          skip = find_skip(node)
+
+          return if skip.nil? || use_skip_method_in_rescue?(skip) || valid_conditional_skip?(skip, node)
+
+          add_offense(node.loc.keyword)
+        end
+
+        private
+
+        def find_skip(node)
+          node.node_parts.first.descendants.detect { |n| n.send_type? && n.receiver.nil? && n.method?(:skip) }
+        end
+
+        def use_skip_method_in_rescue?(skip_method)
+          skip_method.ancestors.detect(&:rescue_type?)
+        end
+
+        def valid_conditional_skip?(skip_method, ensure_node)
+          if_node = skip_method.ancestors.detect(&:if_type?)
+          return false unless ensure_node.body.if_type?
+
+          match_keyword = ensure_node.body.if? ? if_node.if? : if_node&.unless?
+          match_keyword && ensure_node.body.condition == if_node.condition
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/minitest_cops.rb

@@ -24,6 +24,7 @@
 require_relative 'minitest/assert_silent'
 require_relative 'minitest/assert_truthy'
 require_relative 'minitest/duplicate_test_run'
+require_relative 'minitest/ensure_call_even_if_skip'
 require_relative 'minitest/global_expectations'
 require_relative 'minitest/literal_as_actual_argument'
 require_relative 'minitest/multiple_assertions'

test/rubocop/cop/minitest/ensure_call_even_if_skip_test.rb

@@ -0,0 +1,122 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class EnsureCallEvenIfSkipTest < Minitest::Test
+  def test_registers_offense_when_using_skip_in_the_method_body
+    assert_offense(<<~RUBY)
+      def test_skip
+        skip 'This test is skipped.'
+
+        assert 'foo'.present?
+      ensure
+      ^^^^^^ `ensure` is called even though the test is skipped.
+        do_something
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_not_using_skip_in_the_method_body
+    assert_no_offenses(<<~RUBY)
+      def test_skip
+        skip 'This test is skipped.'
+
+        begin
+          assert 'foo'.present?
+        ensure
+          do_something
+        end
+      end
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_not_using_ensure
+    assert_no_offenses(<<~RUBY)
+      def test_skip
+        skip 'This test is skipped.'
+
+        assert 'foo'.present?
+
+        do_something
+      end
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_not_using_skip
+    assert_no_offenses(<<~RUBY)
+      def test_skip
+        assert 'foo'.present?
+      ensure
+        do_something
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_using_skip_with_condition_but_the_condition_is_not_used_in_ensure
+    assert_offense(<<~RUBY)
+      def test_conditional_skip
+        skip 'This test is skipped.' if condition
+
+        assert do_something
+      ensure
+      ^^^^^^ `ensure` is called even though the test is skipped.
+        do_teardown
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_using_skip_with_condition_and_the_condition_is_used_in_ensure
+    assert_no_offenses(<<~RUBY)
+      def test_conditional_skip
+        skip 'This test is skipped.' if condition
+
+        assert do_something
+      ensure
+        if condition
+          do_teardown
+        end
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_using_skip_with_condition_and_the_condition_is_used_in_ensure_but_keyword_is_mismatch
+    assert_offense(<<~RUBY)
+      def test_conditional_skip
+        skip 'This test is skipped.' if condition
+
+        assert do_something
+      ensure
+      ^^^^^^ `ensure` is called even though the test is skipped.
+        unless condition
+          do_teardown
+        end
+      end
+    RUBY
+  end
+
+  def test_does_not_register_offense_when_using_skip_in_rescue
+    assert_no_offenses(<<~RUBY)
+      def test_skip_is_used_in_rescue
+        do_setup
+
+        assert do_something
+      rescue
+        skip 'This test is skipped.'
+      ensure
+        do_teardown
+      end
+    RUBY
+  end
+
+  def test_registers_offense_when_using_
+    assert_no_offenses(<<~RUBY)
+      def test_skip_with_receiver
+        obj.skip 'This test is skipped.'
+
+        assert 'foo'.present?
+      ensure
+        do_something
+      end
+    RUBY
+  end
+end