1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P2)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

For our mochitests, we should switch Fluent to throw real errors and break tests instead of console.warn/error on most errors.

One exception may be if we had to fallback a locale, this likely should not break mochitests.

I'm not completely should how it should look like. Maybe it should be a Pref that we set (`intl.l10n.debug`) that changes how we communicate errors?

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

The two edge cases are:

 - in mochitest if fluent throws, it should fail the test
 - on release channel, if fluent throws, we should salvage as much as possible and let the user use the product

In between are nightly/beta users and exact list of which failures should result in what.

My first proposal is to cluster all communication from Fluent into three buckets:

1) notifications - things that are innocent but may be worth communicating. For example - we had to fallback on a locale
2) warnings - things that may be ok, but quite likely are not. For example - empty translation
3) errors - things that definitely are not ok. For example - missing ID, broken string resolving etc.

For (1) we should do this only in debug mode and on nightly. For (2) we should allow for a way to silence it per-case and throw in tests and console.warn in Nightly/debug, for (3) we should throw in tests and console.error in all channels?

Thoughts?

Comment 2

[:Gijs (he/him)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> 1) notifications - things that are innocent but may be worth communicating.
> For example - we had to fallback on a locale
> 2) warnings - things that may be ok, but quite likely are not. For example -
> empty translation

In the past because of replacement issues we often had before/middle/after type strings, and so empty translations were expected. I don't know if Fluent fixes *all* those cases and therefore empty translations are never correct...

> 3) errors - things that definitely are not ok. For example - missing ID,
> broken string resolving etc.
> 
> For (1) we should do this only in debug mode and on nightly. For (2) we
> should allow for a way to silence it per-case and throw in tests and
> console.warn in Nightly/debug, for (3) we should throw in tests and
> console.error in all channels?


I think we should use Cu.isInAutomation and throw whenever any of (3) and maybe (2) happen in automation, and local builds (opt or debug). I don't think we should throw for production nightly builds.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

> I don't know if Fluent fixes *all* those cases and therefore empty translations are never correct...

Very correct. Not only concatenation is not needed it is also heavily discouraged as per the social contract - https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html#social-contract

> I think we should use Cu.isInAutomation and throw whenever any of (3) and maybe (2) happen in automation, and local builds (opt or debug). I don't think we should throw for production nightly builds.

That sounds like an easy way forward.

Comment 4

[Francesco Lodolo [:flod]]

Does automation means tests? Would it be safe to assume that it would only catch errors in en-US?

I don't believe targeting local and debug builds is enough, if the target is finding issues in our localizations. For that reason, I believe both #2 and #3 would be useful in console.log, at least on Nightly. For #3, it might even be useful beyond that.

I believe pluralForm.jsm is showing errors in Console for all channels, when there's a mismatch between expected and actual number of plural forms
https://searchfox.org/mozilla-central/rev/fca4426325624fecbd493c31389721513fc49fef/intl/locale/PluralForm.jsm#141

Comment 5

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #4)
> Does automation means tests?

Yes.

> Would it be safe to assume that it would only
> catch errors in en-US?

I think it'd be catching errors also for other locales, assuming they fulfill the "is in automation" criteria, which I would expect them to (and if they don't, I'd argue it's a bug that they don't). That said, I don't think we run (m)any tests against non-en-US on infra...

> I don't believe targeting local and debug builds is enough, if the target is
> finding issues in our localizations. For that reason, I believe both #2 and
> #3 would be useful in console.log, at least on Nightly. For #3, it might
> even be useful beyond that.

FWIW, I have no opinion on console.logging, I was only commenting as to whether it should throw / reject / crash or in some other way cause calling functions (and therefore hopefully tests) to fail. console.log(/error/warn/whatever) won't do any of that, which is what I think this bug is concerned about, as per comment #0:


(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> For our mochitests, we should switch Fluent to throw real errors and break
> tests instead of console.warn/error on most errors.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> The two edge cases are:
> 
>  - in mochitest if fluent throws, it should fail the test
>  - on release channel, if fluent throws, we should salvage as much as
> possible and let the user use the product
> 
> In between are nightly/beta users and exact list of which failures should
> result in what.
> 
> My first proposal is to cluster all communication from Fluent into three
> buckets:
> 
> 1) notifications - things that are innocent but may be worth communicating.
> For example - we had to fallback on a locale
> 2) warnings - things that may be ok, but quite likely are not. For example -
> empty translation
> 3) errors - things that definitely are not ok. For example - missing ID,
> broken string resolving etc.

