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 idea: Prefer assert_raises over assert_raise #351

Open
Earlopain opened this issue Oct 20, 2023 · 10 comments
Open

Cop idea: Prefer assert_raises over assert_raise #351

Earlopain opened this issue Oct 20, 2023 · 10 comments

Comments

@Earlopain
Copy link

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
Copy link
Member

koic commented Oct 20, 2023

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

@koic koic transferred this issue from rubocop/rubocop-rails Oct 20, 2023
@andyw8
Copy link
Contributor

andyw8 commented Oct 20, 2023

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
Copy link
Member

koic commented Oct 22, 2023

rubocop/rubocop-rails#913 might be added for background context.

@Earlopain
Copy link
Author

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 rails/rails@aa7857b, apparently from before rails was using minitest.

@koic
Copy link
Member

koic commented Oct 23, 2023

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
Copy link
Author

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
Copy link
Member

koic commented Oct 23, 2023

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
Copy link
Author

Earlopain commented Oct 23, 2023

That does indeed seem to be the prefered way for the ruby project itself. ruby/ruby@068f312 ruby/ruby@f159bbd

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
Copy link
Member

koic commented Oct 23, 2023

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
Copy link
Author

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.

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

No branches or pull requests

3 participants