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

Add support for assert_true and assert_false to RSpec/Rails/MinitestAssertions #1786

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

ydah
Copy link
Member

@ydah ydah commented Jan 20, 2024

related to rubocop/rubocop-rspec_rails#7

This PR add support for assert_true and assert_false to RSpec/Rails/MinitestAssertions.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@ydah ydah requested a review from a team as a code owner January 20, 2024 15:36
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks mostly good.
I agree with @koic that we should auto-correct to stricter be(true)/be(false).

docs/modules/ROOT/pages/cops_rspec_rails.adoc Show resolved Hide resolved
lib/rubocop/cop/rspec/rails/minitest_assertions.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/rails/minitest_assertions.rb Outdated Show resolved Hide resolved
@ydah ydah force-pushed the assert_true_or_false branch from bdd524b to 2ba998b Compare February 1, 2024 15:35
@ydah ydah requested a review from pirj February 1, 2024 15:36
@ydah
Copy link
Member Author

ydah commented Feb 2, 2024

Ah I missed some comments. Hmm. I think we should distinguish between Minitest and Test:: Unit.
I thought it best not to support assert_true and assert_false in this Cop. It would be better to discuss the extent of this support first. For example, it looks like it could be transferred to a Cop called RSpec/TestUnitAssertions. @rubocop/rubocop-rspec WDYT?

@G-Rath
Copy link
Contributor

G-Rath commented Feb 2, 2024

fwiw I currently vote for "have a single cop" because these all seem to be using the same structure and as a downstream user my focus is on "use expect consistently" - that doesn't matter if the assertions come from Minitest, Test::Unit, or Rails.

Put another way: I don't think I've yet seen a technical reason for having different cops? i.e. the assertions so far have followed the same logic and structure regardless of where they're actually from.

If people want to rename it to something like RSpec/PreferExpectInsteadOfAssert, that'd be fine with me but I think having two cops based on "Minitest vs TestUnit" when they have the same behaviours is just adding more work for ourselves (and to take that a step further: should we have a third cop dedicated to Rails' extension assertions?)

@ydah
Copy link
Member Author

ydah commented Feb 2, 2024

@G-Rath Thank you for expressing your opinion.

Put another way: I don't think I've yet seen a technical reason for having different cops?

In many cases, I think there is no problem to combine them into one cop. However, as the devil's advocate, I would like to deepen this discussion.

For example, the following comments seem to be points where different actions are required. The compatibility of assert and refute is different between Minitest and Test:: Unit.

Also, the adapter to Minitest is defined in rspec-rails, which is appropriate for the RSpec/Rails department.
https://github.com/rspec/rspec-rails/blob/main/lib/rspec/rails/adapters.rb#L140

Note, however, that Unit:: Test exists in rspec-core.
https://github.com/rspec/rspec-core/blob/f8c8880dabd8f0544a6f91d8d4c857c1bd8df903/lib/rspec/core/test_unit_assertions_adapter.rb#L9

In other words, it seems odd to enable Cop for the RSpec/Rails department if you are using Unit:: Test-style assertions and not rspec-rails.

@G-Rath
Copy link
Contributor

G-Rath commented Feb 2, 2024

However, as the devil's advocate, I would like to deepen this discussion.

Yup fully in support of that, and in doing so sooner rather than later.

In other words, it seems odd to enable Cop for the RSpec/Rails department if you are using Unit:: Test-style assertions and not rspec-rails

Just want to check that I'm understanding what you mean correctly, which is effectively: it's weird for someone who's using just Unit::Test but not Rails, to enable a Rails cop.

If so then yes I agree with that - I'd wondered why this was considered a Rails cop though you've pointed out it's rspec-rails that has the Minitest adapter so I'm guessing that is (or was...) the why.

@pirj
Copy link
Member

pirj commented Feb 2, 2024

odd to enable Cop for the RSpec/Rails department if you are using Unit:: Test-style assertions and not rspec-rails.

Fair enough, but in the future when we extract rubocop-rspec_rails, we could also extract Rails-specific assertion detectors from this cop, and make the cop open for extension.
We’ll depend on rubocop-rspec from rubocop-rspec_rails, right?

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

@G-Rath
Copy link
Contributor

G-Rath commented Feb 3, 2024

Probably best to take this to the issue, but there's also Capybara assertions we could be translating, and I'd like to also include aggregate_assertions as it seems very solid and 1:1 with aggregate_failures

@ydah
Copy link
Member Author

ydah commented Feb 5, 2024

We’ll depend on rubocop-rspec from rubocop-rspec_rails, right?

Oh, I see. If we needs to extract to rubocop-rspec_rails, maybe we shouldn't add support for this to the RSpec/Rails department's Cop for now?

@pirj
Copy link
Member

pirj commented Feb 5, 2024

I think this all barely exists outside of rspec-rails. Despite theexpect_with setting, rspec-rails exposes those assert methods, and there’s no known) way to hide them.

It’s ok to add all this, and all of this actually, not just the AS aliases should later be extracted to rubocop-rspec_rails.

@ydah ydah force-pushed the assert_true_or_false branch 3 times, most recently from 93bde32 to 354d8a7 Compare February 6, 2024 14:23
@ydah ydah requested a review from pirj February 6, 2024 14:26
@ydah
Copy link
Member Author

ydah commented Feb 6, 2024

I update this PR.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

config/default.yml Outdated Show resolved Hide resolved
@ydah ydah force-pushed the assert_true_or_false branch from 354d8a7 to 2d7c792 Compare February 7, 2024 14:38
@ydah ydah requested a review from bquorning February 7, 2024 14:39
Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Thank you

@bquorning bquorning merged commit 0b153d7 into master Feb 7, 2024
24 checks passed
@bquorning bquorning deleted the assert_true_or_false branch February 7, 2024 20:44
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.

5 participants