I would prefer we throw in automation and local builds for 3 and 2. I would prefer that we keep these all quiet for Nightly production builds. For #1, #2, and #3 we should log to the Browser Console in production Nightly builds. Outside of Nightly we should only log #2 and #3 to the Browser Console.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.

Review commit: https://reviewboard.mozilla.org/r/242944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242944/

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

First attempt, it makes us:

In Automation we throw on #1, #3.
In production we browser console warn on #1 and #3 in Nightly.

We currently don't have #2 at all, so we'll add those warnings as we go.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8974591 [details]
Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.

https://reviewboard.mozilla.org/r/242944/#review248818

::: intl/l10n/DOMLocalization.jsm:661
(Diff revision 1)
>                translateElement(untranslatedElements[i], overlayTranslations[i]);
>              }
>            }
>            this.resumeObserving();
>          })
> -        .catch(() => this.resumeObserving());
> +        .catch((e) => {

Nitty nit: don't need () around single-arg arrow function.

Comment 10

[:Gijs (he/him)]

Your trypush has 2 relevant failures, though:


browser/components/preferences/in-content/tests/browser_extension_controlled.js | A promise chain failed to handle a rejection: Missing translations in en-US: - stack: formatWithFallback@resource://gre/modules/Localization.jsm:163:17

Uh oh. This seems like we're actually missing a translation, but where's the ids of the translations?

Also

intl/l10n/test/dom/test_domloc_overlay_missing_all.html | Test timed out.

Which seems like it needs to handle the throwing, or something.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Uh oh. This seems like we're actually missing a translation, but where's the ids of the translations?

This seems like the function is called with an empty id. Maybe we should wrap the IDs in `""`?

Comment 12

[:Gijs (he/him)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> > Uh oh. This seems like we're actually missing a translation, but where's the ids of the translations?
> 
> This seems like the function is called with an empty id. Maybe we should
> wrap the IDs in `""`?

Sounds good. But also, what's doing that? :-)

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Sounds good. But also, what's doing that? :-)

I was able to debug it - the issue is that we optimize mutation-triggered localization in the document to once per animation frame.

If a human, or a test, will trigger an l10n-id change in the element, the element will get added to the `pendingElements` list. If before AF the l10n-id gets removed, we'll pass that element for localization, without l10n-id.

I'll fix it by filtering out the elements for l10n-id within the rAF callback.

There is one more bug - bug 1442542 updated l10n-ids, but didn't update their search-l10n-ids counterparts. Fortunately, this change makes us throw in automation, so this should be fixed once and for all.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.

Review commit: https://reviewboard.mozilla.org/r/246192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246192/

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Jared: this is mostly a rubber stamp review request. Gijs already reviewed the patch and :stas indicated that he doesn't need to block it.

I fixed a couple issues discovered by this change and now am expecting a clean try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7efaa481bc94a487b364de19e5fa4f21152667c

The reason I'm flagging you is because of MozReview limitations - it doesn't allow me to take off stas from review list without re-requesting review from Gijs who is not taking reviews right now... :)

Comment 16

[:Gijs (he/him)]

Comment on attachment 8980008 [details]
Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.

https://reviewboard.mozilla.org/r/246192/#review252292

::: intl/l10n/Localization.jsm:156
(Diff revision 1)
> -      if (AppConstants.NIGHTLY_BUILD) {
> +      if (AppConstants.NIGHTLY_BUILD || Cu.isInAutomation) {
>          const locale = ctx.locales[0];
>          const ids = Array.from(missingIds).join(", ");
> +        if (Cu.isInAutomation) {
> +          throw new Error(`Missing translations in ${locale}: ${ids}`);
> +        } else {

Nit: No else after return should also mean no else after `throw`. :-)

::: intl/l10n/test/dom/test_domloc_overlay_missing_all.html:30
(Diff revision 1)
> -    await domLoc.translateFragment(document.body);
> -
> -    // we just care that it doesn't throw here.
> +    domLoc.translateFragment(document.body).then(() => {
> +      throw new Error("Expected translateFragment to throw");
> +    }, () => {
> +      // This should throw in testing.
> -    ok(1);
> +      ok(1);
> -
> -    SimpleTest.finish();
> +      SimpleTest.finish();
> -  });
> +    });

I think you're looking for Assert.rejects() or Assert.throws() here. :-)

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs (no reviews; away 25/5-5/6 inclusive; he/him) from comment #16)
> Comment on attachment 8980008 [details]
> Bug 1453765 - Switch Fluent from warning to throwing errors when in
> debug/testing mode.
> 
> https://reviewboard.mozilla.org/r/246192/#review252292
> 
> ::: intl/l10n/Localization.jsm:156
> (Diff revision 1)
> > -      if (AppConstants.NIGHTLY_BUILD) {
> > +      if (AppConstants.NIGHTLY_BUILD || Cu.isInAutomation) {
> >          const locale = ctx.locales[0];
> >          const ids = Array.from(missingIds).join(", ");
> > +        if (Cu.isInAutomation) {
> > +          throw new Error(`Missing translations in ${locale}: ${ids}`);
> > +        } else {
> 
> Nit: No else after return should also mean no else after `throw`. :-)

Mark, is there an existing option for this in the eslint rule, or is it something we could contribute to eslint, or is this harder / am I missing a reason "no else after return" shouldn't also imply "no else after throw" ? :-)

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8980008 [details]
Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246192/diff/1-2/

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8980008 [details]
Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.

https://reviewboard.mozilla.org/r/246192/#review252370

::: intl/l10n/test/dom/test_domloc_overlay_missing_all.html:30
(Diff revision 1)
> -    await domLoc.translateFragment(document.body);
> -
> -    // we just care that it doesn't throw here.
> +    domLoc.translateFragment(document.body).then(() => {
> +      throw new Error("Expected translateFragment to throw");
> +    }, () => {
> +      // This should throw in testing.
> -    ok(1);
> +      ok(1);
> -
> -    SimpleTest.finish();
> +      SimpleTest.finish();
> -  });
> +    });

This doesn't seem to be available in mochitests. I kept a custom version of it.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/541a8cda26a5
Switch Fluent from warning to throwing errors when in debug/testing mode. r=Gijs

Comment 21

[:Gijs (he/him)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19)
> Comment on attachment 8980008 [details]
> Bug 1453765 - Switch Fluent from warning to throwing errors when in
> debug/testing mode.
> 
> https://reviewboard.mozilla.org/r/246192/#review252370
> 
> ::: intl/l10n/test/dom/test_domloc_overlay_missing_all.html:30
> (Diff revision 1)
> > -    await domLoc.translateFragment(document.body);
> > -
> > -    // we just care that it doesn't throw here.
> > +    domLoc.translateFragment(document.body).then(() => {
> > +      throw new Error("Expected translateFragment to throw");
> > +    }, () => {
> > +      // This should throw in testing.
> > -    ok(1);
> > +      ok(1);
> > -
> > -    SimpleTest.finish();
> > +      SimpleTest.finish();
> > -  });
> > +    });
> 
> This doesn't seem to be available in mochitests. I kept a custom version of
> it.

