Enable additional Rubocop rules for Capybara tests

.rubocop.yml

@@ -6,7 +6,6 @@
 require:
   - rubocop-rails
   - rubocop-rspec
-  - rubocop-capybara
   - ./lib/linters/i18n_helper_html_linter.rb
   - ./lib/linters/analytics_event_name_linter.rb
   - ./lib/linters/localized_validation_message_linter.rb
@@ -15,9 +14,12 @@ require:
   - ./lib/linters/redirect_back_linter.rb
   - ./lib/linters/url_options_linter.rb
   - ./lib/linters/errors_add_linter.rb
+  - ./lib/linters/capybara_current_url_expect_linter.rb
+  - ./lib/linters/capybara_current_path_equality_linter.rb
 
 plugins:
   - rubocop-performance
+  - rubocop-capybara
 
 AllCops:
   Exclude:
@@ -46,12 +48,45 @@ Bundler/DuplicatedGem:
 Bundler/InsecureProtocolSource:
   Enabled: true
 
+Capybara/AmbiguousClick:
+  Enabled: false
+
 Capybara/CurrentPathExpectation:
   Enabled: true
 
+Capybara/FindAllFirst:
+  Enabled: true
+
+Capybara/MatchStyle:
+  Enabled: true
+
+Capybara/RedundantWithinFind:
+  Enabled: true
+
+Capybara/RSpec/PredicateMatcher:
+  Enabled: true
+
+Capybara/SpecificActions:
+  Enabled: true
+
+Capybara/SpecificFinders:
+  Enabled: false
+
+Capybara/SpecificMatcher:
+  Enabled: false
+
+Capybara/VisibilityMatcher:
+  Enabled: false
+
 Gemspec/DuplicatedAssignment:
   Enabled: true
 
+IdentityIdp/CapybaraCurrentUrlExpectLinter:
+  Enabled: true
+
+IdentityIdp/CapybaraCurrentPathEqualityLinter:
+  Enabled: true
+
 IdentityIdp/I18nHelperHtmlLinter:
   Enabled: true
   Include:

Gemfile.lock

@@ -633,8 +633,9 @@ GEM
     rubocop-ast (1.46.0)
       parser (>= 3.3.7.2)
       prism (~> 1.4)
-    rubocop-capybara (2.21.0)
-      rubocop (~> 1.41)
+    rubocop-capybara (2.22.1)
+      lint_roller (~> 1.1)
+      rubocop (~> 1.72, >= 1.72.1)
     rubocop-performance (1.25.0)
       lint_roller (~> 1.1)
       rubocop (>= 1.75.0, < 2.0)

lib/linters/capybara_current_path_equality_linter.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module IdentityIdp
+      # This lint is similar to
+      # https://docs.rubocop.org/rubocop-capybara/cops_capybara.html#capybaracurrentpathexpectation
+      # and our `IdentityIdp::CapybaraCurrentUrlExpectLinter` in that it is intended to help prevent
+      # race conditions. A comparison of the `current_path` or `current_url` relies on page
+      # navigation timing which can be asynchronous and lead to inconsistent results.
+      #
+      # These cases typically come up when trying to support multiple action paths in a feature
+      # test. We should prefer being explicit about what actions are expected rather than relying
+      # on changing a shared helper, even if it results in a more verbose test.
+      # Using method parameters to control the flow is another option. The critical part is the
+      # conditional should be stable regardless of the timing of browser activity.
+      #
+      # @example
+      #   #bad
+      #   return if page.current_path == idv_document_capture_path
+      #
+      class CapybaraCurrentPathEqualityLinter < RuboCop::Cop::Base
+        include RangeHelp
+
+        MSG = 'Do not compare equality of `current_path` in Capybara feature specs - instead,' \
+        ' use the `have_current_path` matcher on `page` or avoid it entirely'
+
+        RESTRICT_ON_SEND = %i[== !=].freeze
+
+        def_node_matcher :current_path_equality_lhs, <<~PATTERN
+          (send (send {(send nil? :page) nil?} :current_path) ${:== :!=} (...))
+        PATTERN
+
+        def_node_matcher :current_path_equality_rhs, <<~PATTERN
+          (send (...) ${:== :!=} (send {(send nil? :page) nil?} :current_path))
+        PATTERN
+
+        def on_send(node)
+          if current_path_equality_lhs(node) || current_path_equality_rhs(node)
+            add_offense(node)
+          end
+        end
+      end
+    end
+  end
+end

lib/linters/capybara_current_url_expect_linter.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module IdentityIdp
+      # This lint is a very similar to
+      # https://docs.rubocop.org/rubocop-capybara/cops_capybara.html#capybaracurrentpathexpectation
+      # but for `current_url` instead of `current_path`. The reasoning is the same in that it
+      # ensures usage of Capybara's waiting functionality. This linter is not autocorrectable.
+      #
+      # @example
+      #   #bad
+      #   expect(current_url).to eq authentication_methods_setup_url
+      #   expect(current_url).to eq 'http://localhost:3001/auth/result'
+      #
+      #   #good
+      #   expect(page).to have_current_path(authentication_methods_setup_path)
+      #   expect(page).to have_current_path('http://localhost:3001/auth/result', url: true)
+      #
+      class CapybaraCurrentUrlExpectLinter < RuboCop::Cop::Base
+        include RangeHelp
+
+        MSG = 'Do not set an RSpec expectation on `current_url` in ' \
+          'Capybara feature specs - instead, use the ' \
+          '`have_current_path` matcher on `page`'
+
+        RESTRICT_ON_SEND = %i[expect].freeze
+
+        # @!method expectation_set_on_current_url(node)
+        def_node_matcher :expectation_set_on_current_url, <<~PATTERN
+          (send nil? :expect (send {(send nil? :page) nil?} :current_url))
+        PATTERN
+
+        # Supported matchers: eq(...) / match(/regexp/) / match('regexp')
+        # @!method as_is_matcher(node)
+        def_node_matcher :as_is_matcher, <<~PATTERN
+          (send
+            #expectation_set_on_current_url ${:to :to_not :not_to}
+            ${(send nil? :eq ...) (send nil? :match (...)) (send nil? :include ...) (send nil? :start_with ...)})
+        PATTERN
+
+        # @!method regexp_node_matcher(node)
+        def_node_matcher :regexp_node_matcher, <<~PATTERN
+          (send
+           #expectation_set_on_current_url ${:to :to_not :not_to}
+           $(send nil? :match ${str dstr xstr}))
+        PATTERN
+
+        def on_send(node)
+          expectation_set_on_current_url(node) do
+            as_is_matcher(node.parent) do
+              add_offense(node.parent.loc.selector)
+            end
+
+            regexp_node_matcher(node.parent) do
+              add_offense(node.parent.loc.selector)
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/features/account_creation/completions_cancel_spec.rb

@@ -27,6 +27,7 @@
       url: true,
       ignore_query: true,
     )
-    expect(current_url).to start_with('http://localhost:7654/auth/result?error=access_denied')
+    params = UriService.params(current_url)
+    expect(params['error']).to eq('access_denied')
   end
 end

spec/features/idv/cancel_spec.rb

@@ -20,13 +20,13 @@
   end
 
   it 'shows the user a cancellation message with the option to go back to the step' do
+    expect(page).to have_current_path(idv_agreement_path)
     expect(page).to have_content(t('doc_auth.headings.verify_identity'))
-    original_path = current_path
 
     click_link t('links.cancel')
 
+    expect(page).to have_current_path(idv_cancel_path(step: 'agreement'))
     expect(page).to have_content(t('idv.cancel.headings.prompt.standard'))
-    expect(page).to have_current_path(idv_cancel_path, ignore_query: true)
     expect(fake_analytics).to have_logged_event(
       'IdV: cancellation visited',
       hash_including(step: 'agreement'),
@@ -35,12 +35,13 @@
     expect(page).to have_unique_form_landmark_labels
 
     expect(page).to have_button(t('idv.cancel.actions.start_over'))
+    expect(page).to have_no_button(t('idv.cancel.actions.exit', app_name: APP_NAME))
     expect(page).to have_button(t('idv.cancel.actions.account_page'))
     expect(page).to have_button(t('idv.cancel.actions.keep_going'))
 
     click_on(t('idv.cancel.actions.keep_going'))
+    expect(page).to have_current_path(idv_agreement_path)
     expect(page).to have_content(t('doc_auth.headings.lets_go'))
-    expect(page).to have_current_path(original_path)
     expect(fake_analytics).to have_logged_event(
       'IdV: cancellation go back',
       step: 'agreement',
@@ -51,8 +52,8 @@
     expect(page).to have_content(t('doc_auth.headings.verify_identity'))
     click_link t('links.cancel')
 
+    expect(page).to have_current_path(idv_cancel_path(step: 'agreement'))
     expect(page).to have_content(t('idv.cancel.headings.prompt.standard'))
-    expect(page).to have_current_path(idv_cancel_path, ignore_query: true)
     expect(fake_analytics).to have_logged_event(
       'IdV: cancellation visited',
       hash_including(step: 'agreement'),
@@ -66,8 +67,8 @@
 
     click_on t('idv.cancel.actions.start_over')
 
-    expect(page).to have_content(t('doc_auth.instructions.getting_started'))
     expect(page).to have_current_path(idv_welcome_path)
+    expect(page).to have_content(t('doc_auth.instructions.getting_started'))
     expect(fake_analytics).to have_logged_event(
       'IdV: start over',
       step: 'agreement',
@@ -78,8 +79,8 @@
     expect(page).to have_content(t('doc_auth.headings.verify_identity'))
     click_link t('links.cancel')
 
+    expect(page).to have_current_path(idv_cancel_path(step: 'agreement'))
     expect(page).to have_content(t('idv.cancel.headings.prompt.standard'))
-    expect(page).to have_current_path(idv_cancel_path, ignore_query: true)
     expect(fake_analytics).to have_logged_event(
       'IdV: cancellation visited',
       hash_including(step: 'agreement'),
@@ -118,6 +119,7 @@
       expect(page).to have_content(t('doc_auth.info.ssn'))
       click_link t('links.cancel')
 
+      expect(page).to have_current_path(idv_cancel_path(step: 'ssn'))
       expect(page).to have_content(t('idv.cancel.headings.prompt.standard'))
       expect(fake_analytics).to have_logged_event(
         'IdV: cancellation visited',
@@ -174,8 +176,8 @@
 
       click_link t('links.cancel')
 
+      expect(page).to have_current_path(idv_cancel_path(step: 'agreement'))
       expect(page).to have_content(t('idv.cancel.headings.prompt.standard'))
-      expect(page).to have_current_path(idv_cancel_path, ignore_query: true)
       expect(fake_analytics).to have_logged_event(
         'IdV: cancellation visited',
         hash_including(step: 'agreement'),
@@ -194,7 +196,10 @@
         url: true,
         ignore_query: true,
       )
-      expect(current_url).to start_with('http://localhost:7654/auth/result?error=access_denied')
+
+      params = UriService.params(current_url)
+      expect(params['error']).to eq('access_denied')
+
       expect(fake_analytics).to have_logged_event(
         'IdV: cancellation confirmed',
         step: 'agreement',

spec/features/idv/doc_auth/document_capture_spec.rb

@@ -164,7 +164,7 @@
       complete_doc_auth_steps_before_document_capture_step
     end
 
-    it 'user can go through verification uploading ID and selfie on seprerate pages' do
+    it 'user can go through verification uploading ID and selfie on separate pages' do
       expect(page).to have_current_path(idv_document_capture_url)
       expect(page).not_to have_content(t('doc_auth.tips.document_capture_selfie_text1'))
       attach_images
@@ -553,22 +553,13 @@
           fill_out_ssn_form_ok
           click_idv_continue
           complete_verify_step
-          # expect(page).to have_content(t('doc_auth.headings.document_capture_selfie'))
           expect(page).to have_current_path(idv_phone_url)
         end
       end
     end
 
     context 'when a selfie is required by the SP' do
-      context 'on mobile platform', allow_browser_log: true do
-        before do
-          # mock mobile device as cameraCapable, this allows us to process
-          allow_any_instance_of(ActionController::Parameters)
-            .to receive(:[]).and_wrap_original do |impl, param_name|
-            param_name.to_sym == :skip_hybrid_handoff ? '' : impl.call(param_name)
-          end
-        end
-
+      context 'on mobile platform', driver: :headless_chrome_mobile, allow_browser_log: true do
         context 'with a passing selfie' do
           it 'proceeds to the next page with valid info, including a selfie image' do
             perform_in_browser(:mobile) do
@@ -647,11 +638,6 @@
             # when there are multiple doc auth errors on front and back
             it 'shows the correct error message for the given error' do
               perform_in_browser(:mobile) do
-                if page.current_path == idv_how_to_verify_path
-                  click_button t('forms.buttons.continue_online')
-                else
-                  click_continue
-                end
                 use_id_image('ial2_test_credential_multiple_doc_auth_failures_both_sides.yml')
                 click_continue
                 click_button 'Take photo'

spec/features/idv/doc_auth/redo_document_capture_spec.rb

@@ -415,15 +415,7 @@
     end
 
     context 'when a selfie is required by the SP' do
-      context 'on mobile platform', allow_browser_log: true do
-        before do
-          # mock mobile device as cameraCapable, this allows us to process
-          allow_any_instance_of(ActionController::Parameters)
-            .to receive(:[]).and_wrap_original do |impl, param_name|
-            param_name.to_sym == :skip_hybrid_handoff ? '' : impl.call(param_name)
-          end
-        end
-
+      context 'on mobile platform', driver: :headless_chrome_mobile, allow_browser_log: true do
         context 'with a passing selfie' do
           it 'proceeds to the next page with valid info, including a selfie image' do
             perform_in_browser(:mobile) do
@@ -463,9 +455,6 @@
             # when there are multiple doc auth errors on front and back
             it 'shows the correct error message for the given error' do
               perform_in_browser(:mobile) do
-                if page.current_path == idv_how_to_verify_path
-                  click_button t('forms.buttons.continue_online')
-                end
                 use_id_image('ial2_test_credential_multiple_doc_auth_failures_both_sides.yml')
                 click_continue
                 click_button 'Take photo'