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 Capybara/VisibilityMatcher cop #886

Merged
merged 1 commit into from Mar 26, 2020

Conversation

aried3r
Copy link
Contributor

@aried3r aried3r commented Mar 24, 2020

Fixes #885.

This is a work in progress still, since it's my first cop.


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.
  • Added an entry to the changelog 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).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.

config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/capybara/visibility_matcher.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/capybara/visibility_matcher.rb Outdated Show resolved Hide resolved
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.

Flawless headstart! 👏

config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/capybara/visibility_matcher.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/capybara/visibility_matcher.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/capybara/visibility_matcher.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/capybara/visibility_matcher.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/capybara/visibility_matcher_spec.rb Outdated Show resolved Hide resolved
@aried3r
Copy link
Contributor Author

aried3r commented Mar 25, 2020

I believe I have this PR in an acceptable working state except for the node matchers that are not as strict as they could be. But I don't know how to properly write the possible signatures.

@aried3r aried3r marked this pull request as ready for review March 25, 2020 18:46
@pirj
Copy link
Member

pirj commented Mar 25, 2020

Looks great, I have nothing to add!
To me, it's none of this cop's business to check if the first argument is a sym or not, so a flexible pattern is fine.

Can you please resolve the conflict?

@bquorning @Darhazer do you have any objections on onboarding this new cop?

@aried3r aried3r force-pushed the ar/add_capybara_visibility_cop branch from 04411f5 to 369e500 Compare March 25, 2020 20:09
@aried3r
Copy link
Contributor Author

aried3r commented Mar 25, 2020

Can you please resolve the conflict?

Rebased, squashed and resolved.

it 'registers an offense when using a using multiple options`' do
expect_offense(<<-RUBY)
expect(page).to have_selector('.my_element', count: 1, visible: false, normalize_ws: true)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `:all` or `:hidden` instead of `false`.
Copy link
Collaborator

@bquorning bquorning Mar 26, 2020

Choose a reason for hiding this comment

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

Good stuff! Thanks for contributing.

One comment though. Instead of

expect(page).to have_selector('.my_element', count: 1, visible: false, normalize_ws: true)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `:all` or `:hidden` instead of `false`.

I would prefer

expect(page).to have_selector('.my_element', count: 1, visible: false, normalize_ws: true)
                                                       ^^^^^^^^^^^^^^ Use `:all` or `:hidden` instead of `false`.

That is, highlighting only the offending part of the hash argument.

You can do this by having the node matchers return that part of the node by using $, e.g.

(send nil? :have_selector ... (hash <$(pair (sym :visible) true) ...>))

and then using the return value (which is also yielded, if you call the node matcher with a block) – e.g.

visible_true?(node) do |arg|
  add_offense(arg, message: MSG_TRUE)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I didn't know how to do that, thanks for the pointers. I've added these changes and am now waiting for CI. I will squash if you approve the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Go ahead, looks perfect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@aried3r aried3r force-pushed the ar/add_capybara_visibility_cop branch from 23307c7 to 7aa3340 Compare March 26, 2020 14:30
@pirj pirj merged commit 8c6af61 into rubocop:master Mar 26, 2020
@pirj
Copy link
Member

pirj commented Mar 26, 2020

Thank you for your contribution!

@bquorning
Copy link
Collaborator

🙏 Thank you @aried3r

@aried3r
Copy link
Contributor Author

aried3r commented Mar 27, 2020

Thank you for all the help!

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.

Cop idea: Don't allow visible: true or visible: false for capybara matchers
4 participants