Remove CheckUmaHistogramChangesOnUpload

Remove CheckUmaHistogramChangesOnUpload. This check is only based on
simple regex match so it's not that reliable and might report a lot of
false positives.

The best way to do this is to merge all xmls and extract all histogram
names from it. It can be done in a way that similar to
https://source.chromium.org/chromium/chromium/src/+/master:PRESUBMIT.py;drc=015291ed0b8742662afb9431f176f033a0504a44;l=3610

However, merging xmls and extracting names is an expensive operation so
in order not to confuse users, we should remove this check for now and
we can re-implement a similar check once we have an efficient way to do
so.

Bug: 1138055
Change-Id: Iad81a9331d971b9260559b0e33fcced149c5334c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2500269
Commit-Queue: Weilun Shi <sweilun@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Weilun Shi <sweilun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821513}

PRESUBMIT.py

@@ -1650,111 +1650,9 @@ def CheckDCHECK_IS_ONHasBraces(input_api, output_api):
   return errors
 
 
-def _FindHistogramNameInChunk(histogram_name, chunk):
-  """Tries to find a histogram name or prefix in a line.
-
-  Returns the existence of the histogram name, or None if it needs more chunk
-  to determine."""
-  # A histogram_suffixes tag type has an affected-histogram name as a prefix of
-  # the histogram_name.
-  if '<affected-histogram' in chunk:
-    # If the tag is not completed, needs more chunk to get the name.
-    if not '>' in chunk:
-      return None
-    if not 'name="' in chunk:
-      return False
-    # Retrieve the first portion of the chunk wrapped by double-quotations. We
-    # expect the only attribute is the name.
-    histogram_prefix = chunk.split('"')[1]
-    return histogram_prefix in histogram_name
-  # Typically the whole histogram name should in the line.
-  return histogram_name in chunk
-
-
-def CheckUmaHistogramChangesOnUpload(input_api, output_api):
-  """Check that UMA histogram names in touched lines can still be found in other
-  lines of the patch or in histograms.xml. Note that this check would not catch
-  the reverse: changes in histograms.xml not matched in the code itself."""
-  touched_histograms = []
-  histograms_xml_modifications = []
-  call_pattern_c = r'\bUMA_HISTOGRAM.*\('
-  call_pattern_java = r'\bRecordHistogram\.record[a-zA-Z]+Histogram\('
-  name_pattern = r'"(.*?)"'
-  single_line_c_re = input_api.re.compile(call_pattern_c + name_pattern)
-  single_line_java_re = input_api.re.compile(call_pattern_java + name_pattern)
-  split_line_c_prefix_re = input_api.re.compile(call_pattern_c)
-  split_line_java_prefix_re = input_api.re.compile(call_pattern_java)
-  split_line_suffix_re = input_api.re.compile(r'^\s*' + name_pattern)
-  last_line_matched_prefix = False
-  for f in input_api.AffectedFiles():
-    # If histograms.xml itself is modified, keep the modified lines for later.
-    if f.LocalPath().endswith(('histograms.xml')):
-      histograms_xml_modifications = f.ChangedContents()
-      continue
-    if f.LocalPath().endswith(('cc', 'mm', 'cpp')):
-      single_line_re = single_line_c_re
-      split_line_prefix_re = split_line_c_prefix_re
-    elif f.LocalPath().endswith(('java')):
-      single_line_re = single_line_java_re
-      split_line_prefix_re = split_line_java_prefix_re
-    else:
-      continue
-    for line_num, line in f.ChangedContents():
-      if last_line_matched_prefix:
-        suffix_found = split_line_suffix_re.search(line)
-        if suffix_found :
-          touched_histograms.append([suffix_found.group(1), f, line_num])
-          last_line_matched_prefix = False
-          continue
-      found = single_line_re.search(line)
-      if found:
-        touched_histograms.append([found.group(1), f, line_num])
-        continue
-      last_line_matched_prefix = split_line_prefix_re.search(line)
-
-  # Search for the touched histogram names in the local modifications to
-  # histograms.xml, and, if not found, on the base histograms.xml file.
-  unmatched_histograms = []
-  for histogram_info in touched_histograms:
-    histogram_name_found = False
-    chunk = ''
-    for line_num, line in histograms_xml_modifications:
-      chunk += line
-      histogram_name_found = _FindHistogramNameInChunk(histogram_info[0], chunk)
-      if histogram_name_found is None:
-        continue
-      chunk = ''
-      if histogram_name_found:
-        break
-    if not histogram_name_found:
-      unmatched_histograms.append(histogram_info)
-
-  histograms_xml_path = 'tools/metrics/histograms/histograms.xml'
-  problems = []
-  if unmatched_histograms:
-    with open(histograms_xml_path) as histograms_xml:
-      for histogram_name, f, line_num in unmatched_histograms:
-        histograms_xml.seek(0)
-        histogram_name_found = False
-        chunk = ''
-        for line in histograms_xml:
-          chunk += line
-          histogram_name_found = _FindHistogramNameInChunk(histogram_name,
-                                                           chunk)
-          if histogram_name_found is None:
-            continue
-          chunk = ''
-          if histogram_name_found:
-            break
-        if not histogram_name_found:
-          problems.append(' [%s:%d] %s' %
-                          (f.LocalPath(), line_num, histogram_name))
-
-  if not problems:
-    return []
-  return [output_api.PresubmitPromptWarning('Some UMA_HISTOGRAM lines have '
-    'been modified and the associated histogram name has no match in either '
-    '%s or the modifications of it:' % (histograms_xml_path),  problems)]
+# TODO(crbug/1138055): Reimplement CheckUmaHistogramChangesOnUpload check in a
+# more reliable way. See
+# https://chromium-review.googlesource.com/c/chromium/src/+/2500269
 
 
 def CheckFlakyTestUsage(input_api, output_api):

