i18n: add i18n for LighthouseError messages #6457

exterkamp · 2018-11-02T21:11:54Z

Summary
Adds i18n strings for LighthouseError messages. Adds support for substituting in values into an error message through i18n UIString.

Related Issues/PRs
fixes: #6441

lighthouse-core/lib/lh-error.js

lighthouse-core/test/gather/gather-runner-test.js

lighthouse-core/lib/lh-error.js

brendankenny · 2018-11-02T22:50:10Z

lighthouse-core/gather/gather-runner.js

@@ -173,7 +173,7 @@ class GatherRunner {
      const insecureDescriptions = explanations
        .filter(exp => exp.securityState === 'insecure')
        .map(exp => exp.description);
-      errorDef.message += ` ${insecureDescriptions.join(' ')}`;
+      errorDef.substitute = `${insecureDescriptions.join(' ')}`;


this join seems like it could be a problem. Are these descriptions localized by chrome?

lighthouse-core/lib/lh-error.js

lighthouse-core/runner.js

lighthouse-core/test/gather/driver-test.js

lighthouse-core/lib/lh-error.js

exterkamp · 2018-11-05T19:24:03Z

lighthouse-core/lib/lh-error.js

@@ -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);


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

perfect 👌

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).

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

patrickhulce

love the new naming, thanks so much @exterkamp!

patrickhulce · 2018-11-05T20:19:37Z

lighthouse-core/lib/lh-error.js

@@ -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);


perfect 👌

lighthouse-core/runner.js

patrickhulce · 2018-11-05T20:22:57Z

lighthouse-core/test/gather/gather-runner-test.js

@@ -1059,7 +1062,8 @@ describe('GatherRunner', function() {
        config: new Config({}),
      }).then(artifacts => {
        assert.equal(artifacts.LighthouseRunWarnings.length, 1);
-        assert.ok(/unable.*load the page/.test(artifacts.LighthouseRunWarnings[0]));
+        expect(artifacts.LighthouseRunWarnings[0])


at this stage don't we want to assert that the string itself has already gone through the i18n process and is just a normal string?

I think it was already doing that? I might not get what you're implying. I made the check more accurate since the regex was a little plain.

well I mean that toBeDisplayString does the whole getFormatted dance for you, so I'm thinkin' we want at least one test that checks if our code is using getFormatted at all the right places.

but maybe I'm misunderstanding or we haven't decided to have the strings in artifacts be pre-resolved yet and the test you added in runner-test is good enough for now?

hope that makes more sense

Oh I think I get what you're saying, we should check that the translation did actually get filled in and we don't expose any un-translated strings to the user, right?

I think that the test in runner captures the only new place we have translation. Everything here is translated at the same time all other lhr strings are. Do we have some coverage for that/should we add some for these new strings?

Yeah exactly. I was somewhat wondering outloud if the artifacts is one new place we should do the translation.

lighthouse-core/test/runner-test.js

lighthouse-core/lib/lh-error.js

brendankenny · 2018-11-06T00:39:21Z

lighthouse-core/lib/lh-error.js

@@ -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);


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).

brendankenny · 2018-11-06T00:40:35Z

lighthouse-core/lib/lh-error.js

@@ -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);


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

brendankenny · 2018-11-06T00:41:30Z

lighthouse-core/lib/lh-error.js

    lhrRuntimeError: true,
+    errorDetails: 'No Failure Description Loaded.',


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

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.

paulirish · 2018-11-16T08:27:58Z

ready for review?

exterkamp · 2018-11-16T18:58:05Z

Not ready to review yet 😄 should land #6579 first and then merge those changes in and update to i18n that new error!

exterkamp · 2018-11-28T18:59:30Z

Should be g2g, ptal

patrickhulce

LGTM % nits!

patrickhulce · 2018-11-28T19:26:39Z

lighthouse-core/gather/gather-runner.js

-        errorDef.message += ` ${netErr}.`;
+        return new LHError(
+          LHError.errors.FAILED_DOCUMENT_REQUEST,
+          {errorDetails: `${netErr}`}


do these still need to be in string templates?

patrickhulce · 2018-11-28T19:30:07Z

lighthouse-core/lib/lh-error.js

+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. */


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'

patrickhulce · 2018-11-28T19:31:00Z

lighthouse-core/lib/lh-error.js

    lhrRuntimeError: true,
  },
