1353680 - Create automated test to check for string conflicts with cross-channel localization

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Description

[Axel Hecht [:Pike]]

With x-channel localization, modifying exisisting strings becomes not only a question of modifying the current strings, but also those used in the past in versions we still ship.

As part of x-channel, we should finally have a test run in automation that checks if developers modify strings they shouldn't.

This should be a tier-1 test, run as part of regular Firefox automation, similar to linters.

The code for this might live in m-c, or vct.

I'm not yet 100% how we build this exactly, though. Maybe we can just compare the strings in the current repo with the known-good-bookmarks in the generated x-channel repo?

Comment 1

[Axel Hecht [:Pike]]

Attachment: bug 1353680, create test to prevent bad content in localizable strings

TBD: Still need to figure out where to put the whitelist of strings
that are actually OK to exist in different versions.

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

Comment 2

[Axel Hecht [:Pike]]

Attachment: bug 1353680, hook up l10n linter to automation

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

Comment 3

[Axel Hecht [:Pike]]

Pushed to try with an additional commit to actually have something to complain about.

Comment 4

[Axel Hecht [:Pike]]

Comment on attachment 8915747 [details]
bug 1353680, create test to prevent bad content in localizable strings

Thanks for offering the reviews, Andrew.

Starting with a feedback request for now. One thing we'll need to change for sure is the upstream repo url once bug 1405985 is fixed.

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 8915748 [details]
bug 1353680, hook up l10n linter to automation

This works great in ./mach try fuzzy if I explicitly request the linter to be run.

Sadly, https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0d7ff91ca59aada2a80e8ebb578ecd68c37cb09 I just tried to create a build, hoping the linters would run, too, but they didn't.

Is there a way to ensure that the linters run, even if they're not explicitly asked for?

Comment 6

[Axel Hecht [:Pike]]

To make it a bit more crisp what I'm testing here now:

For one, I'm testing if the current file actually validates against itself. This should catch parsing errors, and bad parameters to some extent, at least.

In addition, I check the current file against the corresponding file in our gecko-strings repository, which combines the strings from central and beta, and so forth as this rides the trains. This prevents changing strings accidently, or adding strings on central which are already used on release by the same ID.

One open question that I need to think more about is that we'll want a manually maintained whitelist for changes that are actually OK. I haven't really made up my mind on where that best lives, as the test will be run against m-c, m-b, and eventually release. So, having the whitelist in-tree might require uplifting it to multiple branches. Or, maybe the whitelist should be in the gecko-strings repository itself. Or, we have it in m-c, but make tests on beta fetch it from m-c. All the latter options make it harder to test a successful modification of the whitelist on try, though.

Comment 7

[Andrew Halberstadt [:ahal]]

(In reply to Axel Hecht [:Pike] from comment #5)
> Comment on attachment 8915748 [details]
> bug 1353680, hook up l10n linter to automation
> 
> This works great in ./mach try fuzzy if I explicitly request the linter to
> be run.
> 
> Sadly,
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a0d7ff91ca59aada2a80e8ebb578ecd68c37cb09 I just tried
> to create a build, hoping the linters would run, too, but they didn't.
> 
> Is there a way to ensure that the linters run, even if they're not
> explicitly asked for?

No, not with |mach try fuzzy| at least. That's kind of by design, it's goal is more to schedule *exactly* what was requested without any sort of magic. I believe it should be scheduled automatically if using try syntax or a blank push though. It will also get run for anyone who has one of the hooks enabled:
https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915747 [details]
bug 1353680, create test to prevent bad content in localizable strings

https://reviewboard.mozilla.org/r/186954/#review192334

::: python/mozlint/mozlint/pathutils.py:18
(Diff revision 1)
> +        if exclude is None:
> +            exclude = []
>          self.exclude = exclude

self.exclude = exclude or []

What error was this fixing? Might be good to add a test case.

::: tools/lint/l10n.yml:7
(Diff revision 1)
> +        - browser/locales/en-US
> +        - browser/branding/official/locales/en-US
> +        - browser/extensions/onboarding/locales/en-US
> +        - browser/extensions/webcompat-reporter/locales/en-US
> +        - devtools/client/locales/en-US
> +        - devtools/shared/locales/en-US
> +        - devtools/shim/locales/en-US
> +        - dom/locales/en-US
> +        - netwerk/locales/en-US
> +        - security/manager/locales/en-US
> +        - services/sync/locales/en-US
> +        - toolkit/locales/en-US

This could be `**/locales/en-US/**` if you want.

::: tools/lint/python/l10n_lint.py:25
(Diff revision 1)
> +    mozversioncontrol.repoupdate.update_mercurial_repo(
> +        hg,
> +        'https://hg.mozilla.org/users/axel_mozilla.com/gecko-strings',
> +        mozpath.join(state_dir, 'gecko-strings'))

Do you know how long this takes? I ask because if people have the commit/push hook configured, it could slow down their vcs operations. Though it would only run if they modified a file under `locale/en-US`, so maybe it's not a big deal.

::: tools/lint/python/l10n_lint.py:37
(Diff revision 1)
> +        return [
> +            result.from_config(config,
> +                               path=config['path'],
> +                               message='l10n linter not properly configured')]

Better to return raise an exception here, users shouldn't have to deal with this.

::: tools/lint/python/l10n_lint.py:53
(Diff revision 1)
> +    all_files = []
> +    for p in paths:
> +        fp = pathutils.FilterPath(p)
> +        if fp.isdir:
> +            for _, fileobj in fp.finder:
> +                all_files.append(fileobj.path)
> +        if fp.isfile:
> +            all_files.append(p)
> +    # filter again, our directories might have picked up files we ignore
> +    all_files = pathutils.filterpaths(all_files, config, **lintargs)

What's the reason for re-doing path filtering? Would be good to at least have a detailed comment explaining why this is necessary.

::: tools/lint/python/l10n_lint.py:66
(Diff revision 1)
> +    # filter again, our directories might have picked up files we ignore
> +    all_files = pathutils.filterpaths(all_files, config, **lintargs)
> +    obs = compare.Observer(file_stats=True)
> +    comparer = compare.ContentComparer([obs])
> +    results = []
> +    for path in all_files:

I can't say I understand what's happening from here on down, but from a mozlint point of view, it looks fine. You might want to get a co-review from someone familiar with compare-locales.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8915747 [details]
bug 1353680, create test to prevent bad content in localizable strings

Looks mostly good! Fyi, I'm going to be away from now until Wednesday. If you can't wait that long, you could ask :bc for review.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 8915747 [details]
bug 1353680, create test to prevent bad content in localizable strings

https://reviewboard.mozilla.org/r/186954/#review213798

::: python/mozlint/mozlint/pathutils.py:18
(Diff revision 1)
> +        if exclude is None:
> +            exclude = []
>          self.exclude = exclude

FilterPath.finder is currently only used after explicitly setting .exclude, so the argument to the constructor is actually not used at all.

I'll add a test.

::: tools/lint/l10n.yml:7
(Diff revision 1)
> +        - browser/locales/en-US
> +        - browser/branding/official/locales/en-US
> +        - browser/extensions/onboarding/locales/en-US
> +        - browser/extensions/webcompat-reporter/locales/en-US
> +        - devtools/client/locales/en-US
> +        - devtools/shared/locales/en-US
> +        - devtools/shim/locales/en-US
> +        - dom/locales/en-US
> +        - netwerk/locales/en-US
> +        - security/manager/locales/en-US
> +        - services/sync/locales/en-US
> +        - toolkit/locales/en-US

I'd like to keep these more flexible, there's stuff in directories called `locales/en-US` that's not our problem, and we might want to expose things to l10n that are not in a `locales/en-US` path in the future.

::: tools/lint/python/l10n_lint.py:25
(Diff revision 1)
> +    mozversioncontrol.repoupdate.update_mercurial_repo(
> +        hg,
> +        'https://hg.mozilla.org/users/axel_mozilla.com/gecko-strings',
> +        mozpath.join(state_dir, 'gecko-strings'))

The linter in total takes some 3 seconds to lint all of browser/locales/en-US. Looking at the linter times I get for browser/components, that looks OK?

::: tools/lint/python/l10n_lint.py:53
(Diff revision 1)
> +    all_files = []
> +    for p in paths:
> +        fp = pathutils.FilterPath(p)
> +        if fp.isdir:
> +            for _, fileobj in fp.finder:
> +                all_files.append(fileobj.path)
> +        if fp.isfile:
> +            all_files.append(p)
> +    # filter again, our directories might have picked up files we ignore
> +    all_files = pathutils.filterpaths(all_files, config, **lintargs)

We rarely have files that are just optional, or not part of the regular l10n workflow, but still in a localizable location. firefox-l10.js is an example. I'll try to make the comment more explicit.

::: tools/lint/python/l10n_lint.py:66
(Diff revision 1)
> +    # filter again, our directories might have picked up files we ignore
> +    all_files = pathutils.filterpaths(all_files, config, **lintargs)
> +    obs = compare.Observer(file_stats=True)
> +    comparer = compare.ContentComparer([obs])
> +    results = []
> +    for path in all_files:

I'll get an additional reviewer from our team.

Comment 11

[Axel Hecht [:Pike]]

Heads up, I'm resurrecting this bug from the dead. Also, moving from mozreview to phabricator, and I'll volunteer flod to be our internal reviewer.

I think I managed to make sense of the comments in mozreview that were reflected here.

Notable differences, as we won't have an interdiff:

• I dropped the check for l10n_configs completely. As Andrew suggested to throw an exception, might as well be a KeyError. I didn't check other entries from the l10n.yml, too.
• I added a check that there's no missing entries in include in l10n.yml
• Added support-files to rerun the linter on updates to compare-locales
• Generally better comments
• Since back then, compare-locales has a --validate option, mimic what that is doing in detail.
• Added an exception logic

Flod, as we talked about this: This should actually check for conflicting strings in beta ;-)

A bit more detail on the exception logic:

The idea is that we allow strings to change if they're explicitly white-listed in l10n.yml, by file name and ID. The whitelist should be pruned once the changed string is in gecko-strings. I'm not particularly fond of this, but I also don't have a better idea. Maybe Andrew?

Comment 12

[Axel Hecht [:Pike]]

Attachment: Bug 1353680, create test to prevent bad content in localizable strings, r=ahal,flod

Comment 13

[Axel Hecht [:Pike]]

Attachment: Bug 1353680, modify some strings with whitelist, add back OK beta and conflicting beta

Depends on D20465

Comment 14

[Axel Hecht [:Pike]]

Attachment: Bug 1353680, modify some strings with whitelist, add back OK beta and conflicting beta

Depends on D20465

Comment 15

[Axel Hecht [:Pike]]

Attachment: Bug 1353680, add lintargs to lint setup, r=ahal

Comment 16

[Phabricator Automation]

Comment on attachment 9046344 [details]
Bug 1353680, add lintargs to lint setup, r=ahal

Revision D21010 was moved to bug 1530352. Setting attachment 9046344 [details] to obsolete.

Comment 17

[Axel Hecht [:Pike]]

Andrew, I started addressing your comments, and then got stuck on the setup part.

Turns out that setup runs for all linters available, so in the case where you just change python files, we still call the eslint setup. That's probably OK as long as setup is dead-fast, but the l10n setup isn't that fast.

I'm wondering how to mitigate that.

I could refactor the setup part a bit, and only call setup for linters that are actually gonna get called. Maybe only do that if --setup isn't passed in?

Or, add a separate hook that's more like pre-run. Also done in a single step before parallelizing out into the actual linter jobs, but only run for the linters that are run. That would leave the setup behavior as it is right now.

What do you think?

Comment 18

[Andrew Halberstadt [:ahal]]

I think only calling the setup functions of linters that are running is very reasonable (and even desirable), but it's also adding scope bloat. So unless you're motivated to make that change, it might just be best to just move that logic back to the start of the lint function (I'll leave this up to you). I know I originally said to move it to setup, sorry for the churn :(.

Comment 19

[Axel Hecht [:Pike]]

Attachment: Bug 1353680, update compare-locales to 7.2.1, r=flod

Comment 20

[Axel Hecht [:Pike]]

Attachment: Bug 1353680, test the linter on phabricator, r=flod

Depends on D20465

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dce9826524bc
update compare-locales to 7.2.1, r=flod
https://hg.mozilla.org/integration/autoland/rev/af9d93e2536a
create test to prevent bad content in localizable strings, r=ahal,flod

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/dce9826524bc
https://hg.mozilla.org/mozilla-central/rev/af9d93e2536a

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Great work, bravo :)