PRESUBMIT_test.py

@@ -41,168 +41,6 @@ class VersionControlConflictsTest(unittest.TestCase):
     self.assertEqual(0, len(errors))
 
 
-class UmaHistogramChangeMatchedOrNotTest(unittest.TestCase):
-  def testTypicalCorrectlyMatchedChange(self):
-    diff_cc = ['UMA_HISTOGRAM_BOOL("Bla.Foo.Dummy", true)']
-    diff_java = [
-      'RecordHistogram.recordBooleanHistogram("Bla.Foo.Dummy", true)']
-    diff_xml = ['<histogram name="Bla.Foo.Dummy"> </histogram>']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-      MockFile('tools/metrics/histograms/histograms.xml', diff_xml),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(0, len(warnings))
-
-  def testTypicalNotMatchedChange(self):
-    diff_cc = ['UMA_HISTOGRAM_BOOL("Bla.Foo.Dummy", true)']
-    diff_java = [
-      'RecordHistogram.recordBooleanHistogram("Bla.Foo.Dummy", true)']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(1, len(warnings))
-    self.assertEqual('warning', warnings[0].type)
-    self.assertTrue('foo.cc' in warnings[0].items[0])
-    self.assertTrue('foo.java' in warnings[0].items[1])
-
-  def testTypicalNotMatchedChangeViaSuffixes(self):
-    diff_cc = ['UMA_HISTOGRAM_BOOL("Bla.Foo.Dummy", true)']
-    diff_java = [
-      'RecordHistogram.recordBooleanHistogram("Bla.Foo.Dummy", true)']
-    diff_xml = ['<histogram_suffixes name="SuperHistogram">',
-                '  <suffix name="Dummy"/>',
-                '  <affected-histogram name="Snafu.Dummy"/>',
-                '</histogram>']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-      MockFile('tools/metrics/histograms/histograms.xml', diff_xml),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(1, len(warnings))
-    self.assertEqual('warning', warnings[0].type)
-    self.assertTrue('foo.cc' in warnings[0].items[0])
-    self.assertTrue('foo.java' in warnings[0].items[1])
-
-  def testTypicalCorrectlyMatchedChangeViaSuffixes(self):
-    diff_cc = ['UMA_HISTOGRAM_BOOL("Bla.Foo.Dummy", true)']
-    diff_java = [
-      'RecordHistogram.recordBooleanHistogram("Bla.Foo.Dummy", true)']
-    diff_xml = ['<histogram_suffixes name="SuperHistogram">',
-                '  <suffix name="Dummy"/>',
-                '  <affected-histogram name="Bla.Foo"/>',
-                '</histogram>']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-      MockFile('tools/metrics/histograms/histograms.xml', diff_xml),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(0, len(warnings))
-
-  def testTypicalCorrectlyMatchedChangeViaSuffixesWithSeparator(self):
-    diff_cc = ['UMA_HISTOGRAM_BOOL("Snafu_Dummy", true)']
-    diff_java = ['RecordHistogram.recordBooleanHistogram("Snafu_Dummy", true)']
-    diff_xml = ['<histogram_suffixes name="SuperHistogram" separator="_">',
-                '  <suffix name="Dummy"/>',
-                '  <affected-histogram name="Snafu"/>',
-                '</histogram>']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-      MockFile('tools/metrics/histograms/histograms.xml', diff_xml),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(0, len(warnings))
-
-  def testCorrectlyMatchedChangeViaSuffixesWithLineWrapping(self):
-    diff_cc = [
-        'UMA_HISTOGRAM_BOOL("LongHistogramNameNeedsLineWrapping.Dummy", true)']
-    diff_java = ['RecordHistogram.recordBooleanHistogram(' +
-                 '"LongHistogramNameNeedsLineWrapping.Dummy", true)']
-    diff_xml = ['<histogram_suffixes',
-                '    name="LongHistogramNameNeedsLineWrapping"',
-                '    separator=".">',
-                '  <suffix name="Dummy"/>',
-                '  <affected-histogram',
-                '      name="LongHistogramNameNeedsLineWrapping"/>',
-                '</histogram>']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-      MockFile('tools/metrics/histograms/histograms.xml', diff_xml),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(0, len(warnings))
-
-  def testNameMatch(self):
-    # Check that the detected histogram name is "Dummy" and not, e.g.,
-    # "Dummy\", true);  // The \"correct"
-    diff_cc = ['UMA_HISTOGRAM_BOOL("Dummy", true);  // The "correct" histogram']
-    diff_java = [
-      'RecordHistogram.recordBooleanHistogram("Dummy", true);' +
-      '  // The "correct" histogram']
-    diff_xml = ['<histogram name="Dummy"> </histogram>']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-      MockFile('tools/metrics/histograms/histograms.xml', diff_xml),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(0, len(warnings))
-
-  def testSimilarMacroNames(self):
-    diff_cc = ['PUMA_HISTOGRAM_COOL("Mountain Lion", 42)']
-    diff_java = [
-      'FakeRecordHistogram.recordFakeHistogram("Mountain Lion", 42)']
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo.java', diff_java),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(0, len(warnings))
-
-  def testMultiLine(self):
-    diff_cc = ['UMA_HISTOGRAM_BOOLEAN(', '    "Multi.Line", true)']
-    diff_cc2 = ['UMA_HISTOGRAM_BOOLEAN(', '    "Multi.Line"', '    , true)']
-    diff_java = [
-      'RecordHistogram.recordBooleanHistogram(',
-      '    "Multi.Line", true);',
-    ]
-    mock_input_api = MockInputApi()
-    mock_input_api.files = [
-      MockFile('some/path/foo.cc', diff_cc),
-      MockFile('some/path/foo2.cc', diff_cc2),
-      MockFile('some/path/foo.java', diff_java),
-    ]
-    warnings = PRESUBMIT.CheckUmaHistogramChangesOnUpload(mock_input_api,
-                                                   MockOutputApi())
-    self.assertEqual(1, len(warnings))
-    self.assertEqual('warning', warnings[0].type)
-    self.assertTrue('foo.cc' in warnings[0].items[0])
-    self.assertTrue('foo2.cc' in warnings[0].items[1])
-
-
 class BadExtensionsTest(unittest.TestCase):
   def testBadRejFile(self):
     mock_input_api = MockInputApi()