From 6402141d1f56c8b6e909a17b50474ae744bd5180 Mon Sep 17 00:00:00 2001 From: local_bot Date: Wed, 8 Jul 2020 21:05:39 +0000 Subject: [PATCH] Rename blocklist/allowlist to skip_list/check_list This change is backwards compatible. R=dpranke@google.com Bug: 1098560 Change-Id: Ic9447041a68b8c379f65900af11c89b006e3f806 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2285156 Commit-Queue: Josip Sokcevic Reviewed-by: Dirk Pranke --- presubmit_canned_checks.py | 78 ++++++++++++++++++++----------------- presubmit_support.py | 65 ++++++++++++++++++++++--------- tests/presubmit_unittest.py | 54 ++++++++++++++++--------- 3 files changed, 124 insertions(+), 73 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index e30960e4a3..10d1729514 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -359,7 +359,7 @@ def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None): """Checks that there are no tab characters in any of the text files to be submitted. """ - # In addition to the filter, make sure that makefiles are blocklisted. + # In addition to the filter, make sure that makefiles are skipped. if not source_file_filter: # It's the default filter. source_file_filter = input_api.FilterSourceFile @@ -608,15 +608,17 @@ def CheckTreeIsOpen(input_api, output_api, return [] def GetUnitTestsInDirectory( - input_api, output_api, directory, allowlist=None, blocklist=None, env=None, - run_on_python2=True, run_on_python3=True, whitelist=None, blacklist=None): + input_api, output_api, directory, files_to_check=None, files_to_skip=None, + env=None, run_on_python2=True, run_on_python3=True, whitelist=None, + blacklist=None, allowlist=None, blocklist=None): """Lists all files in a directory and runs them. Doesn't recurse. It's mainly a wrapper for RunUnitTests. Use allowlist and blocklist to filter tests accordingly. """ - allowlist = allowlist or whitelist - blocklist = blocklist or blacklist + # TODO(https://crbug.com/1098560): Add warnings before removing bc. + files_to_check = files_to_check or allowlist or whitelist + files_to_skip = files_to_skip or blocklist or blacklist unit_tests = [] test_path = input_api.os_path.abspath( input_api.os_path.join(input_api.PresubmitLocalPath(), directory)) @@ -630,9 +632,9 @@ def GetUnitTestsInDirectory( fullpath = input_api.os_path.join(test_path, filename) if not input_api.os_path.isfile(fullpath): continue - if allowlist and not check(filename, allowlist): + if files_to_check and not check(filename, files_to_check): continue - if blocklist and check(filename, blocklist): + if files_to_skip and check(filename, files_to_skip): continue unit_tests.append(input_api.os_path.join(directory, filename)) to_run += 1 @@ -641,8 +643,8 @@ def GetUnitTestsInDirectory( if not to_run: return [ output_api.PresubmitPromptWarning( - 'Out of %d files, found none that matched w=%r, b=%r in directory %s' - % (found, allowlist, blocklist, directory)) + 'Out of %d files, found none that matched c=%r, s=%r in directory %s' + % (found, files_to_check, files_to_skip, directory)) ] return GetUnitTests( input_api, output_api, unit_tests, env, run_on_python2, run_on_python3) @@ -700,22 +702,23 @@ def GetUnitTests( def GetUnitTestsRecursively(input_api, output_api, directory, - allowlist=None, blocklist=None, run_on_python2=True, - run_on_python3=True, whitelist=None, - blacklist=None): + files_to_check=None, files_to_skip=None, + run_on_python2=True, run_on_python3=True, + whitelist=None, blacklist=None, allowlist=None, + blocklist=None): """Gets all files in the directory tree (git repo) that match the whitelist. Restricts itself to only find files within the Change's source repo, not dependencies. """ - allowlist = allowlist or whitelist - blocklist = blocklist or blacklist - assert allowlist is not None - assert blocklist is not None + files_to_check = files_to_check or allowlist or whitelist + files_to_skip = files_to_skip or blocklist or blacklist + assert files_to_check is not None + assert files_to_skip is not None def check(filename): - return (any(input_api.re.match(f, filename) for f in allowlist) and - not any(input_api.re.match(f, filename) for f in blocklist)) + return (any(input_api.re.match(f, filename) for f in files_to_check) and + not any(input_api.re.match(f, filename) for f in files_to_skip)) tests = [] @@ -729,8 +732,8 @@ def GetUnitTestsRecursively(input_api, output_api, directory, if not to_run: return [ output_api.PresubmitPromptWarning( - 'Out of %d files, found none that matched w=%r, b=%r in directory %s' - % (found, allowlist, blocklist, directory)) + 'Out of %d files, found none that matched c=%r, s=%r in directory %s' + % (found, files_to_check, files_to_skip, directory)) ] return GetUnitTests(input_api, output_api, tests, @@ -814,7 +817,7 @@ def RunPythonUnitTests(input_api, *args, **kwargs): GetPythonUnitTests(input_api, *args, **kwargs), False) -def _FetchAllFiles(input_api, allow_list, block_list): +def _FetchAllFiles(input_api, files_to_check, files_to_skip): """Hack to fetch all files.""" # We cannot use AffectedFiles here because we want to test every python # file on each single python change. It's because a change in a python file @@ -836,24 +839,27 @@ def _FetchAllFiles(input_api, allow_list, block_list): # Passes dirnames in block list to speed up search. for item in dirnames[:]: filepath = input_api.os_path.join(dirpath, item)[path_len + 1:] - if Find(filepath, block_list): + if Find(filepath, files_to_skip): dirnames.remove(item) for item in filenames: filepath = input_api.os_path.join(dirpath, item)[path_len + 1:] - if Find(filepath, allow_list) and not Find(filepath, block_list): + if Find(filepath, files_to_check) and not Find(filepath, files_to_skip): files.append(filepath) return files -def GetPylint(input_api, output_api, allow_list=None, block_list=None, +def GetPylint(input_api, output_api, files_to_check=None, files_to_skip=None, disabled_warnings=None, extra_paths_list=None, pylintrc=None, - white_list=None, black_list=None): + white_list=None, black_list=None, allow_list=None, + block_list=None): """Run pylint on python files. - The default allow_list enforces looking only at *.py files. + The default files_to_check enforces looking only at *.py files. """ - allow_list = tuple(allow_list or white_list or (r'.*\.py$',)) - block_list = tuple(block_list or black_list or input_api.DEFAULT_BLOCK_LIST) + files_to_check = tuple(files_to_check or allow_list or white_list or + (r'.*\.py$',)) + files_to_skip = tuple(files_to_skip or block_list or black_list or + input_api.DEFAULT_FILES_TO_SKIP) extra_paths_list = extra_paths_list or [] if input_api.is_committing: @@ -877,7 +883,7 @@ def GetPylint(input_api, output_api, allow_list=None, block_list=None, input_api.PresubmitLocalPath(), input_api.change.RepositoryRoot()), '') return input_api.re.escape(prefix) + regex src_filter = lambda x: input_api.FilterSourceFile( - x, map(rel_path, allow_list), map(rel_path, block_list)) + x, map(rel_path, files_to_check), map(rel_path, files_to_skip)) if not input_api.AffectedSourceFiles(src_filter): input_api.logging.info('Skipping pylint: no matching changes.') return [] @@ -890,7 +896,7 @@ def GetPylint(input_api, output_api, allow_list=None, block_list=None, if disabled_warnings: extra_args.extend(['-d', ','.join(disabled_warnings)]) - files = _FetchAllFiles(input_api, allow_list, block_list) + files = _FetchAllFiles(input_api, files_to_check, files_to_skip) if not files: return [] files.sort() @@ -1184,15 +1190,15 @@ def PanProjectChecks(input_api, output_api, } results = [] - # This code loads the default block list (e.g. third_party, experimental, etc) - # and add our block list (breakpad, skia and v8 are still not following + # This code loads the default skip list (e.g. third_party, experimental, etc) + # and add our skip list (breakpad, skia and v8 are still not following # google style and are not really living this repository). # See presubmit_support.py InputApi.FilterSourceFile for the (simple) usage. - block_list = input_api.DEFAULT_BLOCK_LIST + excluded_paths - allow_list = input_api.DEFAULT_ALLOW_LIST + text_files - sources = lambda x: input_api.FilterSourceFile(x, block_list=block_list) + files_to_skip = input_api.DEFAULT_FILES_TO_SKIP + excluded_paths + files_to_check = input_api.DEFAULT_FILES_TO_CHECK + text_files + sources = lambda x: input_api.FilterSourceFile(x, files_to_skip=files_to_skip) text_files = lambda x: input_api.FilterSourceFile( - x, block_list=block_list, allow_list=allow_list) + x, files_to_skip=files_to_skip, files_to_check=files_to_check) snapshot_memory = [] def snapshot(msg): diff --git a/presubmit_support.py b/presubmit_support.py index 968d8c2ebd..7baa711c79 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -492,8 +492,8 @@ class InputApi(object): # # Files without an extension aren't included in the list. If you want to # filter them as source files, add r'(^|.*?[\\\/])[^.]+$' to the allow list. - # Note that ALL CAPS files are blocked in DEFAULT_BLOCK_LIST below. - DEFAULT_ALLOW_LIST = ( + # Note that ALL CAPS files are skipped in DEFAULT_FILES_TO_SKIP below. + DEFAULT_FILES_TO_CHECK = ( # C++ and friends r'.+\.c$', r'.+\.cc$', r'.+\.cpp$', r'.+\.h$', r'.+\.m$', r'.+\.mm$', r'.+\.inl$', r'.+\.asm$', r'.+\.hxx$', r'.+\.hpp$', r'.+\.s$', r'.+\.S$', @@ -506,7 +506,7 @@ class InputApi(object): # Path regexp that should be excluded from being considered containing source # files. Don't modify this list from a presubmit script! - DEFAULT_BLOCK_LIST = ( + DEFAULT_FILES_TO_SKIP = ( r'testing_support[\\\/]google_appengine[\\\/].*', r'.*\bexperimental[\\\/].*', # Exclude third_party/.* but NOT third_party/{WebKit,blink} @@ -530,22 +530,42 @@ class InputApi(object): # TODO(https://crbug.com/1098562): Remove once no longer used @property def DEFAULT_WHITE_LIST(self): - return self.DEFAULT_ALLOW_LIST + return self.DEFAULT_FILES_TO_CHECK # TODO(https://crbug.com/1098562): Remove once no longer used @DEFAULT_WHITE_LIST.setter def DEFAULT_WHITE_LIST(self, value): - self.DEFAULT_ALLOW_LIST = value + self.DEFAULT_FILES_TO_CHECK = value + + # TODO(https://crbug.com/1098562): Remove once no longer used + @property + def DEFAULT_ALLOW_LIST(self): + return self.DEFAULT_FILES_TO_CHECK + + # TODO(https://crbug.com/1098562): Remove once no longer used + @DEFAULT_ALLOW_LIST.setter + def DEFAULT_ALLOW_LIST(self, value): + self.DEFAULT_FILES_TO_CHECK = value # TODO(https://crbug.com/1098562): Remove once no longer used @property def DEFAULT_BLACK_LIST(self): - return self.DEFAULT_BLOCK_LIST + return self.DEFAULT_FILES_TO_SKIP # TODO(https://crbug.com/1098562): Remove once no longer used @DEFAULT_BLACK_LIST.setter def DEFAULT_BLACK_LIST(self, value): - self.DEFAULT_BLOCK_LIST = value + self.DEFAULT_FILES_TO_SKIP = value + + # TODO(https://crbug.com/1098562): Remove once no longer used + @property + def DEFAULT_BLOCK_LIST(self): + return self.DEFAULT_FILES_TO_SKIP + + # TODO(https://crbug.com/1098562): Remove once no longer used + @DEFAULT_BLOCK_LIST.setter + def DEFAULT_BLOCK_LIST(self, value): + self.DEFAULT_FILES_TO_SKIP = value def __init__(self, change, presubmit_path, is_committing, verbose, gerrit_obj, dry_run=None, thread_pool=None, parallel=False): @@ -693,26 +713,33 @@ class InputApi(object): """An alias to AffectedTestableFiles for backwards compatibility.""" return self.AffectedTestableFiles(include_deletes=include_deletes) - def FilterSourceFile(self, affected_file, allow_list=None, block_list=None, + def FilterSourceFile(self, affected_file, files_to_check=None, + files_to_skip=None, allow_list=None, block_list=None, white_list=None, black_list=None): """Filters out files that aren't considered 'source file'. - If allow_list or block_list is None, InputApi.DEFAULT_ALLOW_LIST - and InputApi.DEFAULT_BLOCK_LIST is used respectively. + If files_to_check or files_to_skip is None, InputApi.DEFAULT_FILES_TO_CHECK + and InputApi.DEFAULT_FILES_TO_SKIP is used respectively. The lists will be compiled as regular expression and AffectedFile.LocalPath() needs to pass both list. - Note: if allow_list or block_list is not set, and white_list or black_list - is, then those values are used. This is used for backward compatibility - reasons. + Note: if files_to_check or files_to_skip is not set, and + white_list/allow_list or black_list/block_list is, then those values are + used. This is used for backward compatibility reasons. Note: Copy-paste this function to suit your needs or use a lambda function. """ - if allow_list is None: - allow_list = white_list - if block_list is None: - block_list = black_list + # TODO(https://crbug.com/1098560): Add warnings before removing bc. + if files_to_check is None: + files_to_check = allow_list or white_list + if files_to_skip is None: + files_to_skip = block_list or black_list + + if files_to_check is None: + files_to_check = self.DEFAULT_FILES_TO_CHECK + if files_to_skip is None: + files_to_skip = self.DEFAULT_FILES_TO_SKIP def Find(affected_file, items): local_path = affected_file.LocalPath() @@ -720,8 +747,8 @@ class InputApi(object): if self.re.match(item, local_path): return True return False - return (Find(affected_file, allow_list or self.DEFAULT_ALLOW_LIST) and - not Find(affected_file, block_list or self.DEFAULT_BLOCK_LIST)) + return (Find(affected_file, files_to_check) and + not Find(affected_file, files_to_skip)) def AffectedSourceFiles(self, source_file): """Filter the list of AffectedTestableFiles by the function source_file. diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 85ec610bd0..b3c1a9bb5a 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1389,26 +1389,44 @@ class InputApiUnittest(PresubmitTestsBase): def testDefaultOverrides(self): input_api = presubmit.InputApi( self.fake_change, './PRESUBMIT.py', False, None, False) - self.assertEqual(len(input_api.DEFAULT_ALLOW_LIST), 24) - self.assertEqual(len(input_api.DEFAULT_BLOCK_LIST), 12) - self.assertEqual(input_api.DEFAULT_ALLOW_LIST, input_api.DEFAULT_WHITE_LIST) - self.assertEqual(input_api.DEFAULT_BLOCK_LIST, input_api.DEFAULT_BLACK_LIST) - - input_api.DEFAULT_ALLOW_LIST = (r'.+\.c$',) - input_api.DEFAULT_BLOCK_LIST = (r'.+\.patch$', r'.+\.diff') - self.assertEqual(len(input_api.DEFAULT_ALLOW_LIST), 1) - self.assertEqual(len(input_api.DEFAULT_BLOCK_LIST), 2) - self.assertEqual(input_api.DEFAULT_ALLOW_LIST, input_api.DEFAULT_WHITE_LIST) - self.assertEqual(input_api.DEFAULT_BLOCK_LIST, input_api.DEFAULT_BLACK_LIST) + self.assertEqual(len(input_api.DEFAULT_FILES_TO_CHECK), 24) + self.assertEqual(len(input_api.DEFAULT_FILES_TO_SKIP), 12) + self.assertEqual( + input_api.DEFAULT_FILES_TO_CHECK, input_api.DEFAULT_WHITE_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_CHECK, input_api.DEFAULT_ALLOW_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_SKIP, input_api.DEFAULT_BLACK_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_SKIP, input_api.DEFAULT_BLOCK_LIST) + + input_api.DEFAULT_FILES_TO_CHECK = (r'.+\.c$',) + input_api.DEFAULT_FILES_TO_SKIP = (r'.+\.patch$', r'.+\.diff') + self.assertEqual(len(input_api.DEFAULT_FILES_TO_CHECK), 1) + self.assertEqual(len(input_api.DEFAULT_FILES_TO_SKIP), 2) + self.assertEqual( + input_api.DEFAULT_FILES_TO_CHECK, input_api.DEFAULT_WHITE_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_CHECK, input_api.DEFAULT_ALLOW_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_SKIP, input_api.DEFAULT_BLACK_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_SKIP, input_api.DEFAULT_BLOCK_LIST) # Test backward compatiblity of setting old property names # TODO(https://crbug.com/1098562): Remove once no longer used input_api.DEFAULT_WHITE_LIST = () input_api.DEFAULT_BLACK_LIST = () - self.assertEqual(len(input_api.DEFAULT_ALLOW_LIST), 0) - self.assertEqual(len(input_api.DEFAULT_BLOCK_LIST), 0) - self.assertEqual(input_api.DEFAULT_ALLOW_LIST, input_api.DEFAULT_WHITE_LIST) - self.assertEqual(input_api.DEFAULT_BLOCK_LIST, input_api.DEFAULT_BLACK_LIST) + self.assertEqual(len(input_api.DEFAULT_FILES_TO_CHECK), 0) + self.assertEqual(len(input_api.DEFAULT_FILES_TO_SKIP), 0) + self.assertEqual( + input_api.DEFAULT_FILES_TO_CHECK, input_api.DEFAULT_WHITE_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_CHECK, input_api.DEFAULT_ALLOW_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_SKIP, input_api.DEFAULT_BLACK_LIST) + self.assertEqual( + input_api.DEFAULT_FILES_TO_SKIP, input_api.DEFAULT_BLOCK_LIST) def testCustomFilter(self): def FilterSourceFile(affected_file): @@ -1431,8 +1449,8 @@ class InputApiUnittest(PresubmitTestsBase): self.assertEqual(got_files[1].LocalPath(), 'eeabee') def testLambdaFilter(self): - allow_list = presubmit.InputApi.DEFAULT_BLOCK_LIST + (r".*?a.*?",) - block_list = [r".*?b.*?"] + files_to_check = presubmit.InputApi.DEFAULT_FILES_TO_SKIP + (r".*?a.*?",) + files_to_skip = [r".*?b.*?"] files = [('A', 'eeaee'), ('M', 'eeabee'), ('M', 'eebcee'), ('M', 'eecaee')] known_files = [ os.path.join(self.fake_root_dir, item) @@ -1445,7 +1463,7 @@ class InputApiUnittest(PresubmitTestsBase): change, './PRESUBMIT.py', False, None, False) # Sample usage of overriding the default white and black lists. got_files = input_api.AffectedSourceFiles( - lambda x: input_api.FilterSourceFile(x, allow_list, block_list)) + lambda x: input_api.FilterSourceFile(x, files_to_check, files_to_skip)) self.assertEqual(len(got_files), 2) self.assertEqual(got_files[0].LocalPath(), 'eeaee') self.assertEqual(got_files[1].LocalPath(), 'eecaee')