-  /* Used when security error prevents page load. */
+  /* Used when security error prevents page load.
+   * Requires an additional `securityMessages` field for translation.


I feel like we're going to forget to update these if they ever change, but I'm not sure what to do about it

Well the current plan is that if we change them, then there will be a ton of errors thrown from str_ because it is missing the required substitution fields. But I'm not sure that's a long term strategy.

we could maybe figure out something with typescript, but I can't think of a way without keeping a manual list of error codes that will require an extra field.

@exterkamp would you want to do that? or wait on it.

main issue with

there will be a ton of errors thrown from str_

is we need to make sure error cases are getting hit in unit tests and that the unit tests aren't testing for any throw, since str_ errors would fit that bill

patrickhulce · 2018-11-28T19:32:29Z

lighthouse-core/test/runner-test.js

+    } catch (err) {
+      assert.equal(err.code, LHError.errors.PROTOCOL_TIMEOUT.code);
+      assert.ok(/^Waiting for DevTools protocol.*Method: Method.Failure/.test(err.friendlyMessage),
+        'should have prevented run');


should have a localized error message :)

Oh I see now my comment was confusing! I was suggesting to change the failure message to should have a localized error message not to change the test :)

patrickhulce

Hold up a sec, maybe we forgot something for LR? Travis is unhappy with runLighthouseInLR https://travis-ci.org/GoogleChrome/lighthouse/builds/460924770#L1067

lighthouse-core/gather/gather-runner.js

lighthouse-core/test/runner-test.js

patrickhulce

LGTM!

patrickhulce · 2018-11-29T04:20:54Z

lighthouse-core/test/runner-test.js

+    } catch (err) {
+      assert.equal(err.code, LHError.errors.PROTOCOL_TIMEOUT.code);
+      assert.ok(/^Waiting for DevTools protocol.*Method: Method.Failure/.test(err.friendlyMessage),
+        'should have prevented run');


Oh I see now my comment was confusing! I was suggesting to change the failure message to should have a localized error message not to change the test :)

lighthouse-core/test/runner-test.js

Co-Authored-By: exterkamp <[email protected]>

googlebot · 2018-11-29T19:17:52Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

brendankenny

really liking this last form for the LHError constructor.

Mostly some style nits and two bigger questions left from me

lighthouse-core/gather/driver.js

brendankenny · 2018-11-29T19:39:07Z

lighthouse-core/gather/driver.js

+      const asyncTimeout = setTimeout((_ => {
+        const err = new LHError(
+          LHError.errors.PROTOCOL_TIMEOUT,
+          {protocolMethod: `${method}`}


can drop the template string.

optional style nit: a lot of these new LHError() lines could also fit on one line now. Was it intentional breaking them up over 4? I feel like they're more readable on one, but understand if you feel otherwise

Personally I like splitting complex constructors into lines, and with it all on one line it skirts the line of es-lint yelling that the lines are too long. I feel like 4 lines is more readable than crammed onto 1.

lighthouse-core/gather/gather-runner.js

brendankenny · 2018-11-29T20:10:14Z

lighthouse-core/lib/lh-error.js

@@ -24,7 +57,7 @@ class LighthouseError extends Error {
    super(errorDefinition.code);
    this.name = 'LHError';
    this.code = errorDefinition.code;
-    this.friendlyMessage = errorDefinition.message;
+    this.friendlyMessage = str_(errorDefinition.message, properties);


brendankenny · 2018-11-29T20:20:48Z

lighthouse-core/lib/lh-error.js

    lhrRuntimeError: true,
  },
-  /* Used when security error prevents page load. */
+  /* Used when security error prevents page load.
+   * Requires an additional `securityMessages` field for translation.


we could maybe figure out something with typescript, but I can't think of a way without keeping a manual list of error codes that will require an extra field.

@exterkamp would you want to do that? or wait on it.

main issue with

there will be a ton of errors thrown from str_

is we need to make sure error cases are getting hit in unit tests and that the unit tests aren't testing for any throw, since str_ errors would fit that bill

brendankenny · 2018-11-29T20:26:22Z

clients/test/lightrider-entry-test.js

+      const connectionError = new LHError(
+        LHError.errors.FAILED_DOCUMENT_REQUEST,
+        {errorDetails: 'Bad connection req'}
+      );


what happens with the parsedResult.runtimeError.message.includes check below? Are they expected to be localized by that point? Can we assert that? (and can we check that the errorDetails replacement worked)?

I think that is what the includes checks, that the runtime message in the end is the same as the orig friendly msg post a round of translation since it is outside of the Runner.run which translates errors before it exits. It uses the includes instead of equal b/c LR appends the msg to the friendly msg which may not be desired? It's the first I'm really seeing it.

I had noticed that runLighthouseInLR doesn't run i18n on its generated output, hence the question, but the new err.friendlyMessage = i18n.getFormatted(err.friendlyMessage, settings.locale); line in runner.js translates the error friendlyMessage before it gets back to runLighthouseInLR, so localization is good to go!

brendankenny · 2018-11-29T20:27:34Z

lighthouse-core/gather/gather-runner.js

    if (!mainRecord) {
-      errorDef = LHError.errors.NO_DOCUMENT_REQUEST;
+      return new LHError(LHError.errors.NO_DOCUMENT_REQUEST);


… i18n-errors

paulirish

Digging it. small tweaks but I like the overall.

lighthouse-core/lib/lh-error.js

Added paulirish's suggestions for i18n strings. Co-Authored-By: exterkamp <[email protected]>

brendankenny

LGTM!

exterkamp added 6 commits November 2, 2018 13:43

Added i18n for runtime error messages.

d1be64d

Merge branch 'master' into i18n-errors

6799fab

Formatting on substitutes.

3db8628

Removed redundant 'Method'.

b9c4b4e

All backticks for variable interpolation highlighting

9b6062b

Thanks linter!

b0d369f

exterkamp requested a review from brendankenny November 2, 2018 21:11

exterkamp requested review from patrickhulce and paulirish as code owners November 2, 2018 21:11

exterkamp commented Nov 2, 2018

View reviewed changes

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

exterkamp commented Nov 2, 2018

View reviewed changes

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

exterkamp commented Nov 2, 2018

View reviewed changes

lighthouse-core/test/gather/gather-runner-test.js Outdated Show resolved Hide resolved

exterkamp commented Nov 2, 2018

View reviewed changes

lighthouse-core/lib/lh-error.js Show resolved Hide resolved

Added substitution check to PROTOCOL_TIMEOUT.

1481b08

patrickhulce reviewed Nov 2, 2018

View reviewed changes

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

lighthouse-core/lib/lh-error.js Show resolved Hide resolved

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

Using test-utils toBeDisplayString.

cf8e569

brendankenny requested changes Nov 2, 2018

View reviewed changes

exterkamp added 2 commits November 5, 2018 11:11

Added descriptive ICU names from feedback.

df29d17

Added Method back into test for protocol timeout.

020056a

exterkamp commented Nov 5, 2018

View reviewed changes

Removed unnecessary var.

56e3daf

patrickhulce suggested changes Nov 5, 2018

View reviewed changes

exterkamp added 3 commits November 5, 2018 14:24

Fixed i18n of errors and added test to make sure it works!

e764e2d

Better gather runner test comparison.

7a65d02

Linter caught me slippin'

a6cd8bc

brendankenny reviewed Nov 6, 2018

View reviewed changes

exterkamp mentioned this pull request Nov 6, 2018

Translate new i18n accessibility audits #6488

Closed

Moved icu args to properties.

02765e4

exterkamp added 3 commits November 28, 2018 10:49

Added early exit. Fixed i18n punc. formatting.

879c333

Collected new strings, and fixed sec-msg test.

345d3e4

Formatting fix.

9a87b04

exterkamp added the waiting4reviewer label Nov 28, 2018

patrickhulce approved these changes Nov 28, 2018

View reviewed changes

patrickhulce suggested changes Nov 28, 2018

View reviewed changes

connorjclark reviewed Nov 28, 2018

View reviewed changes

lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved

exterkamp added 2 commits November 28, 2018 12:22

Fixed LR test.

9beb615

Removed backticks, and added localized test.

bdfcc4e

patrickhulce suggested changes Nov 28, 2018

View reviewed changes

lighthouse-core/test/runner-test.js Outdated Show resolved Hide resolved

I was led astray.

c56c83f

patrickhulce approved these changes Nov 29, 2018

View reviewed changes

Update lighthouse-core/test/runner-test.js

9857760

Co-Authored-By: exterkamp <[email protected]>

brendankenny requested changes Nov 29, 2018

View reviewed changes

exterkamp added 2 commits November 29, 2018 13:36

Switched up string templates.

d1e1943

Merge branch 'i18n-errors' of github.com:GoogleChrome/lighthouse into…

c6cfb3b

… i18n-errors

exterkamp mentioned this pull request Dec 3, 2018

runTimeError exists when there are NO_ERRORs #6336

Closed

exterkamp added waiting4committer and removed waiting4reviewer labels Dec 5, 2018

paulirish approved these changes Dec 6, 2018

View reviewed changes

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved

paulirish and others added 2 commits December 7, 2018 15:21

Apply suggestions from code review

14ceeb2

Added paulirish's suggestions for i18n strings. Co-Authored-By: exterkamp <[email protected]>

Updated strings and test.

972ddb5

brendankenny approved these changes Dec 10, 2018

View reviewed changes

brendankenny merged commit dce4485 into master Dec 10, 2018

brendankenny deleted the i18n-errors branch December 10, 2018 19:49

This was referenced Dec 14, 2018

Audit errorMessage not i18n'd #6804

Closed

Extension Error: INSECURE_DOCUMENT_REQUEST #6809

Closed

brendankenny mentioned this pull request May 24, 2019

feature-request: add reason to runtime error output #5838

Closed

		lhrRuntimeError: true,
		errorDetails: 'No Failure Description Loaded.',

i18n: add i18n for LighthouseError messages #6457

i18n: add i18n for LighthouseError messages #6457

Conversation

exterkamp commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Nov 16, 2018

exterkamp commented Nov 16, 2018

exterkamp commented Nov 28, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Nov 29, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment