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.

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

core: create flag to prevent fatal error on bad status code #15494

Merged
merged 13 commits into from
Oct 20, 2023
Merged

core: create flag to prevent fatal error on bad status code #15494

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6043bf7
warn for a page that returns with a 404
adrianaixba Sep 6, 2023
ee725e4
Merge branch 'main' into not-found
adrianaixba Sep 26, 2023
36daa38
create ignore-status-code flag
adrianaixba Oct 9, 2023
46d9b28
updates
adrianaixba Oct 10, 2023
2f5c676
updates pt2
adrianaixba Oct 10, 2023
352e661
include all error statuses
adrianaixba Oct 10, 2023
9282be7
Merge branch 'main' into not-found
adrianaixba Oct 10, 2023
1674f51
Merge branch 'main' into not-found
adrianaixba Oct 16, 2023
b413f72
adjust message
adrianaixba Oct 16, 2023
079bbc8
add code to warning message
adrianaixba Oct 19, 2023
ee48649
Merge branch 'main' into not-found
adrianaixba Oct 19, 2023
4dcea8f
oopsies fix
adrianaixba Oct 19, 2023
b5062f8
fix
adrianaixba Oct 19, 2023
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
Next Next commit
warn for a page that returns with a 404
  • Loading branch information
@adrianaixba
adrianaixba committed Sep 6, 2023
commit 6043bf7025f7d8e64ae6a7e774c331ecc2225342
12 changes: 10 additions & 2 deletions core/lib/navigation-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ const UIStrings = {
*/
warningXhtml:
'The page MIME type is XHTML: Lighthouse does not explicitly support this document type',
/**
* Warning shown in report when the page under test responds with a 404, which Lighthouse is not able to reliably load
* so we display a warning.
*/
warning404: 'The page returns a 404. Lighthouse is unable to reliably load this page.',
};

const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);
Expand All @@ -27,9 +32,10 @@ const XHTML_MIME_TYPE = 'application/xhtml+xml';
/**
* Returns an error if the original network request failed or wasn't found.
* @param {LH.Artifacts.NetworkRequest|undefined} mainRecord
* @param {{warnings: Array<string | LH.IcuMessage>}} context
* @return {LH.LighthouseError|undefined}
*/
function getNetworkError(mainRecord) {
function getNetworkError(mainRecord, context) {
if (!mainRecord) {
return new LighthouseError(LighthouseError.errors.NO_DOCUMENT_REQUEST);
} else if (mainRecord.failed) {
Expand All @@ -46,6 +52,8 @@ function getNetworkError(mainRecord) {
return new LighthouseError(
LighthouseError.errors.FAILED_DOCUMENT_REQUEST, {errorDetails: netErr});
}
} else if (mainRecord.statusCode === 404) {
context.warnings.push(str_(UIStrings.warning404));
} else if (mainRecord.hasErrorStatusCode()) {
Copy link
Member

Choose a reason for hiding this comment

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

We should generalize this to all error status codes (4xx/5xx). So I think it's better to use this mainRecord.hasErrorStatusCode() block instead of doing a === 404 check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

does generalizing this to include all error status codes change our initial thoughts on the defaults? or do they feel good still?

  • devtools - ignore status code
  • node - fail on status code
  • cli - fail on status code
  • etc

Copy link
Member

Choose a reason for hiding this comment

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

I think the defaults should be the same as we discussed

return new LighthouseError(LighthouseError.errors.ERRORED_DOCUMENT_REQUEST, {
statusCode: `${mainRecord.statusCode}`,
Expand Down Expand Up @@ -144,7 +152,7 @@ function getPageLoadError(navigationError, context) {
context.warnings.push(str_(UIStrings.warningXhtml));
}

const networkError = getNetworkError(mainRecord);
const networkError = getNetworkError(mainRecord, context);
const interstitialError = getInterstitialError(mainRecord, networkRecords);
const nonHtmlError = getNonHtmlError(finalRecord);

Expand Down
21 changes: 14 additions & 7 deletions core/test/lib/navigation-error-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('#getNetworkError', () => {
* @param {NetworkRequest=} mainRecord
*/
function getAndExpectError(mainRecord) {
const error = getNetworkError(mainRecord);
const error = getNetworkError(mainRecord, {warnings: []});
if (!error) throw new Error('expected a network error');
return error;
}
Expand All @@ -42,7 +42,7 @@ describe('#getNetworkError', () => {
const url = 'http://the-page.com';
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
expect(getNetworkError(mainRecord)).toBeUndefined();
expect(getNetworkError(mainRecord, {warnings: []})).toBeUndefined();
});

it('fails when page fails to load', () => {
Expand All @@ -66,15 +66,22 @@ describe('#getNetworkError', () => {
expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load/);
});

it('fails when page returns with a 404', () => {
it('warns when page returns with a 404', () => {
const url = 'http://the-page.com';
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const error = getAndExpectError(mainRecord);
expect(error.message).toEqual('ERRORED_DOCUMENT_REQUEST');
expect(error.code).toEqual('ERRORED_DOCUMENT_REQUEST');
expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load.*404/);
const context = {
url,
networkRecords: [mainRecord],
warnings: [],
loadFailureMode: LoadFailureMode.warn,
};

const error = getPageLoadError(undefined, context);
expect(error).toBeUndefined();
expect(context.warnings[0]).toBeDisplayString(
'The page returns a 404. Lighthouse is unable to reliably load this page.');
});

it('fails when page returns with a 500', () => {
Expand Down