From f198be4b4d267d0afeb9a43bf8cf063e9be626c6 Mon Sep 17 00:00:00 2001 From: ydah Date: Fri, 1 Nov 2024 01:13:14 +0900 Subject: [PATCH] Fix an incorrect autocorrect for `Capybara/RedundantWithinFind` when escape required css selector Fix: https://github.com/rubocop/rubocop-capybara/issues/136 Previously, the following automatic corrections were made. before: ```ruby within find_by_id("array-form-session.dates") do expect(page).to have_text(:visible, "YYYY/MM/DD") end ``` after: ```ruby within "#array-form-session.dates" do expect(page).to have_text(:visible, "YYYY/MM/DD") end ``` This is `.' in find_by_id. ` has the same meaning as the escaped id. In other words, when replacing within, the `. ` must be escaped. This PR has been modified so that escaping is correctly done in selectors that require escaping. This will prevent the behavior from changing before and after the automatic correction. --- CHANGELOG.md | 1 + .../cop/capybara/mixin/css_selector.rb | 45 ++++++++++++ .../cop/capybara/redundant_within_find.rb | 10 ++- .../cop/capybara/mixin/css_selector_spec.rb | 69 +++++++++++++++++++ .../capybara/redundant_within_find_spec.rb | 13 ++++ 5 files changed, 135 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a868d81..03b7396 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Add `Capybara/AmbiguousClick` cop and make soft-deprecated `Capybara/ClickLinkOrButtonStyle` cop. If you want to use `EnforcedStyle: strict`, use `Capybara/AmbiguousClick` cop instead. ([@ydah]) - Add new `Capybara/FindAllFirst` cop. ([@ydah]) - Fix an error for `Capybara/RSpec/HaveSelector` when passing no arguments. ([@earlopain]) +- Fix an incorrect autocorrect for `Capybara/RedundantWithinFind` when escape required css selector. ([@ydah]) ## 2.21.0 (2024-06-08) diff --git a/lib/rubocop/cop/capybara/mixin/css_selector.rb b/lib/rubocop/cop/capybara/mixin/css_selector.rb index 43a264e..e4e2a7a 100644 --- a/lib/rubocop/cop/capybara/mixin/css_selector.rb +++ b/lib/rubocop/cop/capybara/mixin/css_selector.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'strscan' + module RuboCop module Cop module Capybara @@ -83,6 +85,49 @@ def multiple_selectors?(selector) normalize = selector.gsub(/(\\[>,+~]|\(.*\))/, '') normalize.match?(/[ >,+~]/) end + + # @param selector [String] + # @return [String] + # @example + # css_escape('some-id') # => some-id + # css_escape('some-id.with-priod') # => some-id\.with-priod + # @reference + # https://github.com/mathiasbynens/CSS.escape/blob/master/css.escape.js + def css_escape(selector) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity,Metrics/MethodLength,Metrics/PerceivedComplexity + scanner = StringScanner.new(selector) + result = +'' + + # Special case: if the selector is of length 1 and + # the first character is `-` + if selector.length == 1 && scanner.peek(1) == '-' + return "\\#{selector}" + end + + until scanner.eos? + # NULL character (U+0000) + if scanner.scan(/\0/) + result << "\uFFFD" + # Control characters (U+0001 to U+001F, U+007F) + elsif scanner.scan(/[\x01-\x1F\x7F]/) + result << "\\#{scanner.matched.ord.to_s(16)} " + # First character is a digit (U+0030 to U+0039) + elsif scanner.pos.zero? && scanner.scan(/\d/) + result << "\\#{scanner.matched.ord.to_s(16)} " + # Second character is a digit and first is `-` + elsif scanner.pos == 1 && scanner.scan(/\d/) && + scanner.pre_match == '-' + result << "\\#{scanner.matched.ord.to_s(16)} " + # Alphanumeric characters, `-`, `_` + elsif scanner.scan(/[a-zA-Z0-9\-_]/) + result << scanner.matched + # Any other characters, escape them + elsif scanner.scan(/./) + result << "\\#{scanner.matched}" + end + end + + result + end end end end diff --git a/lib/rubocop/cop/capybara/redundant_within_find.rb b/lib/rubocop/cop/capybara/redundant_within_find.rb index 8e0085b..59e051c 100644 --- a/lib/rubocop/cop/capybara/redundant_within_find.rb +++ b/lib/rubocop/cop/capybara/redundant_within_find.rb @@ -53,11 +53,15 @@ def msg(node) end def replaced(node) - replaced = node.arguments.map(&:source).join(', ') if node.method?(:find_by_id) - replaced.sub(/\A(["'])/, '\1#') + node.first_argument.source.match(/\A(['"])(.*)['"]\z/) do + quote = ::Regexp.last_match(1) + css = ::Regexp.last_match(2) + return ["#{quote}##{CssSelector.css_escape(css)}#{quote}", + *node.arguments[1..].map(&:source)].join(', ') + end else - replaced + node.arguments.map(&:source).join(', ') end end end diff --git a/spec/rubocop/cop/capybara/mixin/css_selector_spec.rb b/spec/rubocop/cop/capybara/mixin/css_selector_spec.rb index af7cda2..cd795ae 100644 --- a/spec/rubocop/cop/capybara/mixin/css_selector_spec.rb +++ b/spec/rubocop/cop/capybara/mixin/css_selector_spec.rb @@ -143,4 +143,73 @@ expect(described_class.multiple_selectors?('a.cls\>b')).to be false end end + + describe 'CssSelector.css_escape' do + context 'when selector is a single hyphen' do + let(:selector) { '-' } + + it 'escapes the hyphen character' do + expect(described_class.css_escape(selector)).to eq('\\-') + end + end + + context 'when selector contains NULL character' do + let(:selector) { "abc\0def" } + + it 'replaces NULL character with U+FFFD' do + expect(described_class.css_escape(selector)).to eq('abc�def') + end + end + + context 'when selector contains control characters' do + let(:selector) { "abc\x01\x1F\x7Fdef" } + + it 'escapes control characters as hexadecimal with a trailing space' do + expect(described_class.css_escape(selector)) + .to eq('abc\\1 \\1f \\7f def') + end + end + + context 'when selector starts with a digit' do + let(:selector) { '1abc' } + + it 'escapes the starting digit as hexadecimal with a trailing space' do + expect(described_class.css_escape(selector)).to eq('\\31 abc') + end + end + + context 'when selector starts with a hyphen followed by a digit' do + let(:selector) { '-1abc' } + + it 'escapes the digit following a hyphen as hexadecimal with a ' \ + 'trailing space' do + expect(described_class.css_escape(selector)).to eq('-\\31 abc') + end + end + + context 'when selector contains alphanumeric characters, hyphen, or ' \ + 'underscore' do + let(:selector) { 'a-Z_0-9' } + + it 'does not escape alphanumeric characters, hyphen, or underscore' do + expect(described_class.css_escape(selector)).to eq('a-Z_0-9') + end + end + + context 'when selector contains special characters needing escape' do + let(:selector) { 'a!b@c#d$' } + + it 'escapes special characters' do + expect(described_class.css_escape(selector)).to eq('a\\!b\\@c\\#d\\$') + end + end + + context 'when selector contains mixed cases' do + let(:selector) { "ab\x00\x7F" } + + it 'handles mixed cases appropriately' do + expect(described_class.css_escape(selector)).to eq('ab�\\7f ') + end + end + end end diff --git a/spec/rubocop/cop/capybara/redundant_within_find_spec.rb b/spec/rubocop/cop/capybara/redundant_within_find_spec.rb index 9de7f97..9a81457 100644 --- a/spec/rubocop/cop/capybara/redundant_within_find_spec.rb +++ b/spec/rubocop/cop/capybara/redundant_within_find_spec.rb @@ -66,6 +66,19 @@ RUBY end + it 'registers an offense when using `within find_by_id("foo.bar")`' do + expect_offense(<<~RUBY) + within find_by_id('foo.bar') do + ^^^^^^^^^^^^^^^^^^^^^ Redundant `within find_by_id(...)` call detected. + end + RUBY + + expect_correction(<<~'RUBY') + within '#foo\.bar' do + end + RUBY + end + it 'registers an offense when using `within find_by_id(...)` with ' \ 'other argument' do expect_offense(<<~RUBY)