Revert of Add PRESUBMIT check if modified UMA histogram name can be found (patchset #2 id:40001 of https://codereview.chromium.org/766713004/)
Reason for revert: Produces false positive warnings on histograms using histogram_suffixes, see: https://code.google.com/p/chromium/issues/detail?id=445265 Original issue's description: > Add PRESUBMIT check if modified UMA histogram name can be found > > 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 > > Committed: https://crrev.com/2ece5270462346b1ac3bccd3bfe5e68d024b98b5 > Cr-Commit-Position: refs/heads/master@{#306388} TBR=phajdan.jr@chromium.org,mcasas@chromium.org NOTREECHECKS=true NOTRY=true BUG=434420, 445265 Review URL: https://codereview.chromium.org/841323002 Cr-Commit-Position: refs/heads/master@{#310863}
This commit is contained in:
48
PRESUBMIT.py
48
PRESUBMIT.py
@ -357,53 +357,6 @@ 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."""
|
||||
@ -1628,7 +1581,6 @@ 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))
|
||||
results.extend(
|
||||
input_api.canned_checks.CheckGNFormatted(input_api, output_api))
|
||||
return results
|
||||
|
@ -270,28 +270,6 @@ class VersionControlConflictsTest(unittest.TestCase):
|
||||
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):
|
||||
|
Reference in New Issue
Block a user