From c3094ad38fbfdb8b0e2c66d5fae4597688ada291 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 7 Aug 2021 07:49:47 +0900 Subject: [PATCH] [Fix #140] Make `AssertNil` and `RefuteNil` aware of `assert(obj.nil?)` Fixes #140. This PR makes `Minitest/AssertNil` and `Minitest/RefuteNil` aware of `assert(obj.nil?)` and `refute(obj.nil?)`. --- CHANGELOG.md | 4 ++ config/default.yml | 4 +- docs/modules/ROOT/pages/cops_minitest.adoc | 12 ++-- lib/rubocop/cop/minitest/assert_nil.rb | 34 +++++++----- lib/rubocop/cop/minitest/refute_nil.rb | 36 ++++++------ lib/rubocop/cop/minitest_cops.rb | 1 + .../cop/mixin/nil_assertion_handleable.rb | 55 +++++++++++++++++++ test/rubocop/cop/minitest/assert_nil_test.rb | 38 +++++++++++++ test/rubocop/cop/minitest/refute_nil_test.rb | 38 +++++++++++++ 9 files changed, 184 insertions(+), 38 deletions(-) create mode 100644 lib/rubocop/cop/mixin/nil_assertion_handleable.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index e29d0583..6af41e95 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/config/default.yml b/config/default.yml index 3b308159..1c098d55 100644 --- a/config/default.yml +++ b/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' diff --git a/docs/modules/ROOT/pages/cops_minitest.adoc b/docs/modules/ROOT/pages/cops_minitest.adoc index a83bb863..2e574147 100644 --- a/docs/modules/ROOT/pages/cops_minitest.adoc +++ b/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) diff --git a/lib/rubocop/cop/minitest/assert_nil.rb b/lib/rubocop/cop/minitest/assert_nil.rb index 88e58470..fcf6a061 100644 --- a/lib/rubocop/cop/minitest/assert_nil.rb +++ b/lib/rubocop/cop/minitest/assert_nil.rb @@ -3,13 +3,15 @@ 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) @@ -17,27 +19,29 @@ module Minitest # class AssertNil < Base include ArgumentRangeHelper + include NilAssertionHandleable extend AutoCorrector - MSG = 'Prefer using `assert_nil(%s)` over ' \ - '`assert_equal(nil, %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 diff --git a/lib/rubocop/cop/minitest/refute_nil.rb b/lib/rubocop/cop/minitest/refute_nil.rb index d33f6bf8..a9fa198c 100644 --- a/lib/rubocop/cop/minitest/refute_nil.rb +++ b/lib/rubocop/cop/minitest/refute_nil.rb @@ -3,13 +3,15 @@ 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) @@ -17,29 +19,29 @@ module Minitest # class RefuteNil < Base include ArgumentRangeHelper + include NilAssertionHandleable extend AutoCorrector - MSG = 'Prefer using `refute_nil(%s)` over ' \ - '`refute_equal(nil, %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 diff --git a/lib/rubocop/cop/minitest_cops.rb b/lib/rubocop/cop/minitest_cops.rb index d9ab21e6..bcaba86b 100644 --- a/lib/rubocop/cop/minitest_cops.rb +++ b/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' diff --git a/lib/rubocop/cop/mixin/nil_assertion_handleable.rb b/lib/rubocop/cop/mixin/nil_assertion_handleable.rb new file mode 100644 index 00000000..3ae57d4c --- /dev/null +++ b/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 `%s_nil(%s)` over `%s(%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 diff --git a/test/rubocop/cop/minitest/assert_nil_test.rb b/test/rubocop/cop/minitest/assert_nil_test.rb index d48a0371..146b0d71 100644 --- a/test/rubocop/cop/minitest/assert_nil_test.rb +++ b/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 diff --git a/test/rubocop/cop/minitest/refute_nil_test.rb b/test/rubocop/cop/minitest/refute_nil_test.rb index e29534a4..afa81076 100644 --- a/test/rubocop/cop/minitest/refute_nil_test.rb +++ b/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