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.

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 correcting assert_nil and refute_nil to RSpec/Rails/MinitestAssertions #1773

Merged
merged 1 commit into from
Jan 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Master (Unreleased)

- Support correcting `assert_nil` and `refute_nil` to `RSpec/Rails/MinitestAssertions`. ([@G-Rath])

## 2.26.1 (2024-01-05)

- Fix an error for `RSpec/SharedExamples` when using examples without argument. ([@ydah])
Expand Down
1 change: 1 addition & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,7 @@ RSpec/Rails/MinitestAssertions:
Description: Check if using Minitest matchers.
Enabled: pending
VersionAdded: '2.17'
VersionChanged: "<<next>>"
Copy link
Member

Choose a reason for hiding this comment

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

This change should not have been made. This should only be changed when the default settings are changed. For example, when you add a configurable option or change the default for a configurable option.

Copy link
Member

Choose a reason for hiding this comment

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

fix: #1777

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, my bad. I should have caught that in review.

Thanks for discovering, and fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for what it's worth I feel like that was required by bundle exec rake, but it might have been I just messed something up or changed something else I shouldn't have locally to make it think this needed updating 🤷

I'll keep this in mind for future contributions and let you know if I do find the tasks behaving wrongly

Copy link
Member

Choose a reason for hiding this comment

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

Never mind! Thank you for your work.

Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Rails/MinitestAssertions

RSpec/Rails/NegationBeValid:
Expand Down
8 changes: 7 additions & 1 deletion docs/modules/ROOT/pages/cops_rspec_rails.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ end
| Yes
| Yes
| 2.17
| -
| <<next>>
|===

Check if using Minitest matchers.
Expand All @@ -257,10 +257,16 @@ assert_equal(a, b)
assert_equal a, b, "must be equal"
refute_equal(a, b)

assert_nil a
refute_nil a

# good
expect(b).to eq(a)
expect(b).to(eq(a), "must be equal")
expect(b).not_to eq(a)

expect(a).to eq(nil)
expect(a).not_to eq(nil)
----

=== References
Expand Down
56 changes: 46 additions & 10 deletions lib/rubocop/cop/rspec/rails/minitest_assertions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,35 +12,61 @@ module Rails
# assert_equal a, b, "must be equal"
# refute_equal(a, b)
#
# assert_nil a
# refute_nil a
#
# # good
# expect(b).to eq(a)
# expect(b).to(eq(a), "must be equal")
# expect(b).not_to eq(a)
#
# expect(a).to eq(nil)
# expect(a).not_to eq(nil)
#
class MinitestAssertions < Base
extend AutoCorrector

MSG = 'Use `%<prefer>s`.'
RESTRICT_ON_SEND = %i[assert_equal refute_equal].freeze
RESTRICT_ON_SEND = %i[
assert_equal
refute_equal
assert_nil
refute_nil
].freeze

# @!method minitest_assertion(node)
def_node_matcher :minitest_assertion, <<~PATTERN
# @!method minitest_equal_assertion(node)
def_node_matcher :minitest_equal_assertion, <<~PATTERN
(send nil? {:assert_equal :refute_equal} $_ $_ $_?)
PATTERN

# @!method minitest_nil_assertion(node)
def_node_matcher :minitest_nil_assertion, <<~PATTERN
(send nil? {:assert_nil :refute_nil} $_ $_?)
PATTERN

def on_send(node)
minitest_assertion(node) do |expected, actual, failure_message|
prefer = replacement(node, expected, actual,
failure_message.first)
add_offense(node, message: message(prefer)) do |corrector|
corrector.replace(node, prefer)
end
minitest_equal_assertion(node) do |expected, actual, fail_message|
prefer = replace_equal_assertion(node, expected, actual,
fail_message.first)
add_an_offense(node, prefer)
end

minitest_nil_assertion(node) do |actual, fail_message|
prefer = replace_nil_assertion(node, actual,
fail_message.first)
add_an_offense(node, prefer)
end
end

private

def replacement(node, expected, actual, failure_message)
def add_an_offense(node, prefer)
add_offense(node, message: message(prefer)) do |corrector|
corrector.replace(node, prefer)
end
end

def replace_equal_assertion(node, expected, actual, failure_message)
runner = node.method?(:assert_equal) ? 'to' : 'not_to'
if failure_message.nil?
"expect(#{actual.source}).#{runner} eq(#{expected.source})"
Expand All @@ -50,6 +76,16 @@ def replacement(node, expected, actual, failure_message)
end
end

