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

Detection of assert when assert_equal was intended #32

Closed
herwinw opened this issue Oct 16, 2019 · 2 comments · Fixed by #38
Closed

Detection of assert when assert_equal was intended #32

herwinw opened this issue Oct 16, 2019 · 2 comments · Fixed by #38

Comments

@herwinw
Copy link
Contributor

herwinw commented Oct 16, 2019

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

I was wondering why some unit tests weren't failing after code changes. Turns out they were written like this:

assert([], result.errors)

The test should have use assert_equal (or maybe assert_empty without the first argument), but since the empty array is trueish, this test always passes, regardless of the value of result.errors, which is only used as the additional (optional) message parameter.

Describe the solution you'd like

I would like Rubocop-Minitest to find this error

Additional context

I have absolutely no idea how this error could be detected by Rubocop. In case a third parameter is added, this could be used to detect errors:

assert([], result.errors, 'Expected no errors')

But this is pretty pointless, since it would result in runtime errors anyway.

If anybody has an idea on how to detect this, I could probably implement it. If it appears to be impossible, we shouldn't attempt to implement it ;)

@koic
Copy link
Member

koic commented Oct 30, 2019

This seems to be a cop when the following conditions are satisfied about how to use assert method.

  • Two arguments are specified for assert method.
  • An empty literal is used as the first argument in assert method.

The following is an example.

# bad
assert([], result.errors)

# good
assert_empty(result.errors)
assert_empty(result.errors, 'Expected no errors')

Hash literals may also be targeted to cop. Also, I think that auto-correct is unsafe because it is a correction to an incompatible method.

For example, I considered AssertEmptyLiteral cop as the name of cop, but naming is difficult :-)

tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 7, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 7, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 7, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 7, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 7, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 8, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 19, 2019
tejasbubane added a commit to tejasbubane/rubocop-minitest that referenced this issue Nov 20, 2019
@koic koic closed this as completed in #38 Nov 20, 2019
koic added a commit that referenced this issue Nov 20, 2019
[Fix #32] Add new cop assert_empty_literal
@kitop
Copy link

kitop commented Nov 28, 2019

This issue can still happen when doing something like

assert(1, User.count)
assert("OK", response.body)

The intent was to do assert_equal but it's common to do assert instead, and then tests will pass no matter what.

Would you be open to consider updating the cop to include any literal (or number and strings t least)? I'd be happy to make a PR if so.
And in that case, it might be worth considering renaming too, to something like AssertWithLiteral.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants