Skip to content

Commit

Permalink
Add {Assert,Refute}Match OnlyRegexpLiteral config
Browse files Browse the repository at this point in the history
`AssertMatch` & `RefuteMatch` enforce that

    assert matcher.match?(object)
    refute matcher.match?(object)

be replaced with

    assert_match(matcher, object)
    refute_match(matcher, object)

respectively, which use `=~` under the hood. This makes the cops highly
succeptible to false positives, in cases where the matcher object
responds to `match?`, but not to `=~`

   class SuitColorMatcher
     BLACK = [:clubs,    :spades]
     RED   = [:diamonds, :hearts]
     SUITS = BLACK + RED

     def initialize(suit)
       raise ArgumentError, suit.inspect unless SUITS.include?(suit)
       @suit = suit
     end

     def match?(other)
       case @suit
       when *BLACK then BLACK.include?(other)
       when *RED   then RED  .include?(other)
       end
     end
   end

   SuitColorMatcher.new(:clubs).match?(:spades) # => true
   SuitColorMatcher.new(:clubs) =~ :spades      # NoMethodError

or cases where the matcher object does also respond to `=~`, but in an
incompatible way

    "".match? "" # => true
    "" =~ ""     # TypeError

Given RuboCop doesn't have runtime type information, the only case we
can reliably identify is the case where a `Regexp` literal is used.

Therefore, this adds an `OnlyRegexpLiteral` config (false by default),
which consumers can enable if their codebase has many false positives.
  • Loading branch information
sambostock committed Oct 4, 2024
1 parent f2fe8bd commit 909b1e9
Show file tree
Hide file tree
Showing 7 changed files with 197 additions and 2 deletions.
1 change: 1 addition & 0 deletions changelog/new_add_assert_refute_match.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#323](https://github.com/rubocop/rubocop-minitest/pull/323): Add `{Assert,Refute}Match` `OnlyRegexpLiteral` config. ([@sambostock][])
2 changes: 2 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ Minitest/AssertMatch:
StyleGuide: 'https://minitest.rubystyle.guide#assert-match'
Enabled: true
VersionAdded: '0.6'
OnlyRegexpLiteral: false

Minitest/AssertNil:
Description: 'This cop enforces the test to use `assert_nil` instead of using `assert_equal(nil, something)` or `assert(something.nil?)`.'
Expand Down Expand Up @@ -266,6 +267,7 @@ Minitest/RefuteMatch:
StyleGuide: 'https://minitest.rubystyle.guide#refute-match'
Enabled: true
VersionAdded: '0.6'
OnlyRegexpLiteral: false

Minitest/RefuteNil:
Description: 'This cop enforces the test to use `refute_nil` instead of using `refute_equal(nil, something)` or `refute(something.nil?)`.'
Expand Down
19 changes: 19 additions & 0 deletions lib/rubocop/cop/minitest/assert_match.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,25 @@ module Minitest
# assert_match(regex, string)
# assert_match(matcher, string, 'message')
#
# @example OnlyRegexpLiteral: false (default)
# # bad
# assert /.../.match?(object)
# assert object.match?(/.../)
# assert matcher.match?(object)
#
# # good
# assert_match(/.../, object)
# assert_match(matcher, object)
#
# @example OnlyRegexpLiteral: true
# # bad
# assert /.../.match?(object)
# assert object.match?(/.../)
#
# # good
# assert_match(/.../, object)
# assert_match(matcher, object)
#
class AssertMatch < Base
include ArgumentRangeHelper
include AssertRefuteMatchHelper
Expand Down
18 changes: 18 additions & 0 deletions lib/rubocop/cop/minitest/refute_match.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,24 @@ module Minitest
# refute_match(matcher, string)
# refute_match(matcher, string, 'message')
#
# @example OnlyRegexpLiteral: false (default)
# # bad
# refute /.../.match?(object)
# refute object.match?(/.../)
# refute matcher.match?(object)
#
# # good
# refute_match(/.../, object)
# refute_match(matcher, object)
#
# @example OnlyRegexpLiteral: true
# # bad
# refute /.../.match?(object)
# refute object.match?(/.../)
#
# # good
# refute_match(/.../, object)
# refute_match(matcher, object)
class RefuteMatch < Base
include ArgumentRangeHelper
include AssertRefuteMatchHelper
Expand Down
15 changes: 13 additions & 2 deletions lib/rubocop/cop/mixin/assert_refute_match_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ module AssertRefuteMatchHelper

def check_match_assertion(node)
match_assertion(node) do |expected, actual, rest_args|
basic_arguments = order_expected_and_actual(expected, actual)
basic_argument_nodes = order_expected_and_actual(expected, actual)
next if allowed_matcher?(basic_argument_nodes.first)

basic_arguments = basic_argument_nodes.map(&:source).join(', ')

add_offense(node, message: format_message(basic_arguments, rest_args)) do |corrector|
corrector.replace(node.loc.selector, preferred_assertion_method_name)
Expand All @@ -17,6 +20,14 @@ def check_match_assertion(node)
end
end

def allowed_matcher?(matcher_node)
if cop_config['OnlyRegexpLiteral']
!matcher_node.regexp_type?
else
false
end
end

def format_message(basic_arguments, rest_args)
preferred = (message_arg = rest_args.first) ? "#{basic_arguments}, #{message_arg.source}" : basic_arguments

Expand All @@ -36,7 +47,7 @@ def order_expected_and_actual(expected, actual)
[actual, expected]
else
[expected, actual]
end.map(&:source).join(', ')
end
end
end
end
Expand Down
72 changes: 72 additions & 0 deletions test/rubocop/cop/minitest/assert_match_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,76 @@ def test_do_something
end
RUBY
end

def test_does_not_register_offense_when_neither_receiver_nor_first_argument_is_a_regexp_and_only_regexp_is_enabled
enable_only_regexp

assert_no_offenses(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert matcher.match?(object)
end
end
RUBY
end

def test_does_not_register_offense_when_receiver_and_arguments_are_string_literals_and_only_regexp_is_enabled
enable_only_regexp

assert_no_offenses(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert "...".match?("...")
end
end
RUBY
end

def test_does_register_offense_when_receiver_is_a_regexp_literal_and_only_regexp_is_enabled
enable_only_regexp

assert_offense(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert /.../.match?(object)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_match(/.../, object)`.
end
end
RUBY

assert_correction(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert_match /.../, object
end
end
RUBY
end

def test_does_register_offense_when_first_argument_is_a_regexp_literal_and_only_regexp_is_enabled
enable_only_regexp

assert_offense(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert object.match?(/.../)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `assert_match(/.../, object)`.
end
end
RUBY

assert_correction(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
assert_match /.../, object
end
end
RUBY
end

private

def enable_only_regexp
@configuration = RuboCop::Config.new({ 'Minitest/AssertMatch' => { 'OnlyRegexpLiteral' => true } })
end
end
72 changes: 72 additions & 0 deletions test/rubocop/cop/minitest/refute_match_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,4 +204,76 @@ def test_do_something
end
RUBY
end

def test_does_not_register_offense_when_neither_receiver_nor_first_argument_is_a_regexp_and_only_regexp_is_enabled
enable_only_regexp

assert_no_offenses(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
refute matcher.match?(object)
end
end
RUBY
end

def test_does_not_register_offense_when_receiver_and_arguments_are_string_literals_and_only_regexp_is_enabled
enable_only_regexp

assert_no_offenses(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
refute "...".match?("...")
end
end
RUBY
end

def test_does_register_offense_when_receiver_is_a_regexp_literal_and_only_regexp_is_enabled
enable_only_regexp

assert_offense(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
refute /.../.match?(object)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `refute_match(/.../, object)`.
end
end
RUBY

assert_correction(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
refute_match /.../, object
end
end
RUBY
end

def test_does_register_offense_when_first_argument_is_a_regexp_literal_and_only_regexp_is_enabled
enable_only_regexp

assert_offense(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
refute object.match?(/.../)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `refute_match(/.../, object)`.
end
end
RUBY

assert_correction(<<~RUBY)
class FooTest < Minitest::Test
def test_do_something
refute_match /.../, object
end
end
RUBY
end

private

def enable_only_regexp
@configuration = RuboCop::Config.new({ 'Minitest/RefuteMatch' => { 'OnlyRegexpLiteral' => true } })
end
end

0 comments on commit 909b1e9

Please sign in to comment.