def replace_nil_assertion(node, actual, failure_message)
runner = node.method?(:assert_nil) ? 'to' : 'not_to'
if failure_message.nil?
"expect(#{actual.source}).#{runner} eq(nil)"
else
"expect(#{actual.source}).#{runner}(eq(nil), " \
"#{failure_message.source})"
end
end

def message(prefer)
format(MSG, prefer: prefer)
end
Expand Down
189 changes: 131 additions & 58 deletions spec/rubocop/cop/rspec/rails/minitest_assertions_spec.rb
Original file line number Diff line number Diff line change
@@ -1,73 +1,146 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::Rails::MinitestAssertions do
it 'registers an offense when using `assert_equal`' do
expect_offense(<<~RUBY)
assert_equal(a, b)
^^^^^^^^^^^^^^^^^^ Use `expect(b).to eq(a)`.
RUBY

expect_correction(<<~RUBY)
expect(b).to eq(a)
RUBY
end
context 'with equal assertions' do
it 'registers an offense when using `assert_equal`' do
expect_offense(<<~RUBY)
assert_equal(a, b)
Copy link
Member

Choose a reason for hiding this comment

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

It was a miss in the original implementation, it’s confusing that a is “expected, and b is “actual”.
Up to you if you want to fix this along the way or leave out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I'll leave it for now since this diff is already pretty big, but happy to do a follow up addressing it

^^^^^^^^^^^^^^^^^^ Use `expect(b).to eq(a)`.
RUBY

it 'registers an offense when using `assert_equal` with no parentheses' do
expect_offense(<<~RUBY)
assert_equal a, b
^^^^^^^^^^^^^^^^^ Use `expect(b).to eq(a)`.
RUBY
expect_correction(<<~RUBY)
expect(b).to eq(a)
RUBY
end

expect_correction(<<~RUBY)
expect(b).to eq(a)
RUBY
end
it 'registers an offense when using `assert_equal` with no parentheses' do
expect_offense(<<~RUBY)
assert_equal a, b
^^^^^^^^^^^^^^^^^ Use `expect(b).to eq(a)`.
RUBY

it 'registers an offense when using `assert_equal` with failure message' do
expect_offense(<<~RUBY)
assert_equal a, b, "must be equal"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(b).to(eq(a), "must be equal")`.
RUBY
expect_correction(<<~RUBY)
expect(b).to eq(a)
RUBY
end

expect_correction(<<~RUBY)
expect(b).to(eq(a), "must be equal")
RUBY
end
it 'registers an offense when using `assert_equal` with failure message' do
expect_offense(<<~RUBY)
assert_equal a, b, "must be equal"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(b).to(eq(a), "must be equal")`.
RUBY

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

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

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

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

it 'does not register an offense when using `expect(b).to eq(a)`' do
expect_no_offenses(<<~RUBY)
expect(b).to eq(a)
RUBY
it 'registers an offense when using `refute_equal`' do
expect_offense(<<~RUBY)
refute_equal a, b
^^^^^^^^^^^^^^^^^ Use `expect(b).not_to eq(a)`.
RUBY

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

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

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

it 'does not register an offense when using `expect(b).not_to eq(a)`' do
expect_no_offenses(<<~RUBY)
expect(b).not_to eq(a)
RUBY
context 'with nil assertions' do
it 'registers an offense when using `assert_nil`' do
expect_offense(<<~RUBY)
assert_nil(a)
^^^^^^^^^^^^^ Use `expect(a).to eq(nil)`.
RUBY

expect_correction(<<~RUBY)
expect(a).to eq(nil)
RUBY
end

it 'registers an offense when using `assert_nil` with no parentheses' do
expect_offense(<<~RUBY)
assert_nil a
^^^^^^^^^^^^ Use `expect(a).to eq(nil)`.
RUBY

expect_correction(<<~RUBY)
expect(a).to eq(nil)
RUBY
end

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

expect_correction(<<~RUBY)
expect(a).to(eq(nil), "must be nil")
RUBY
end

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

expect_correction(<<~RUBY)
expect(a).to(eq(nil), "must be nil")
RUBY
end

it 'registers an offense when using `refute_nil`' do
expect_offense(<<~RUBY)
refute_nil a
^^^^^^^^^^^^ Use `expect(a).not_to eq(nil)`.
RUBY

expect_correction(<<~RUBY)
expect(a).not_to eq(nil)
RUBY
end

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

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