Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support *_includes assertions in RSpec/Rails/MinitestAssertions #1779

Merged
merged 1 commit into from
Jan 12, 2024
Merged

feat: support *_includes assertions in RSpec/Rails/MinitestAssertions #1779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
## Master (Unreleased)

- Add support for `assert_empty`, `assert_not_empty` and `refute_empty` to `RSpec/Rails/MinitestAssertions`. ([@ydah])
- Support correcting `assert_nil` and `refute_nil` in `RSpec/Rails/MinitestAssertions`. ([@G-Rath])
- Support correcting `assert_not_equal` and `assert_not_equal` in `RSpec/Rails/MinitestAssertions`. ([@G-Rath])
- Support correcting `*_includes` assertions in `RSpec/Rails/MinitestAssertions`. ([@G-Rath])
- Support correcting `assert_not_equal` and `assert_not_nil` in `RSpec/Rails/MinitestAssertions`. ([@G-Rath])
- Fix a false positive for `RSpec/ExpectActual` when used with rspec-rails routing matchers. ([@naveg])
- Add new `RSpec/RepeatedSubjectCall` cop. ([@drcapulet])

Expand Down
2 changes: 2 additions & 0 deletions docs/modules/ROOT/pages/cops_rspec_rails.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,13 +255,15 @@ Check if using Minitest matchers.
# bad
assert_equal(a, b)
assert_equal a, b, "must be equal"
assert_not_includes a, b
refute_equal(a, b)
assert_nil a
refute_empty(b)

# good
expect(b).to eq(a)
expect(b).to(eq(a), "must be equal")
expect(a).not_to include(b)
expect(b).not_to eq(a)
expect(a).to eq(nil)
expect(a).not_to be_empty
Expand Down
39 changes: 38 additions & 1 deletion lib/rubocop/cop/rspec/rails/minitest_assertions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ module Rails
# # bad
# assert_equal(a, b)
# assert_equal a, b, "must be equal"
# assert_not_includes a, b
# refute_equal(a, b)
# assert_nil a
# refute_empty(b)
#
# # good
# expect(b).to eq(a)
# expect(b).to(eq(a), "must be equal")
# expect(a).not_to include(b)
# expect(b).not_to eq(a)
# expect(a).to eq(nil)
# expect(a).not_to be_empty
Expand All @@ -28,11 +30,14 @@ class MinitestAssertions < Base
RESTRICT_ON_SEND = %i[
assert_equal
assert_not_equal
assert_includes
assert_not_includes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's defined by Rails: https://github.com/rails/rails/blob/main/activesupport/lib/active_support/test_case.rb#L206

assert_nil
assert_not_nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s an “a-ha” moment, since all those assert_no_* are from test-unit, not minitest.

But I’d say we don’t care much if there’s a corresponding native RSpec expectation/matcher we can suggest using instead.

test-unit assertions https://www.rubydoc.info/gems/test-unit/2.3.2/Test/Unit/Assertions
Minitest assertions https://ruby-doc.org/stdlib-3.0.0/libdoc/minitest/rdoc/Minitest/Assertions.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea though also they're defined by Rails which is actually why I was including them: https://github.com/rails/rails/blob/main/activesupport/lib/active_support/test_case.rb#L206

assert_empty
assert_not_empty
refute_equal
refute_includes
refute_nil
refute_empty
].freeze
Expand All @@ -42,6 +47,11 @@ class MinitestAssertions < Base
(send nil? {:assert_equal :assert_not_equal :refute_equal} $_ $_ $_?)
PATTERN

# @!method minitest_includes(node)
def_node_matcher :minitest_includes, <<~PATTERN
(send nil? {:assert_includes :assert_not_includes :refute_includes} $_ $_ $_?)
PATTERN

# @!method minitest_nil(node)
def_node_matcher :minitest_nil, <<~PATTERN
(send nil? {:assert_nil :assert_not_nil :refute_nil} $_ $_?)
Expand All @@ -52,12 +62,17 @@ class MinitestAssertions < Base
(send nil? {:assert_empty :assert_not_empty :refute_empty} $_ $_?)
PATTERN

