search

rubocop / rubocop-capybara

Code style checking for Capybara files.
https://docs.rubocop.org/rubocop-capybara
MIT License
41 stars 8 forks source link

`Capybara/SpecificFinders` conflicts with `Rails/DynamicFindBy` #23

Closed FunnyHector closed 1 year ago

FunnyHector commented 1 year ago

Capybara/SpecificFinders suggests the following change:

# bad
find('#some-id')
find('[visible][id=some-id]')

# good
find_by_id('some-id')
find_by_id('some-id', visible: true)

However, find_by_id is going to be flagged by Rails/DynamicFindBy.

These two cops can't be enabled together. If I had to choose from the two, Rails/DynamicFindBy would be my choice and Capybara/SpecificFinders would be disabled.

Expected behavior

Capybara/SpecificFinders could somehow respect Rails/DynamicFindBy.

Actual behavior

They have conflicting opinions on code like these.

RuboCop version

➜  rubocop -V
1.43.0 (using Parser 3.2.0.0, rubocop-ast 1.24.1, running on ruby 3.1.3) [arm64-darwin22]
  - rubocop-performance 1.15.2
  - rubocop-rails 2.14.2
  - rubocop-rspec 2.18.0
ydah commented 1 year ago

Hi, thanks for your feedback! I also recognize the issue.

FYI, rubocop-rails provides a measure to mitigate false positives for this capybara-derived method.

How about doing the following to avoid this?

Rails/DynamicFindBy:
  AllowedMethods:
    - find_by_sql # default by rubocop-rails
    - find_by_token_for # default by rubocop-rails
    - find_by_id
pirj commented 1 year ago

I don't entirely understand, can you please provide an example of an offence raised by DynamicFinders? It should ignore an implicit receiver and page, right? Also, when multiple arguments are passed.

FunnyHector commented 1 year ago

How about doing the following to avoid this?

@ydah Thanks for your suggestion. I've seen this somewhere, but I don't really want to whitelist those methods for Rails/DynamicFindBy. I think having find_by_id calls on ActiveRecord in production code is worse than not having a capybara derived method call in tests. Currently I've disabled Capybara/SpecificFinders for that reason.

FunnyHector commented 1 year ago

can you please provide an example of an offence raised by DynamicFinders?

@pirj I have this code:

expect(page.find("#id")).to be_checked

Capybara/SpecificFinders auto-corrects this to

expect(page.find_by_id('id')).to be_checked

which is flagged by Rails/DynamicFindBy:

Offenses:

spec/test.rb:9:14: C: [Correctable] Rails/DynamicFindBy: Use find_by instead of dynamic find_by_id.
      expect(page.find_by_id('id')).to be_checked
             ^^^^^^^^^^^^^^^^^^^^^
ydah commented 1 year ago

@FunnyHector Ah. I overlooked one thing, but if you keep rubocop-rails up-to-date, I don't think that problem will occur. Some of the following changes to Rails/DynamicFindBy have already been incorporated into rubocop-rails, which should be at least 2.17.3 or higher.

FunnyHector commented 1 year ago

@ydah ah ye my rubocop-rails isn't very up to date. #862 should resolve this issue. Thanks to everyone for this! We can close this issue now I believe.

johnpash commented 1 year ago

Sorry for the hi-jack but I have a quick question.

Having just started using rubocop-capybara I was presented with this error. The exact error message is: Prefer find_by over find.

But if the only option is to use find_by_id wouldn't it be better if the message read Prefer find_by_id over find.

ydah commented 1 year ago

Thank you for your feedback! We will open the next Pull Request to improve the message.