Ugh. Mike, why aren't we providing Assert.jsm by default in mochitest-chrome ? Is there a bug on file to track that?

Comment 22

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/541a8cda26a5

Comment 23

[Mark Banner (:standard8)]

(In reply to :Gijs (no reviews; away 25/5-5/6 inclusive; he/him) from comment #17)
> (In reply to :Gijs (no reviews; away 25/5-5/6 inclusive; he/him) from
> comment #16)
> > Nit: No else after return should also mean no else after `throw`. :-)
> 
> Mark, is there an existing option for this in the eslint rule, or is it
> something we could contribute to eslint, or is this harder / am I missing a
> reason "no else after return" shouldn't also imply "no else after throw" ?
> :-)

Looks like ESLint doesn't have that available at the moment:

https://github.com/eslint/eslint/issues/8028

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs (no reviews; away 25/5-5/6 inclusive; he/him) from comment #21)
> Ugh. Mike, why aren't we providing Assert.jsm by default in mochitest-chrome
> ? Is there a bug on file to track that?

There's no bug for that. Since mochitest-bc and xpcshell run with test harness code, I was able to load the module and inject Assert in the test scope.
With mochitest-chrome you roll your on (err, copy-paste your own). There are a couple of tests that do that - I did a few during the e10s test rewrite sprint - but that's the best I could do. At least, that's what I thought.
Please feel free to file a bug in the testing component and CC me; I'm totally interested in getting there!