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

Handle assertions in conditionals branches in Minitest/MultipleAssertions cop #249

Merged

Conversation

fatkodima
Copy link
Contributor

These changes are to allow to have assertions like:

def test_foo
  if ActiveRecord::VERSION >= "7.0"
    assert ActiveRecord.old_foo_method(1, 2)
  else
    assert ActiveRecord.new_foo_method(1, 2)
  end 
end

A recent example that inspired this - https://github.com/ixti/sidekiq-throttled/pull/142/files#diff-87c291ea2af263111982285d3dea3083faa62260cc6080cc5396f4bf82a37cf4R22-R27 (it is using rspec, but it does not matter)

# else
# assert bar
# end
# end
Copy link
Member

Choose a reason for hiding this comment

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

I worry about losing the symmetry between #bad and #good examples. Can you replace the added example code with a description of the cop using natural language?


assertions =
case node.type
when :if, :case
Copy link
Member

Choose a reason for hiding this comment

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

Same for case_match?

assertions_count_in_branches(node.branches)
when :rescue
assertions_count(node.body) + assertions_count_in_branches(node.branches)
when :block
Copy link
Member

Choose a reason for hiding this comment

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

Same for numblock?

else
assert foo
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a source code comment to the line which line is being counted?

else
assert foo
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@fatkodima fatkodima force-pushed the multiple_assertions-handle-conditionals branch from 774da8b to ab4332c Compare April 17, 2023 11:23
@fatkodima
Copy link
Contributor Author

Applied suggested changes. CI is failing, because rubocop now requires ruby 2.7, but this gem - ruby 2.6. So it is needed to first drop support for ruby 2.6 and then this PR can be updated.

Should I open a separate PR with that change or should you?

@koic
Copy link
Member

koic commented Apr 18, 2023

Should I open a separate PR with that change or should you?

The build error has been fixed by #250. Either way, I understand that the issue is unrelated to this PR change. Thank you :-)

@fatkodima fatkodima force-pushed the multiple_assertions-handle-conditionals branch from ab4332c to f743944 Compare April 18, 2023 09:19
@fatkodima
Copy link
Contributor Author

Updated, now CI is green 🟢

@koic koic merged commit f7c71ed into rubocop:master Apr 22, 2023
@fatkodima fatkodima deleted the multiple_assertions-handle-conditionals branch April 22, 2023 08:43
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 this pull request may close these issues.

2 participants