Add presubmit check for relnotes field in accessibility changes.

R=akihiroota,dmazzoni,jam,dpranke

Change-Id: I37d5035967941da1d2e3d671426133668b87f0f3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2162427
Commit-Queue: Chris Hall <chrishall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Akihiro Ota <akihiroota@chromium.org>
Cr-Commit-Position: refs/heads/master@{#764594}

PRESUBMIT.py

@@ -4196,6 +4196,46 @@ def _AndroidSpecificOnCommitChecks(input_api, output_api):
   results.extend(_CheckAndroidXmlStyle(input_api, output_api, False))
   return results
 
+# TODO(chrishall): could we additionally match on any path owned by
+#                  ui/accessibility/OWNERS ?
+_ACCESSIBILITY_PATHS = (
+    r"^chrome[\\/]browser.*[\\/]accessibility[\\/]",
+    r"^chrome[\\/]browser[\\/]extensions[\\/]api[\\/]automation.*[\\/]",
+    r"^chrome[\\/]renderer[\\/]extensions[\\/]accessibility_.*",
+    r"^chrome[\\/]tests[\\/]data[\\/]accessibility[\\/]",
+    r"^content[\\/]browser[\\/]accessibility[\\/]",
+    r"^content[\\/]renderer[\\/]accessibility[\\/]",
+    r"^content[\\/]tests[\\/]data[\\/]accessibility[\\/]",
+    r"^extensions[\\/]renderer[\\/]api[\\/]automation[\\/]",
+    r"^ui[\\/]accessibility[\\/]",
+    r"^ui[\\/]views[\\/]accessibility[\\/]",
+)
+
+def _CheckAccessibilityRelnotesField(input_api, output_api):
+  """Checks that commits to accessibility code contain an AX-Relnotes field in
+  their commit message."""
+  def FileFilter(affected_file):
+    paths = _ACCESSIBILITY_PATHS
+    return input_api.FilterSourceFile(affected_file, white_list=paths)
+
+  # Only consider changes affecting accessibility paths.
+  if not any(input_api.AffectedFiles(file_filter=FileFilter)):
+    return []
+
+  relnotes = input_api.change.GitFootersFromDescription().get('AX-Relnotes', [])
+  if relnotes:
+    return []
+
+  # TODO(chrishall): link to Relnotes documentation in message.
+  message = ("Missing 'AX-Relnotes:' field required for accessibility changes"
+             "\n  please add 'AX-Relnotes: [release notes].' to describe any "
+             "user-facing changes"
+             "\n  otherwise add 'AX-Relnotes: n/a.' if this change has no "
+             "user-facing effects"
+             "\n  if this is confusing or annoying then please contact members "
+             "of ui/accessibility/OWNERS.")
+
+  return [output_api.PresubmitNotifyResult(message)]
 
 def _CommonChecks(input_api, output_api):
   """Checks common to both upload and commit."""
@@ -4209,6 +4249,7 @@ def _CommonChecks(input_api, output_api):
     results.extend(
         input_api.canned_checks.CheckAuthorizedAuthor(input_api, output_api))
 
+  results.extend(_CheckAccessibilityRelnotesField(input_api, output_api))
   results.extend(
       _CheckNoProductionCodeUsingTestOnlyFunctions(input_api, output_api))
   results.extend(

PRESUBMIT_test.py

@@ -1022,6 +1022,90 @@ class IncludeGuardTest(unittest.TestCase):
                      'Header using the wrong include guard name '
                      'WrongInBlink_h')
 
+class AccessibilityRelnotesFieldTest(unittest.TestCase):
+  def testRelnotesPresent(self):
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [MockAffectedFile('ui/accessibility/foo.bar', [''])]
+    mock_input_api.change.footers['AX-Relnotes'] = [
+        'Important user facing change']
+
+    msgs = PRESUBMIT._CheckAccessibilityRelnotesField(
+        mock_input_api, mock_output_api)
+    self.assertEqual(0, len(msgs),
+                     'Expected %d messages, found %d: %s'
+                     % (0, len(msgs), msgs))
+
+  def testRelnotesMissingFromAccessibilityChange(self):
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [
+        MockAffectedFile('some/file', ['']),
+        MockAffectedFile('ui/accessibility/foo.bar', ['']),
+        MockAffectedFile('some/other/file', [''])
+    ]
+
+    msgs = PRESUBMIT._CheckAccessibilityRelnotesField(
+        mock_input_api, mock_output_api)
+    self.assertEqual(1, len(msgs),
+                     'Expected %d messages, found %d: %s'
+                     % (1, len(msgs), msgs))
+    self.assertTrue("Missing 'AX-Relnotes:' field" in msgs[0].message,
+                    'Missing AX-Relnotes field message not found in errors')
+
+  # The relnotes footer is not required for changes which do not touch any
+  # accessibility directories.
+  def testIgnoresNonAccesssibilityCode(self):
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+
+    mock_input_api.files = [
+        MockAffectedFile('some/file', ['']),
+        MockAffectedFile('some/other/file', [''])
+    ]
+
+    msgs = PRESUBMIT._CheckAccessibilityRelnotesField(
+        mock_input_api, mock_output_api)
+    self.assertEqual(0, len(msgs),
+                     'Expected %d messages, found %d: %s'
+                     % (0, len(msgs), msgs))
+
+  # Test that our presubmit correctly raises an error for a set of known paths.
+  def testExpectedPaths(self):
+    filesToTest = [
+      "chrome/browser/accessibility/foo.py",
+      "chrome/browser/chromeos/arc/accessibility/foo.cc",
+      "chrome/browser/ui/views/accessibility/foo.h",
+      "chrome/browser/extensions/api/automation/foo.h",
+      "chrome/browser/extensions/api/automation_internal/foo.cc",
+      "chrome/renderer/extensions/accessibility_foo.h",
+      "chrome/tests/data/accessibility/foo.html",
+      "content/browser/accessibility/foo.cc",
+      "content/renderer/accessibility/foo.h",
+      "content/tests/data/accessibility/foo.cc",
+      "extensions/renderer/api/automation/foo.h",
+      "ui/accessibility/foo/bar/baz.cc",
+      "ui/views/accessibility/foo/bar/baz.h",
+    ]
+
+    for testFile in filesToTest:
+      mock_input_api = MockInputApi()
+      mock_output_api = MockOutputApi()
+
+      mock_input_api.files = [
+          MockAffectedFile(testFile, [''])
+      ]
+
+      msgs = PRESUBMIT._CheckAccessibilityRelnotesField(
+          mock_input_api, mock_output_api)
+      self.assertEqual(1, len(msgs),
+                       'Expected %d messages, found %d: %s, for file %s'
+                       % (1, len(msgs), msgs, testFile))
+      self.assertTrue("Missing 'AX-Relnotes:' field" in msgs[0].message,
+                      ('Missing AX-Relnotes field message not found in errors '
+                       ' for file %s' % (testFile)))
 
 class AndroidDeprecatedTestAnnotationTest(unittest.TestCase):
   def testCheckAndroidTestAnnotationUsage(self):

PRESUBMIT_test_mocks.py

@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from collections import defaultdict
 import fnmatch
 import json
 import os
@@ -240,6 +241,7 @@ class MockChange(object):
 
   def __init__(self, changed_files):
     self._changed_files = changed_files
+    self.footers = defaultdict(list)
 
   def LocalPaths(self):
     return self._changed_files
@@ -247,3 +249,7 @@ class MockChange(object):
   def AffectedFiles(self, include_dirs=False, include_deletes=True,
                     file_filter=None):
     return self._changed_files
+
+  def GitFootersFromDescription(self):
+    return self.footers
+