Conversation
changelog: Internal, Automated Testing, Configure automated tests to more aggressively identify unresponsive user interactions
This should probably be available as an option for a step, but it doesn't work in IPP because IPP relies on frame ticks resulting from the click on the button, since otherwise the state change in setting submit metadata results in the subsequent submit event using a stale version of that metadata state. In earlier iterations here, we changed this to a mutative ref, but refs are not good for use in the public interface of the hook, since updating the ref would not trigger an update to consuming components.
aduth
requested review from
jmax-gsa,
jskinne3,
kbighorse,
mitchellhenke,
soniaconnolly and
zachmargolis
spec/support/capybara.rb
Outdated
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@soniaconnolly @jskinne3 I added the environment variable back in ebd4c5e. Can you please take a look?
It's designed for this purpose, is more readable and succinct
spec/support/capybara.rb
Outdated
| 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 |
There was a problem hiding this comment.
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.
soniaconnolly
left a comment
soniaconnolly
left a comment
There was a problem hiding this comment.
LGTM. Thanks for digging into this. I am still suspicious of needing so many file handles, but that seems to be a separate issue.
5 participants
🛠 Summary of changes
This updates the Capybara
default_max_wait_timeconfiguration to be zero in all cases, disallowing any default waiting in local development or CI environments.As mentioned in Capybara documentation, the purpose of this value is to allow for delays resulting from asynchronous JavaScript. While there are instance where we may need to await the result of some asynchronous behavior, these should be minimized and we should be explicit in our specs where we expect these delays to occur. Failing by default for any delay will force us to acknowledge where these delays exist, and will ideally encourage compensating behaviors in the user experience (e.g. spinner buttons, or optimistic rendering). Otherwise, we risk introducing UIs which may appear to be unresponsive, and likely more severely unresponsive for those using underpowered devices or on slower network connections.
An example of this identified by the updated configuration is the in-person location "Select" button, which awaited the result of a network request before continuing to the next step, without any feedback to the user. Since the updated configuration flagged this, it presented an opportunity to introduce a spinner button in order to give the user some feedback that their interaction is being processed.
📜 Testing Plan