Add location data to PRESUBMIT.py BanRule violations for desktop-android
This attaches file location data to each violation of the IS_DESKTOP_ANDROID ban rule. This data will then be consumed by the presubmit bots and plumbed to ayeaye to expose the ban rule violation as a gerrit lint comment, eg: http://screen/5dcPwjNrtizEVBK Violations of ban rules that are warning-only are currently only exposed to the author at git-cl-upload time, and can be ignored. These lint comments should surface them to reviewers as well, and are harder to ignore. Similar to current lint comments, they shouldn't be blocking unless a reviewer clicks "please fix". The location data plumbing currently supports the specific columns in each line that contain the violation. We forgo specifying that extra detail here, and simply flag the entire line as a violation. This is simply due to laziness around the regex handling of BanRule's current implementation. Can add that extra location data in a follow-up if desired. Bug: 403341439 Change-Id: Id99d3a014ce59d5de34df18ff1b6cb48ff7f7736 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6480470 Reviewed-by: Andrew Grieve <agrieve@chromium.org> Reviewed-by: Nate Fischer <ntfschr@chromium.org> Commit-Queue: Ben Pastene <bpastene@chromium.org> Cr-Commit-Position: refs/heads/main@{#1450737}
This commit is contained in:
Ben Pastene
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
53eb165d2b
commit
e79d6611f3
41
PRESUBMIT.py
41
PRESUBMIT.py
@ -144,6 +144,9 @@ class BanRule:
|
||||
# expression that will be matched against the path of the file being checked
|
||||
# relative to the root of the source tree.
|
||||
excluded_paths: Optional[Sequence[str]] = None
|
||||
# If True, surfaces any violation as a Gerrit comment on the CL after
|
||||
# running the CQ.
|
||||
surface_as_gerrit_lint: Optional[bool] = None
|
||||
|
||||
|
||||
_BANNED_JAVA_IMPORTS : Sequence[BanRule] = (
|
||||
@ -316,6 +319,7 @@ _BANNED_JAVA_FUNCTIONS : Sequence[BanRule] = (
|
||||
r'^infra/', # This is permitted in infra/ folder.
|
||||
r'^tools/', # This is permitted in tools/ folder.
|
||||
],
|
||||
surface_as_gerrit_lint=True,
|
||||
),
|
||||
)
|
||||
|
||||
@ -2227,6 +2231,7 @@ _BANNED_CPP_FUNCTIONS: Sequence[BanRule] = (
|
||||
r'^infra/', # This is permitted in infra/ folder.
|
||||
r'^tools/', # This is permitted in tools/ folder.
|
||||
],
|
||||
surface_as_gerrit_lint=True,
|
||||
),
|
||||
)
|
||||
|
||||
@ -2996,8 +3001,7 @@ def _GetMessageForMatchingType(input_api, affected_file, line_number, line,
|
||||
|
||||
def CheckNoBannedFunctions(input_api, output_api):
|
||||
"""Make sure that banned functions are not used."""
|
||||
warnings = []
|
||||
errors = []
|
||||
results= []
|
||||
|
||||
def IsExcludedFile(affected_file, excluded_paths):
|
||||
if not excluded_paths:
|
||||
@ -3027,13 +3031,27 @@ def CheckNoBannedFunctions(input_api, output_api):
|
||||
if IsExcludedFile(affected_file, ban_rule.excluded_paths):
|
||||
return
|
||||
|
||||
problems = _GetMessageForMatchingType(input_api, f, line_num, line,
|
||||
message = _GetMessageForMatchingType(input_api, f, line_num, line,
|
||||
ban_rule)
|
||||
if problems:
|
||||
if message:
|
||||
result_loc = []
|
||||
if ban_rule.surface_as_gerrit_lint:
|
||||
result_loc.append(output_api.PresubmitResultLocation(
|
||||
file_path=affected_file.LocalPath(),
|
||||
start_line=line_num,
|
||||
end_line=line_num,
|
||||
))
|
||||
if ban_rule.treat_as_error is not None and ban_rule.treat_as_error:
|
||||
errors.extend(problems)
|
||||
results.append(
|
||||
output_api.PresubmitError('A banned function was used.\n' +
|
||||
'\n'.join(message),
|
||||
locations=result_loc))
|
||||
|
||||
else:
|
||||
warnings.extend(problems)
|
||||
results.append(
|
||||
output_api.PresubmitPromptWarning('A banned function was used.\n' +
|
||||
'\n'.join(message),
|
||||
locations=result_loc))
|
||||
|
||||
file_filter = lambda f: f.LocalPath().endswith(('.java'))
|
||||
for f in input_api.AffectedFiles(file_filter=file_filter):
|
||||
@ -3096,17 +3114,8 @@ def CheckNoBannedFunctions(input_api, output_api):
|
||||
for ban_rule in _BANNED_MOJOM_PATTERNS:
|
||||
CheckForMatch(f, line_num, line, ban_rule)
|
||||
|
||||
return results
|
||||
|
||||
result = []
|
||||
if (warnings):
|
||||
result.append(
|
||||
output_api.PresubmitPromptWarning('Banned functions were used.\n' +
|
||||
'\n'.join(warnings)))
|
||||
if (errors):
|
||||
result.append(
|
||||
output_api.PresubmitError('Banned functions were used.\n' +
|
||||
'\n'.join(errors)))
|
||||
return result
|
||||
|
||||
def _CheckAndroidNoBannedImports(input_api, output_api):
|
||||
"""Make sure that banned java imports are not used."""
|
||||
|
||||
@ -2979,35 +2979,34 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
]
|
||||
|
||||
errors = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
self.assertEqual(2, len(errors))
|
||||
self.assertEqual(12, len(errors))
|
||||
self.assertTrue(
|
||||
'some/java/problematic/diskread.java' in errors[0].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/diskwrite.java' in errors[0].message)
|
||||
self.assertFalse('some/java/ok/diskwrite.java' in errors[0].message)
|
||||
self.assertFalse('some/java/ok/diskwrite.java' in errors[1].message)
|
||||
'some/java/problematic/diskwrite.java' in errors[1].message)
|
||||
self.assertTrue(all('some/java/ok/diskwrite.java' not in e.message for e in errors))
|
||||
self.assertTrue(
|
||||
'some/java/problematic/waitidleforsync.java' in errors[0].message)
|
||||
'some/java/problematic/waitidleforsync.java' in errors[2].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/registerreceiver.java' in errors[1].message)
|
||||
'some/java/problematic/registerreceiver.java' in errors[3].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/property.java' in errors[0].message)
|
||||
'some/java/problematic/property.java' in errors[4].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/requestlayout.java' in errors[0].message)
|
||||
'some/java/problematic/requestlayout.java' in errors[5].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/lastprofile.java' in errors[0].message)
|
||||
'some/java/problematic/lastprofile.java' in errors[6].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/getdrawable1.java' in errors[0].message)
|
||||
'some/java/problematic/getdrawable1.java' in errors[7].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/getdrawable2.java' in errors[0].message)
|
||||
'some/java/problematic/getdrawable2.java' in errors[8].message)
|
||||
self.assertTrue('some/java/problematic/announceForAccessibility.java'
|
||||
in errors[0].message)
|
||||
in errors[9].message)
|
||||
self.assertTrue(
|
||||
'some/java/problematic/accessibilityTypeAnnouncement.java' in
|
||||
errors[0].message)
|
||||
errors[10].message)
|
||||
self.assertTrue(
|
||||
'content/java/problematic/desktopandroid.java' in
|
||||
errors[0].message)
|
||||
errors[11].message)
|
||||
|
||||
|
||||
def testBannedCppFunctions(self):
|
||||
@ -3035,29 +3034,36 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
['std::ranges::subrange(first, last)']),
|
||||
MockFile('views_usage.cc', ['std::views::all(vec)']),
|
||||
MockFile('content/desktop_android.cc',
|
||||
['#if BUILDFLAG(IS_DESKTOP_ANDROID)']),
|
||||
[
|
||||
'// some first line',
|
||||
'#if BUILDFLAG(IS_DESKTOP_ANDROID)',
|
||||
'// some third line',
|
||||
]),
|
||||
]
|
||||
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
|
||||
# warnings are results[0], errors are results[1]
|
||||
self.assertEqual(2, len(results))
|
||||
self.assertTrue('some/cpp/problematic/file.cc' in results[1].message)
|
||||
# Each entry in results corresponds to a BanRule with a violation, in
|
||||
# the order they were encountered.
|
||||
self.assertEqual(8, len(results))
|
||||
self.assertTrue('some/cpp/problematic/file.cc' in results[0].message)
|
||||
self.assertTrue(
|
||||
'third_party/blink/problematic/file.cc' in results[0].message)
|
||||
self.assertTrue('some/cpp/ok/file.cc' not in results[1].message)
|
||||
self.assertTrue('some/cpp/problematic/file2.cc' in results[0].message)
|
||||
self.assertTrue('some/cpp/problematic/file3.cc' in results[0].message)
|
||||
self.assertTrue('some/cpp/problematic/file4.cc' in results[0].message)
|
||||
self.assertFalse('some/cpp/nocheck/file.cc' in results[0].message)
|
||||
self.assertFalse('some/cpp/nocheck/file.cc' in results[1].message)
|
||||
self.assertFalse('some/cpp/comment/file.cc' in results[0].message)
|
||||
self.assertFalse('some/cpp/comment/file.cc' in results[1].message)
|
||||
self.assertFalse('allowed_ranges_usage.cc' in results[0].message)
|
||||
self.assertFalse('allowed_ranges_usage.cc' in results[1].message)
|
||||
self.assertTrue('banned_ranges_usage.cc' in results[1].message)
|
||||
self.assertTrue('views_usage.cc' in results[1].message)
|
||||
self.assertTrue('content/desktop_android.cc' in results[0].message)
|
||||
'third_party/blink/problematic/file.cc' in results[1].message)
|
||||
self.assertTrue(all('some/cpp/ok/file.cc' not in r.message for r in results))
|
||||
self.assertTrue('some/cpp/problematic/file2.cc' in results[2].message)
|
||||
self.assertTrue('some/cpp/problematic/file3.cc' in results[3].message)
|
||||
self.assertTrue('some/cpp/problematic/file4.cc' in results[4].message)
|
||||
self.assertTrue(all('some/cpp/nocheck/file.cc' not in r.message for r in results))
|
||||
self.assertTrue(all('some/cpp/comment/file.cc' not in r.message for r in results))
|
||||
self.assertTrue(all('allowed_ranges_usage.cc' not in r.message for r in results))
|
||||
self.assertTrue('banned_ranges_usage.cc' in results[5].message)
|
||||
self.assertTrue('views_usage.cc' in results[6].message)
|
||||
self.assertTrue('content/desktop_android.cc' in results[7].message)
|
||||
|
||||
# Check ResultLocation data. Line nums start at 1.
|
||||
self.assertEqual(results[7].locations[0].file_path, 'content/desktop_android.cc')
|
||||
self.assertEqual(results[7].locations[0].start_line, 2)
|
||||
self.assertEqual(results[7].locations[0].end_line, 2)
|
||||
|
||||
def testBannedCppRandomFunctions(self):
|
||||
banned_rngs = [
|
||||
@ -3092,15 +3098,14 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
]
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api,
|
||||
MockOutputApi())
|
||||
self.assertEqual(1, len(results), banned_rng)
|
||||
self.assertEqual(2, len(results), banned_rng)
|
||||
self.assertTrue(
|
||||
'some/cpp/problematic/file.cc' in results[0].message,
|
||||
banned_rng)
|
||||
self.assertTrue(
|
||||
'third_party/blink/problematic/file.cc' in results[0].message,
|
||||
banned_rng)
|
||||
self.assertFalse('third_party/ok/file.cc' in results[0].message,
|
||||
'third_party/blink/problematic/file.cc' in results[1].message,
|
||||
banned_rng)
|
||||
self.assertTrue(all('third_party/ok/file.cc' not in r.message for r in results))
|
||||
|
||||
def testBannedIosObjcFunctions(self):
|
||||
input_api = MockInputApi()
|
||||
@ -3120,12 +3125,12 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
]
|
||||
|
||||
errors = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(errors))
|
||||
self.assertEqual(3, len(errors))
|
||||
self.assertTrue('some/ios/file.mm' in errors[0].message)
|
||||
self.assertTrue('another/ios_file.mm' in errors[0].message)
|
||||
self.assertTrue('some/mac/file.mm' not in errors[0].message)
|
||||
self.assertTrue('some/ios/file_egtest.mm' in errors[0].message)
|
||||
self.assertTrue('some/ios/file_unittest.mm' not in errors[0].message)
|
||||
self.assertTrue('another/ios_file.mm' in errors[1].message)
|
||||
self.assertTrue(all('some/mac/file.mm' not in e.message for e in errors))
|
||||
self.assertTrue('some/ios/file_egtest.mm' in errors[2].message)
|
||||
self.assertTrue(all('some/ios/file_unittest.mm' not in e.message for e in errors))
|
||||
|
||||
def testBannedMojoFunctions(self):
|
||||
input_api = MockInputApi()
|
||||
@ -3137,7 +3142,8 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
|
||||
# warnings are results[0], errors are results[1]
|
||||
# Each entry in results corresponds to a BanRule with a violation, in
|
||||
# the order they were encountered.
|
||||
self.assertEqual(1, len(results))
|
||||
self.assertTrue('some/cpp/problematic/file2.cc' in results[0].message)
|
||||
self.assertTrue(
|
||||
@ -3161,7 +3167,8 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
|
||||
# warnings are results[0], errors are results[1]
|
||||
# Each entry in results corresponds to a BanRule with a violation, in
|
||||
# the order they were encountered.
|
||||
self.assertEqual(1, len(results))
|
||||
self.assertTrue('bad.mojom' in results[0].message)
|
||||
self.assertTrue('good.mojom' not in results[0].message)
|
||||
@ -3184,23 +3191,23 @@ class BannedTypeCheckTest(unittest.TestCase):
|
||||
['string extension_id']),
|
||||
]
|
||||
|
||||
# warnings are results[0], errors are results[1]
|
||||
# Each entry in results corresponds to a BanRule with a violation, in
|
||||
# the order they were encountered.
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
|
||||
# Only warnings and no errors.
|
||||
self.assertEqual(1, len(results))
|
||||
self.assertEqual(3, len(results))
|
||||
|
||||
# Pattern test assertions.
|
||||
self.assertTrue('bad.mojom' in results[0].message)
|
||||
self.assertTrue('bad_struct.mojom' in results[0].message)
|
||||
self.assertTrue('good.mojom' not in results[0].message)
|
||||
self.assertTrue('good_struct.mojom' not in results[0].message)
|
||||
self.assertTrue('bad_struct.mojom' in results[1].message)
|
||||
self.assertTrue(all('good.mojom' not in r.message for r in results))
|
||||
self.assertTrue(all('good_struct.mojom' not in r.message for r in results))
|
||||
|
||||
# Path exclusion assertions.
|
||||
self.assertTrue('some/included/extensions/path/bad_extension_id.mojom'
|
||||
in results[0].message)
|
||||
self.assertTrue('some/excluded/path/bad_extension_id.mojom' not in
|
||||
results[0].message)
|
||||
in results[2].message)
|
||||
self.assertTrue(all('some/excluded/path/bad_extension_id.mojom' not in r.message for r in results))
|
||||
|
||||
|
||||
class NoProductionCodeUsingTestOnlyFunctionsTest(unittest.TestCase):
|
||||
|
||||
@ -5732,20 +5739,19 @@ class CheckDeprecatedSyncConsentFunctionsTest(unittest.TestCase):
|
||||
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
|
||||
self.assertEqual(1, len(results))
|
||||
self.assertFalse(
|
||||
'chrome/browser/android/file.cc' in results[0].message),
|
||||
self.assertEqual(7, len(results))
|
||||
self.assertTrue(all('chrome/browser/android/file.cc' not in r.message for r in results))
|
||||
self.assertTrue('chrome/android/file.cc' in results[0].message),
|
||||
self.assertTrue('ios/file.mm' in results[0].message),
|
||||
self.assertTrue('components/foo/ios/file.cc' in results[0].message),
|
||||
self.assertTrue('ios/file.mm' in results[1].message),
|
||||
self.assertTrue('components/foo/ios/file.cc' in results[2].message),
|
||||
self.assertTrue(
|
||||
'components/foo/delegate_android.cc' in results[0].message),
|
||||
'components/foo/delegate_android.cc' in results[3].message),
|
||||
self.assertTrue(
|
||||
'components/foo/delegate_ios.cc' in results[0].message),
|
||||
'components/foo/delegate_ios.cc' in results[4].message),
|
||||
self.assertTrue(
|
||||
'components/foo/android_delegate.cc' in results[0].message),
|
||||
'components/foo/android_delegate.cc' in results[5].message),
|
||||
self.assertTrue(
|
||||
'components/foo/ios_delegate.cc' in results[0].message),
|
||||
'components/foo/ios_delegate.cc' in results[6].message),
|
||||
|
||||
def testCppNonMobilePlatformPath(self):
|
||||
input_api = MockInputApi()
|
||||
@ -5771,12 +5777,12 @@ class CheckDeprecatedSyncConsentFunctionsTest(unittest.TestCase):
|
||||
|
||||
results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())
|
||||
|
||||
self.assertEqual(1, len(results))
|
||||
self.assertFalse('components/foo/file1.java' in results[0].message),
|
||||
self.assertEqual(4, len(results))
|
||||
self.assertTrue(all('components/foo/file1.java' not in r.message for r in results))
|
||||
self.assertTrue('components/foo/file2.java' in results[0].message),
|
||||
self.assertTrue('chrome/foo/file3.java' in results[0].message),
|
||||
self.assertTrue('chrome/foo/file4.java' in results[0].message),
|
||||
self.assertTrue('chrome/foo/file5.java' in results[0].message),
|
||||
self.assertTrue('chrome/foo/file3.java' in results[1].message),
|
||||
self.assertTrue('chrome/foo/file4.java' in results[2].message),
|
||||
self.assertTrue('chrome/foo/file5.java' in results[3].message),
|
||||
|
||||
|
||||
class CheckAnonymousNamespaceTest(unittest.TestCase):
|
||||
|
||||
@ -210,42 +210,46 @@ class MockOutputApi(object):
|
||||
|
||||
class PresubmitResult(object):
|
||||
|
||||
def __init__(self, message, items=None, long_text=''):
|
||||
def __init__(self, message, items=None, long_text='', locations=[]):
|
||||
self.message = message
|
||||
self.items = items
|
||||
self.long_text = long_text
|
||||
self.locations = locations
|
||||
|
||||
def __repr__(self):
|
||||
return self.message
|
||||
|
||||
class PresubmitError(PresubmitResult):
|
||||
|
||||
def __init__(self, message, items=None, long_text=''):
|
||||
MockOutputApi.PresubmitResult.__init__(self, message, items,
|
||||
long_text)
|
||||
def __init__(self, *args, **kwargs):
|
||||
MockOutputApi.PresubmitResult.__init__(self, *args, **kwargs)
|
||||
self.type = 'error'
|
||||
|
||||
class PresubmitPromptWarning(PresubmitResult):
|
||||
|
||||
def __init__(self, message, items=None, long_text=''):
|
||||
MockOutputApi.PresubmitResult.__init__(self, message, items,
|
||||
long_text)
|
||||
def __init__(self, *args, **kwargs):
|
||||
MockOutputApi.PresubmitResult.__init__(self, *args, **kwargs)
|
||||
self.type = 'warning'
|
||||
|
||||
class PresubmitNotifyResult(PresubmitResult):
|
||||
|
||||
def __init__(self, message, items=None, long_text=''):
|
||||
MockOutputApi.PresubmitResult.__init__(self, message, items,
|
||||
long_text)
|
||||
def __init__(self, *args, **kwargs):
|
||||
MockOutputApi.PresubmitResult.__init__(self, *args, **kwargs)
|
||||
self.type = 'notify'
|
||||
|
||||
class PresubmitPromptOrNotify(PresubmitResult):
|
||||
|
||||
def __init__(self, message, items=None, long_text=''):
|
||||
MockOutputApi.PresubmitResult.__init__(self, message, items,
|
||||
long_text)
|
||||
def __init__(self, *args, **kwargs):
|
||||
MockOutputApi.PresubmitResult.__init__(self, *args, **kwargs)
|
||||
self.type = 'promptOrNotify'
|
||||
|
||||
class PresubmitResultLocation(object):
|
||||
|
||||
def __init__(self, file_path, start_line, end_line):
|
||||
self.file_path = file_path
|
||||
self.start_line = start_line
|
||||
self.end_line = end_line
|
||||
|
||||
def __init__(self):
|
||||
self.more_cc = []
|
||||
|
||||
|
||||
Reference in New Issue
Block a user