Skip to content

Commit

Permalink
core: create flag to prevent fatal error on bad status code (#15494)
Browse files Browse the repository at this point in the history
  • Loading branch information
adrianaixba authored Oct 20, 2023
1 parent de41051 commit 9832e96
Show file tree
Hide file tree
Showing 16 changed files with 79 additions and 13 deletions.
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
24 changes: 18 additions & 6 deletions core/lib/navigation-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ const UIStrings = {
*/
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
*/
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 @@ 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>, 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 @@ function getNetworkError(mainRecord) {
LighthouseError.errors.FAILED_DOCUMENT_REQUEST, {errorDetails: netErr});
}
} else if (mainRecord.hasErrorStatusCode()) {
return new LighthouseError(LighthouseError.errors.ERRORED_DOCUMENT_REQUEST, {
statusCode: `${mainRecord.statusCode}`,
});
if (context.ignoreStatusCode) {
context.warnings.push(str_(UIStrings.warningStatusCode, {errorCode: mainRecord.statusCode}));
} else {
return new LighthouseError(LighthouseError.errors.ERRORED_DOCUMENT_REQUEST, {
statusCode: `${mainRecord.statusCode}`,
});
}
}
}

Expand Down Expand Up @@ -113,7 +125,7 @@ function getNonHtmlError(finalRecord) {
* 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 @@ 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
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
4 changes: 4 additions & 0 deletions core/test/fixtures/user-flows/reports/sample-flow-result.json
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
39 changes: 33 additions & 6 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/);
});

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

0 comments on commit 9832e96

Please sign in to comment.