Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 10 commits into from
Oct 26, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b0d8046
Configure global feature spec console log monitoring
aduth Oct 8, 2021
a8c6570
Refactor message customization as custom class
aduth Oct 12, 2021
a7c0780
Temporarily allow document-capture errors
aduth Oct 12, 2021
14810b6
Allow skipping expected browser console errors
aduth Oct 12, 2021
b0aaa14
Only assert console logging on supported browser drivers
aduth Oct 12, 2021
e1ede13
Remove redundant visit
aduth Oct 12, 2021
e2510fd
Fix path for email delete confirmation accessibility
aduth Oct 15, 2021
e9b5706
Increment reload times for JS-disabled doc polling scenario
aduth Oct 15, 2021
2fbabe1
Rename allow_js_error to allow_browser_log
aduth Oct 26, 2021
1ed8d6a
Always get logs
aduth Oct 26, 2021
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
2 changes: 1 addition & 1 deletion spec/features/accessibility/static_pages_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
require 'axe-rspec'

feature 'Accessibility on static pages', :js do
scenario 'not found page' do
scenario 'not found page', allow_browser_log: true do
visit '/non_existent_page'

expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa
Expand Down
4 changes: 2 additions & 2 deletions spec/features/accessibility/user_pages_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,11 @@
expect(page).to be_uniquely_titled
end

scenario 'edit email page' do
scenario 'delete email page' do
user = create(:user)
sign_in_and_2fa_user(user)

visit delete_email_path(id: user.email_addresses.take.id)
visit manage_email_confirm_delete_path(id: user.email_addresses.take.id)

expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa
expect(page).to label_required_fields
Expand Down
10 changes: 1 addition & 9 deletions spec/features/idv/doc_auth/link_sent_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@
metadata[:js] = true
let(:doc_capture_polling_enabled) { true }

before do
visit current_path
end

it 'automatically advances when the mobile flow is complete' do
expect(page).to_not have_css 'meta[http-equiv="refresh"]', visible: false
expect(page).to_not have_button(t('forms.buttons.continue'))
Expand All @@ -100,10 +96,6 @@
shared_examples 'with doc capture polling disabled' do
let(:doc_capture_polling_enabled) { false }

before do
visit current_path
end

context 'clicks back link' do
before do
click_doc_auth_back_link
Expand All @@ -117,7 +109,7 @@
end

it 'refreshes page 4x with meta refresh extending timeout by 40 min and can start over' do
3.times do
4.times do
expect(page).to have_css 'meta[http-equiv="refresh"]', visible: false
visit idv_doc_auth_link_sent_step
end
Expand Down
2 changes: 1 addition & 1 deletion spec/features/idv/doc_auth/test_credentials_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
expect(page).to have_content('Jane')
end

it 'triggers an error if the test credentials have a friendly error' do
it 'triggers an error if the test credentials have a friendly error', allow_browser_log: true do
complete_doc_auth_steps_before_document_capture_step

attach_file(
Expand Down
14 changes: 14 additions & 0 deletions spec/rails_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,18 @@ class Analytics
example.run
Bullet.enable = false
end

config.after(:each, type: :feature, js: true) do |spec|
next unless page.driver.browser.respond_to?(:manage)

# Always get the logs, even if logs are allowed for the spec, since otherwise unexpected
# messages bleed over between specs.
javascript_errors = page.driver.browser.manage.logs.get(:browser).map(&:message)
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Oct 12, 2021

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
Copy Markdown
Contributor Author

@aduth aduth Oct 12, 2021

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
Copy Markdown
Contributor

@zachmargolis zachmargolis Oct 12, 2021

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).

next if spec.metadata[:allow_browser_log]

# Temporarily allow for document-capture bundle, since it uses React error boundaries to poll.
javascript_errors.reject! { |e| e.include? 'js/document-capture-' }
# Consider any browser console logging as a failure.
raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present?
end
end
9 changes: 9 additions & 0 deletions spec/support/browser_console_log_error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class BrowserConsoleLogError < StandardError
def initialize(messages)
@messages = messages
end

def to_s
"Unexpected browser console logging:\n\n#{@messages.join("\n\n")}"
end
end