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.

Sign up for GitHub

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

Jump to bottom

i18n: localize audit error strings #6812

Merged
merged 4 commits into from
Dec 18, 2018
Merged

i18n: localize audit error strings #6812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4c0a06
Add default properties to badTracing errors.
exterkamp Dec 14, 2018
17c9c34
Changed to unique strings for each!
exterkamp Dec 14, 2018
8b58d2b
Flip flop again. Gives access to error code to ICU replacement texts.
exterkamp Dec 14, 2018
273c486
feedback.
exterkamp Dec 18, 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: 3 additions & 3 deletions lighthouse-core/lib/i18n/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,11 +928,11 @@
"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.",
"message": "Something went wrong with recording the trace over your page load. Please run Lighthouse again. ({errorCode})",
"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.",
"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. ({errorCode})",
"description": "Error message explaining that the Lighthouse run was not able to collect screenshots through Chrome."
},
"lighthouse-core/lib/lh-error.js | dnsFailure": {
Expand Down Expand Up @@ -964,7 +964,7 @@
"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.",
"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. ({errorCode})",
"description": "Error message explaining that the page loaded too slowly to perform a Lighthouse run."
},
"lighthouse-core/lib/lh-error.js | protocolTimeout": {
Expand Down
19 changes: 5 additions & 14 deletions lighthouse-core/lib/lh-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ 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.`,
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. ({errorCode})`,
/** Error message explaining that the network trace was not able to be recorded for the Lighthouse run. */
badTraceRecording: 'Something went wrong with recording the trace over your page load. Please run Lighthouse again.',
badTraceRecording: 'Something went wrong with recording the trace over your page load. Please run Lighthouse again. ({errorCode})',
/** 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.',
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. ({errorCode})',
/** 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.',
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */
Expand Down Expand Up @@ -57,23 +57,14 @@ class LighthouseError extends Error {
super(errorDefinition.code);
this.name = 'LHError';
this.code = errorDefinition.code;
this.friendlyMessage = str_(errorDefinition.message, properties);
// Insert the i18n reference with errorCode and all additional ICU replacement properties.
this.friendlyMessage = str_(errorDefinition.message, {errorCode: this.code, ...properties});
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
this.lhrRuntimeError = !!errorDefinition.lhrRuntimeError;
if (properties) Object.assign(this, properties);

Error.captureStackTrace(this, LighthouseError);
}

/**
* @param {LighthouseError} err
* @return {LighthouseError}
*/
static fromLighthouseError(err) {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const {code, friendlyMessage: message, ...rest} = err;
// Note: {...rest} convinces tsc 3.1 that it's assignable to a Record.
return new LighthouseError({code, message}, {...rest});
}

/**
* @param {string} method
* @param {{message: string, data?: string|undefined}} protocolError
Expand Down
4 changes: 1 addition & 3 deletions lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,7 @@ class Runner {

Sentry.captureException(err, {tags: {audit: audit.meta.id}, level: 'error'});
// Errors become error audit result.
const errorMessage = err.friendlyMessage ?
`${err.friendlyMessage} (${err.message})` :
`Audit error: ${err.message}`;
const errorMessage = err.friendlyMessage ? err.friendlyMessage : err.message;
Copy link
Collaborator

Choose a reason for hiding this comment

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

so what errors do we start losing for the code for after this, just the speedline ones? or the page load ones too?

I agree there's not a strong need for our users to know the code, but having it somewhere in the LHR would be nice.

Copy link
Member Author

@exterkamp exterkamp Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should follow the same pattern of splitting the speedline ones out into unique values.

auditResult = Audit.generateErrorAuditResult(audit, errorMessage);
}

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2669,7 +2669,7 @@
"score": null,
"scoreDisplayMode": "error",
"rawValue": null,
"errorMessage": "Audit error: Required RobotsTxt gatherer did not run."
"errorMessage": "Required RobotsTxt gatherer did not run."
},
"robots-txt": {
"id": "robots-txt",
Expand All @@ -2678,7 +2678,7 @@
"score": null,
"scoreDisplayMode": "error",
"rawValue": null,
"errorMessage": "Audit error: Required RobotsTxt gatherer did not run."
"errorMessage": "Required RobotsTxt gatherer did not run."
},
"hreflang": {
"id": "hreflang",
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/test/runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,8 @@ describe('Runner', () => {
assert.ok(lhr.audits['test-audit'].errorMessage.includes(NO_FCP.code));
// And it bubbled up to the runtimeError.
assert.strictEqual(lhr.runtimeError.code, NO_FCP.code);
assert.ok(lhr.runtimeError.message.includes(NO_FCP.message));
expect(lhr.runtimeError.message)
.toBeDisplayString(/Something .*\(NO_FCP\)/);
});

it('localized errors thrown from driver', async () => {
Expand Down
4 changes: 2 additions & 2 deletions proto/sample_v2_round_trip.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,7 +969,7 @@
},
"is-crawlable": {
"description": "Search engines are unable to include your pages in search results if they don't have permission to crawl them. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/indexing).",
"errorMessage": "Audit error: Required RobotsTxt gatherer did not run.",
"errorMessage": "Required RobotsTxt gatherer did not run.",
"id": "is-crawlable",
"score": null,
"scoreDisplayMode": "error",
Expand Down Expand Up @@ -1821,7 +1821,7 @@
},
"robots-txt": {
"description": "If your robots.txt file is malformed, crawlers may not be able to understand how you want your website to be crawled or indexed.",
"errorMessage": "Audit error: Required RobotsTxt gatherer did not run.",
"errorMessage": "Required RobotsTxt gatherer did not run.",
"id": "robots-txt",
"score": null,
"scoreDisplayMode": "error",
Expand Down