Add test coverage for histogram PRESUBMIT checks

This was done to ensure no regressions in crbug.com/40281617 but while
doing that I realized that there is a lot of ground to cover and
potential controversies in how to actually cover this with tests so for
the sake of keeping CLs relatively small I think it makes sense to
dedicate this one to simply covering existing functionality with tests.

Main changes done here:

1. Add `PRESUBMIT_test.py` for histogram that adds a basic pass/fails
scenarios for the 3 presubmit checks we have.

2. Add 2 extra versions of `.*histogram.xml` within `test_data` with
comments describing what each of them representing.

3. Add basic injectability by exposing internal versions of check
methods in `PRESUBMIT.py` as a lot of those checks relying on various
types of globals - it was a way to inject things like xml paths, or
actually not skip the whole checks where the data comes from `test_data`
directory when testing this functionality.

4. Make `CheckWebViewHistogramsAllowlist` accept histograms_files as
params to allow dependency on global test enabling testing.

Probably the biggest controversy is using PRESUBMIT_test_mocks which
come from top level of the repo. AFAICT there are no official mocks
available and there is a prior art to reusing top level ones for example
(but not limited to):
https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/PRESUBMIT_test.py;l=12-17

Tested: This whole PR is adding test coverage, but on top of that I
validated that presubmits are still properly passing IRL by checking
that they pass on prod version of histograms.xml and then tripping each
of them intentionally with local changes.

Bug: 391532700
Change-Id: I23a4c29ac8a3dc77abb84b06cdd5864921b6eedf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187542
Commit-Queue: Andrzej Fiedukowicz <afie@google.com>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1411638}

android_webview/java/res/raw/PRESUBMIT.py

@@ -27,4 +27,9 @@ def _CheckHistogramsAllowlist(input_api, output_api):
     # src_path should point to chromium/src
     src_path = os.path.join(input_api.PresubmitLocalPath(), '..', '..', '..',
                             '..')
-    return CheckWebViewHistogramsAllowlist(src_path, output_api)
+
+    histograms_path = os.path.join(src_path, 'tools', 'metrics', 'histograms')
+    sys.path.append(histograms_path)
+    import print_histogram_names
+    return CheckWebViewHistogramsAllowlist(
+        src_path, output_api, print_histogram_names.histogram_xml_files())

android_webview/java/res/raw/histograms_allowlist_check.py

@@ -16,7 +16,7 @@ def get_histograms_allowlist_content(src_path):
         return [line.rstrip() for line in file]
 
 
-def CheckWebViewHistogramsAllowlist(src_path, output_api):
+def CheckWebViewHistogramsAllowlist(src_path, output_api, histograms_files):
     """Checks that histograms_allowlist.txt contains valid histograms.
     src_path should point to chromium/src
     """
@@ -24,8 +24,7 @@ def CheckWebViewHistogramsAllowlist(src_path, output_api):
     sys.path.append(histograms_path)
     import print_histogram_names
 
-    all_histograms = print_histogram_names.get_names(
-        print_histogram_names.histogram_xml_files())
+    all_histograms = print_histogram_names.get_names(histograms_files)
 
     histograms_allowlist = get_histograms_allowlist_content(src_path)
 

tools/metrics/BUILD.gn

@@ -131,6 +131,7 @@ group("metrics_python_tests") {
     "//tools/metrics/histograms/merge_xml.py",
     "//tools/metrics/histograms/merge_xml_test.py",
     "//tools/metrics/histograms/populate_enums.py",
+    "//tools/metrics/histograms/PRESUBMIT_test.py",
     "//tools/metrics/histograms/pretty_print.py",
     "//tools/metrics/histograms/pretty_print_test.py",
     "//tools/metrics/histograms/validate_token.py",

tools/metrics/histograms/PRESUBMIT.py

@@ -1,7 +1,6 @@
 # Copyright 2013 The Chromium Authors
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
-
 """
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details on the presubmit API built into depot_tools.
@@ -17,8 +16,9 @@ import sys
 def GetPrettyPrintErrors(input_api, output_api, cwd, rel_path, results):
   """Runs pretty-print command for specified file."""
   args = [
-      input_api.python3_executable, 'pretty_print.py', rel_path, '--presubmit',
-      '--non-interactive'
+      input_api.python3_executable,
+      os.path.join(input_api.PresubmitLocalPath(), 'pretty_print.py'), rel_path,
+      '--presubmit', '--non-interactive'
   ]
   exit_code = input_api.subprocess.call(args, cwd=cwd)
 
@@ -30,8 +30,12 @@ def GetPrettyPrintErrors(input_api, output_api, cwd, rel_path, results):
 
 def GetTokenErrors(input_api, output_api, cwd, rel_path, results):
   """Validates histogram tokens in specified file."""
-  exit_code = input_api.subprocess.call(
-      [input_api.python3_executable, 'validate_token.py', rel_path], cwd=cwd)
+  exit_code = input_api.subprocess.call([
+      input_api.python3_executable,
+      os.path.join(input_api.PresubmitLocalPath(), 'validate_token.py'),
+      rel_path
+  ],
+                                        cwd=cwd)
 
   if exit_code != 0:
     error_msg = (
@@ -42,8 +46,11 @@ def GetTokenErrors(input_api, output_api, cwd, rel_path, results):
 
 def GetValidateHistogramsError(input_api, output_api, cwd, results):
   """Validates histograms format and index file."""
-  exit_code = input_api.subprocess.call(
-      [input_api.python3_executable, 'validate_format.py'], cwd=cwd)
+  exit_code = input_api.subprocess.call([
+      input_api.python3_executable,
+      os.path.join(input_api.PresubmitLocalPath(), 'validate_format.py')
+  ],
+                                        cwd=cwd)
 
   if exit_code != 0:
     error_msg = (
@@ -51,17 +58,21 @@ def GetValidateHistogramsError(input_api, output_api, cwd, results):
         'and fix the reported errors.' % cwd)
     results.append(output_api.PresubmitError(error_msg))
 
-  exit_code = input_api.subprocess.call(
-      [input_api.python3_executable, 'validate_histograms_index.py'], cwd=cwd)
+  exit_code = input_api.subprocess.call([
+      input_api.python3_executable,
+      os.path.join(input_api.PresubmitLocalPath(),
+                   'validate_histograms_index.py')
+  ],
+                                        cwd=cwd)
 
   if exit_code != 0:
-    error_msg = (
-        'Histograms index file is not up-to-date. Please run '
-        '%s/histogram_paths.py to update it' % cwd)
+    error_msg = ('Histograms index file is not up-to-date. Please run '
+                 '%s/histogram_paths.py to update it' % cwd)
     results.append(output_api.PresubmitError(error_msg))
 
 
-def ValidateSingleFile(input_api, output_api, file_obj, cwd, results):
+def ValidateSingleFile(input_api, output_api, file_obj, cwd, results,
+                       allow_test_paths):
   """Does corresponding validations if histograms.xml or enums.xml is changed.
 
   Args:
@@ -70,6 +81,13 @@ def ValidateSingleFile(input_api, output_api, file_obj, cwd, results):
     file_obj: A file object of one of the changed files.
     cwd: Path to current working directory.
     results: The returned variable which is a list of output_api results.
+    allow_testing_paths: A boolean that determines if the test_data directory
+      changes should be validated. If it's False, all the files under
+      `test_data` directory will be ignored. This is needed as the `test_data`
+      xmls contains intentional errors to trip the presubmit checks and we want
+      to trip those presubmits in checks, but a the same time due to the nature
+      of how the input_api gives all changed files in the directory, we don't
+      want "production" checks to trip over those mistakes.
 
   Returns:
     A boolean that True if a histograms.xml or enums.xml file is changed.
@@ -80,7 +98,7 @@ def ValidateSingleFile(input_api, output_api, file_obj, cwd, results):
     return False
   filepath = input_api.os_path.relpath(p, cwd)
 
-  if 'test_data' in filepath:
+  if not allow_test_paths and 'test_data' in filepath:
     return False
 
   # If the changed file is histograms.xml or histogram_suffixes_list.xml,
@@ -99,8 +117,13 @@ def ValidateSingleFile(input_api, output_api, file_obj, cwd, results):
   return False
 
 
-def CheckHistogramFormatting(input_api, output_api):
-  """Checks that histograms.xml is pretty-printed and well-formatted."""
+def CheckHistogramFormatting(input_api, output_api, allow_test_paths=False):
+  """Checks that histograms.xml is pretty-printed and well-formatted.
+
+  This is a method that is called by the PRESUBMIT system and those it
+  represents a production check rather then a test one. This is why we
+  set allow_test_paths to False by default.
+  """
   results = []
   cwd = input_api.PresubmitLocalPath()
   xml_changed = False
@@ -108,8 +131,8 @@ def CheckHistogramFormatting(input_api, output_api):
   # Only for changed files, do corresponding checks if the file is
   # histograms.xml or enums.xml.
   for file_obj in input_api.AffectedFiles():
-    is_changed = ValidateSingleFile(
-        input_api, output_api, file_obj, cwd, results)
+    is_changed = ValidateSingleFile(input_api, output_api, file_obj, cwd,
+                                    results, allow_test_paths)
     xml_changed = xml_changed or is_changed
 
   # Run validate_format.py and validate_histograms_index.py, if changed files
@@ -120,20 +143,36 @@ def CheckHistogramFormatting(input_api, output_api):
   return results
 
 
-def CheckWebViewHistogramsAllowlistOnUpload(input_api, output_api):
+def CheckWebViewHistogramsAllowlistOnUpload(
+    input_api,
+    output_api,
+    xml_paths_override=None,
+):
   """Checks that histograms_allowlist.txt contains valid histograms."""
   xml_filter = lambda f: Path(f.LocalPath()).suffix == '.xml'
   xml_files = input_api.AffectedFiles(file_filter=xml_filter)
   if not xml_files:
     return []
 
+  sys.path.append(input_api.PresubmitLocalPath())
+  from histogram_paths import ALL_XMLS
+  if xml_paths_override is not None:
+    xml_files_paths = xml_paths_override
+  else:
+    xml_files_paths = ALL_XMLS
+
+  xml_files = [open(f, encoding='utf-8') for f in xml_files_paths]
+
   # src_path should point to chromium/src
   src_path = os.path.join(input_api.PresubmitLocalPath(), '..', '..', '..')
   histograms_allowlist_check_path = os.path.join(src_path, 'android_webview',
                                                  'java', 'res', 'raw')
   sys.path.append(histograms_allowlist_check_path)
   from histograms_allowlist_check import CheckWebViewHistogramsAllowlist
-  return CheckWebViewHistogramsAllowlist(src_path, output_api)
+  result = CheckWebViewHistogramsAllowlist(src_path, output_api, xml_files)
+  for f in xml_files:
+    f.close()
+  return result
 
 
 def CheckBooleansAreEnums(input_api, output_api):
@@ -157,4 +196,4 @@ def CheckBooleansAreEnums(input_api, output_api):
   # If a histograms.xml file was changed, check for units="[Bb]oolean".
   if results:
     return [output_api.PresubmitPromptOrNotify(units_warning, results)]
-  return results
+  return results

tools/metrics/histograms/PRESUBMIT_test.py

@@ -0,0 +1,143 @@
+# Copyright 2025 The Chromium Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import os.path
+import unittest
+
+import sys
+
+# Force local directory to be in front of sys.path to avoid importing different
+# version of PRESUBMIT.py which can be added in different files within python
+# invocation.
+sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
+import PRESUBMIT
+
+# Append chrome source root to import `PRESUBMIT_test_mocks.py`.
+sys.path.append(
+    os.path.dirname(
+        os.path.dirname(
+            os.path.dirname(os.path.dirname(os.path.abspath(__file__))))))
+from PRESUBMIT_test_mocks import MockAffectedFile, MockInputApi, MockOutputApi
+
+
+_BASE_DIR = os.path.dirname(os.path.abspath(__file__))
+
+class MetricsPresubmitTest(unittest.TestCase):
+
+  def testCheckHistogramFormattingFailureIsDetected(self):
+    malformed_histograms_path = (
+        f'{os.path.dirname(__file__)}'
+        '/test_data/tokens/token_errors_histograms.xml')
+
+    with open(malformed_histograms_path, 'r') as f:
+      malformed_contents = f.read()
+    mock_input_api = MockInputApi()
+    mock_input_api.presubmit_local_path = _BASE_DIR
+    mock_input_api.files = [
+        MockAffectedFile(malformed_histograms_path, malformed_contents),
+    ]
+
+    results = PRESUBMIT.CheckHistogramFormatting(mock_input_api,
+                                                 MockOutputApi(),
+                                                 allow_test_paths=True)
+    self.assertEqual(len(results), 1)
+    self.assertEqual(results[0].type, 'error')
+    self.assertRegex(
+        results[0].message,
+        '.*histograms.xml contains histogram.* using <variants> not defined in'
+        ' the file, please run validate_token.py .*histograms.xml to fix.')
+
+  def testCheckWebViewHistogramsAllowlistOnUploadFailureIsDetected(self):
+    missing_allow_list_entries_histograms_path = (
+        f'{os.path.dirname(__file__)}'
+        '/test_data'
+        '/no_allowlist_entries_histograms.xml')
+
+    with open(missing_allow_list_entries_histograms_path, 'r') as f:
+      invalid_histograms_contents = f.read()
+    mock_input_api = MockInputApi()
+    mock_input_api.presubmit_local_path = _BASE_DIR
+    mock_input_api.files = [
+        MockAffectedFile(missing_allow_list_entries_histograms_path,
+                         invalid_histograms_contents),
+    ]
+
+    results = PRESUBMIT.CheckWebViewHistogramsAllowlistOnUpload(
+        mock_input_api,
+        MockOutputApi(),
+        xml_paths_override=[missing_allow_list_entries_histograms_path],
+    )
+    self.assertEqual(len(results), 1)
+    self.assertRegex(
+        results[0].message.replace('\n', ' '), 'All histograms in'
+        ' .*histograms_allowlist.txt must be valid.')
+    self.assertEqual(results[0].type, 'error')
+
+  def testCheckBooleansAreEnumsFailureIsDetected(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.presubmit_local_path = _BASE_DIR
+    mock_input_api.files = [
+        MockAffectedFile('histograms.xml',
+                         ['<histogram name="Foo" units="Boolean" />']),
+    ]
+
+    results = PRESUBMIT.CheckBooleansAreEnums(mock_input_api, MockOutputApi())
+    self.assertEqual(len(results), 1)
+    self.assertRegex(
+        results[0].message.replace('\n', ' '),
+        '.*You are using .units. for a boolean histogram, but you should be'
+        ' using\s+.enum. instead\.')
+    self.assertEqual(results[0].type, 'promptOrNotify')
+
+  def testCheckHistogramFormattingPasses(self):
+    valid_histograms_path = (f'{os.path.dirname(__file__)}'
+                             '/test_data'
+                             '/example_valid_histograms.xml')
+
+    with open(valid_histograms_path, 'r') as f:
+      valid_contents = f.read()
+
+    mock_input_api = MockInputApi()
+    mock_input_api.presubmit_local_path = _BASE_DIR
+    mock_input_api.files = [
+        MockAffectedFile(valid_histograms_path, valid_contents),
+    ]
+
+    results = PRESUBMIT.CheckHistogramFormatting(mock_input_api,
+                                                 MockOutputApi(),
+                                                 allow_test_paths=True)
+    self.assertEqual(len(results), 0)
+
+  def testCheckWebViewHistogramsAllowlistOnUploadPasses(self):
+    valid_histograms_path = (f'{os.path.dirname(__file__)}'
+                             '/test_data'
+                             '/example_valid_histograms.xml')
+
+    with open(valid_histograms_path, 'r') as f:
+      valid_contents = f.read()
+
+    mock_input_api = MockInputApi()
+    mock_input_api.presubmit_local_path = _BASE_DIR
+    mock_input_api.files = [
+        MockAffectedFile(valid_histograms_path, valid_contents),
+    ]
+
+    results = PRESUBMIT.CheckWebViewHistogramsAllowlistOnUpload(
+        mock_input_api, MockOutputApi())
+    self.assertEqual(len(results), 0)
+
+  def testCheckBooleansAreEnumsPasses(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.presubmit_local_path = _BASE_DIR
+    mock_input_api.files = [
+        MockAffectedFile('histograms.xml',
+                         ['<histogram name="Foo" enum="Boolean" />']),
+    ]
+
+    results = PRESUBMIT.CheckBooleansAreEnums(mock_input_api, MockOutputApi())
+    self.assertEqual(len(results), 0)
+
+
+if __name__ == '__main__':
+  unittest.main()

tools/metrics/histograms/print_histogram_names.py

@@ -38,7 +38,6 @@ def get_names(xml_files):
   return set(extract_histograms.ExtractNames(histograms))
 
 
-# Used in android_webview/java/res/raw/histograms_allowlist_check.py.
 def histogram_xml_files():
   return [open(f, encoding="utf-8") for f in histogram_paths.ALL_XMLS]
 
@@ -100,7 +99,7 @@ def main(argv):
   if args.diff is not None:
     _print_diff_names(args.diff)
   else:
-    name_set = get_names(histogram_xml_files())
+    name_set = get_names(_histogram_xml_files())
     for name in sorted(list(name_set)):
       print(name)
 

tools/metrics/histograms/test_data/no_allowlist_entries_histograms.xml

@@ -0,0 +1,12 @@
+<!--
+  This is a version of histograms.xml file that doesn't contain all the entries
+  that are expected due to their presence in
+  android_webview/java/res/raw/histograms_allowlist.txt, this file can be used
+  to validate test scenarios that rely on such circumstances.
+-->
+
+<histogram name="RandomHistogramName"
+    units="%" expires_after="M298">
+  <owner>person@chromium.org</owner>
+  <summary>Foo</summary>
+</histogram>

tools/metrics/histograms/test_data/tokens/token_errors_histograms.xml

@@ -1,3 +1,9 @@
+<!--
+  This is a version of histograms.xml file that is an example of a histogram
+  that contains undefined tokens in the name. This file can be used to validate
+  test scenarios that rely on such circumstances.
+-->
+
 <histogram-configuration>
 
 <histograms>

tools/metrics/histograms/validate_token_test.py

@@ -24,7 +24,8 @@ class ValidateTokenTests(unittest.TestCase):
   def test_invalid_tokens(self):
     with self.assertLogs() as logs:
       has_token_error = validate_token.ValidateTokenInFile(
-          f'{os.path.dirname(__file__)}/test_data/tokens/histograms.xml')
+          f'{os.path.dirname(__file__)}'
+          '/test_data/tokens/token_errors_histograms.xml')
       self.assertTrue(has_token_error)
     self.assertEqual(len(logs.output), 1)
     output = logs.output[0]

tools/metrics/metrics_python_tests.py

@@ -45,6 +45,7 @@ sys.exit(
             'histograms/generate_expired_histograms_array_unittest.py',
             'histograms/generate_allowlist_from_histograms_file_unittest.py',
             'histograms/merge_xml_test.py',
+            'histograms/PRESUBMIT_test.py',
             'histograms/pretty_print_test.py',
             'histograms/validate_token_test.py',
             '../json_comment_eater/json_comment_eater_test.py',