def on_send(node) # rubocop:disable Metrics/MethodLength
def on_send(node) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just disabled AbcSize for now because I don't want to go too far from #1778 until it's landed, but I think a good next step for improving this could be to move the node matchers / block into the assertion classes too, so it becomes something like:

EqualAssertion.check(node)
IncludesAssertion.check(node)
NilAssertion.check(node)
EmptyAssertion.check(node)
# ...

(which you could probably take further by having an ASSERTION_CHECKERS array, and then the rest of this rule just iterates over that in a few places to pull through node matchers, restrict sends, check, etc)

minitest_equal(node) do |expected, actual, failure_message|
on_assertion(node, EqualAssertion.new(expected, actual,
failure_message.first))
end

minitest_includes(node) do |collection, expected, failure_message|
on_assertion(node, IncludesAssertion.new(collection, expected,
failure_message.first))
end

minitest_nil(node) do |actual, failure_message|
on_assertion(node, NilAssertion.new(actual,
failure_message.first))
Expand Down Expand Up @@ -99,6 +114,28 @@ def replaced(node)
end
end

# :nodoc:
class IncludesAssertion
def initialize(collection, expected, fail_message)
@collection = collection
@expected = expected
@fail_message = fail_message
end

def replaced(node)
a_source = @collection.source
b_source = @expected.source

runner = node.method?(:assert_includes) ? 'to' : 'not_to'
if @fail_message.nil?
"expect(#{a_source}).#{runner} include(#{b_source})"
else
"expect(#{a_source}).#{runner}(include(#{b_source})," \
" #{@fail_message.source})"
end
end
end

# :nodoc:
class NilAssertion
def initialize(actual, fail_message)
Expand Down
86 changes: 86 additions & 0 deletions spec/rubocop/cop/rspec/rails/minitest_assertions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,92 @@
end
end

context 'with includes assertions' do
it 'registers an offense when using `assert_includes`' do
expect_offense(<<~RUBY)
assert_includes(a, b)
^^^^^^^^^^^^^^^^^^^^^ Use `expect(a).to include(b)`.
RUBY

expect_correction(<<~RUBY)
expect(a).to include(b)
RUBY
end

it 'registers an offense when using `assert_includes` with ' \
'no parentheses' do
expect_offense(<<~RUBY)
assert_includes a, b
^^^^^^^^^^^^^^^^^^^^ Use `expect(a).to include(b)`.
RUBY

expect_correction(<<~RUBY)
expect(a).to include(b)
RUBY
end

it 'registers an offense when using `assert_includes` with ' \
'failure message' do
expect_offense(<<~RUBY)
assert_includes a, b, "must be include"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(a).to(include(b), "must be include")`.
RUBY

expect_correction(<<~RUBY)
expect(a).to(include(b), "must be include")
RUBY
end

it 'registers an offense when using `assert_includes` with ' \
'multi-line arguments' do
expect_offense(<<~RUBY)
assert_includes(a,
^^^^^^^^^^^^^^^^^^ Use `expect(a).to(include(b), "must be include")`.
b,
"must be include")
RUBY

expect_correction(<<~RUBY)
expect(a).to(include(b), "must be include")
RUBY
end

it 'registers an offense when using `assert_not_includes`' do
expect_offense(<<~RUBY)
assert_not_includes a, b
^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(a).not_to include(b)`.
RUBY

expect_correction(<<~RUBY)
expect(a).not_to include(b)
RUBY
end

it 'registers an offense when using `refute_includes`' do
expect_offense(<<~RUBY)
refute_includes a, b
^^^^^^^^^^^^^^^^^^^^ Use `expect(a).not_to include(b)`.
RUBY

expect_correction(<<~RUBY)
expect(a).not_to include(b)
RUBY
end

it 'does not register an offense when using `expect(a).to include(b)`' do
expect_no_offenses(<<~RUBY)
expect(a).to include(b)
RUBY
end

it 'does not register an offense when ' \
'using `expect(a).not_to include(b)`' do
expect_no_offenses(<<~RUBY)
expect(a).not_to include(b)
RUBY
end
end

context 'with nil assertions' do
it 'registers an offense when using `assert_nil`' do
expect_offense(<<~RUBY)
Expand Down