From a73eec2873007750f75c24648ebaeaeceaa6ec60 Mon Sep 17 00:00:00 2001 From: Fabrice de Gans Date: Sat, 26 Jun 2021 00:17:49 +0000 Subject: [PATCH] Add a new argument to skip shebang checks on python3 A bug in the logic enabling tests to be run on python3 resulted in tests never being run in python3 unless they also have a shebang line pointing to python3. Since this bug was introduced, effectively, most tests that are supposed to be run on python3 do not run at all. Since then a lot of python3 tests have regressed in Chromium. In order to fix the issue moving forward, this introduces a new argument that skips the shebang check to run a test in python3. Tests will be re-enabled one by one until every single instance in Chromium has been updated. Bug: 1223478 Change-Id: I91a0688c6f4d9b4fbf18e3d446366cded8c7f2f1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2986400 Reviewed-by: Dirk Pranke Reviewed-by: Josip Sokcevic Reviewed-by: Ben Pastene Commit-Queue: Fabrice de Gans --- presubmit_canned_checks.py | 40 +++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 74d5d16e6..3ffd7d4b9 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -648,6 +648,7 @@ def GetUnitTestsInDirectory(input_api, env=None, run_on_python2=True, run_on_python3=True, + skip_shebang_check=False, allowlist=None, blocklist=None): """Lists all files in a directory and runs them. Doesn't recurse. @@ -682,13 +683,17 @@ def GetUnitTestsInDirectory(input_api, '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) + return GetUnitTests(input_api, output_api, unit_tests, env, run_on_python2, + run_on_python3, skip_shebang_check) -def GetUnitTests( - input_api, output_api, unit_tests, env=None, run_on_python2=True, - run_on_python3=True): +def GetUnitTests(input_api, + output_api, + unit_tests, + env=None, + run_on_python2=True, + run_on_python3=True, + skip_shebang_check=False): """Runs all unit tests in a directory. On Windows, sys.executable is used for unit tests ending with ".py". @@ -721,19 +726,32 @@ def GetUnitTests( kwargs=kwargs, message=message_type)) else: - if has_py3_shebang(unit_test) and run_on_python3: + test_run = False + # TODO(crbug.com/1223478): The intent for this line was to run the test + # on python3 if the file has a shebang OR if it was explicitly requested + # to run on python3. Since tests have been broken since this landed, we + # introduced the |skip_shebang_check| argument to work around the issue + # until every caller in Chromium has been fixed. + if (skip_shebang_check or has_py3_shebang(unit_test)) and run_on_python3: results.append(input_api.Command( name=unit_test, cmd=cmd, kwargs=kwargs, message=message_type, python3=True)) + test_run = True if run_on_python2: results.append(input_api.Command( name=unit_test, cmd=cmd, kwargs=kwargs, message=message_type)) + test_run = True + if not test_run: + output_api.PresubmitPromptWarning( + "Some python tests were not run. You may need to add\n" + "skip_shebang_check=True for python3 tests.", + items=unit_test) return results @@ -743,7 +761,8 @@ def GetUnitTestsRecursively(input_api, files_to_check, files_to_skip, run_on_python2=True, - run_on_python3=True): + run_on_python3=True, + skip_shebang_check=False): """Gets all files in the directory tree (git repo) that match files_to_check. Restricts itself to only find files within the Change's source repo, not @@ -769,9 +788,12 @@ def GetUnitTestsRecursively(input_api, % (found, files_to_check, files_to_skip, directory)) ] - return GetUnitTests(input_api, output_api, tests, + return GetUnitTests(input_api, + output_api, + tests, run_on_python2=run_on_python2, - run_on_python3=run_on_python3) + run_on_python3=run_on_python3, + skip_shebang_check=skip_shebang_check) def GetPythonUnitTests(input_api, output_api, unit_tests):