Skip to content

Fail JavaScript feature specs if any console messages are logged#5492

Merged
aduth merged 10 commits intomainfrom
aduth-capture-browser-logging
Oct 26, 2021
Merged

Fail JavaScript feature specs if any console messages are logged#5492
aduth merged 10 commits intomainfrom
aduth-capture-browser-logging

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 12, 2021

Extracted from: #5481 (631d27efd42b0e314cbc9de87844cf2d01e6b3ec)

Why: Browser logging is unexpected, and should be treated as a failure, similar to what we already do with Mocha-based JavaScript tests via useConsoleLogSpy. Often, logging will indicate some failing of the JavaScript execution, which may or may not fail subsequent spec assertions. Raising the logging will often improve clarity of debugging the root cause.

@aduth
Copy link
Contributor Author

aduth commented Oct 12, 2021

The test failure is happening at the document capture step, where we use React Suspense + error boundary to handle the polling (source). So, in a sense, the logging is expected, though I've wondered if there's a way we can handle it more gracefully. Otherwise, we'd need to exempt this screen from the checking.

Related: facebook/react#15069

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

end

config.after(:each, type: :feature, js: true) do |spec|
javascript_errors = page.driver.browser.manage.logs.get(:browser).map(&:message)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we do some sort like next if spec.metadata[:allow_js_error] and then add allow_js_error: true to the polling spec mentioned in #5492 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we do some sort like next if spec.metadata[:allow_js_error] and then add allow_js_error: true to the polling spec mentioned in #5492 (comment)

Yeah, I like that idea. It's a bit trickier though, since any spec which would include document capture as part of its prerequisite setup would fail (e.g. complete_doc_auth_steps_before_verify_step), so it's not very self-contained. That being said, I only see 3 failures currently in Circle, so maybe it's fine enough.

Alternatively, we could target the offending script here as being allowed to log errors.

next if javascript_errors.reject { |e| e.include? 'js/document-capture-' }.blank?

Feels pretty gross, but also I'm really thinking maybe to refactor the document capture polling to not rely on these errors, so could be a temporary band-aid fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had an idea that inside thecomplete_doc_auth_steps_before_verify_step we could modify spec.metadata[:allow_js_error] = true ... however, ran into this error: :[

`metadata` is not available from within an example (e.g. an `it` block) or from
constructs that run in the scope of an example (e.g. `before`, `let`, etc). It is only
available on an example group (e.g. a `describe` or `context` block).

@aduth
Copy link
Contributor Author

aduth commented Oct 12, 2021

Many of the remaining failures smell of actual issues to me:

  1) doc auth link sent step behaves like with doc capture polling enabled automatically advances when the mobile flow is complete
     Failure/Error: raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present?
     
     BrowserConsoleLogError:
       Unexpected browser console logging:
     
       http://127.0.0.1:41039/verify/doc_auth/link_sent - Blocked attempt to show a 'beforeunload' confirmation panel for a frame that never had a user gesture since its load. https://www.chromestatus.com/feature/5082396709879808
     Shared Example Group: "with doc capture polling enabled" called from ./spec/features/idv/doc_auth/link_sent_step_spec.rb:134
     # ./spec/rails_helper.rb:116:in `block (2 levels) in <top (required)>'
     # ./spec/rails_helper.rb:105:in `block (2 levels) in <top (required)>'

The onbeforeunload should be unbound before the redirect happens (source).

  2) Accessibility on static pages 500 page
     Failure/Error: raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present?
     
     BrowserConsoleLogError:
       Unexpected browser console logging:
     
       http://127.0.0.1:41039/non_existent_page - Failed to load resource: the server responded with a status of 404 (Not Found)
     # ./spec/rails_helper.rb:116:in `block (2 levels) in <top (required)>'
     # ./spec/rails_helper.rb:105:in `block (2 levels) in <top (required)>'

Why is the 500 page test visiting this 404 page?

  3) Accessibility on pages that require authentication edit email page
     Failure/Error: raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present?
     
     BrowserConsoleLogError:
       Unexpected browser console logging:
     
       http://127.0.0.1:41039/manage/email/712 - Failed to load resource: the server responded with a status of 404 (Not Found)
     # ./spec/rails_helper.rb:116:in `block (2 levels) in <top (required)>'
     # ./spec/rails_helper.rb:105:in `block (2 levels) in <top (required)>'

