-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(gather): define new DNS failure LH Error #6579
Changes from 5 commits
5b7c403
32476eb
20c104c
e2eb323
803d334
5ac35a0
69e0d49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,8 +150,19 @@ class GatherRunner { | |
if (!mainRecord) { | ||
errorDef = LHError.errors.NO_DOCUMENT_REQUEST; | ||
} else if (mainRecord.failed) { | ||
errorDef = {...LHError.errors.FAILED_DOCUMENT_REQUEST}; | ||
errorDef.message += ` ${mainRecord.localizedFailDescription}.`; | ||
const netErr = mainRecord.localizedFailDescription; | ||
// Match all resolution and DNS failures | ||
// https://cs.chromium.org/chromium/src/net/base/net_error_list.h?rcl=cd62979b | ||
if ( | ||
netErr.startsWith('net::ERR_NAME_NOT_RESOLVED') || | ||
netErr.startsWith('net::ERR_NAME_RESOLUTION_FAILED') || | ||
netErr.startsWith('net::ERR_DNS_') | ||
) { | ||
errorDef = {...LHError.errors.DNS_FAILURE}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if no string/message altering, no need to object spread to get a copy (e.g. see |
||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these conditionals are multiplying in an unseemly fashion. Maybe #6457 can take on cleaning them up with early returns @exterkamp |
||
errorDef = {...LHError.errors.FAILED_DOCUMENT_REQUEST}; | ||
errorDef.message += ` ${netErr}.`; | ||
} | ||
} else if (mainRecord.hasErrorStatusCode()) { | ||
errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST}; | ||
errorDef.message += ` Status code: ${mainRecord.statusCode}.`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,6 +188,13 @@ const ERRORS = { | |
lhrRuntimeError: true, | ||
}, | ||
|
||
// DNS failure on main document (no resolution, broken DNS) | ||
DNS_FAILURE: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. proto update :) |
||
code: 'DNS_FAILURE', | ||
message: strings.dnsFailure, | ||
lhrRuntimeError: true, | ||
}, | ||
|
||
// Hey! When adding a new error type, update lighthouse-result.proto too. | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,8 @@ enum LighthouseError { | |
PROTOCOL_TIMEOUT = 17; | ||
// Used when the page is not responding after maxWaitForLoad. | ||
PAGE_HUNG = 18; | ||
// DNS timed out for the main document request. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handles more cases than just a timeout? |
||
DNS_FAILURE = 19; | ||
} | ||
|
||
// The overarching Lighthouse Response object (LHR) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just test equality for these two? Or are these expected as prefixes?