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}
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
index 364fdba1..6fa5eb3 100755
--- a/PRESUBMIT_test.py
+++ b/PRESUBMIT_test.py
@@ -2979,35 +2979,34 @@
]
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 @@
['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 @@
]
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,
+ 'third_party/blink/problematic/file.cc' in results[1].message,
banned_rng)
- self.assertFalse('third_party/ok/file.cc' in results[0].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 @@
]
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 @@
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 @@
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 @@
['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 @@
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 @@
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):
