Skip to content

Commit

Permalink
misc: localize logged GatherRunner error (#9291)
Browse files Browse the repository at this point in the history
  • Loading branch information
brendankenny authored and patrickhulce committed Jun 27, 2019
1 parent 1ab2929 commit b671932
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 43 deletions.
6 changes: 5 additions & 1 deletion lighthouse-core/gather/gather-runner.js
Expand Up @@ -12,6 +12,7 @@ const LHError = require('../lib/lh-error.js');
const URL = require('../lib/url-shim.js');
const NetworkRecorder = require('../lib/network-recorder.js');
const constants = require('../config/constants.js');
const i18n = require('../lib/i18n/i18n.js');

/** @typedef {import('../gather/driver.js')} Driver */

Expand Down Expand Up @@ -646,7 +647,10 @@ class GatherRunner {
// In case of load error, save log and trace with an error prefix, return no artifacts for this pass.
const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData, possibleNavError);
if (pageLoadError) {
log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url);
const localizedMessage = i18n.getFormatted(pageLoadError.friendlyMessage,
passContext.settings.locale);
log.error('GatherRunner', localizedMessage, passContext.url);

passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);
GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData,
`pageLoadError-${passConfig.passName}`);
Expand Down
9 changes: 7 additions & 2 deletions lighthouse-core/lib/i18n/i18n.js
Expand Up @@ -183,6 +183,8 @@ const _ICUMsgNotFoundMsg = 'ICU message not found in destination locale';
*/
function _formatIcuMessage(locale, icuMessageId, fallbackMessage, values) {
const localeMessages = LOCALES[locale];
if (!localeMessages) throw new Error(`Unsupported locale '${locale}'`);

const localeMessage = localeMessages[icuMessageId] && localeMessages[icuMessageId].message;
// fallback to the original english message if we couldn't find a message in the specified locale
// better to have an english message than no message at all, in some number cases it won't even matter
Expand Down Expand Up @@ -220,13 +222,16 @@ function _formatPathAsString(pathInLHR) {
* @return {LH.I18NRendererStrings}
*/
function getRendererFormattedStrings(locale) {
const icuMessageIds = Object.keys(LOCALES[locale]).filter(f => f.includes('core/report/html/'));
const localeMessages = LOCALES[locale];
if (!localeMessages) throw new Error(`Unsupported locale '${locale}'`);

const icuMessageIds = Object.keys(localeMessages).filter(f => f.includes('core/report/html/'));
/** @type {LH.I18NRendererStrings} */
const strings = {};
for (const icuMessageId of icuMessageIds) {
const [filename, varName] = icuMessageId.split(' | ');
if (!filename.endsWith('util.js')) throw new Error(`Unexpected message: ${icuMessageId}`);
strings[varName] = LOCALES[locale][icuMessageId].message;
strings[varName] = localeMessages[icuMessageId].message;
}

return strings;
Expand Down
72 changes: 32 additions & 40 deletions lighthouse-core/test/gather/gather-runner-test.js
Expand Up @@ -424,7 +424,7 @@ describe('GatherRunner', function() {
assert.equal(tests.calledCleanBrowserCaches, true);
});

it('returns a pageLoadError and no artifacts with network errors', async () => {
it('returns a pageLoadError and no artifacts when there is a network error', async () => {
const requestedUrl = 'https://example.com';
// This page load error should be overriden by NO_DOCUMENT_REQUEST (for being
// more specific) since requestedUrl does not match any network request in fixture.
Expand All @@ -434,31 +434,27 @@ describe('GatherRunner', function() {
gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError),
});

const passConfig = {
gatherers: [
{instance: new TestGatherer()},
],
};

const settings = {};

const passContext = {
url: requestedUrl,
const config = new Config({
passes: [{
recordTrace: true,
passName: 'firstPass',
gatherers: [{instance: new TestGatherer()}],
}],
});
const options = {
driver,
passConfig,
settings,
LighthouseRunWarnings: [],
baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}),
requestedUrl,
settings: config.settings,
};

const {artifacts, pageLoadError} = await GatherRunner.runPass(passContext);
expect(passContext.LighthouseRunWarnings).toHaveLength(1);
expect(pageLoadError).toBeInstanceOf(Error);
expect(pageLoadError.code).toEqual('NO_DOCUMENT_REQUEST');
expect(artifacts).toEqual({});
const artifacts = await GatherRunner.run(config.passes, options);
expect(artifacts.LighthouseRunWarnings).toHaveLength(1);
expect(artifacts.PageLoadError).toBeInstanceOf(Error);
expect(artifacts.PageLoadError.code).toEqual('NO_DOCUMENT_REQUEST');
expect(artifacts.TestGatherer).toBeUndefined();
});

it('returns a pageLoadError and no artifacts with navigation errors', async () => {
it('returns a pageLoadError and no artifacts when there is a navigation error', async () => {
const requestedUrl = 'https://example.com';
// This time, NO_FCP should win because it's the only error left.
const navigationError = new LHError(LHError.errors.NO_FCP);
Expand All @@ -470,28 +466,24 @@ describe('GatherRunner', function() {
},
});

const passConfig = {
gatherers: [
{instance: new TestGatherer()},
],
};

const settings = {};

const passContext = {
url: requestedUrl,
const config = new Config({
passes: [{
recordTrace: true,
passName: 'firstPass',
gatherers: [{instance: new TestGatherer()}],
}],
});
const options = {
driver,
passConfig,
settings,
LighthouseRunWarnings: [],
baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}),
requestedUrl,
settings: config.settings,
};

const {artifacts, pageLoadError} = await GatherRunner.runPass(passContext);
expect(passContext.LighthouseRunWarnings).toHaveLength(1);
expect(pageLoadError).toBeInstanceOf(Error);
expect(pageLoadError.code).toEqual('NO_FCP');
expect(artifacts).toEqual({});
const artifacts = await GatherRunner.run(config.passes, options);
expect(artifacts.LighthouseRunWarnings).toHaveLength(1);
expect(artifacts.PageLoadError).toBeInstanceOf(Error);
expect(artifacts.PageLoadError.code).toEqual('NO_FCP');
expect(artifacts.TestGatherer).toBeUndefined();
});

it('does not clear origin storage with flag --disable-storage-reset', () => {
Expand Down
16 changes: 16 additions & 0 deletions lighthouse-core/test/lib/i18n/i18n-test.js
Expand Up @@ -61,6 +61,22 @@ describe('i18n', () => {
expect(strings.passedAuditsGroupTitle).toEqual('[Þåššéð åûðîţš one two]');
expect(strings.snippetCollapseButtonLabel).toEqual('[Çöļļåþšé šñîþþéţ one two]');
});

it('throws an error for invalid locales', () => {
expect(_ => i18n.getRendererFormattedStrings('not-a-locale'))
.toThrow(`Unsupported locale 'not-a-locale'`);
});
});

describe('#getFormatted', () => {
it('throws an error for invalid locales', () => {
// Populate a string to try to localize to a bad locale.
const UIStrings = {testMessage: 'testy test'};
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

expect(_ => i18n.getFormatted(str_(UIStrings.testMessage), 'still-not-a-locale'))
.toThrow(`Unsupported locale 'still-not-a-locale'`);
});
});

describe('#lookupLocale', () => {
Expand Down

0 comments on commit b671932

Please sign in to comment.