Skip to content

Commit

Permalink
Add PRESUBMIT check if modified UMA histogram name can be found
Browse files Browse the repository at this point in the history
This Presubmit checks if some diffs affect any UMA_HISTOGRAM_*
macro and, if so, checks if the histogram name is to be found
in either tools/metrics/histograms/histograms.xml or in the
CL diffs.

Addresses the problem of someone modifying code and
inadvertently forgetting a corresponding histograms.xml
adaptation, that has happened in the past.

BUG=434420

Review URL: https://codereview.chromium.org/766713004

Cr-Commit-Position: refs/heads/master@{#306388}
  • Loading branch information
yell0wd0g authored and Commit bot committed Dec 2, 2014
1 parent 1e519da commit 2ece527
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 0 deletions.
48 changes: 48 additions & 0 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,53 @@ def _CheckNoUNIT_TESTInSourceFiles(input_api, output_api):
return [output_api.PresubmitPromptWarning('UNIT_TEST is only for headers.\n' +
'\n'.join(problems))]

def _CheckUmaHistogramChanges(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 = []
pattern = input_api.re.compile('UMA_HISTOGRAM.*\("(.*)"')
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 (not f.LocalPath().endswith(('cc', 'mm', 'cpp'))):
continue
for line_num, line in f.ChangedContents():
found = pattern.search(line)
if found:
touched_histograms.append([found.group(1), f, line_num])

# Search for the touched histogram names in the local modifications to
# histograms.xml, and if not found on the base file.
problems = []
for histogram_name, f, line_num in touched_histograms:
histogram_name_found = False
for line_num, line in histograms_xml_modifications:
if histogram_name in line:
histogram_name_found = True;
break;
if histogram_name_found:
continue

with open('tools/metrics/histograms/histograms.xml') as histograms_xml:
for line in histograms_xml:
if histogram_name in line:
histogram_name_found = True;
break;
if histogram_name_found:
continue
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 '
'metrics/histograms.xml or the modifications of it:', problems)]


def _CheckNoNewWStrings(input_api, output_api):
"""Checks to make sure we don't introduce use of wstrings."""
Expand Down Expand Up @@ -1520,6 +1567,7 @@ def CheckChangeOnUpload(input_api, output_api):
results.extend(_CommonChecks(input_api, output_api))
results.extend(_CheckValidHostsInDEPS(input_api, output_api))
results.extend(_CheckJavaStyle(input_api, output_api))
results.extend(_CheckUmaHistogramChanges(input_api, output_api))
return results


Expand Down
22 changes: 22 additions & 0 deletions PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,28 @@ def testTypicalConflict(self):
self.assertTrue('3' in errors[1])
self.assertTrue('5' in errors[2])

class UmaHistogramChangeMatchedOrNotTest(unittest.TestCase):
def testTypicalNotMatchedChange(self):
diff = ['UMA_HISTOGRAM_BOOL("Bla.Foo.Dummy", true)']
mock_input_api = MockInputApi()
mock_input_api.files = [MockFile('some/path/foo.cc', diff)]
warnings = PRESUBMIT._CheckUmaHistogramChanges(mock_input_api,
MockOutputApi())
self.assertEqual(1, len(warnings))
self.assertEqual('warning', warnings[0].type)

def testTypicalCorrectlyMatchedChange(self):
diff_cc = ['UMA_HISTOGRAM_BOOL("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('tools/metrics/histograms/histograms.xml', diff_xml),
]
warnings = []
warnings = PRESUBMIT._CheckUmaHistogramChanges(mock_input_api,
MockOutputApi())
self.assertEqual(0, len(warnings))

class BadExtensionsTest(unittest.TestCase):
def testBadRejFile(self):
Expand Down

0 comments on commit 2ece527

Please sign in to comment.