rubocop / rails-style-guide

A community-driven Ruby on Rails style guide
http://rails.rubystyle.guide
6.48k stars 1.06k forks source link

Cop idea: Prefer `assert_raises` over `assert_raise` #351

Open Earlopain opened 1 year ago

Earlopain commented 1 year ago

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

Minitest by itself defines assert_raises and rails defined the alias assert_raise for it.

Describe the solution you'd like

Prefer the native assert_raises over assert_raise.

Additional context

https://api.rubyonrails.org/classes/ActiveSupport/Testing/Assertions.html#method-i-assert_raise

koic commented 1 year ago

I'm not sure whether assert_raises or assert_raise is preferable as the default. Please discuss it in the style guide first.

andyw8 commented 1 year ago

assert_raise is an alias for Rails' own assert_raises, which adds support for a match parameter, so it's not Minitest's assert_raises:

https://github.com/rails/rails/blob/23938052acd773fa24068debe56cd892cbf8d868/activesupport/lib/active_support/testing/assertions.rb#L34-L39

But I would support a guideline to prefer assert_raises over assert_raise for consistency.

koic commented 1 year ago

https://github.com/rubocop/rubocop-rails/issues/913 might be added for background context.

Earlopain commented 1 year ago

Rails itself doesn't document assert_raise in their testing guides https://guides.rubyonrails.org/testing.html The alias itself was added 12 years ago and marked as for backwards compatibility reasons https://github.com/rails/rails/commit/aa7857b61742d2785a1bec39269d25031c840757, apparently from before rails was using minitest.

koic commented 1 year ago

For example, in the rails/rails repository, both assert_raise and assert_raises are used. If the Rails development team has a consistent preference for assert_raises, I feel it should be respected. e.g.

Use assert_not methods instead of refute.

https://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#follow-the-coding-conventions

But, I'm not sure what the actual situation is.

Earlopain commented 1 year ago

Well, rails doesn't accept cosmetic changes. If someone where to come along wanting to change it the pr would just be rejected.

Is there something which collects rails applications like https://github.com/eliotsykes/real-world-rails? I have seen this repo come up in the past but it hasn't been maintained for the last few years.

koic commented 1 year ago

The Ruby API itself is primarily named using the base form of verbs. For instance, in Ruby 3.2, File.exists was removed, leaving only File.exist. Given this, some might prefer assert_raise over assert_raises.

Earlopain commented 1 year ago

That does indeed seem to be the prefered way for the ruby project itself. https://github.com/ruby/ruby/commit/068f312a7c642a1b6c358c17ef83421756568545 https://github.com/ruby/ruby/commit/f159bbd17da88a7f456e0ec7fbf7890135d456a3

A code search on github for assert_raise and assert_raises also reveals that assert_raise has more usages than I would have expected: assert_raises: 14k + 10.4k assert_raise: 7.9k + 5.2k

Per these numbers assert_raises is used about twice as often as assert_raise. But then again these searches also include projects that don't use Rails. I don't think you're able to query for rails specifically, or get the numbers of actual matches instead of the amount of times the query appears in a file.

Personally I would still go with assert_raises since that is the method defined by minitest. But it looks like if this cop would be implemented there needs to be a config option so you're able to go with the variant you prefer.

koic commented 1 year ago

When implementing a cop, default is important for users. Respecting Ruby's API naming and choosing assert_raise seems better, but the best choice remains uncertain.

Earlopain commented 1 year ago

When implementing cops, is there precedence for disabled by default and a required config key? Early adopters could opt in and must set their prefered style explicitly.

It's not obvious which variant is better but I'd still like to try and work on this since it'll eventually go one way or another. If/When a conclusion is reached the config can be changed.