Skip to content
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.

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

Raise Ferrum::StatusError for any top frame navigation error #341

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Raise Ferrum::StatusError for any top frame navigation error #341

merged 1 commit into from
Feb 16, 2023

Conversation

francisbeaudoin
Copy link
Contributor

@francisbeaudoin francisbeaudoin commented Feb 15, 2023

Context

The Page#go_to is only raising a Ferrum::StatusError for a subset of exceptions:

ferrum/lib/ferrum/page.rb

Lines 113 to 124 in 0cab51d

def go_to(url = nil)
options = { url: combine_url!(url) }
options.merge!(referrer: referrer) if referrer
response = command("Page.navigate", wait: GOTO_WAIT, **options)
# https://cs.chromium.org/chromium/src/net/base/net_error_list.h
if %w[net::ERR_NAME_NOT_RESOLVED
net::ERR_NAME_RESOLUTION_FAILED
net::ERR_INTERNET_DISCONNECTED
net::ERR_CONNECTION_TIMED_OUT
net::ERR_FILE_NOT_FOUND].include?(response["errorText"])
raise StatusError, options[:url]
end

Per the Chrome DevTools documentation an errorText is present if and only if navigation has failed.

Proposal

This PR is proposing to raise regardless of the errorText given that the navigation failed.

Warning
This could be considered as a breaking change.

Reference

Discussion: #340

@route route self-requested a review February 16, 2023 07:48
@route route merged commit a6708f9 into rubycdp:main Feb 16, 2023
@route
Copy link
Member

route commented Feb 16, 2023

Thank you! Awesome!

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