Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist
setAutoSubmit(true);
setImmediate(() => {
// continue with navigation
e.target.disabled = false;
e.target.click();
// allow process to be re-triggered in case submission did not work as expected
setAutoSubmit(false);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Button } from '@18f/identity-components';
import { SpinnerButton } from '@18f/identity-spinner-button';
import { useI18n } from '@18f/identity-react-i18n';

interface LocationCollectionItemProps {
distance?: string;
formattedCityStateZip: string;
handleSelect: (event: React.FormEvent<HTMLInputElement>, selection: number) => void;
handleSelect: (event: React.MouseEvent, selection: number) => void;
name?: string;
saturdayHours: string;
selectId: number;
Expand Down Expand Up @@ -60,26 +60,24 @@ function LocationCollectionItem({
{`${t('in_person_proofing.body.location.retail_hours_sun')} ${sundayHours}`}
</div>
)}
<Button
id={`location_button_mobile_${selectId}`}
<SpinnerButton
className="tablet:display-none margin-top-2 width-full"
onClick={(event) => handleSelect(event, selectId)}
type="submit"
>
{t('in_person_proofing.body.location.location_button')}
</Button>
</SpinnerButton>
</div>
<div className="grid-col-auto">
<Button
id={`location_button_desktop_${selectId}`}
<SpinnerButton
className="display-none tablet:display-inline-block"
onClick={(event) => {
handleSelect(event, selectId);
}}
type="submit"
>
{t('in_person_proofing.body.location.location_button')}
</Button>
</SpinnerButton>
</div>
</div>
</div>
Expand Down
25 changes: 15 additions & 10 deletions spec/features/idv/in_person_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@
search_for_post_office

# location page
bethesda_location = page.find_all('.location-collection-item')[1]
bethesda_location.click_button(t('in_person_proofing.body.location.location_button'))
within page.first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
end

# prepare page
complete_prepare_step(user)
Expand Down Expand Up @@ -125,8 +126,9 @@
expect_in_person_step_indicator_current_step(t('step_indicator.flows.idv.find_a_post_office'))
expect(page).to have_content(t('in_person_proofing.headings.po_search.location'))
search_for_post_office
bethesda_location = page.find_all('.location-collection-item')[1]
bethesda_location.click_button(t('in_person_proofing.body.location.location_button'))
within page.first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
end

# prepare page
expect_in_person_step_indicator_current_step(t('step_indicator.flows.idv.find_a_post_office'))
Expand Down Expand Up @@ -260,8 +262,9 @@
# location page
expect(page).to have_content(t('in_person_proofing.headings.po_search.location'))
search_for_post_office
bethesda_location = page.find_all('.location-collection-item')[1]
bethesda_location.click_button(t('in_person_proofing.body.location.location_button'))
within page.first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
end

# prepare page
expect(page).to have_content(t('in_person_proofing.headings.prepare'))
Expand Down Expand Up @@ -340,8 +343,9 @@

click_link t('in_person_proofing.body.cta.button')
search_for_post_office
bethesda_location = page.find_all('.location-collection-item')[1]
bethesda_location.click_button(t('in_person_proofing.body.location.location_button'))
within page.first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
end

click_idv_continue

Expand Down Expand Up @@ -500,8 +504,9 @@
search_for_post_office

# location page
location = page.find_all('.location-collection-item')[1]
location.click_button(t('in_person_proofing.body.location.location_button'))
within page.first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
end

# prepare page
complete_prepare_step(user)
Expand Down
16 changes: 14 additions & 2 deletions spec/features/users/sign_up_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,19 @@
page.driver.browser.execute_cdp('Browser.resetPermissions')
end

def clipboard_text
# `evaluate_async_script` is expected to be asynchronous, but internally it sets the browser
# script timeout based on Capybara's configured default wait time. Allow for delay in this
# asynchronous result while avoiding modifying the default otherwise.
#
# See: https://github.com/teamcapybara/capybara/blob/3.38.0/lib/capybara/selenium/driver.rb#L146
original_default_max_wait_time = Capybara.default_max_wait_time
Capybara.default_max_wait_time = 5
result = page.evaluate_async_script('navigator.clipboard.readText().then(arguments[0])')
Capybara.default_max_wait_time = original_default_max_wait_time
result
end

context 'user enters their email as their password', email: true do
it 'treats it as a weak password' do
email = 'test@test.com'
Expand All @@ -169,11 +182,10 @@
did_validate_name = -> { name.evaluate_script('this.didValidate') }

click_on t('components.clipboard_button.label')
copied_text = page.evaluate_async_script('navigator.clipboard.readText().then(arguments[0])')
expect(did_validate_name.call).to_not eq true

otp_input = page.find('.one-time-code-input__input')
otp_input.set(generate_totp_code(copied_text))
otp_input.set(generate_totp_code(clipboard_text))
click_button 'Submit'
expect(did_validate_name.call).to eq true

Expand Down
2 changes: 1 addition & 1 deletion spec/support/capybara.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@

Capybara.server = :puma, { Silent: true }

Capybara.default_max_wait_time = (ENV['CAPYBARA_WAIT_TIME_SECONDS'] || '0.5').to_f
Capybara.default_max_wait_time = 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the capacity to set a longer wait time locally with an environment variable? I'm doing a full feature test run locally on this branch, and I saw some failures go by on document capture, which already gives the user a lot of feedback about the process taking a while.

Just finished! (45 minutes altogether) Failed examples:

rspec ./spec/features/accessibility/idv_pages_spec.rb:65 # Accessibility on IDV pages IDV pages doc auth steps accessibility on mobile
rspec './spec/features/idv/steps/phone_step_spec.rb[1:10:2:2:1]' # idv phone step after the max number of attempts behaves like verification step max attempts after completing one less than the max attempts allows the user to continue if their last attempt is successful
rspec ./spec/features/idv/steps/gpo_step_spec.rb:92 # idv gpo step Verified resets password, requests GPO, then signs in using SP shows the user the GPO code entry screen
rspec ./spec/features/users/sign_in_spec.rb:260 # Sign in session approaches timeout user sees warning before session times out
rspec ./spec/features/users/sign_in_spec.rb:279 # Sign in session approaches timeout user can continue browsing
rspec ./spec/features/idv/in_person_spec.rb:119 # In Person Proofing works for a happy path
rspec ./spec/features/idv/in_person_spec.rb:245 # In Person Proofing allows the user to cancel and start over from the beginning
rspec ./spec/features/idv/in_person_spec.rb:258 # In Person Proofing allows the user to go back to document capture from prepare step
rspec ./spec/features/idv/in_person_spec.rb:406 # In Person Proofing verify address by mail (GPO letter) lets the user clear and start over from gpo confirmation
rspec ./spec/features/idv/in_person_spec.rb:376 # In Person Proofing verify address by mail (GPO letter) requires address verification before showing instructions
rspec ./spec/features/idv/in_person_spec.rb:426 # In Person Proofing transliteration shows validation errors
rspec ./spec/features/idv/in_person_spec.rb:501 # In Person Proofing validate_id_and_residential_addresses feature flag enabled captures the address, address line 2, city, state and zip code
rspec './spec/features/idv/in_person_spec.rb[1:5:1:1]' # In Person Proofing after in-person proofing is completed and passed for a partner using oidc sends a survey when they share information with that partner
rspec './spec/features/idv/in_person_spec.rb[1:5:2:1]' # In Person Proofing after in-person proofing is completed and passed for a partner using saml sends a survey when they share information with that partner
rspec ./spec/features/idv/in_person_spec.rb:23 # In Person Proofing ThreatMetrix review pending allows the user to continue down the happy path
rspec ./spec/features/idv/in_person_spec.rb:325 # In Person Proofing with hybrid document capture resumes desktop session with in-person proofing
rspec ./spec/features/idv/doc_capture/document_capture_step_spec.rb:75 # doc capture document capture step does not advance original session with errors
rspec ./spec/features/idv/doc_capture/document_capture_step_spec.rb:61 # doc capture document capture step advances original session once complete
rspec ./spec/features/idv/doc_capture/document_capture_step_spec.rb:108 # doc capture document capture step when using async uploads advances original session once complete
rspec ./spec/features/idv/doc_capture/document_capture_step_spec.rb:125 # doc capture document capture step when using async uploads does not advance original session with errors
rspec ./spec/features/idv/doc_capture/document_capture_step_spec.rb:149 # doc capture document capture step when using async uploads with attention with barcode result advances original session only after confirmed
rspec ./spec/features/idv/doc_capture/document_capture_step_spec.rb:91 # doc capture document capture step with attention with barcode result advances original session only after confirmed
rspec ./spec/features/two_factor_authentication/sign_in_spec.rb:63 # Two Factor Authentication When the user has not set up 2FA with international phone that does not support voice delivery updates international code as user types

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide more detail about those errors? I had initially encountered some failures in my environment because of configuration in my local config/application.yml was taking precedent, but they went away after resetting my configuration, and weren't related to wait timeouts.

The description of the pull request explains why I think the default wait is an anti-pattern. Allowing this to be configurable would imply that there are instances where we want to tolerate the application to be unresponsive in areas that we aren't explicitly expecting it to be unresponsive. Setting this to a single value also avoids inconsistencies like where previously CI was more lax than local development, which contributed to flakiness of tests, difficulty in reproducing them, and lack of clarity that a distinction existed at all between CI and local development.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emailed error output as far back as my terminal history goes.

I understand the reasoning behind changing it to 0 on CI/CD. And I think we need to be able to run feature tests locally.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest changing the default from 0.5 to 0 without hard-coding a magic number replacing the variable. Those of us with inconsistently failing tests still need this knob to twiddle as we attempt to improve the problem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jskinne3 Could you share some detail about the specs you're seeing failing on this branch? My hope is that these changes address all of the intermittent failings related to this setting and normalize the behavior across systems to prevent the possibility of any "wait"-related intermittent failures from being introduced in the future.

@jskinne3 jskinne3 Mar 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I started a bundle exec make_test on this branch. I'll email you the output when finished.
UPDATE: I sent you test output with a bottom line of: 7791 examples, 1192 failures, 7 pendings

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending the test output. Reviewing the errors, I don't see any which strike me as being related to this configuration. I still think a non-configurable zero timeout is the best option and I'd worry if the configuration could distract developers from other root causes of a test failure, but I'll reintroduce the environment variable if you expect that it could give some relief to issues you're encountering.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soniaconnolly @jskinne3 I added the environment variable back in ebd4c5e. Can you please take a look?

Capybara.asset_host = ENV['RAILS_ASSET_HOST'] || 'http://localhost:3000'
Capybara.automatic_label_click = true # USWDS styles native checkbox/radio as offscreen
Capybara.enable_aria_label = true
Expand Down
11 changes: 5 additions & 6 deletions spec/support/features/doc_auth_helper.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
require_relative 'document_capture_step_helper'
require_relative 'interaction_helper'

module DocAuthHelper
include InteractionHelper
include DocumentCaptureStepHelper

GOOD_SSN = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn]
Expand Down Expand Up @@ -81,7 +83,7 @@ def complete_doc_auth_steps_before_welcome_step(expect_accessible: false)
end

def complete_welcome_step
click_on t('doc_auth.buttons.continue')
click_spinner_button_and_wait t('doc_auth.buttons.continue')
end

def complete_doc_auth_steps_before_agreement_step(expect_accessible: false)
Expand All @@ -91,11 +93,7 @@ def complete_doc_auth_steps_before_agreement_step(expect_accessible: false)
end

def complete_agreement_step
find(
'label',
text: t('doc_auth.instructions.consent', app_name: APP_NAME),
wait: 5,
).click
find('label', text: t('doc_auth.instructions.consent', app_name: APP_NAME)).click
click_on t('doc_auth.buttons.continue')
end

Expand Down Expand Up @@ -126,6 +124,7 @@ def complete_document_capture_step_with_yml(proofing_yml)
attach_file I18n.t('doc_auth.headings.document_capture_front'), File.expand_path(proofing_yml)
attach_file I18n.t('doc_auth.headings.document_capture_back'), File.expand_path(proofing_yml)
click_on I18n.t('forms.buttons.submit.default')
expect(page).to have_current_path(idv_ssn_url, wait: 10)
end

def complete_doc_auth_steps_before_email_sent_step
Expand Down
4 changes: 0 additions & 4 deletions spec/support/features/idv_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ def click_idv_send_security_code
click_spinner_button_and_wait t('forms.buttons.send_one_time_code')
end

def click_idv_select
click_select_button_and_wait t('in_person_proofing.body.location.location_button')
end

def click_try_again
page.find(
'a',
Expand Down
11 changes: 4 additions & 7 deletions spec/support/features/in_person_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,13 @@ def search_for_post_office

def complete_location_step(_user = nil)
search_for_post_office
first('.location-collection-item').
click_button(t('in_person_proofing.body.location.location_button'))
within first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
end
end

def complete_prepare_step(_user = nil)
# Wait for page to load before clicking continue
expect(page).to have_content(
t('in_person_proofing.headings.prepare'),
)
click_link t('forms.buttons.continue')
click_spinner_button_and_wait t('forms.buttons.continue')
end

def complete_state_id_step(_user = nil, same_address_as_id: true, include_address: false)
Expand Down
13 changes: 7 additions & 6 deletions spec/support/features/interaction_helper.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
module InteractionHelper
def click_spinner_button_and_wait(...)
click_on(...)
expect(page).to have_no_css('lg-spinner-button.spinner-button--spinner-active', wait: 10)
end

def click_select_button_and_wait(...)
click_button(...)
expect(page).to have_no_css('button.usa-button', wait: 10)
begin
expect(page).to have_no_css('lg-spinner-button.spinner-button--spinner-active', wait: 10)
rescue Selenium::WebDriver::Error::StaleElementReferenceError
# A stale element error can occur when attempting to wait for the spinner to disappear if the
# context in which the button was clicked (e.g. a `within` block) itself disappears. This is
# fine, since if the ancestor disappears, it can be assumed that the button is gone too.
end
end
end