From ea321f821b33fce27e11dd8c21cb3e1d92323c12 Mon Sep 17 00:00:00 2001 From: ydah Date: Mon, 24 Nov 2025 00:56:18 +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 | 12 ++++++ .../cop/capybara/redundant_within_find.rb | 21 ++++++++-- .../cop/capybara/mixin/css_selector_spec.rb | 38 +++++++++++++++++++ .../capybara/redundant_within_find_spec.rb | 31 +++++++++++++++ 5 files changed, 99 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32a41da1..22a4c568 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - Add a new `Capybara/NegationMatcherAfterVisit` cop. ([@ydah]) - Fix an error for `Capybara/RSpec/HaveSelector` when passing no arguments. ([@earlopain]) - Make RuboCop Capybara work as a RuboCop plugin. ([@bquorning]) +- 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 43a264ef..a4caf97a 100644 --- a/lib/rubocop/cop/capybara/mixin/css_selector.rb +++ b/lib/rubocop/cop/capybara/mixin/css_selector.rb @@ -83,6 +83,18 @@ def multiple_selectors?(selector) normalize = selector.gsub(/(\\[>,+~]|\(.*\))/, '') normalize.match?(/[ >,+~]/) end + + # @param str [String] + # @param quote [String] the quote character used (' or ") + # @return [String] + # @example + # css_escape('foo.bar', "'") # => 'foo\.bar' + # css_escape('foo.bar', '"') # => 'foo\\.bar' + # css_escape('foo', "'") # => 'foo' + def css_escape(str, quote = "'") + escaped_dot = quote == '"' ? '\\\\\\\\.' : '\\\\.' + str.gsub('.', escaped_dot) + 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 8e0085b2..db9da94a 100644 --- a/lib/rubocop/cop/capybara/redundant_within_find.rb +++ b/lib/rubocop/cop/capybara/redundant_within_find.rb @@ -27,6 +27,7 @@ module Capybara # end # class RedundantWithinFind < ::RuboCop::Cop::Base + include CssSelector extend AutoCorrector MSG = 'Redundant `within %s(...)` call detected.' RESTRICT_ON_SEND = %i[within].freeze @@ -53,13 +54,25 @@ def msg(node) end def replaced(node) - replaced = node.arguments.map(&:source).join(', ') - if node.method?(:find_by_id) - replaced.sub(/\A(["'])/, '\1#') + unless node.method?(:find_by_id) + return node.arguments.map(&:source).join(', ') + end + + if node.first_argument.str_type? + build_escaped_selector(node.first_argument, node) else - replaced + node.arguments.map(&:source).join(', ') + .sub(/\A(["'])/, '\1#') end end + + def build_escaped_selector(first_arg, node) + quote = first_arg.source[0] + escaped_id = CssSelector.css_escape(first_arg.value, quote) + rest_args = node.arguments.drop(1).map(&:source) + + ["#{quote}##{escaped_id}#{quote}", *rest_args].join(', ') + end 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 af7cda24..5b0f5b3b 100644 --- a/spec/rubocop/cop/capybara/mixin/css_selector_spec.rb +++ b/spec/rubocop/cop/capybara/mixin/css_selector_spec.rb @@ -143,4 +143,42 @@ expect(described_class.multiple_selectors?('a.cls\>b')).to be false end end + + describe 'CssSelector.css_escape' do + context 'with single quote' do + it 'escapes dots with single backslash' do + expect(described_class.css_escape('foo.bar', "'")).to eq 'foo\\.bar' + end + + it 'does not escape when no dots' do + expect(described_class.css_escape('foo', "'")).to eq 'foo' + end + + it 'escapes multiple dots' do + expect(described_class.css_escape('foo.bar.baz', + "'")).to eq 'foo\\.bar\\.baz' + end + end + + context 'with double quote' do + it 'escapes dots with double backslash for double-quoted strings' do + expect(described_class.css_escape('foo.bar', '"')).to eq 'foo\\\\.bar' + end + + it 'does not escape when no dots' do + expect(described_class.css_escape('foo', '"')).to eq 'foo' + end + + it 'escapes multiple dots' do + expect(described_class.css_escape('foo.bar.baz', + '"')).to eq 'foo\\\\.bar\\\\.baz' + end + end + + context 'with default quote (single quote)' do + it 'uses single quote escaping by default' do + expect(described_class.css_escape('foo.bar')).to eq 'foo\\.bar' + 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 9de7f973..dc51cc34 100644 --- a/spec/rubocop/cop/capybara/redundant_within_find_spec.rb +++ b/spec/rubocop/cop/capybara/redundant_within_find_spec.rb @@ -66,6 +66,24 @@ 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 + within find_by_id("foo.bar") do + ^^^^^^^^^^^^^^^^^^^^^ Redundant `within find_by_id(...)` call detected. + end + RUBY + + expect_correction(<<~'RUBY') + within '#foo\.bar' do + end + within "#foo\\.bar" do + end + RUBY + end + it 'registers an offense when using `within find_by_id(...)` with ' \ 'other argument' do expect_offense(<<~RUBY) @@ -80,6 +98,19 @@ RUBY end + it 'registers an offense when using `within find_by_id(variable)`' do + expect_offense(<<~RUBY) + within find_by_id(id_variable) do + ^^^^^^^^^^^^^^^^^^^^^^^ Redundant `within find_by_id(...)` call detected. + end + RUBY + + expect_correction(<<~RUBY) + within id_variable do + end + RUBY + end + it 'does not register an offense when using `within` without `find`' do expect_no_offenses(<<~RUBY) within 'foo.bar' do