From b3d2ac5161b44d2f09c211048056248be224100a Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 14 Nov 2018 22:57:40 +0100 Subject: [PATCH] Fail-fast in test/cover/lint scripts (#6487) After #6485 and #6435, it appears that there is no good reason to not fail fast when test, cover or linting scripts are executed. This PR ensures to fail fast by invoking commands throught subprocess.check_call instead of subprocess.call, and by removing the handling of non-zero exit code at the end of theses scripts. As now coverage on Windows is executed with thresholds, I added specific thresholds for this platform. Because some portions of code that are done for Unix platform will not be executed on Windows. Note that coverage reports from Travis and AppVeyor are accumulated on Codecov. So if a file is covered up to 50 % on Linux, and all other parts are covered on Windows, then coverage is 100 % for Codecov. Note: that PR also fixes the ability of coverage tests to fail if thresholds are exceeded. * Use check_call to fail fast in all scripts related to tests/lint/coverage/deploy * Make specific coverage threshold for windows --- tests/modification-check.py | 4 +-- tools/_venv_common.py | 24 +++++++---------- tools/install_and_test.py | 14 ++++------ tools/pip_install.py | 14 ++++------ tools/pip_install_editable.py | 4 +-- tools/venv.py | 4 +-- tools/venv3.py | 4 +-- tox.cover.py | 49 ++++++++++++++++------------------- 8 files changed, 51 insertions(+), 66 deletions(-) diff --git a/tests/modification-check.py b/tests/modification-check.py index e00994b04..8abc0fbfe 100755 --- a/tests/modification-check.py +++ b/tests/modification-check.py @@ -70,7 +70,7 @@ def validate_scripts_content(repo_path, temp_cwd): # Compare file against the latest released version latest_version = subprocess.check_output( [sys.executable, 'fetch.py', '--latest-version'], cwd=temp_cwd) - subprocess.call( + subprocess.check_call( [sys.executable, 'fetch.py', '--le-auto-script', 'v{0}'.format(latest_version.decode().strip())], cwd=temp_cwd) if compare_files( @@ -95,7 +95,7 @@ def main(): os.path.normpath(os.path.join(repo_path, 'letsencrypt-auto-source/letsencrypt-auto')), os.path.join(temp_cwd, 'original-lea') ) - subprocess.call([sys.executable, os.path.normpath(os.path.join( + subprocess.check_call([sys.executable, os.path.normpath(os.path.join( repo_path, 'letsencrypt-auto-source/build.py'))]) shutil.copyfile( os.path.normpath(os.path.join(repo_path, 'letsencrypt-auto-source/letsencrypt-auto')), diff --git a/tools/_venv_common.py b/tools/_venv_common.py index b180518f9..6134bd29d 100755 --- a/tools/_venv_common.py +++ b/tools/_venv_common.py @@ -11,13 +11,13 @@ import sys def subprocess_with_print(command): print(command) - return subprocess.call(command, shell=True) + subprocess.check_call(command, shell=True) def get_venv_python(venv_path): python_linux = os.path.join(venv_path, 'bin/python') - python_windows = os.path.join(venv_path, 'Scripts\\python.exe') if os.path.isfile(python_linux): return python_linux + python_windows = os.path.join(venv_path, 'Scripts\\python.exe') if os.path.isfile(python_windows): return python_windows @@ -35,19 +35,17 @@ def main(venv_name, venv_args, args): if os.path.isdir(venv_name): os.rename(venv_name, '{0}.{1}.bak'.format(venv_name, int(time.time()))) - exit_code = 0 - - exit_code = subprocess_with_print(' '.join([ + subprocess_with_print(' '.join([ sys.executable, '-m', 'virtualenv', '--no-site-packages', '--setuptools', - venv_name, venv_args])) or exit_code + venv_name, venv_args])) python_executable = get_venv_python(venv_name) - exit_code = subprocess_with_print(' '.join([ + subprocess_with_print(' '.join([ python_executable, os.path.normpath('./letsencrypt-auto-source/pieces/pipstrap.py')])) - command = [python_executable, os.path.normpath('./tools/pip_install.py')] or exit_code + command = [python_executable, os.path.normpath('./tools/pip_install.py')] command.extend(args) - exit_code = subprocess_with_print(' '.join(command)) or exit_code + subprocess_with_print(' '.join(command)) if os.path.isdir(os.path.join(venv_name, 'bin')): # Linux/OSX specific @@ -65,9 +63,7 @@ def main(venv_name, venv_args, args): else: raise ValueError('Error, directory {0} is not a valid venv.'.format(venv_name)) - return exit_code - if __name__ == '__main__': - sys.exit(main(os.environ.get('VENV_NAME', 'venv'), - os.environ.get('VENV_ARGS', ''), - sys.argv[1:])) + main(os.environ.get('VENV_NAME', 'venv'), + os.environ.get('VENV_ARGS', ''), + sys.argv[1:]) diff --git a/tools/install_and_test.py b/tools/install_and_test.py index 149ffc776..2842ec069 100755 --- a/tools/install_and_test.py +++ b/tools/install_and_test.py @@ -19,7 +19,7 @@ SKIP_PROJECTS_ON_WINDOWS = [ def call_with_print(command, cwd=None): print(command) - return subprocess.call(command, shell=True, cwd=cwd or os.getcwd()) + subprocess.check_call(command, shell=True, cwd=cwd or os.getcwd()) def main(args): if os.environ.get('CERTBOT_NO_PIN') == '1': @@ -37,12 +37,10 @@ def main(args): else: new_args.append(arg) - exit_code = 0 - for requirement in new_args: current_command = command[:] current_command.append(requirement) - exit_code = call_with_print(' '.join(current_command)) or exit_code + call_with_print(' '.join(current_command)) pkg = re.sub(r'\[\w+\]', '', requirement) if pkg == '.': @@ -50,13 +48,11 @@ def main(args): temp_cwd = tempfile.mkdtemp() try: - exit_code = call_with_print(' '.join([ + call_with_print(' '.join([ sys.executable, '-m', 'pytest', '--numprocesses', 'auto', - '--quiet', '--pyargs', pkg.replace('-', '_')]), cwd=temp_cwd) or exit_code + '--quiet', '--pyargs', pkg.replace('-', '_')]), cwd=temp_cwd) finally: shutil.rmtree(temp_cwd) - return exit_code - if __name__ == '__main__': - sys.exit(main(sys.argv[1:])) + main(sys.argv[1:]) diff --git a/tools/pip_install.py b/tools/pip_install.py index 273ce5ec2..2c4a47c21 100755 --- a/tools/pip_install.py +++ b/tools/pip_install.py @@ -59,14 +59,12 @@ def merge_requirements(tools_path, test_constraints, all_constraints): def call_with_print(command, cwd=None): print(command) - return subprocess.call(command, shell=True, cwd=cwd or os.getcwd()) + subprocess.check_call(command, shell=True, cwd=cwd or os.getcwd()) def main(args): tools_path = find_tools_path() working_dir = tempfile.mkdtemp() - exit_code = 0 - try: test_constraints = os.path.join(working_dir, 'test_constraints.txt') all_constraints = os.path.join(working_dir, 'all_constraints.txt') @@ -79,17 +77,15 @@ def main(args): merge_requirements(tools_path, test_constraints, all_constraints) if requirements: - exit_code = call_with_print(' '.join([ + call_with_print(' '.join([ sys.executable, '-m', 'pip', 'install', '-q', '--constraint', all_constraints, - '--requirement', requirements])) or exit_code + '--requirement', requirements])) command = [sys.executable, '-m', 'pip', 'install', '-q', '--constraint', all_constraints] command.extend(args) - exit_code = call_with_print(' '.join(command)) or exit_code + call_with_print(' '.join(command)) finally: shutil.rmtree(working_dir) - return exit_code - if __name__ == '__main__': - sys.exit(main(sys.argv[1:])) + main(sys.argv[1:]) diff --git a/tools/pip_install_editable.py b/tools/pip_install_editable.py index 35cc2264d..8eaf3a9fa 100755 --- a/tools/pip_install_editable.py +++ b/tools/pip_install_editable.py @@ -14,7 +14,7 @@ def main(args): new_args.append('-e') new_args.append(arg) - return pip_install.main(new_args) + pip_install.main(new_args) if __name__ == '__main__': - sys.exit(main(sys.argv[1:])) + main(sys.argv[1:]) diff --git a/tools/venv.py b/tools/venv.py index 2cc43251d..8b6f92a56 100755 --- a/tools/venv.py +++ b/tools/venv.py @@ -53,7 +53,7 @@ def main(): venv_args = get_venv_args() - return _venv_common.main('venv', venv_args, REQUIREMENTS) + _venv_common.main('venv', venv_args, REQUIREMENTS) if __name__ == '__main__': - sys.exit(main()) + main() diff --git a/tools/venv3.py b/tools/venv3.py index 1bacc9c9a..9710806c5 100755 --- a/tools/venv3.py +++ b/tools/venv3.py @@ -48,7 +48,7 @@ def get_venv_args(): def main(): venv_args = get_venv_args() - return _venv_common.main('venv3', venv_args, REQUIREMENTS) + _venv_common.main('venv3', venv_args, REQUIREMENTS) if __name__ == '__main__': - sys.exit(main()) + main() diff --git a/tox.cover.py b/tox.cover.py index 8bbce2d09..ad6bb1a39 100755 --- a/tox.cover.py +++ b/tox.cover.py @@ -12,36 +12,33 @@ DEFAULT_PACKAGES = [ 'certbot_dns_sakuracloud', 'certbot_nginx', 'certbot_postfix', 'letshelp_certbot'] COVER_THRESHOLDS = { - 'certbot': 98, - 'acme': 100, - 'certbot_apache': 100, - 'certbot_dns_cloudflare': 98, - 'certbot_dns_cloudxns': 99, - 'certbot_dns_digitalocean': 98, - 'certbot_dns_dnsimple': 98, - 'certbot_dns_dnsmadeeasy': 99, - 'certbot_dns_gehirn': 97, - 'certbot_dns_google': 99, - 'certbot_dns_linode': 98, - 'certbot_dns_luadns': 98, - 'certbot_dns_nsone': 99, - 'certbot_dns_ovh': 97, - 'certbot_dns_rfc2136': 99, - 'certbot_dns_route53': 92, - 'certbot_dns_sakuracloud': 97, - 'certbot_nginx': 97, - 'certbot_postfix': 100, - 'letshelp_certbot': 100 + 'certbot': {'linux': 98, 'windows': 94}, + 'acme': {'linux': 100, 'windows': 99}, + 'certbot_apache': {'linux': 100, 'windows': 100}, + 'certbot_dns_cloudflare': {'linux': 98, 'windows': 98}, + 'certbot_dns_cloudxns': {'linux': 99, 'windows': 99}, + 'certbot_dns_digitalocean': {'linux': 98, 'windows': 98}, + 'certbot_dns_dnsimple': {'linux': 98, 'windows': 98}, + 'certbot_dns_dnsmadeeasy': {'linux': 99, 'windows': 99}, + 'certbot_dns_gehirn': {'linux': 97, 'windows': 97}, + 'certbot_dns_google': {'linux': 99, 'windows': 99}, + 'certbot_dns_linode': {'linux': 98, 'windows': 98}, + 'certbot_dns_luadns': {'linux': 98, 'windows': 98}, + 'certbot_dns_nsone': {'linux': 99, 'windows': 99}, + 'certbot_dns_ovh': {'linux': 97, 'windows': 97}, + 'certbot_dns_rfc2136': {'linux': 99, 'windows': 99}, + 'certbot_dns_route53': {'linux': 92, 'windows': 92}, + 'certbot_dns_sakuracloud': {'linux': 97, 'windows': 97}, + 'certbot_nginx': {'linux': 97, 'windows': 97}, + 'certbot_postfix': {'linux': 100, 'windows': 100}, + 'letshelp_certbot': {'linux': 100, 'windows': 100} } SKIP_PROJECTS_ON_WINDOWS = [ 'certbot-apache', 'certbot-nginx', 'certbot-postfix', 'letshelp-certbot'] def cover(package): - threshold = COVER_THRESHOLDS.get(package) - - if not threshold: - raise ValueError('Unrecognized package: {0}'.format(package)) + threshold = COVER_THRESHOLDS.get(package)['windows' if os.name == 'nt' else 'linux'] pkg_dir = package.replace('_', '-') @@ -51,10 +48,10 @@ def cover(package): .format(pkg_dir))) return - subprocess.call([ + subprocess.check_call([ sys.executable, '-m', 'pytest', '--cov', pkg_dir, '--cov-append', '--cov-report=', '--numprocesses', 'auto', '--pyargs', package]) - subprocess.call([ + subprocess.check_call([ sys.executable, '-m', 'coverage', 'report', '--fail-under', str(threshold), '--include', '{0}/*'.format(pkg_dir), '--show-missing'])