From 09dde9057f77d59b7d0a2a1be46ac464b850265c Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sat, 25 Nov 2023 16:10:56 +0200 Subject: [PATCH] Enhance `AssertSame`/`RefuteSame` to check for `object_id` comparison --- .rubocop.yml | 5 ++ .rubocop_todo.yml | 5 -- .../change_assert_same_check_object_ids.md | 1 + lib/rubocop/cop/minitest/assert_same.rb | 5 +- lib/rubocop/cop/minitest/refute_same.rb | 5 +- lib/rubocop/cop/minitest_cops.rb | 1 + .../cop/mixin/assert_refute_same_helper.rb | 57 +++++++++++++++++++ test/rubocop/cop/minitest/assert_same_test.rb | 19 +++++++ test/rubocop/cop/minitest/refute_same_test.rb | 19 +++++++ 9 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 changelog/change_assert_same_check_object_ids.md create mode 100644 lib/rubocop/cop/mixin/assert_refute_same_helper.rb diff --git a/.rubocop.yml b/.rubocop.yml index 3df1d8f7..c35677a8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -56,6 +56,11 @@ Style/FormatStringToken: Exclude: - test/**/* +Style/DocumentDynamicEvalDefinition: + Exclude: + - 'lib/rubocop/cop/mixin/minitest_cop_rule.rb' + - 'lib/rubocop/cop/mixin/assert_refute_same_helper.rb' + Metrics/ClassLength: Exclude: - test/**/* diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 21eaabf8..254fad64 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -12,8 +12,3 @@ Metrics/MethodLength: Max: 14 Exclude: - 'test/rubocop/cop/minitest/global_expectations_test.rb' - -# Offense count: 1 -Style/DocumentDynamicEvalDefinition: - Exclude: - - 'lib/rubocop/cop/mixin/minitest_cop_rule.rb' diff --git a/changelog/change_assert_same_check_object_ids.md b/changelog/change_assert_same_check_object_ids.md new file mode 100644 index 00000000..1af94ca3 --- /dev/null +++ b/changelog/change_assert_same_check_object_ids.md @@ -0,0 +1 @@ +* [#276](https://github.com/rubocop/rubocop-minitest/pull/276): Enhance `AssertSame`/`RefuteSame` to check for `object_id` comparison. ([@fatkodima][]) diff --git a/lib/rubocop/cop/minitest/assert_same.rb b/lib/rubocop/cop/minitest/assert_same.rb index da712928..064e404e 100644 --- a/lib/rubocop/cop/minitest/assert_same.rb +++ b/lib/rubocop/cop/minitest/assert_same.rb @@ -12,14 +12,15 @@ module Minitest # @example # # bad # assert(expected.equal?(actual)) + # assert_equal(expected.object_id, actual.object_id) # # # good # assert_same(expected, actual) # class AssertSame < Base - extend MinitestCopRule + extend AssertRefuteSameHelper - define_rule :assert, target_method: :equal?, preferred_method: :assert_same + define_rule :assert end end end diff --git a/lib/rubocop/cop/minitest/refute_same.rb b/lib/rubocop/cop/minitest/refute_same.rb index b7f6b41c..b07ffae0 100644 --- a/lib/rubocop/cop/minitest/refute_same.rb +++ b/lib/rubocop/cop/minitest/refute_same.rb @@ -12,14 +12,15 @@ module Minitest # @example # # bad # refute(expected.equal?(actual)) + # refute_equal(expected.object_id, actual.object_id) # # # good # refute_same(expected, actual) # class RefuteSame < Base - extend MinitestCopRule + extend AssertRefuteSameHelper - define_rule :refute, target_method: :equal?, preferred_method: :refute_same + define_rule :refute end end end diff --git a/lib/rubocop/cop/minitest_cops.rb b/lib/rubocop/cop/minitest_cops.rb index 58675e7a..1674b1ac 100644 --- a/lib/rubocop/cop/minitest_cops.rb +++ b/lib/rubocop/cop/minitest_cops.rb @@ -7,6 +7,7 @@ require_relative 'mixin/minitest_exploration_helpers' require_relative 'mixin/nil_assertion_handleable' require_relative 'mixin/predicate_assertion_handleable' +require_relative 'mixin/assert_refute_same_helper' require_relative 'minitest/assert_empty' require_relative 'minitest/assert_empty_literal' require_relative 'minitest/assert_equal' diff --git a/lib/rubocop/cop/mixin/assert_refute_same_helper.rb b/lib/rubocop/cop/mixin/assert_refute_same_helper.rb new file mode 100644 index 00000000..d5b41f37 --- /dev/null +++ b/lib/rubocop/cop/mixin/assert_refute_same_helper.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Provide a method to define offense rule for Minitest cops. + module AssertRefuteSameHelper + def define_rule(assertion_method) + class_eval(<<~RUBY, __FILE__, __LINE__ + 1) + extend AutoCorrector + + MSG = 'Prefer using `#{assertion_method}_same(%s)`.' + RESTRICT_ON_SEND = %i[#{assertion_method} #{assertion_method}_equal] + + def_node_matcher :assertion_with_equal?, <<~PATTERN + (send nil? :#{assertion_method} + $(send $_ :equal? $_) + $_?) + PATTERN + + def_node_matcher :equality_with_object_id?, <<~PATTERN + (send nil? :#{assertion_method}_equal + (send $_ :object_id) + (send $_ :object_id) + $_?) + PATTERN + + def on_send(node) + if (equal_node, expected_node, actual_node, message_node = assertion_with_equal?(node)) + add_offense(node, message: message(expected_node, actual_node, message_node.first)) do |corrector| + corrector.replace(node.loc.selector, "#{assertion_method}_same") + corrector.replace(equal_node, "\#{expected_node.source}, \#{actual_node.source}") + end + elsif (expected_node, actual_node, message_node = equality_with_object_id?(node)) + add_offense(node, message: message(expected_node, actual_node, message_node.first)) do |corrector| + corrector.replace(node.loc.selector, "#{assertion_method}_same") + remove_method_call(expected_node.parent, corrector) + remove_method_call(actual_node.parent, corrector) + end + end + end + + private + + def message(expected_node, actual_node, message_node) + arguments = [expected_node, actual_node, message_node].compact.map(&:source).join(", ") + format(MSG, new_arguments: arguments) + end + + def remove_method_call(send_node, corrector) + range = send_node.loc.dot.join(send_node.loc.selector) + corrector.remove(range) + end + RUBY + end + end + end +end diff --git a/test/rubocop/cop/minitest/assert_same_test.rb b/test/rubocop/cop/minitest/assert_same_test.rb index a011ee0b..3badd569 100644 --- a/test/rubocop/cop/minitest/assert_same_test.rb +++ b/test/rubocop/cop/minitest/assert_same_test.rb @@ -85,6 +85,25 @@ def test_do_something RUBY end + def test_registers_offense_when_using_assert_equal_with_object_id + assert_offense(<<~RUBY) + class FooTest < Minitest::Test + def test_do_something + assert_equal(expected.object_id, actual.object_id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_same(expected, actual)`. + end + end + RUBY + + assert_correction(<<~RUBY) + class FooTest < Minitest::Test + def test_do_something + assert_same(expected, actual) + end + end + RUBY + end + def test_does_not_register_offense_when_using_assert_same assert_no_offenses(<<~RUBY) class FooTest < Minitest::Test diff --git a/test/rubocop/cop/minitest/refute_same_test.rb b/test/rubocop/cop/minitest/refute_same_test.rb index 72573791..326b31f5 100644 --- a/test/rubocop/cop/minitest/refute_same_test.rb +++ b/test/rubocop/cop/minitest/refute_same_test.rb @@ -85,6 +85,25 @@ def test_do_something RUBY end + def test_registers_offense_when_using_refute_equal_with_object_id + assert_offense(<<~RUBY) + class FooTest < Minitest::Test + def test_do_something + refute_equal(expected.object_id, actual.object_id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `refute_same(expected, actual)`. + end + end + RUBY + + assert_correction(<<~RUBY) + class FooTest < Minitest::Test + def test_do_something + refute_same(expected, actual) + end + end + RUBY + end + def test_does_not_register_offense_when_using_assert_same assert_no_offenses(<<~RUBY) class FooTest < Minitest::Test