It's 404ing when trying to delete the sampled email address?

  4) doc auth test credentials allows proofing with test credentials
     Failure/Error: raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present?
     
     BrowserConsoleLogError:
       Unexpected browser console logging:
     
       http://127.0.0.1:41039/verify/doc_auth/verify_document_status - Failed to load resource: the server responded with a status of 400 (Bad Request)
     # ./spec/rails_helper.rb:116:in `block (2 levels) in <top (required)>'
     # ./spec/features/idv/doc_auth/test_credentials_spec.rb:15:in `block (3 levels) in <top (required)>'
     # ./spec/features/idv/doc_auth/test_credentials_spec.rb:14:in `block (2 levels) in <top (required)>'
     # ./spec/rails_helper.rb:105:in `block (2 levels) in <top (required)>'

Shouldn't be hitting a 400 error on this test submission?

aduth added a commit that referenced this pull request Oct 15, 2021
Address separately at: #5492

This reverts commit 631d27efd42b0e314cbc9de87844cf2d01e6b3ec.
aduth added a commit that referenced this pull request Oct 15, 2021
* Extract PhoneInputComponent for reuse

**Why**: Toward supporting LG-5193, to display an international phone input on the hybrid "Send Link" screen.

* Remove redundant if conditions

querySelectorAll always returns an array, which is truthy

* Resolve type error on possibly-null input form

* Flatten iti querying

Only one per element, elements available from iti instance

* Remove wrapper hide class

**Why**: Not needed with no-js. Maybe helps make it be a bit more standalone, but still tied to USWDS. YAGNI for now

* Localize accessible label for country code flag

* Clean up code comments

* Simplify condition

codeWrapper not called in constructor

* Set example number from intlTelInput utils

More reliable than waiting on intlTelInput to set placeholder, e.g. in case of failed server-side validation where value is prepopulated, intlTelInput won't actually set the placeholder

* Import intlTelInput from bare identifier

Points to the same place:

https://github.com/jackocnr/intl-tel-input/blob/fa983deab4d1a671e367c5d4585d7ed58f785932/index.js#L4

* Handle form validation with setCustomValidity

* Move phone_code_label to components locale strings

Because it's only used within the component

* Configure global feature spec console log monitoring

**Why**: So that it's less of a mystery when things break

* Revert to "plain" custom element

**Why**: Need to sort out conflicts with Webpack dynamic imports

See: #5481 (comment)

* Change PhoneInput rootNode type signature to Element

More compatible with querySelector result

* Update sign_in_spec for new phone input class names

* Revert "Configure global feature spec console log monitoring"

Address separately at: #5492

This reverts commit 631d27efd42b0e314cbc9de87844cf2d01e6b3ec.

* Avoid within for select code assignment

**Why**: Because it creates scoped variables which aren't accessible at time of assertions

* Set custom validity on select input

**Why**: Avoid conflicts with multiple custom validations on text input

* Use simple_form_for for OTP delivery preference forms

**Why**: Avoid conflicts of validation handling between form-validation and otp-delivery-preference

* Add disabled states assertions to OTP specs

**Why**: Regression spec for disabling submit on invalid phone numbers

* Remove else branch of asset_host assignment

**Why**: Hopefully fix issue with Webpacker root path being set with double leading slashes ("//packs-test")

See: #5481 (review)

* Revert "Revert to "plain" custom element"

This reverts commit c655059eac6728bb344f749a1a3eccf6fc51f1a9.

* Add regression assertions for example number text

* Consolidate to single "country code" label

See:

- #5481 (comment)
- https://gsa-tts.slack.com/archives/CNCGEHG1G/p1634063857008300

* Hide example number when JS disabled

Because we don't set it, so it's more confusing to be visible

* Fix ERB lint errors

* Update translations
aduth added 8 commits October 26, 2021 11:57
**Why**: So that it's less of a mystery when things break
e.g. 404 page, or specs asserting against an error resposne
     NoMethodError:
       undefined method `manage' for #<Capybara::RackTest::Browser:0x000055e95c6122e0>
Because (a) it's not necessary and (b) it makes console log catching angry because the page will prompt before navigating.
delete_email_path is for the POST submission confirming the deletion, so not something the user would "visit" directly
It was relying on the page refresh. Better to handle it in the test though, since it matches the test description
@aduth aduth force-pushed the aduth-capture-browser-logging branch from 26b004f to e9b5706 Compare October 26, 2021 16:00
aduth added 2 commits October 26, 2021 12:03
Because it may not be originating from JS, and may not be an error
Suspected that messages are being held between specs
@aduth aduth merged commit 7650a8a into main Oct 26, 2021
@aduth aduth deleted the aduth-capture-browser-logging branch October 26, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants