Add presubmit for translation expectations

Background (taken from crbug/583195):
translation_expectations.pyl contains the list of translatable grd
files and the languages they're expected to be translated into. It's
used internally during the translation process to determine which grd
files should be copied to google3 for translation, etc. Each
repository with grd files (desktop, Android, iOS) has a
translation_expectations.pyl file, which must list every grd file that
contains strings.

translation_expectations.pyl isn't used at all when building (neither
locally nor on the bots), so there's no indication to developers if
translation_expectations.pyl needs to be updated (e.g. because a grd
file was added) or is malformed. Instead, an error will occur much
later when the weekly translation run happens, causing unnecessary
back-and-forth between TPMs and developers and introducing delays.
This happens every few weeks.

This CL adds a presubmit that checks if the contents of the
translation_expectations.pyl matches the list of .grd files in the
repository. The presubmit only runs if a .grd or .grdp file is
modified, so in most cases it's a no-op. It lists the names of missing
and unlisted files in the warning.

Bug: 583195
Change-Id: I5013311dac5db1f0578f59022832dc8e33c1ee37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1464078Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748339}

PRESUBMIT.py

@@ -4245,6 +4245,7 @@ def _CommonChecks(input_api, output_api):
   results.extend(input_api.RunTests(
     input_api.canned_checks.CheckVPythonSpec(input_api, output_api)))
   results.extend(_CheckTranslationScreenshots(input_api, output_api))
+  results.extend(_CheckTranslationExpectations(input_api, output_api))
   results.extend(_CheckCorrectProductNameInMessages(input_api, output_api))
   results.extend(_CheckBuildtoolsRevisionsAreInSync(input_api, output_api))
   results.extend(_CheckForTooLargeFiles(input_api, output_api))
@@ -4829,3 +4830,48 @@ def _CheckTranslationScreenshots(input_api, output_api):
       sorted(unnecessary_sha1_files)))
 
   return results
+
+
+def _CheckTranslationExpectations(input_api, output_api,
+                                  repo_root=None,
+                                  translation_expectations_path=None,
+                                  grd_files=None):
+  import sys
+  affected_grds = [f for f in input_api.AffectedFiles()
+      if (f.LocalPath().endswith('.grd') or
+          f.LocalPath().endswith('.grdp'))]
+  if not affected_grds:
+    return []
+
+  try:
+    old_sys_path = sys.path
+    sys.path = sys.path + [
+        input_api.os_path.join(
+            input_api.PresubmitLocalPath(), 'tools', 'translation')]
+    from helper import git_helper
+    from helper import translation_helper
+  finally:
+    sys.path = old_sys_path
+
+  # Check that translation expectations can be parsed and we can get a list of
+  # translatable grd files. |repo_root| and |translation_expectations_path| are
+  # only passed by tests.
+  if not repo_root:
+    repo_root = input_api.PresubmitLocalPath()
+  if not translation_expectations_path:
+    translation_expectations_path =  input_api.os_path.join(
+        repo_root, 'tools', 'gritsettings',
+        'translation_expectations.pyl')
+  if not grd_files:
+    grd_files = git_helper.list_grds_in_repository(repo_root)
+
+  try:
+    translation_helper.get_translatable_grds(repo_root, grd_files,
+                                             translation_expectations_path)
+  except Exception as e:
+    return [output_api.PresubmitNotifyResult(
+      'Failed to get a list of translatable grd files. This happens when:\n'
+      ' - One of the modified grd or grdp files cannot be parsed or\n'
+      ' - %s is not updated.\n'
+      'Stack:\n%s' % (translation_expectations_path, str(e)))]
+  return []

PRESUBMIT_test.py

@@ -2407,6 +2407,7 @@ class CheckNoDirectIncludesHeadersWhichRedefineStrCat(unittest.TestCase):
     results = PRESUBMIT._CheckNoStrCatRedefines(mock_input_api, MockOutputApi())
     self.assertEquals(0, len(results))
 
+
 class TranslationScreenshotsTest(unittest.TestCase):
   # An empty grd file.
   OLD_GRD_CONTENTS = """<?xml version="1.0" encoding="UTF-8"?>
@@ -2742,6 +2743,136 @@ class TranslationScreenshotsTest(unittest.TestCase):
     self.assertEqual([], warnings)
 
 
+class TranslationExpectationsTest(unittest.TestCase):
+  ERROR_MESSAGE_FORMAT = (
+    "Failed to get a list of translatable grd files. "
+    "This happens when:\n"
+    " - One of the modified grd or grdp files cannot be parsed or\n"
+    " - %s is not updated.\n"
+    "Stack:\n"
+  )
+  REPO_ROOT = os.path.join('tools', 'translation', 'testdata')
+  # This lists all .grd files under REPO_ROOT.
+  EXPECTATIONS = os.path.join(REPO_ROOT,
+                              "translation_expectations.pyl")
+  # This lists all .grd files under REPO_ROOT except unlisted.grd.
+  EXPECTATIONS_WITHOUT_UNLISTED_FILE = os.path.join(
+      REPO_ROOT, "translation_expectations_without_unlisted_file.pyl")
+
+  # Tests that the presubmit doesn't return when no grd or grdp files are
+  # modified.
+  def testExpectationsNoModifiedGrd(self):
+    input_api = MockInputApi()
+    input_api.files = [
+        MockAffectedFile('not_used.txt', 'not used', 'not used', action='M')
+    ]
+    # Fake list of all grd files in the repo. This list is missing all grd/grdps
+    # under tools/translation/testdata. This is OK because the presubmit won't
+    # run in the first place since there are no modified grd/grps in input_api.
+    grd_files = ['doesnt_exist_doesnt_matter.grd']
+    warnings = PRESUBMIT._CheckTranslationExpectations(
+        input_api, MockOutputApi(), self.REPO_ROOT, self.EXPECTATIONS,
+        grd_files)
+    self.assertEqual(0, len(warnings))
+
+
+  # Tests that the list of files passed to the presubmit matches the list of
+  # files in the expectations.
+  def testExpectationsSuccess(self):
+    # Mock input file list needs a grd or grdp file in order to run the
+    # presubmit. The file itself doesn't matter.
+    input_api = MockInputApi()
+    input_api.files = [
+        MockAffectedFile('dummy.grd', 'not used', 'not used', action='M')
+    ]
+    # List of all grd files in the repo.
+    grd_files = ['test.grd', 'unlisted.grd', 'not_translated.grd',
+                 'internal.grd']
+    warnings = PRESUBMIT._CheckTranslationExpectations(
+        input_api, MockOutputApi(), self.REPO_ROOT, self.EXPECTATIONS,
+        grd_files)
+    self.assertEqual(0, len(warnings))
+
+  # Tests that the presubmit warns when a file is listed in expectations, but
+  # does not actually exist.
+  def testExpectationsMissingFile(self):
+    # Mock input file list needs a grd or grdp file in order to run the
+    # presubmit.
+    input_api = MockInputApi()
+    input_api.files = [
+      MockAffectedFile('dummy.grd', 'not used', 'not used', action='M')
+    ]
+    # unlisted.grd is listed under tools/translation/testdata but is not
+    # included in translation expectations.
+    grd_files = ['unlisted.grd', 'not_translated.grd', 'internal.grd']
+    warnings = PRESUBMIT._CheckTranslationExpectations(
+        input_api, MockOutputApi(), self.REPO_ROOT, self.EXPECTATIONS,
+        grd_files)
+    self.assertEqual(1, len(warnings))
+    self.assertTrue(warnings[0].message.startswith(
+        self.ERROR_MESSAGE_FORMAT % self.EXPECTATIONS))
+    self.assertTrue(
+        ("test.grd is listed in the translation expectations, "
+         "but this grd file does not exist")
+        in warnings[0].message)
+
+  # Tests that the presubmit warns when a file is not listed in expectations but
+  # does actually exist.
+  def testExpectationsUnlistedFile(self):
+    # Mock input file list needs a grd or grdp file in order to run the
+    # presubmit.
+    input_api = MockInputApi()
+    input_api.files = [
+      MockAffectedFile('dummy.grd', 'not used', 'not used', action='M')
+    ]
+    # unlisted.grd is listed under tools/translation/testdata but is not
+    # included in translation expectations.
+    grd_files = ['test.grd', 'unlisted.grd', 'not_translated.grd',
+                 'internal.grd']
+    warnings = PRESUBMIT._CheckTranslationExpectations(
+        input_api, MockOutputApi(), self.REPO_ROOT,
+        self.EXPECTATIONS_WITHOUT_UNLISTED_FILE, grd_files)
+    self.assertEqual(1, len(warnings))
+    self.assertTrue(warnings[0].message.startswith(
+        self.ERROR_MESSAGE_FORMAT % self.EXPECTATIONS_WITHOUT_UNLISTED_FILE))
+    self.assertTrue(
+        ("unlisted.grd appears to be translatable "
+         "(because it contains <file> or <message> elements), "
+         "but is not listed in the translation expectations.")
+        in warnings[0].message)
+
+  # Tests that the presubmit warns twice:
+  # - for a non-existing file listed in expectations
+  # - for an existing file not listed in expectations
+  def testMultipleWarnings(self):
+    # Mock input file list needs a grd or grdp file in order to run the
+    # presubmit.
+    input_api = MockInputApi()
+    input_api.files = [
+      MockAffectedFile('dummy.grd', 'not used', 'not used', action='M')
+    ]
+    # unlisted.grd is listed under tools/translation/testdata but is not
+    # included in translation expectations.
+    # test.grd is not listed under tools/translation/testdata but is included
+    # in translation expectations.
+    grd_files = ['unlisted.grd', 'not_translated.grd', 'internal.grd']
+    warnings = PRESUBMIT._CheckTranslationExpectations(
+        input_api, MockOutputApi(), self.REPO_ROOT,
+        self.EXPECTATIONS_WITHOUT_UNLISTED_FILE, grd_files)
+    self.assertEqual(1, len(warnings))
+    self.assertTrue(warnings[0].message.startswith(
+        self.ERROR_MESSAGE_FORMAT % self.EXPECTATIONS_WITHOUT_UNLISTED_FILE))
+    self.assertTrue(
+        ("unlisted.grd appears to be translatable "
+         "(because it contains <file> or <message> elements), "
+         "but is not listed in the translation expectations.")
+        in warnings[0].message)
+    self.assertTrue(
+        ("test.grd is listed in the translation expectations, "
+         "but this grd file does not exist")
+        in warnings[0].message)
+
+
 class DISABLETypoInTest(unittest.TestCase):
 
   def testPositive(self):

tools/gritsettings/translation_expectations.pyl

@@ -107,6 +107,7 @@
     "tools/grit/grit/testdata/whitelist_strings.grd": "Test data",
     "tools/translation/testdata/not_translated.grd": "Test data",
     "tools/translation/testdata/test.grd": "Test data",
+    "tools/translation/testdata/unlisted.grd": "Test data",
     "ui/base/test/ui_base_test_resources.grd": "Test data",
     "ui/strings/app_locale_settings.grd": "Not UI strings; localized separately",
   },

tools/translation/helper/git_helper.py

+# Copyright 2019 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import subprocess
+import sys
+
+if sys.platform.startswith('win'):
+  # Use the |git.bat| in the depot_tools/ on Windows.
+  GIT = 'git.bat'
+else:
+  GIT = 'git'
+
+
+def list_grds_in_repository(repo_path):
+  """Returns a list of all the grd files in the current git repository."""
+  # This works because git does its own glob expansion even though there is no
+  # shell to do it.
+  # TODO(meacer): This should use list_grds_in_repository() from the internal
+  #               translate.py.
+  output = subprocess.check_output([GIT, 'ls-files', '--', '*.grd'],
+                                   cwd=repo_path)
+  return output.strip().splitlines()
+
+
+def git_add(files, repo_root):
+  """Adds relative paths given in files to the current CL."""
+  # Upload in batches in order to not exceed command line length limit.
+  BATCH_SIZE = 50
+  added_count = 0
+  while added_count < len(files):
+    batch = files[added_count:added_count + BATCH_SIZE]
+    command = [GIT, 'add'] + batch
+    subprocess.check_call(command, cwd=repo_root)
+    added_count += len(batch)

tools/translation/helper/translation_helper_unittest.py

@@ -18,7 +18,8 @@ class TcHelperTest(unittest.TestCase):
   def test_get_translatable_grds(self):
     grds = translation_helper.get_translatable_grds(
         testdata_path, ['test.grd', 'not_translated.grd', 'internal.grd'],
-        os.path.join(testdata_path, 'translation_expectations.pyl'))
+        os.path.join(testdata_path,
+                     'translation_expectations_without_unlisted_file.pyl'))
     self.assertEqual(1, len(grds))
 
     # There should be no references to not_translated.grd (mentioning the
@@ -39,8 +40,8 @@ class TcHelperTest(unittest.TestCase):
   # The expectations list an untranslatable file (not_translated.grd), but the
   # grd list doesn't contain it.
   def test_missing_untranslatable(self):
-    TRANSLATION_EXPECTATIONS = os.path.join(testdata_path,
-                                            'translation_expectations.pyl')
+    TRANSLATION_EXPECTATIONS = os.path.join(
+        testdata_path, 'translation_expectations_without_unlisted_file.pyl')
     with self.assertRaises(Exception) as context:
       translation_helper.get_translatable_grds(
           testdata_path, ['test.grd', 'internal.grd'], TRANSLATION_EXPECTATIONS)
@@ -53,8 +54,8 @@ class TcHelperTest(unittest.TestCase):
   # The expectations list an internal file (internal.grd), but the grd list
   # doesn't contain it.
   def test_missing_internal(self):
-    TRANSLATION_EXPECTATIONS = os.path.join(testdata_path,
-                                            'translation_expectations.pyl')
+    TRANSLATION_EXPECTATIONS = os.path.join(
+        testdata_path, 'translation_expectations_without_unlisted_file.pyl')
     with self.assertRaises(Exception) as context:
       translation_helper.get_translatable_grds(
           testdata_path, ['test.grd', 'not_translated.grd'],
@@ -68,8 +69,8 @@ class TcHelperTest(unittest.TestCase):
   # The expectations list a translatable file (test.grd), but the grd list
   # doesn't contain it.
   def test_missing_translatable(self):
-    TRANSLATION_EXPECTATIONS = os.path.join(testdata_path,
-                                            'translation_expectations.pyl')
+    TRANSLATION_EXPECTATIONS = os.path.join(
+        testdata_path, 'translation_expectations_without_unlisted_file.pyl')
     with self.assertRaises(Exception) as context:
       translation_helper.get_translatable_grds(
           testdata_path, ['not_translated.grd', 'internal.grd'],
@@ -83,8 +84,8 @@ class TcHelperTest(unittest.TestCase):
   # The grd list contains a file (part.grdp) that's not listed in translation
   # expectations.
   def test_expectations_not_updated(self):
-    TRANSLATION_EXPECTATIONS = os.path.join(testdata_path,
-                                            'translation_expectations.pyl')
+    TRANSLATION_EXPECTATIONS = os.path.join(
+        testdata_path, 'translation_expectations_without_unlisted_file.pyl')
     with self.assertRaises(Exception) as context:
       translation_helper.get_translatable_grds(
           testdata_path,

tools/translation/testdata/translation_expectations.pyl

@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 #
+# This file is used in TranslationExpectationsTest in PRESUBMIT_test.py
+#
 # This is a .pyl, or "Python Literal", file. You can treat it just like a
 # .json file, with the following exceptions:
 # * all keys must be quoted (use single quotes, please);
@@ -17,6 +19,7 @@
     ],
     "files": [
       "test.grd",
+      "unlisted.grd"
     ],
   },
   # Grd files that contain <message> or <translations> elements, but that

tools/translation/testdata/translation_expectations_without_unlisted_file.pyl

+# Copyright 2018 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+#
+# This file is used in TranslationExpectationsTest in PRESUBMIT_test.py
+#
+# This is a .pyl, or "Python Literal", file. You can treat it just like a
+# .json file, with the following exceptions:
+# * all keys must be quoted (use single quotes, please);
+# * comments are allowed, using '#' syntax; and
+# * trailing commas are allowed.
+#
+# Specifies which grd files should be translated and into which languages they
+# should be translated. Used by the internal translation process.
+{
+  "desktop_grds": {
+    "languages": [
+      "en-GB"
+    ],
+    "files": [
+      "test.grd",
+      # Missing unlisted.grd.
+    ],
+  },
+  # Grd files that contain <message> or <translations> elements, but that
+  # shouldn't be translated as part of the normal translation process. Each
+  # entry needs an explanation for why it shouldn't be translated.
+  "untranslated_grds": {
+    "not_translated.grd": "Not translated"
+  },
+  # Grd files that should not be processed, since they might contain parts not
+  # available publicly.
+  "internal_grds": [
+    "internal.grd",
+  ],
+}

tools/translation/testdata/unlisted.grd

+<?xml version="1.0" encoding="UTF-8"?>
+
+<grit latest_public_release="1" current_release="1">
+  <translations>
+      <file path="unlisted_en-GB.xtb" lang="en-GB" />
+  </translations>
+  <release seq="1">
+    <messages>
+    </messages>
+  </release>
+</grit>

tools/translation/testdata/unlisted_en-GB.xtb

+<?xml version="1.0" ?>
+<!DOCTYPE translationbundle>
+<translationbundle lang="en-GB">
+</translationbundle>
\ No newline at end of file

tools/translation/upload_screenshots.py

@@ -24,6 +24,7 @@ import os
 import subprocess
 
 import helper.translation_helper as translation_helper
+import helper.git_helper as git_helper
 
 here = os.path.dirname(os.path.realpath(__file__))
 src_path = os.path.normpath(os.path.join(here, '..', '..'))
@@ -83,31 +84,11 @@ def query_yes_no(question, default='no'):
       print("Please respond with 'yes' or 'no' (or 'y' or 'n').")
 
 
-def list_grds_in_repository(repo_path):
-  """Returns a list of all the grd files in the current git repository."""
-  # This works because git does its own glob expansion even though there is no
-  # shell to do it.
-  output = subprocess.check_output(
-      [GIT, 'ls-files', '--', '*.grd'], cwd=repo_path)
-  return output.strip().splitlines()
-
-
-def git_add(files, repo_root):
-  """Adds relative paths given in files to the current CL."""
-  # Upload in batches in order to not exceed command line length limit.
-  BATCH_SIZE = 50
-  added_count = 0
-  while added_count < len(files):
-    batch = files[added_count:added_count+BATCH_SIZE]
-    command = [GIT, 'add'] + batch
-    subprocess.check_call(command, cwd=repo_root)
-    added_count += len(batch)
-
-
 def find_screenshots(repo_root, translation_expectations):
   """Returns a list of translation related .png files in the repository."""
   translatable_grds = translation_helper.get_translatable_grds(
-      repo_root, list_grds_in_repository(repo_root), translation_expectations)
+      repo_root, git_helper.list_grds_in_repository(repo_root),
+      translation_expectations)
 
   # Add the paths of grds and any files they include. This includes grdp files
   # and files included via <structure> elements.
@@ -208,7 +189,7 @@ def main():
     exit(0)
 
   if not args.dry_run:
-    git_add(signatures, src_path)
+    git_helper.git_add(signatures, src_path)
 
   print('DONE.')