rubocop / rubocop-capybara

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

`RSpec/Capybara/SpecificFinders` autocorrection shoud remove escape #4

Closed Tietew closed 1 year ago

Tietew commented 1 year ago

Reproducion steps

Run RSpec/Capybara/SpecificFinders autocorrection on:

find('#domain-deadbeef\.org-delete')

This returns a tag <tag id="domain-deadbeef.org-delete">, which id contains .. The . shoud be escaped because bare . is a class selector in CSS.

Expected result

find_by_id('domain-deadbeef.org-delete')

CSS selector escape should be removed.

Actual result

find_by_id('domain-deadbeef\.org-delete')

References

Per HTML Standard, id attribute can contain any character except whitespace. https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id

ydah commented 1 year ago

Thanks for the report!

Perhaps, in this case, the following autocorrection is needed.

# before
find_by('#foo.bar')

# after
find_by_id('foo', class: 'bar')
Tietew commented 1 year ago

Unfortunately, no. In my case, the id string contains the dot . character; not class.

# for <tag id="foo" class="bar">
find('#foo.bar')
find('.bar#foo')

# after
find_by_id('foo', class: 'bar')
# for <tag id="foo.bar">
find('#foo\.bar')

# after
find_by_id('foo.bar')

I tried to fix this issue, but it's not easy because RuboCop::Cop::RSpec::CssSelector is regexp based. To fix perfectly, it's needed to rewrite CssSelector with correct CSS selector parser.

Current best fix may be marking SafeAutocorrect: false on the cop.

Tietew commented 1 year ago

I tested more edge cases:

# before
find('#foo')
find('#foo\.bar')
find('[id="foo"]')
find('[id="foo[bar]"]')
find('[id="foo"i]')
find('#foo.bar')
find('#foo[hidden]')
find('#foo:enabled')
find('#foo:not(:disabled)')
find('#foo:is(:enabled,:checked)')
find('#foo\,bar')

# after autocorection
find_by_id('foo') # OK
find_by_id('foo\.bar') # incorrect; should be find_by_id('foo.bar')
find_by_id('"foo"') # incorrect; should be find_by_id('foo')
find_by_id('"foo[bar') # incorrect; should be find_by_id('foo[bar]')
find_by_id('"foo"i') # incorrect; should not be modified
find_by_id('foo.bar') # incorrect; should be find_by_id('foo', class: 'bar')
find_by_id('foo[hidden]') # incorrect; should not be modified
find_by_id('foo:enabled') # incorrect; should not be modified
find_by_id('foo:not(:disabled)') # incorrect; should not be modified
find('#foo:is(:enabled,:checked)') # OK
find('#foo\,bar') # incorrect; should be find_by_id('foo,bar')
Tietew commented 1 year ago

More case...

# before
find('[id=foo][class=bar]')

# after
find_by_id('foo', class: 'bar') # incorrect

This correction is incorrect because the selector [class=bar] means the value of the attribute class is exactly bar. find_**(class: 'bar'), .bar and [class~=bar] means the class contains bar for example <tag class="deadbeef bar">.

ydah commented 1 year ago

Thank you very much. I understand. We will attempt to accommodate edge cases.