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

i18n: add i18n for LighthouseError messages #6457

Merged
merged 27 commits into from
Dec 10, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d1be64d
Added i18n for runtime error messages.
exterkamp Nov 2, 2018
6799fab
Merge branch 'master' into i18n-errors
exterkamp Nov 2, 2018
3db8628
Formatting on substitutes.
exterkamp Nov 2, 2018
b9c4b4e
Removed redundant 'Method'.
exterkamp Nov 2, 2018
9b6062b
All backticks for variable interpolation highlighting
exterkamp Nov 2, 2018
b0d369f
Thanks linter!
exterkamp Nov 2, 2018
1481b08
Added substitution check to PROTOCOL_TIMEOUT.
exterkamp Nov 2, 2018
cf8e569
Using test-utils toBeDisplayString.
exterkamp Nov 2, 2018
df29d17
Added descriptive ICU names from feedback.
exterkamp Nov 5, 2018
020056a
Added Method back into test for protocol timeout.
exterkamp Nov 5, 2018
56e3daf
Removed unnecessary var.
exterkamp Nov 5, 2018
e764e2d
Fixed i18n of errors and added test to make sure it works!
exterkamp Nov 5, 2018
7a65d02
Better gather runner test comparison.
exterkamp Nov 5, 2018
a6cd8bc
Linter caught me slippin'
exterkamp Nov 5, 2018
02765e4
Moved icu args to properties.
exterkamp Nov 14, 2018
a991732
Merge branch 'master' into i18n-errors
exterkamp Nov 28, 2018
879c333
Added early exit. Fixed i18n punc. formatting.
exterkamp Nov 28, 2018
345d3e4
Collected new strings, and fixed sec-msg test.
exterkamp Nov 28, 2018
9a87b04
Formatting fix.
exterkamp Nov 28, 2018
9beb615
Fixed LR test.
exterkamp Nov 28, 2018
bdfcc4e
Removed backticks, and added localized test.
exterkamp Nov 28, 2018
c56c83f
I was led astray.
exterkamp Nov 28, 2018
9857760
Update lighthouse-core/test/runner-test.js
patrickhulce Nov 29, 2018
d1e1943
Switched up string templates.
exterkamp Nov 29, 2018
c6cfb3b
Merge branch 'i18n-errors' of github.com:GoogleChrome/lighthouse into…
exterkamp Nov 29, 2018
14ceeb2
Apply suggestions from code review
paulirish Dec 7, 2018
972ddb5
Updated strings and test.
exterkamp Dec 7, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,10 @@ class Driver {
}
return new Promise(async (resolve, reject) => {
const asyncTimeout = setTimeout((_ => {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT);
err.message += ` Method: ${method}`;
const err = new LHError({
...LHError.errors.PROTOCOL_TIMEOUT,
protocolMethod: `${method}`,
});
reject(err);
}), timeout);
try {
Expand Down
18 changes: 12 additions & 6 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,15 @@ class GatherRunner {
if (!mainRecord) {
errorDef = LHError.errors.NO_DOCUMENT_REQUEST;
} else if (mainRecord.failed) {
errorDef = {...LHError.errors.FAILED_DOCUMENT_REQUEST};
errorDef.message += ` ${mainRecord.localizedFailDescription}.`;
errorDef = {
...LHError.errors.FAILED_DOCUMENT_REQUEST,
errorDetails: `${mainRecord.localizedFailDescription}.`,
};
} else if (mainRecord.hasErrorStatusCode()) {
errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST};
errorDef.message += ` Status code: ${mainRecord.statusCode}.`;
errorDef = {
...LHError.errors.ERRORED_DOCUMENT_REQUEST,
statusCode: `${mainRecord.statusCode}.`,
};
Copy link
Member

Choose a reason for hiding this comment

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

this is getting complex. Can we start doing early returns on these? Should be easier to read with those.

}

if (errorDef) {
Expand All @@ -169,11 +173,13 @@ class GatherRunner {
*/
static assertNoSecurityIssues({securityState, explanations}) {
if (securityState === 'insecure') {
const errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST};
const insecureDescriptions = explanations
.filter(exp => exp.securityState === 'insecure')
.map(exp => exp.description);
errorDef.message += ` ${insecureDescriptions.join(' ')}`;
const errorDef = {
...LHError.errors.INSECURE_DOCUMENT_REQUEST,
'securityMessages': `${insecureDescriptions.join(' ')}`,
};
throw new LHError(errorDef);
}
}
Expand Down
44 changes: 44 additions & 0 deletions lighthouse-core/lib/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,50 @@
"message": "{timeInMs, number, seconds} s",
"description": "Used to show the duration in seconds that something lasted. The {timeInMs} placeholder will be replaced with the time duration, shown in seconds (e.g. 5.2 s)"
},
"lighthouse-core/lib/lh-error.js | badTraceRecording": {
"message": "Something went wrong with recording the trace over your page load. Please run Lighthouse again.",
"description": "Error message explaining that the network trace was not able to be recorded for the Lighthouse run."
},
"lighthouse-core/lib/lh-error.js | didntCollectScreenshots": {
"message": "Chrome didn't collect any screenshots during the page load. Please make sure there is content visible on the page, and then try re-running Lighthouse.",
"description": "Error message explaining that the Lighthouse run was not able to collect screenshots through Chrome."
},
"lighthouse-core/lib/lh-error.js | internalChromeError": {
"message": "An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.",
"description": "Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted."
},
"lighthouse-core/lib/lh-error.js | pageLoadFailed": {
"message": "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.",
"description": "Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability."
},
"lighthouse-core/lib/lh-error.js | pageLoadFailedInsecure": {
"message": "The URL you have provided does not have valid security credentials. {securityMessages}",
"description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed."
},
"lighthouse-core/lib/lh-error.js | pageLoadFailedWithDetails": {
"message": "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. Detailed Error: {errorDetails}",
"description": "Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability."
},
"lighthouse-core/lib/lh-error.js | pageLoadFailedWithStatusCode": {
"message": "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: {statusCode}",
"description": "Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability."
},
"lighthouse-core/lib/lh-error.js | pageLoadTookTooLong": {
"message": "Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.",
"description": "Error message explaining that the page loaded too slowly to perform a Lighthouse run."
},
"lighthouse-core/lib/lh-error.js | protocolTimeout": {
"message": "Waiting for DevTools protocol response has exceeded the allotted time. Method: {protocolMethod}",
"description": "Error message explaining that the Chrome Devtools protocol has exceeded the maximum timeout allowed."
},
"lighthouse-core/lib/lh-error.js | requestContentTimeout": {
"message": "Fetching resource content has exceeded the allotted time",
"description": "Error message explaining that fetching the resources of the webpage has taken longer than the maximum time."
},
"lighthouse-core/lib/lh-error.js | urlInvalid": {
"message": "The URL you have provided appears to be invalid.",
"description": "Error message explaining that the provided URL Lighthouse points to is not valid, and cannot be loaded."
},
"lighthouse-core/report/html/renderer/util.js | auditGroupExpandTooltip": {
"message": "Show audits",
"description": "The tooltip text on an expandable chevron icon. Clicking the icon expands a section to reveal a list of audit results that was hidden by default."
Expand Down
89 changes: 64 additions & 25 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,49 @@
*/
'use strict';

const strings = require('./strings');
const i18n = require('./i18n/i18n.js');

/* eslint-disable max-len */
const UIStrings = {
/** Error message explaining that the Lighthouse run was not able to collect screenshots through Chrome.*/
didntCollectScreenshots: `Chrome didn't collect any screenshots during the page load. Please make sure there is content visible on the page, and then try re-running Lighthouse.`,
/** Error message explaining that the network trace was not able to be recorded for the Lighthouse run. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe for some of these descriptions we should include for the translators the distinction of whether it was something weird that retrying should fix, or something that the user needs to fix and try again? I'm thinking a stock explanation.

This is an error message that was because of certain user conditions they must fix before retrying.
vs.
This is an error message that was because of irregularities with Lighthouse that should be fixed by trying again.

just spitballin'

badTraceRecording: 'Something went wrong with recording the trace over your page load. Please run Lighthouse again.',
/** Error message explaining that the page loaded too slowly to perform a Lighthouse run. */
pageLoadTookTooLong: 'Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.',
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */
pageLoadFailed: '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.',
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */
pageLoadFailedWithStatusCode: '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: {statusCode}',
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */
pageLoadFailedWithDetails: '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. Detailed Error: {errorDetails}',
/** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}',
/** Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted. */
internalChromeError: 'An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.',
/** Error message explaining that fetching the resources of the webpage has taken longer than the maximum time. */
requestContentTimeout: 'Fetching resource content has exceeded the allotted time',
/** Error message explaining that the provided URL Lighthouse points to is not valid, and cannot be loaded. */
urlInvalid: 'The URL you have provided appears to be invalid.',
/** Error message explaining that the Chrome Devtools protocol has exceeded the maximum timeout allowed. */
protocolTimeout: 'Waiting for DevTools protocol response has exceeded the allotted time. Method: {protocolMethod}',
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);


/**
* @typedef LighthouseErrorDefinition
* @property {string} code
* @property {string} message
* @property {RegExp} [pattern]
* @property {boolean} [lhrRuntimeError] True if it should appear in the top-level LHR.runtimeError property.
* ICU argument replacement properties
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
* @property {string} [statusCode]
* @property {string} [errorDetails]
* @property {string} [securityMessages]
* @property {string} [protocolMethod]
*
*/

class LighthouseError extends Error {
Expand All @@ -24,7 +59,7 @@ class LighthouseError extends Error {
super(errorDefinition.code);
this.name = 'LHError';
this.code = errorDefinition.code;
this.friendlyMessage = errorDefinition.message;
this.friendlyMessage = str_(errorDefinition.message, errorDefinition);
Copy link
Member Author

Choose a reason for hiding this comment

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

@patrickhulce @brendankenny is this what you had in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

perfect 👌

Copy link
Member

Choose a reason for hiding this comment

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

What if the second argument to the LHError constructor becomes the replacement values? I feel like it would be conceptually easier to understand without having all of it on the first argument and

const err = new LHError(
  LHError.errors.PROTOCOL_TIMEOUT,
  protocolMethod: `${method}`,
);
reject(err);

is a lot easier to understand at a glance than

const err = new LHError({
  ...LHError.errors.PROTOCOL_TIMEOUT,
  protocolMethod: `${method}`,
});
reject(err);

I was going to say ideally we'd do this but the existing properties second arg makes that a problem, but I think it's actually fine. The only place we ever use that argument is in this file for fromProtocolMessage(), and we use it for protocolMethod and protocolError, one of which is already an icu argument name :)

What if properties just did both? It puts its properties on the error object (as it does now), and it gets passed into str_ (as errorDefinition is doing here).

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 later on we should do stricter type checking...if you use a string that needs a replacement, you should have to provide the replacement

this.lhrRuntimeError = !!errorDefinition.lhrRuntimeError;
if (properties) Object.assign(this, properties);

Expand Down Expand Up @@ -70,116 +105,120 @@ const ERRORS = {
// Screenshot/speedline errors
NO_SPEEDLINE_FRAMES: {
code: 'NO_SPEEDLINE_FRAMES',
message: strings.didntCollectScreenshots,
message: UIStrings.didntCollectScreenshots,
lhrRuntimeError: true,
},
SPEEDINDEX_OF_ZERO: {
code: 'SPEEDINDEX_OF_ZERO',
message: strings.didntCollectScreenshots,
message: UIStrings.didntCollectScreenshots,
lhrRuntimeError: true,
},
NO_SCREENSHOTS: {
code: 'NO_SCREENSHOTS',
message: strings.didntCollectScreenshots,
message: UIStrings.didntCollectScreenshots,
lhrRuntimeError: true,
},
INVALID_SPEEDLINE: {
code: 'INVALID_SPEEDLINE',
message: strings.didntCollectScreenshots,
message: UIStrings.didntCollectScreenshots,
lhrRuntimeError: true,
},

// Trace parsing errors
NO_TRACING_STARTED: {
code: 'NO_TRACING_STARTED',
message: strings.badTraceRecording,
message: UIStrings.badTraceRecording,
lhrRuntimeError: true,
},
NO_NAVSTART: {
code: 'NO_NAVSTART',
message: strings.badTraceRecording,
message: UIStrings.badTraceRecording,
lhrRuntimeError: true,
},
NO_FCP: {
code: 'NO_FCP',
message: strings.badTraceRecording,
message: UIStrings.badTraceRecording,
lhrRuntimeError: true,
},
NO_DCL: {
code: 'NO_DCL',
message: strings.badTraceRecording,
message: UIStrings.badTraceRecording,
lhrRuntimeError: true,
},
NO_FMP: {
code: 'NO_FMP',
message: strings.badTraceRecording,
message: UIStrings.badTraceRecording,
},

// TTI calculation failures
FMP_TOO_LATE_FOR_FCPUI: {code: 'FMP_TOO_LATE_FOR_FCPUI', message: strings.pageLoadTookTooLong},
NO_FCPUI_IDLE_PERIOD: {code: 'NO_FCPUI_IDLE_PERIOD', message: strings.pageLoadTookTooLong},
NO_TTI_CPU_IDLE_PERIOD: {code: 'NO_TTI_CPU_IDLE_PERIOD', message: strings.pageLoadTookTooLong},
FMP_TOO_LATE_FOR_FCPUI: {code: 'FMP_TOO_LATE_FOR_FCPUI', message: UIStrings.pageLoadTookTooLong},
NO_FCPUI_IDLE_PERIOD: {code: 'NO_FCPUI_IDLE_PERIOD', message: UIStrings.pageLoadTookTooLong},
NO_TTI_CPU_IDLE_PERIOD: {code: 'NO_TTI_CPU_IDLE_PERIOD', message: UIStrings.pageLoadTookTooLong},
NO_TTI_NETWORK_IDLE_PERIOD: {
code: 'NO_TTI_NETWORK_IDLE_PERIOD',
message: strings.pageLoadTookTooLong,
message: UIStrings.pageLoadTookTooLong,
},

// Page load failures
NO_DOCUMENT_REQUEST: {
code: 'NO_DOCUMENT_REQUEST',
message: strings.pageLoadFailed,
message: UIStrings.pageLoadFailed,
lhrRuntimeError: true,
},
/* Used when DevTools reports loading failed. Usually an internal (Chrome) issue. */
FAILED_DOCUMENT_REQUEST: {
code: 'FAILED_DOCUMENT_REQUEST',
message: strings.pageLoadFailed,
message: UIStrings.pageLoadFailedWithDetails,
lhrRuntimeError: true,
errorDetails: 'No Failure Description Loaded.',
Copy link
Member

Choose a reason for hiding this comment

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

do we want defaults for these? What happens if you don't provide them? Throwing if you don't provide one seems ideal

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the ICU message translator throws. With a 'value that wasn't replaced', good point, we probably do want to throw in order to avoid a silent lack of real replacement.

},
/* Used when status code is 4xx or 5xx. */
ERRORED_DOCUMENT_REQUEST: {
code: 'ERRORED_DOCUMENT_REQUEST',
message: strings.pageLoadFailed,
message: UIStrings.pageLoadFailedWithStatusCode,
lhrRuntimeError: true,
statusCode: 'No Error Code Loaded.',
},
/* Used when security error prevents page load. */
INSECURE_DOCUMENT_REQUEST: {
code: 'INSECURE_DOCUMENT_REQUEST',
message: strings.pageLoadFailedInsecure,
message: UIStrings.pageLoadFailedInsecure,
lhrRuntimeError: true,
securityMessages: 'No Security Descriptions Loaded.',
},

// Protocol internal failures
TRACING_ALREADY_STARTED: {
code: 'TRACING_ALREADY_STARTED',
message: strings.internalChromeError,
message: UIStrings.internalChromeError,
pattern: /Tracing.*started/,
lhrRuntimeError: true,
},
PARSING_PROBLEM: {
code: 'PARSING_PROBLEM',
message: strings.internalChromeError,
message: UIStrings.internalChromeError,
pattern: /Parsing problem/,
lhrRuntimeError: true,
},
READ_FAILED: {
code: 'READ_FAILED',
message: strings.internalChromeError,
message: UIStrings.internalChromeError,
pattern: /Read failed/,
lhrRuntimeError: true,
},

// URL parsing failures
INVALID_URL: {
code: 'INVALID_URL',
message: strings.urlInvalid,
message: UIStrings.urlInvalid,
},

// Protocol timeout failures
PROTOCOL_TIMEOUT: {
code: 'PROTOCOL_TIMEOUT',
message: strings.protocolTimeout,
message: UIStrings.protocolTimeout,
lhrRuntimeError: true,
protocolMethod: 'No Method Loaded.',
},

// Hey! When adding a new error type, update lighthouse-result.proto too.
Expand All @@ -190,4 +229,4 @@ LighthouseError.errors = ERRORS;
LighthouseError.NO_ERROR = 'NO_ERROR';
LighthouseError.UNKNOWN_ERROR = 'UNKNOWN_ERROR';
module.exports = LighthouseError;

module.exports.UIStrings = UIStrings;
19 changes: 0 additions & 19 deletions lighthouse-core/lib/strings.js

This file was deleted.

5 changes: 3 additions & 2 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ class Runner {
* @return {Promise<LH.RunnerResult|undefined>}
*/
static async run(connection, runOpts) {
const settings = runOpts.config.settings;
try {
const settings = runOpts.config.settings;
const runnerStatus = {msg: 'Runner setup', id: 'lh:runner:run'};
log.time(runnerStatus, 'verbose');

Expand Down Expand Up @@ -157,6 +157,8 @@ class Runner {

return {lhr, artifacts, report};
} catch (err) {
// i18n error strings
err.friendlyMessage = i18n.getFormatted(err.friendlyMessage, settings.locale);
await Sentry.captureException(err, {level: 'fatal'});
throw err;
}
Expand Down Expand Up @@ -193,7 +195,6 @@ class Runner {
if (!runnerOpts.config.passes) {
throw new Error('No browser artifacts are either provided or requested.');
}

const driver = runnerOpts.driverMock || new Driver(connection);
const gatherOpts = {
driver,
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/test/gather/driver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ describe('Browser Driver', () => {
assert.ok(false, 'long-running getRequestContent supposed to reject');
}, e => {
assert.equal(e.code, 'PROTOCOL_TIMEOUT');
expect(e.friendlyMessage).toBeDisplayString(
/^Waiting for DevTools.*Method: Network.getResponseBody/);
});
});

Expand Down
Loading