[chromium/src] Switch to canned CheckInclusiveLanguage PRESUBMIT check.
Change-Id: I4dd72e7c2c965940ec2824fc6e8b7ca01640ffff Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2811478 Reviewed-by: Dirk Pranke <dpranke@google.com> Commit-Queue: Sean McCullough <seanmccullough@chromium.org> Cr-Commit-Position: refs/heads/master@{#870662}
This commit is contained in:
seanmccullough
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
e437a11d1f
commit
4a935625e1
155
PRESUBMIT.py
155
PRESUBMIT.py
@ -4266,6 +4266,26 @@ def CheckAccessibilityRelnotesField(input_api, output_api):
|
||||
|
||||
return [output_api.PresubmitNotifyResult(message)]
|
||||
|
||||
# string pattern, sequence of strings to show when pattern matches,
|
||||
# error flag. True if match is a presubmit error, otherwise it's a warning.
|
||||
_NON_INCLUSIVE_TERMS = (
|
||||
(
|
||||
# Note that \b pattern in python re is pretty particular. In this
|
||||
# regexp, 'class WhiteList ...' will match, but 'class FooWhiteList
|
||||
# ...' will not. This may require some tweaking to catch these cases
|
||||
# without triggering a lot of false positives. Leaving it naive and
|
||||
# less matchy for now.
|
||||
r'/\b(?i)((black|white)list|slave)\b', # nocheck
|
||||
(
|
||||
'Please don\'t use blacklist, whitelist, ' # nocheck
|
||||
'or slave in your', # nocheck
|
||||
'code and make every effort to use other terms. Using "// nocheck"',
|
||||
'"# nocheck" or "<!-- nocheck -->"',
|
||||
'at the end of the offending line will bypass this PRESUBMIT error',
|
||||
'but avoid using this whenever possible. Reach out to',
|
||||
'community@chromium.org if you have questions'),
|
||||
True),)
|
||||
|
||||
def ChecksCommon(input_api, output_api):
|
||||
"""Checks common to both upload and commit."""
|
||||
results = []
|
||||
@ -4297,6 +4317,13 @@ def ChecksCommon(input_api, output_api):
|
||||
results.extend(
|
||||
input_api.canned_checks.CheckNoNewMetadataInOwners(
|
||||
input_api, output_api))
|
||||
results.extend(input_api.canned_checks.CheckInclusiveLanguage(
|
||||
input_api, output_api,
|
||||
excluded_directories_relative_path = [
|
||||
'infra',
|
||||
'inclusive_language_presubmit_exempt_dirs.txt'
|
||||
],
|
||||
non_inclusive_terms=_NON_INCLUSIVE_TERMS))
|
||||
|
||||
for f in input_api.AffectedFiles():
|
||||
path, name = input_api.os_path.split(f.LocalPath())
|
||||
@ -5375,131 +5402,3 @@ def CheckDeprecationOfPreferences(input_api, output_api):
|
||||
potential_problems
|
||||
)]
|
||||
return []
|
||||
|
||||
|
||||
# string pattern, sequence of strings to show when pattern matches,
|
||||
# error flag. True if match is a presubmit error, otherwise it's a warning.
|
||||
_NON_INCLUSIVE_TERMS = (
|
||||
(
|
||||
# Note that \b pattern in python re is pretty particular. In this
|
||||
# regexp, 'class WhiteList ...' will match, but 'class FooWhiteList
|
||||
# ...' will not. This may require some tweaking to catch these cases
|
||||
# without triggering a lot of false positives. Leaving it naive and
|
||||
# less matchy for now.
|
||||
r'/\b(?i)((black|white)list|slave)\b', # nocheck
|
||||
(
|
||||
'Please don\'t use blacklist, whitelist, ' # nocheck
|
||||
'or slave in your', # nocheck
|
||||
'code and make every effort to use other terms. Using "// nocheck"',
|
||||
'"# nocheck" or "<!-- nocheck -->"',
|
||||
'at the end of the offending line will bypass this PRESUBMIT error',
|
||||
'but avoid using this whenever possible. Reach out to',
|
||||
'community@chromium.org if you have questions'),
|
||||
True),)
|
||||
|
||||
|
||||
def _GetMessageForMatchingTerm(input_api, affected_file, line_number, line,
|
||||
term, message):
|
||||
"""Helper method for CheckInclusiveLanguage.
|
||||
|
||||
Returns an string composed of the name of the file, the line number where the
|
||||
match has been found and the additional text passed as |message| in case the
|
||||
target type name matches the text inside the line passed as parameter.
|
||||
"""
|
||||
result = []
|
||||
|
||||
# A // nocheck comment will bypass this error.
|
||||
if line.endswith(" nocheck") or line.endswith("<!-- nocheck -->"):
|
||||
return result
|
||||
|
||||
# Ignore C-style single-line comments about banned terms.
|
||||
if input_api.re.search(r"//.*$", line):
|
||||
line = input_api.re.sub(r"//.*$", "", line)
|
||||
|
||||
# Ignore lines from C-style multi-line comments.
|
||||
if input_api.re.search(r"^\s*\*", line):
|
||||
return result
|
||||
|
||||
# Ignore Python-style comments about banned terms.
|
||||
# This actually removes comment text from the first # on.
|
||||
if input_api.re.search(r"#.*$", line):
|
||||
line = input_api.re.sub(r"#.*$", "", line)
|
||||
|
||||
matched = False
|
||||
if term[0:1] == '/':
|
||||
regex = term[1:]
|
||||
if input_api.re.search(regex, line):
|
||||
matched = True
|
||||
elif term in line:
|
||||
matched = True
|
||||
|
||||
if matched:
|
||||
result.append(' %s:%d:' % (affected_file.LocalPath(), line_number))
|
||||
for message_line in message:
|
||||
result.append(' %s' % message_line)
|
||||
|
||||
return result
|
||||
|
||||
|
||||
def CheckInclusiveLanguage(input_api, output_api):
|
||||
"""Make sure that banned non-inclusive terms are not used."""
|
||||
|
||||
# Presubmit checks may run on a bot where the changes are actually
|
||||
# in a repo that isn't chromium/src (e.g., when testing src + tip-of-tree
|
||||
# ANGLE), but this particular check only makes sense for changes to
|
||||
# chromium/src.
|
||||
if input_api.change.RepositoryRoot() != input_api.PresubmitLocalPath():
|
||||
return []
|
||||
|
||||
warnings = []
|
||||
errors = []
|
||||
|
||||
# Note that this matches exact path prefixes, and does not match
|
||||
# subdirectories. Only files directly in an exlcluded path will
|
||||
# match.
|
||||
def IsExcludedFile(affected_file, excluded_paths):
|
||||
local_dir = input_api.os_path.dirname(affected_file.LocalPath())
|
||||
|
||||
return local_dir in excluded_paths
|
||||
|
||||
def CheckForMatch(affected_file, line_num, line, term, message, error):
|
||||
problems = _GetMessageForMatchingTerm(input_api, affected_file, line_num,
|
||||
line, term, message)
|
||||
|
||||
if problems:
|
||||
if error:
|
||||
errors.extend(problems)
|
||||
else:
|
||||
warnings.extend(problems)
|
||||
|
||||
excluded_paths = []
|
||||
|
||||
dirs_file_path = input_api.os_path.join(
|
||||
input_api.change.RepositoryRoot(), 'infra',
|
||||
'inclusive_language_presubmit_exempt_dirs.txt')
|
||||
f = input_api.ReadFile(dirs_file_path)
|
||||
|
||||
for line in f.split('\n'):
|
||||
path = line.split(' ')[0]
|
||||
if len(path) > 0:
|
||||
excluded_paths.append(path)
|
||||
|
||||
excluded_paths = set(excluded_paths)
|
||||
for f in input_api.AffectedFiles():
|
||||
for line_num, line in f.ChangedContents():
|
||||
for term, message, error in _NON_INCLUSIVE_TERMS:
|
||||
if IsExcludedFile(f, excluded_paths):
|
||||
continue
|
||||
CheckForMatch(f, line_num, line, term, message, error)
|
||||
|
||||
result = []
|
||||
if (warnings):
|
||||
result.append(
|
||||
output_api.PresubmitPromptWarning(
|
||||
'Banned non-inclusive language was used.\n' + '\n'.join(warnings)))
|
||||
if (errors):
|
||||
result.append(
|
||||
output_api.PresubmitError('Banned non-inclusive language was used.\n' +
|
||||
'\n'.join(errors)))
|
||||
return result
|
||||
|
||||
|
||||
@ -3949,189 +3949,5 @@ class CheckDeprecationOfPreferencesTest(unittest.TestCase):
|
||||
errors[0].message)
|
||||
|
||||
|
||||
class InclusiveLanguageCheckTest(unittest.TestCase):
|
||||
def testBlockedTerms(self):
|
||||
input_api = MockInputApi()
|
||||
input_api.change.RepositoryRoot = lambda: ''
|
||||
input_api.presubmit_local_path = ''
|
||||
|
||||
input_api.files = [
|
||||
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
|
||||
'some/dir 2 1',
|
||||
'some/other/dir 2 1',
|
||||
]),
|
||||
MockFile('some/ios/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
|
||||
'}']),
|
||||
MockFile('some/mac/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, BlackList) {', # nocheck
|
||||
'}']),
|
||||
MockFile('another/ios_file.mm',
|
||||
['class SomeTest : public testing::Test blocklist {};']),
|
||||
MockFile('some/ios/file_egtest.mm',
|
||||
['- (void)testSomething { V(whitelist); }']), # nocheck
|
||||
MockFile('some/ios/file_unittest.mm',
|
||||
['TEST_F(SomeTest, Whitelist) { V(allowlist); }']), # nocheck
|
||||
MockFile('some/doc/file.md',
|
||||
['# Title',
|
||||
'Some markdown text includes master.', # nocheck
|
||||
]),
|
||||
MockFile('some/doc/ok_file.md',
|
||||
['# Title',
|
||||
# This link contains a '//' which the matcher thinks is a
|
||||
# C-style comment, and the 'master' term appears after the
|
||||
# '//' in the URL, so it gets ignored as a side-effect.
|
||||
'[Ignored](https://git/project.git/+/master/foo)', # nocheck
|
||||
]),
|
||||
MockFile('some/doc/branch_name_file.md',
|
||||
['# Title',
|
||||
# Matches appearing before `//` still trigger the check.
|
||||
'[src/master](https://git/p.git/+/master/foo)', # nocheck
|
||||
]),
|
||||
MockFile('some/java/file/TestJavaDoc.java',
|
||||
['/**',
|
||||
' * This line contains the word master,', # nocheck
|
||||
'* ignored because this is a comment. See {@link',
|
||||
' * https://s/src/+/master:tools/README.md}', # nocheck
|
||||
' */']),
|
||||
MockFile('some/java/file/TestJava.java',
|
||||
['class TestJava {',
|
||||
' public String master;', # nocheck
|
||||
'}']),
|
||||
MockFile('some/html/file.html',
|
||||
['<-- an existing html multiline comment',
|
||||
'says "master" here', # nocheck
|
||||
'in the comment -->'])
|
||||
]
|
||||
|
||||
errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(errors))
|
||||
self.assertTrue('some/ios/file.mm' in errors[0].message)
|
||||
self.assertTrue('another/ios_file.mm' not in errors[0].message)
|
||||
self.assertTrue('some/mac/file.mm' in errors[0].message)
|
||||
self.assertTrue('some/ios/file_egtest.mm' in errors[0].message)
|
||||
self.assertTrue('some/ios/file_unittest.mm' in errors[0].message)
|
||||
self.assertTrue('some/doc/file.md' not in errors[0].message)
|
||||
self.assertTrue('some/doc/ok_file.md' not in errors[0].message)
|
||||
self.assertTrue('some/doc/branch_name_file.md' not in errors[0].message)
|
||||
self.assertTrue('some/java/file/TestJavaDoc.java' not in errors[0].message)
|
||||
self.assertTrue('some/java/file/TestJava.java' not in errors[0].message)
|
||||
self.assertTrue('some/html/file.html' not in errors[0].message)
|
||||
|
||||
def testBlockedTermsWithLegacy(self):
|
||||
input_api = MockInputApi()
|
||||
input_api.change.RepositoryRoot = lambda: ''
|
||||
input_api.presubmit_local_path = ''
|
||||
|
||||
input_api.files = [
|
||||
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
|
||||
'some/ios 2 1',
|
||||
'some/other/dir 2 1',
|
||||
]),
|
||||
MockFile('some/ios/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
|
||||
'}']),
|
||||
MockFile('some/ios/subdir/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
|
||||
'}']),
|
||||
MockFile('some/mac/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, BlackList) {', # nocheck
|
||||
'}']),
|
||||
MockFile('another/ios_file.mm',
|
||||
['class SomeTest : public testing::Test blocklist {};']),
|
||||
MockFile('some/ios/file_egtest.mm',
|
||||
['- (void)testSomething { V(whitelist); }']), # nocheck
|
||||
MockFile('some/ios/file_unittest.mm',
|
||||
['TEST_F(SomeTest, Whitelist) { V(allowlist); }']), # nocheck
|
||||
]
|
||||
|
||||
errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(errors))
|
||||
self.assertTrue('some/ios/file.mm' not in errors[0].message)
|
||||
self.assertTrue('some/ios/subdir/file.mm' in errors[0].message)
|
||||
self.assertTrue('another/ios_file.mm' not in errors[0].message)
|
||||
self.assertTrue('some/mac/file.mm' in errors[0].message)
|
||||
self.assertTrue('some/ios/file_egtest.mm' not in errors[0].message)
|
||||
self.assertTrue('some/ios/file_unittest.mm' not in errors[0].message)
|
||||
|
||||
def testBlockedTermsWithNocheck(self):
|
||||
input_api = MockInputApi()
|
||||
input_api.change.RepositoryRoot = lambda: ''
|
||||
input_api.presubmit_local_path = ''
|
||||
|
||||
input_api.files = [
|
||||
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
|
||||
'some/dir 2 1',
|
||||
'some/other/dir 2 1',
|
||||
]),
|
||||
MockFile('some/ios/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, ',
|
||||
' blacklist) { // nocheck', # nocheck
|
||||
'}']),
|
||||
MockFile('some/mac/file.mm',
|
||||
['TEST(SomeClassTest, SomeInteraction, ',
|
||||
'BlackList) { // nocheck', # nocheck
|
||||
'}']),
|
||||
MockFile('another/ios_file.mm',
|
||||
['class SomeTest : public testing::Test blocklist {};']),
|
||||
MockFile('some/ios/file_egtest.mm',
|
||||
['- (void)testSomething { ',
|
||||
'V(whitelist); } // nocheck']), # nocheck
|
||||
MockFile('some/ios/file_unittest.mm',
|
||||
['TEST_F(SomeTest, Whitelist) // nocheck', # nocheck
|
||||
' { V(allowlist); }']),
|
||||
MockFile('some/doc/file.md',
|
||||
['Master in markdown <!-- nocheck -->', # nocheck
|
||||
'## Subheading is okay']),
|
||||
MockFile('some/java/file/TestJava.java',
|
||||
['class TestJava {',
|
||||
' public String master; // nocheck', # nocheck
|
||||
'}']),
|
||||
MockFile('some/html/file.html',
|
||||
['<-- an existing html multiline comment',
|
||||
'says "master" here --><!-- nocheck -->', # nocheck
|
||||
'<!-- in the comment -->'])
|
||||
]
|
||||
|
||||
errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
|
||||
self.assertEqual(0, len(errors))
|
||||
|
||||
def testTopLevelDirExcempt(self):
|
||||
input_api = MockInputApi()
|
||||
input_api.change.RepositoryRoot = lambda: ''
|
||||
input_api.presubmit_local_path = ''
|
||||
|
||||
input_api.files = [
|
||||
MockFile('infra/inclusive_language_presubmit_exempt_dirs.txt', [
|
||||
'. 2 1',
|
||||
'some/other/dir 2 1',
|
||||
]),
|
||||
MockFile('PRESUBMIT_test.py',
|
||||
['TEST(SomeClassTest, SomeInteraction, blacklist) {', # nocheck
|
||||
'}']),
|
||||
MockFile('PRESUBMIT.py',
|
||||
['- (void)testSth { V(whitelist); } // nocheck']), # nocheck
|
||||
]
|
||||
|
||||
errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(errors))
|
||||
self.assertTrue('PRESUBMIT_test.py' in errors[0].message)
|
||||
self.assertTrue('PRESUBMIT.py' not in errors[0].message)
|
||||
|
||||
|
||||
def testChangeIsForSomeOtherRepo(self):
|
||||
input_api = MockInputApi()
|
||||
input_api.change.RepositoryRoot = lambda: 'v8'
|
||||
input_api.presubmit_local_path = ''
|
||||
|
||||
input_api.files = [
|
||||
MockFile('some_file', [
|
||||
'# this is a blacklist', # nocheck
|
||||
]),
|
||||
]
|
||||
errors = PRESUBMIT.CheckInclusiveLanguage(input_api, MockOutputApi())
|
||||
self.assertEqual([], errors)
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user