Skip to content

Commit

Permalink
core(gather-runner): include error status codes in pageLoadError
Browse files Browse the repository at this point in the history
  • Loading branch information
brendankenny committed Sep 18, 2018
1 parent 8e228bd commit dcb468a
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 8 deletions.
3 changes: 3 additions & 0 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ class GatherRunner {
} else if (mainRecord.failed) {
errorCode = LHError.errors.FAILED_DOCUMENT_REQUEST;
errorReason = mainRecord.localizedFailDescription;
} else if (mainRecord.hasErrorStatusCode()) {
errorCode = LHError.errors.ERROR_STATUS_CODE_RESPONSE;
errorReason = `Request returned with status code ${mainRecord.statusCode}`;
}

if (errorCode) {
Expand Down
5 changes: 5 additions & 0 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ const ERRORS = {
message: strings.pageLoadFailed,
lhrRuntimeError: true,
},
ERROR_STATUS_CODE_RESPONSE: {

This comment has been minimized.

Copy link
@paulirish

paulirish Sep 18, 2018

Member

naming bikeshed time!?

ERRORED_DOCUMENT_REQUEST ? only because we have NO_DOCUMENT_REQUEST and
FAILED_DOCUMENT_REQUEST.

if we didn't i think DOCUMENT_HTTP_ERROR would be pretty good


also can you add a comment that clarifies that FAILED_DOCUMENT_REQUEST is when devtools reports the LoadingFailed, which is a chrome-internal issue (usually), whereas this is about 4xx and 5xx errors.

code: 'ERROR_STATUS_CODE_RESPONSE',
message: strings.pageLoadFailed,
lhrRuntimeError: true,
},

// Protocol internal failures
TRACING_ALREADY_STARTED: {
Expand Down
7 changes: 7 additions & 0 deletions lighthouse-core/lib/network-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ module.exports = class NetworkRequest {
this.isLinkPreload = false;
}

/**
* @return {boolean}
*/
hasErrorStatusCode() {
return this.statusCode >= 400;
}

/**
* @param {NetworkRequest} initiator
*/
Expand Down
42 changes: 34 additions & 8 deletions lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const GatherRunner = require('../../gather/gather-runner');
const assert = require('assert');
const Config = require('../../config/config');
const unresolvedPerfLog = require('./../fixtures/unresolved-perflog.json');
const NetworkRequest = require('../../lib/network-request.js');

class TestGatherer extends Gatherer {
constructor() {
Expand Down Expand Up @@ -606,31 +607,56 @@ describe('GatherRunner', function() {
describe('#getPageLoadError', () => {
it('passes when the page is loaded', () => {
const url = 'http://the-page.com';
const records = [{url}];
assert.ok(!GatherRunner.getPageLoadError(url, records));
const mainRecord = new NetworkRequest();
mainRecord.url = url;
assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord]));
});

it('passes when the page is loaded, ignoring any fragment', () => {
const url = 'http://example.com/#/page/list';
const records = [{url: 'http://example.com'}];
assert.ok(!GatherRunner.getPageLoadError(url, records));
const mainRecord = new NetworkRequest();
mainRecord.url = 'http://example.com';
assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord]));
});

it('throws when page fails to load', () => {
it('fails when page fails to load', () => {
const url = 'http://the-page.com';
const records = [{url, failed: true, localizedFailDescription: 'foobar'}];
const error = GatherRunner.getPageLoadError(url, records);
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
const error = GatherRunner.getPageLoadError(url, [mainRecord]);
assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST');
assert.ok(/Your page failed to load/.test(error.friendlyMessage));
});

it('throws when page times out', () => {
it('fails when page times out', () => {
const url = 'http://the-page.com';
const records = [];
const error = GatherRunner.getPageLoadError(url, records);
assert.equal(error.message, 'NO_DOCUMENT_REQUEST');
assert.ok(/Your page failed to load/.test(error.friendlyMessage));
});

it('fails when page returns with a 404', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const error = GatherRunner.getPageLoadError(url, [mainRecord]);
assert.equal(error.message, 'ERROR_STATUS_CODE_RESPONSE');
assert.ok(/Your page failed to load/.test(error.friendlyMessage));
});

it('fails when page returns with a 500', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 500;
const error = GatherRunner.getPageLoadError(url, [mainRecord]);
assert.equal(error.message, 'ERROR_STATUS_CODE_RESPONSE');
assert.ok(/Your page failed to load/.test(error.friendlyMessage));
});
});

describe('artifact collection', () => {
Expand Down

0 comments on commit dcb468a

Please sign in to comment.