Closed Bug 1453765 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Internationalization, enhancement, P2)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1453765 - Switch Fluent from warning to throwing errors when in debug/testing mode.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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?
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?
Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(francesco.lodolo)
Blocks: 1415730
Priority: -- → P2
(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.
Flags: needinfo?(gijskruitbosch+bugs)
> 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.
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
Flags: needinfo?(francesco.lodolo)
(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.
(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.
Flags: needinfo?(jaws)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(stas)
Flags: needinfo?(l10n)
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 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.
Attachment #8974591 - Flags: review?(gijskruitbosch+bugs) → review+
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.
> 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 `""`?
(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? :-)
> 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.
Attachment #8974591 - Attachment is obsolete: true
Attachment #8974591 - Flags: review?(stas)
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 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. :-)
Attachment #8980008 - Flags: review+
(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" ? :-)
Flags: needinfo?(standard8)
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.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/541a8cda26a5
Switch Fluent from warning to throwing errors when in debug/testing mode. r=Gijs
(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?
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/541a8cda26a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(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
Flags: needinfo?(standard8)
(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!
Flags: needinfo?(mdeboer)
Depends on: 1483036
See Also: → 1581098
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: