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

Cop Feature Request: Catch assert expression.nil? and suggest assert_nil instead #140

Closed
Ryan1729 opened this issue Aug 5, 2021 · 4 comments · Fixed by #141
Closed

Cop Feature Request: Catch assert expression.nil? and suggest assert_nil instead #140

Ryan1729 opened this issue Aug 5, 2021 · 4 comments · Fixed by #141
Labels
enhancement New feature or request

Comments

@Ryan1729
Copy link

Ryan1729 commented Aug 5, 2021

Is your feature request related to a problem? Please describe.

The AssertNil and RefuteNil cops do catch cases like the following, and suggests assert_nil instead:

assert_equal nil, expression

But they don't catch cases that use .nil? like this:

assert expression.nil?

The minitest style guide currently even uses a .nil? example for Refute Nil.

Describe the solution you'd like

It would be nice if the AssertNil and RefuteNil cops could catch these .nil? cases too.

Describe alternatives you've considered

It's not hard to write a regex for this myself, but having this be part of rubocop-minitest would be more convenient.

Additional context

N/A

@abhaynikam
Copy link
Contributor

abhaynikam commented Aug 5, 2021

IMO, it is not a bad practice to use assert expression.nil? or refute expression.nil?. I think the documentation should be updated.

The cop was introduced because assert_nil and refute_nil is deprecated in minitest. 😊

@andyw8
Copy link
Contributor

andyw8 commented Aug 5, 2021

Do you know when they were deprecated? I can't see a mention in https://github.com/seattlerb/minitest/blob/master/History.rdoc

@abhaynikam
Copy link
Contributor

The cop was introduced because assert_nil and refute_nil is deprecated in minitest.

Sorry. My bad. I meant assert_equal nil, expression was deprecated. Here is the deprecation warning: https://github.com/seattlerb/minitest/blob/master/lib/minitest/assertions.rb#L227 😊

@Ryan1729
Copy link
Author

Ryan1729 commented Aug 6, 2021

I think a good reason to prefer assert_nil expression over assert expression.nil? is because with assert_nil you get more informative error messages.

For example, this assert:

assert 1.nil?

results in this error message:

Minitest::Assertion: Expected false to be truthy.

But if you use assert_nil instead like this:

assert_nil 1

you get this more informative error message:

Minitest::Assertion: Expected 1 to be nil.

Yes, you can go look at the test to figure out what happened, but it's slightly nicer to see the expression that caused the issue right in the error message.

@koic koic added the enhancement New feature or request label Aug 6, 2021
koic added a commit to koic/rubocop-minitest that referenced this issue Aug 6, 2021
…bj.nil?)`

Fixes rubocop#140.

This PR makes `Minitest/AssertNil` and `Minitest/RefuteNil` aware of
`assert(obj.nil?)` and `refute(obj.nil?)`.
@koic koic closed this as completed in #141 Aug 7, 2021
koic added a commit that referenced this issue Aug 7, 2021
…e_of_assert_method

[Fix #140] Make `AssertNil` and `RefuteNil` aware of `assert(obj.nil?)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants