chromium/src
0
Fork 0
Code Activity

Check more cases in android log tag PRESUBMIT

Browse Source 
Relax a regex to catch Log.e("log message", ..) as a bad/missing TAG
instead of silently accepting it. Also add a rough TAG-change detection
regex to try and make changes that only touch a TAG also run the checks.
Adjust tests to match.

Bug: 1063573
Change-Id: I6a5c302d581aa76ebfc86a286e6048412641f4de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2113572
Auto-Submit: Tomasz Śniatowski <tsniatowski@vewd.com>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#752441}
This commit is contained in:
Tomasz Śniatowski
2020-03-23 15:35:55 +00:00
committed by Commit Bot
parent 1112bc34b6
commit 3ae2f10e43
2 changed files with 25 additions and 8 deletions

6
PRESUBMIT.py

@ -3269,9 +3269,10 @@ def _CheckAndroidCrLogUsage(input_api, output_api):
has_some_log_import_pattern = input_api.re.compile(
r'^import .*\.Log;$', input_api.re.MULTILINE)
# Extract the tag from lines like `Log.d(TAG, "*");` or `Log.d("TAG", "*");`
log_call_pattern = input_api.re.compile(r'^\s*Log\.\w\((?P<tag>\"?\w+\"?)\,')
log_call_pattern = input_api.re.compile(r'\bLog\.\w\((?P<tag>\"?\w+)')
log_decl_pattern = input_api.re.compile(
r'static final String TAG = "(?P<name>(.*))"')
rough_log_decl_pattern = input_api.re.compile(r'\bString TAG\s*=')
REF_MSG = ('See docs/android_logging.md for more info.')
sources = lambda x: input_api.FilterSourceFile(x, white_list=[r'.*\.java$'],
@ -3286,13 +3287,14 @@ def _CheckAndroidCrLogUsage(input_api, output_api):
for f in input_api.AffectedSourceFiles(sources):
file_content = input_api.ReadFile(f)
has_modified_logs = False
# Per line checks
if (cr_log_import_pattern.search(file_content) or
(class_in_base_pattern.search(file_content) and
not has_some_log_import_pattern.search(file_content))):
# Checks to run for files using cr log
for line_num, line in f.ChangedContents():
if rough_log_decl_pattern.search(line):
has_modified_logs = True
# Check if the new line is doing some logging
match = log_call_pattern.search(line)

27
PRESUBMIT_test.py

@ -1290,6 +1290,12 @@ class LogUsageTest(unittest.TestCase):
'private static final String TAG = "cr_Foo";',
'Log.d("TAG", "foo");',
]),
MockAffectedFile('HasInlineTagWithSpace.java', [
'import org.chromium.base.Log;',
'some random stuff',
'private static final String TAG = "cr_Foo";',
'Log.d("log message", "foo");',
]),
MockAffectedFile('HasUnprefixedTag.java', [
'import org.chromium.base.Log;',
'some random stuff',
@ -1302,6 +1308,11 @@ class LogUsageTest(unittest.TestCase):
'private static final String TAG = "21_charachers_long___";',
'Log.d(TAG, "foo");',
]),
MockAffectedFile('HasTooLongTagWithNoLogCallsInDiff.java', [
'import org.chromium.base.Log;',
'some random stuff',
'private static final String TAG = "21_charachers_long___";',
]),
]
msgs = PRESUBMIT._CheckAndroidCrLogUsage(
@ -1319,21 +1330,25 @@ class LogUsageTest(unittest.TestCase):
# Tag length
nb = len(msgs[1].items)
self.assertEqual(1, nb,
'Expected %d items, found %d: %s' % (1, nb, msgs[1].items))
self.assertEqual(2, nb,
'Expected %d items, found %d: %s' % (2, nb, msgs[1].items))
self.assertTrue('HasTooLongTag.java' in msgs[1].items)
self.assertTrue('HasTooLongTagWithNoLogCallsInDiff.java' in msgs[1].items)
# Tag must be a variable named TAG
nb = len(msgs[2].items)
self.assertEqual(1, nb,
'Expected %d items, found %d: %s' % (1, nb, msgs[2].items))
self.assertEqual(3, nb,
'Expected %d items, found %d: %s' % (3, nb, msgs[2].items))
self.assertTrue('HasBothLog.java:5' in msgs[2].items)
self.assertTrue('HasInlineTag.java:4' in msgs[2].items)
self.assertTrue('HasInlineTagWithSpace.java:4' in msgs[2].items)
# Util Log usage
nb = len(msgs[3].items)
self.assertEqual(2, nb,
'Expected %d items, found %d: %s' % (2, nb, msgs[3].items))
self.assertEqual(3, nb,
'Expected %d items, found %d: %s' % (3, nb, msgs[3].items))
self.assertTrue('HasAndroidLog.java:3' in msgs[3].items)
self.assertTrue('HasExplicitUtilLog.java:2' in msgs[3].items)
self.assertTrue('IsInBasePackageButImportsLog.java:4' in msgs[3].items)
# Tag must not contain