-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
Fix a false negative for RSpec/Capybara/VisibilityMatcher
with negative matcher
#1391
Conversation
ArgumentError
with RSpec/Capybara/VisibilityMatcher
where :visible
is an invalid optionRSpec/Capybara/VisibilityMatcher
with negative matcher.
RSpec/Capybara/VisibilityMatcher
with negative matcher.RSpec/Capybara/VisibilityMatcher
with negative matcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having those in one PR is fine 👍
Please fix the changelog (and take a look at the other comments, though they are not required changes)
b8883bc
to
ccfb743
Compare
CHANGELOG.md
Outdated
@@ -3,6 +3,8 @@ | |||
## Master (Unreleased) | |||
|
|||
* Fix an error for `RSpec/Capybara/SpecificFinders` with no parentheses. ([@ydah][]) | |||
* Exclude methods that raise `ArgumentError` with `RSpec/Capybara/VisibilityMatcher` where `:visible` is an invalid option. ([@ydah][]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's better to list which elements were excluded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks better. I have modified it to list it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now you have to entries for the same commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, looks like the second commit contained an unnecessary entry. Fixed it.
have_unchecked_field | ||
have_text | ||
have_content | ||
have_button have_checked_field have_css have_field have_link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT of such an approach:
%w[button
checked_field
css
field
link
select
selector
table
unchecked_field
xpath].flat_map { |element| ["have_#{element}", "have_no_#{element}"]}
That way we'd never forget to add the no_ version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had to make it a symbol, so I modified it a bit, how do you like it?
…h `RSpec/Capybara/VisibilityMatcher` where `:visible` is an invalid option If `:visible` is specified for `have_text` and its alias method, `have_content`, it will result in an `ArgumentError` as follows. Therefore, This PR is excluded `have_text` and `have_content` for `RSpec/Capybara/VisibilityMatcher`. ``` Failure/Error: expect(page).to have_text('a', visible: true) ArgumentError: Invalid option(s) :visible, should be one of :count, :minimum, :maximum, :between, :wait, :exact, :normalize_ws # ./spec/capybara/sandbox_spec.rb:20:in `block (2 levels) in <top (required)>' Failure/Error: expect(page).to have_content('a', visible: true) ArgumentError: Invalid option(s) :visible, should be one of :count, :minimum, :maximum, :between, :wait, :exact, :normalize_ws # ./spec/capybara/sandbox_spec.rb:68:in `block (2 levels) in <top (required)>' ```
ccfb743
to
ff1254b
Compare
…tive matchers Fix: rubocop#1390
ff1254b
to
567adbe
Compare
This PR fixes a problem with
RSpec/Capybara/VisibilityMatcher
It is difficult to separate the PRs, and we have divided them into two commits in the PR and as a changelog, but please let us know if we need to separate the PRs.
@ commit 1 9778492
If
:visible
is specified forhave_text
and its alias method,have_content
, it will result in anArgumentError
as follows.Therefore, This PR is excluded
have_text
andhave_content
forRSpec/Capybara/VisibilityMatcher
.@ commit 2 b8883bc
Fix: #1390
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.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:
config/default.yml
.Enabled: pending
inconfig/default.yml
.Enabled: true
in.rubocop.yml
.VersionAdded
indefault/config.yml
to the next minor version.If you have modified an existing cop's configuration options:
VersionChanged
inconfig/default.yml
to the next major version.