From 5ddc6340f153f8a9400db69e506fc77ac956321b Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 20 May 2022 12:29:04 +0900 Subject: [PATCH] [Fix #169] Add new `Minitest/EnsureCallEvenIfSkip` cop Closes #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 ``` --- ...ew_add_new_ensure_call_even_if_skip_cop.md | 1 + config/default.yml | 5 + .../cop/minitest/ensure_call_even_if_skip.rb | 95 ++++++++++++++ lib/rubocop/cop/minitest_cops.rb | 1 + .../minitest/ensure_call_even_if_skip_test.rb | 122 ++++++++++++++++++ 5 files changed, 224 insertions(+) create mode 100644 changelog/new_add_new_ensure_call_even_if_skip_cop.md create mode 100644 lib/rubocop/cop/minitest/ensure_call_even_if_skip.rb create mode 100644 test/rubocop/cop/minitest/ensure_call_even_if_skip_test.rb diff --git a/changelog/new_add_new_ensure_call_even_if_skip_cop.md b/changelog/new_add_new_ensure_call_even_if_skip_cop.md new file mode 100644 index 00000000..7eacad2e --- /dev/null +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 274b4fcc..74a3565b 100644 --- a/config/default.yml +++ b/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: '<>' + Minitest/GlobalExpectations: Description: 'This cop checks for deprecated global expectations.' StyleGuide: 'https://minitest.rubystyle.guide#global-expectations' diff --git a/lib/rubocop/cop/minitest/ensure_call_even_if_skip.rb b/lib/rubocop/cop/minitest/ensure_call_even_if_skip.rb new file mode 100644 index 00000000..49d915ec --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/minitest_cops.rb b/lib/rubocop/cop/minitest_cops.rb index 10b12991..633e34cb 100644 --- a/lib/rubocop/cop/minitest_cops.rb +++ b/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' diff --git a/test/rubocop/cop/minitest/ensure_call_even_if_skip_test.rb b/test/rubocop/cop/minitest/ensure_call_even_if_skip_test.rb new file mode 100644 index 00000000..10f56beb --- /dev/null +++ b/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