Skip to content

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

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

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 404 status code, 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 @@ -175,6 +175,7 @@ async function _computeNavigationResult(
? getPageLoadError(navigationError, {
url: mainDocumentUrl,
loadFailureMode: navigationContext.navigation.loadFailureMode,
ignoreStatusCode: navigationContext.resolvedConfig.settings.ignoreStatusCode,
networkRecords: debugData.records,
warnings,
})
Expand Down
16 changes: 13 additions & 3 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',
/**
* 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: '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: 404)',
};

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 @@ -46,6 +54,8 @@ function getNetworkError(mainRecord) {
return new LighthouseError(
LighthouseError.errors.FAILED_DOCUMENT_REQUEST, {errorDetails: netErr});
}
} else if (mainRecord.statusCode === 404 && context.ignoreStatusCode) {
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 @@ -113,7 +123,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, loadFailureMode: LH.Config.SharedPassNavigationJson['loadFailureMode'], networkRecords: Array<LH.Artifacts.NetworkRequest>, warnings: Array<string | LH.IcuMessage>}} context
* @param {{url: string, loadFailureMode: LH.Config.SharedPassNavigationJson['loadFailureMode'], 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 +154,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
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,15 +67,41 @@ 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', () => {
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
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