Skip to content

Commit

Permalink
Improve presubmit for histogram name typos
Browse files Browse the repository at this point in the history
Currently, _CheckUmaHistogramChanges detects the cases like calling
  UMA_HISTOGRAM_BOOLEAN("NonexistingHistogram.Typo", true)
when "NonexistingHistogram.Typo" is not a histogram name defined in
histograms.xml.

However, it won't detect the case when the name is after a line break:
  UMA_HISTOGRAM_BOOLEAN(
      "NonexistingHistogram.VeeeeeeeryLoooooooongName.WithSubitems",
      true)

This will be often the case once the check gets extended to Java,
where the UMA_HISTOGRAM* macros are replaced with the
RecordHistogram.record*Histogram methods, which have longer names.

Bug: 821981
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I71d01f3b7012e8a8d6c4628d67a470c57005cd56
Reviewed-on: https://chromium-review.googlesource.com/978219
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545676}
  • Loading branch information
Vaclav Brozek authored and Commit Bot committed Mar 24, 2018
1 parent 5ba3f26 commit 0e730cb
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 2 deletions.
13 changes: 11 additions & 2 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -731,9 +731,10 @@ def _CheckUmaHistogramChanges(input_api, output_api):
the reverse: changes in histograms.xml not matched in the code itself."""
touched_histograms = []
histograms_xml_modifications = []
# For now, the check only detects the case of the macro and histogram names
# being on a single line.
single_line_re = input_api.re.compile(r'\bUMA_HISTOGRAM.*\("(.*?)"')
split_line_prefix_re = input_api.re.compile(r'\bUMA_HISTOGRAM.*\(')
split_line_suffix_re = input_api.re.compile(r'^\s*"([^"]*)"')
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')):
Expand All @@ -742,9 +743,17 @@ def _CheckUmaHistogramChanges(input_api, output_api):
if not f.LocalPath().endswith(('cc', 'mm', 'cpp')):
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.
Expand Down
15 changes: 15 additions & 0 deletions PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,21 @@ def testSimilarMacroNames(self):
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)']
mock_input_api = MockInputApi()
mock_input_api.files = [
MockFile('some/path/foo.cc', diff_cc),
MockFile('some/path/foo2.cc', diff_cc2),
]
warnings = PRESUBMIT._CheckUmaHistogramChanges(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()
Expand Down

0 comments on commit 0e730cb

Please sign in to comment.