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

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

Merged
merged 13 commits into from
Oct 20, 2023
6 changes: 5 additions & 1 deletion cli/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,16 @@ function getYargsParser(manualArgv) {
type: 'boolean',
describe: 'Disables collection of the full page screenshot, which can be quite large',
},
'ignore-status-code': {
type: 'boolean',
describe: 'Disables failing on all error status codes, and instead issues a warning.',
},
})
.group([
'save-assets', 'list-all-audits', 'list-locales', 'list-trace-categories', 'additional-trace-categories',
'config-path', 'preset', 'chrome-flags', 'port', 'hostname', 'form-factor', 'screenEmulation', 'emulatedUserAgent',
'max-wait-for-load', 'enable-error-reporting', 'gather-mode', 'audit-mode',
'only-audits', 'only-categories', 'skip-audits', 'budget-path', 'disable-full-page-screenshot',
'only-audits', 'only-categories', 'skip-audits', 'budget-path', 'disable-full-page-screenshot', 'ignore-status-code',
], 'Configuration:')

// Output
Expand Down
1 change: 1 addition & 0 deletions core/config/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ const defaultSettings = {
disableFullPageScreenshot: false,
skipAboutBlank: false,
blankPage: 'about:blank',
ignoreStatusCode: false,

// the following settings have no defaults but we still want ensure that `key in settings`
// in config will work in a typechecked way
Expand Down
1 change: 1 addition & 0 deletions core/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ async function _computeNavigationResult(
const pageLoadError = debugData.records
? getPageLoadError(navigationError, {
url: mainDocumentUrl,
ignoreStatusCode: navigationContext.resolvedConfig.settings.ignoreStatusCode,
networkRecords: debugData.records,
warnings: navigationContext.baseArtifacts.LighthouseRunWarnings,
})
Expand Down
28 changes: 20 additions & 8 deletions core/lib/navigation-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,18 @@

const UIStrings = {
/**
* Warning shown in report when the page under test is an XHTML document, which Lighthouse does not directly support
* so we display a warning.
* @description Warning shown in report when the page under test is an XHTML document, which Lighthouse does not directly support so we display a warning.
adrianaixba marked this conversation as resolved.
Show resolved Hide resolved
*/
warningXhtml:
'The page MIME type is XHTML: Lighthouse does not explicitly support this document type',
/**
* @description Warning shown in report when the page under test returns an error code, which Lighthouse is not able to reliably load so we display a warning.
* @example {404} errorCode
*
*/
adrianaixba marked this conversation as resolved.
Show resolved Hide resolved
warningStatusCode: 'Lighthouse was unable to reliably load the page you requested. Make sure' +
' you are testing the correct URL and that the server is properly responding' +
' to all requests. (Status code: {errorCode})',
};

const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);
Expand All @@ -27,9 +34,10 @@
/**
* 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>, ignoreStatusCode?: LH.Config.Settings['ignoreStatusCode']}} 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 @@ -47,9 +55,13 @@
LighthouseError.errors.FAILED_DOCUMENT_REQUEST, {errorDetails: netErr});
}
} 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}`,
});
if (context.ignoreStatusCode) {
context.warnings.push(str_(UIStrings.warningStatusCode, {errorCode: mainRecord.statusCode}));

Check warning on line 59 in core/lib/navigation-error.js

View check run for this annotation

Codecov / codecov/patch

core/lib/navigation-error.js#L59

Added line #L59 was not covered by tests
} else {
return new LighthouseError(LighthouseError.errors.ERRORED_DOCUMENT_REQUEST, {
statusCode: `${mainRecord.statusCode}`,
});
}
}
}

Expand Down Expand Up @@ -113,7 +125,7 @@
* Returns an error if the page load should be considered failed, e.g. from a
* main document request failure, a security issue, etc.
* @param {LH.LighthouseError|undefined} navigationError
* @param {{url: string, networkRecords: Array<LH.Artifacts.NetworkRequest>, warnings: Array<string | LH.IcuMessage>}} context
* @param {{url: string, ignoreStatusCode?: LH.Config.Settings['ignoreStatusCode'], networkRecords: Array<LH.Artifacts.NetworkRequest>, warnings: Array<string | LH.IcuMessage>}} context
* @return {LH.LighthouseError|undefined}
*/
function getPageLoadError(navigationError, context) {
Expand Down Expand Up @@ -144,7 +156,7 @@
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
2 changes: 2 additions & 0 deletions core/scripts/i18n/collect-strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,8 @@ function checkKnownFixedCollisions(strings) {
'Document has a valid $MARKDOWN_SNIPPET_0$',
'Failing Elements',
'Failing Elements',
'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: $ICU_0$)',
'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: $ICU_0$)',
'Name',
'Name',
'Pages that use portals are not currently eligible for back/forward cache.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": true,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": false,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": false,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": true,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3753,6 +3753,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": true,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down Expand Up @@ -10677,6 +10678,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": false,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down Expand Up @@ -14798,6 +14800,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": false,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down Expand Up @@ -21359,6 +21362,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": true,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": null,
"locale": "en-US",
"blockedUrlPatterns": null,
Expand Down
41 changes: 34 additions & 7 deletions core/test/lib/navigation-error-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ function makeNetworkRequest() {
describe('#getNetworkError', () => {
/**
* @param {NetworkRequest=} mainRecord
* @param {{warnings: Array<string | LH.IcuMessage>, ignoreStatusCode?: LH.Config.Settings['ignoreStatusCode']}=} context
*/
function getAndExpectError(mainRecord) {
const error = getNetworkError(mainRecord);
function getAndExpectError(mainRecord, context) {
const error = getNetworkError(mainRecord, {warnings: [], ...context});
if (!error) throw new Error('expected a network error');
return error;
}
Expand All @@ -42,7 +43,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,18 +67,44 @@ 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 with flag', () => {
const url = 'http://the-page.com';
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const error = getAndExpectError(mainRecord);
const context = {
url,
networkRecords: [mainRecord],
warnings: [],
loadFailureMode: LoadFailureMode.warn,
ignoreStatusCode: true,
};

const error = getNetworkError(mainRecord, context);
expect(error).toBeUndefined();
expect(context.warnings[0]).toBeDisplayString(/^Lighthouse was unable to reliably load/);
});

it('fails when page returns with a 404 without flag', () => {
const url = 'http://the-page.com';
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const context = {
url,
networkRecords: [mainRecord],
warnings: [],
loadFailureMode: LoadFailureMode.warn,
};

const error = getAndExpectError(mainRecord, context);
expect(error).toBeDefined();
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/);
expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load/);
Copy link
Member

Choose a reason for hiding this comment

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

The 404 is important IMO, otherwise this could pass for one of the non-status code related page load errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also likely be helpful to add a documentStatusCode (or something similar) to track the result easily pragmatically.

Copy link
Member

Choose a reason for hiding this comment

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

runWarnings only accepts string type currently. I think it would be a larger change to allow extra data fields on warnings. Seems out of scope for this PR.

FWIW the bad status code is also tracked by our http-status-code audit which is easier to track programatically.

});

it('fails when page returns with a 500', () => {
it('fails when page returns with a 500 without flag', () => {
const url = 'http://the-page.com';
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
Expand Down
1 change: 1 addition & 0 deletions core/test/results/artifacts/artifacts.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": false,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": [
{
"path": "/",
Expand Down
1 change: 1 addition & 0 deletions core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -5749,6 +5749,7 @@
"disableFullPageScreenshot": false,
"skipAboutBlank": false,
"blankPage": "about:blank",
"ignoreStatusCode": false,
"budgets": [
{
"path": "/",
Expand Down
3 changes: 3 additions & 0 deletions shared/localization/locales/en-US.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions shared/localization/locales/en-XL.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions types/lhr/settings.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ export type ScreenEmulationSettings = {
blankPage?: string;
/** Disables collection of the full page screenshot, which can be rather large and possibly leave the page in an undesirable state. */
disableFullPageScreenshot?: boolean;

/** Disables failing on 404 status code, and instead issues a warning */
ignoreStatusCode?: boolean;
}

export interface ConfigSettings extends Required<SharedFlagsSettings> {
Expand Down
Loading