From 39369828c2d929790cc08a339c3d79e2017ee557 Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Sat, 11 Sep 2021 22:24:12 +0100 Subject: [PATCH 1/4] tooling: Use upstream python_check Signed-off-by: Ryan Northey --- ci/format_pre.sh | 2 +- tools/base/requirements.in | 1 + tools/base/requirements.txt | 65 ++-- tools/code_format/BUILD | 15 +- tools/code_format/python_check.py | 119 +----- tools/code_format/tests/test_python_check.py | 384 ------------------- 6 files changed, 59 insertions(+), 527 deletions(-) delete mode 100644 tools/code_format/tests/test_python_check.py diff --git a/ci/format_pre.sh b/ci/format_pre.sh index 831e57ca4a298..08808386b16b2 100755 --- a/ci/format_pre.sh +++ b/ci/format_pre.sh @@ -53,7 +53,7 @@ CURRENT=configs bazel run "${BAZEL_BUILD_OPTIONS[@]}" //configs:example_configs_validation CURRENT=python -bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/code_format:python_check -- --diff-file="$DIFF_OUTPUT" --fix "$(pwd)" +bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/code_format:python_check -- --diff-file="$DIFF_OUTPUT" --fix CURRENT=extensions bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/extensions:extensions_check diff --git a/tools/base/requirements.in b/tools/base/requirements.in index 8ec1dbb9be567..004cbfd280086 100644 --- a/tools/base/requirements.in +++ b/tools/base/requirements.in @@ -3,6 +3,7 @@ colorama coloredlogs coverage envoy.base.utils +envoy.code_format.python_check>=0.0.4 envoy.distribution.release envoy.distribution.verify envoy.gpg.sign diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index 06e91962896a6..481dbdd1c0fc9 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -2,7 +2,7 @@ # This file is autogenerated by pip-compile # To update, run: # -# pip-compile --allow-unsafe --generate-hashes tools/base/requirements.in +# pip-compile --allow-unsafe --generate-hashes requirements.in # abstracts==0.0.12 \ --hash=sha256:acc01ff56c8a05fb88150dff62e295f9071fc33388c42f1dfc2787a8d1c755ff @@ -10,6 +10,7 @@ abstracts==0.0.12 \ # aio.functional # envoy.abstract.command # envoy.base.utils + # envoy.code-format.python-check # envoy.github.abstract # envoy.github.release aio.functional==0.0.9 \ @@ -22,9 +23,13 @@ aio.functional==0.0.9 \ aio.stream==0.0.2 \ --hash=sha256:6f5baaff48f6319db134cd56c06ccf89db1f7c5f67a26382e081efc96f2f675d # via envoy.github.release +aio.subprocess==0.0.4 \ + --hash=sha256:fd504a7c02423c40fde19ad87b62932b9eaa091f5a22d26b89b452059a728750 + # via envoy.code-format.python-check aio.tasks==0.0.4 \ --hash=sha256:9abd4b0881edb292c4f91a2f63b1dea7a9829a4bd4e8440225a1a412a90461fc # via + # envoy.code-format.python-check # envoy.github.abstract # envoy.github.release aiodocker==0.21.0 \ @@ -263,6 +268,7 @@ envoy.abstract.command==0.0.3 \ envoy.base.checker==0.0.2 \ --hash=sha256:2ac81efa20fd01fff644ff7dc7fadeac1c3e4dbb6210881ac7a7919ec0e048d8 # via + # envoy.code-format.python-check # envoy.distribution.distrotest # envoy.distribution.verify envoy.base.runner==0.0.4 \ @@ -276,9 +282,13 @@ envoy.base.utils==0.0.8 \ --hash=sha256:b82e18ab0535207b7136d6980239c9350f7113fa5da7dda781bcb6ad1e05b3ab # via # -r requirements.in + # envoy.code-format.python-check # envoy.distribution.distrotest # envoy.github.release # envoy.gpg.sign +envoy.code-format.python-check==0.0.4 \ + --hash=sha256:5e166102d1f873f0c14640bcef87b46147cbad1cb68888c977acfde7fce96e04 + # via -r requirements.in envoy.distribution.distrotest==0.0.3 \ --hash=sha256:c094adbd959eb1336f93afc00aedb7ee4e68e8252e2365be816a6f9ede8a3de7 # via envoy.distribution.verify @@ -305,17 +315,18 @@ envoy.gpg.identity==0.0.2 \ envoy.gpg.sign==0.0.3 \ --hash=sha256:31667931f5d7ff05fd809b89748f277511486311c777652af4cb8889bd641049 # via -r requirements.in +flake8-polyfill==1.0.2 \ + --hash=sha256:12be6a34ee3ab795b19ca73505e7b55826d5f6ad7230d31b18e106400169b9e9 \ + --hash=sha256:e44b087597f6da52ec6393a709e7108b2905317d0c0b744cdca6208e670d8eda + # via pep8-naming flake8==3.9.2 \ --hash=sha256:07528381786f2a6237b061f6e96610a4167b226cb926e2aa2b6b1d78057c576b \ --hash=sha256:bf8fd333346d844f616e8d47905ef3a3384edae6b4e9beb0c5101e25e3110907 # via # -r requirements.in + # envoy.code-format.python-check # flake8-polyfill # pep8-naming -flake8-polyfill==1.0.2 \ - --hash=sha256:12be6a34ee3ab795b19ca73505e7b55826d5f6ad7230d31b18e106400169b9e9 \ - --hash=sha256:e44b087597f6da52ec6393a709e7108b2905317d0c0b744cdca6208e670d8eda - # via pep8-naming frozendict==2.0.6 \ --hash=sha256:3f00de72805cf4c9e81b334f3f04809278b967d2fed84552313a0fcce511beb1 \ --hash=sha256:5d3f75832c35d4df041f0e19c268964cbef29c1eb34cd3517cf883f1c2d089b9 @@ -471,7 +482,9 @@ packaging==21.0 \ pep8-naming==0.12.1 \ --hash=sha256:4a8daeaeb33cfcde779309fc0c9c0a68a3bbe2ad8a8308b763c5068f86eb9f37 \ --hash=sha256:bb2455947757d162aa4cad55dba4ce029005cd1692f2899a21d51d8630ca7841 - # via -r requirements.in + # via + # -r requirements.in + # envoy.code-format.python-check pluggy==1.0.0 \ --hash=sha256:4224373bacce55f955a878bf9cfa763c1e360858e330072059e10bad68531159 \ --hash=sha256:74134bbf457f031a36d68416e1509f34bd5ccc019f0bcc952c7b909d06b37bd3 @@ -535,14 +548,6 @@ pyparsing==2.4.7 \ pyreadline==2.1 \ --hash=sha256:4530592fc2e85b25b1a9f79664433da09237c1a270e4d78ea5aa3a2c7229e2d1 # via -r requirements.in -pytest==6.2.5 \ - --hash=sha256:131b36680866a76e6781d13f101efb86cf674ebb9762eb70d3082b6f29889e89 \ - --hash=sha256:7310f8d27bc79ced999e760ca304d69f6ba6c6649c0b60fb0e04a4a77cacc134 - # via - # -r requirements.in - # pytest-asyncio - # pytest-cov - # pytest-patches pytest-asyncio==0.15.1 \ --hash=sha256:2564ceb9612bbd560d19ca4b41347b54e7835c2f792c504f698e05395ed63f6f \ --hash=sha256:3042bcdf1c5d978f6b74d96a151c4cfb9dcece65006198389ccd7e6c60eb1eea @@ -554,6 +559,14 @@ pytest-cov==2.12.1 \ pytest-patches==0.0.3 \ --hash=sha256:6f8cdc8641c708c4812f58ae48d410f373a6fd16cd6cc4dc4d3fb8951df9c92a # via -r requirements.in +pytest==6.2.5 \ + --hash=sha256:131b36680866a76e6781d13f101efb86cf674ebb9762eb70d3082b6f29889e89 \ + --hash=sha256:7310f8d27bc79ced999e760ca304d69f6ba6c6649c0b60fb0e04a4a77cacc134 + # via + # -r requirements.in + # pytest-asyncio + # pytest-cov + # pytest-patches python-gnupg==0.4.7 \ --hash=sha256:2061f56b1942c29b92727bf9aecbd3cea3893acc9cccbdc7eb4604285efe4ac7 \ --hash=sha256:3ff5b1bf5e397de6e1fe41a7c0f403dad4e242ac92b345f440eaecfb72a7ebae @@ -615,16 +628,6 @@ snowballstemmer==2.1.0 \ --hash=sha256:b51b447bea85f9968c13b650126a888aabd4cb4463fca868ec596826325dedc2 \ --hash=sha256:e997baa4f2e9139951b6f4c631bad912dfd3c792467e2f03d7239464af90e914 # via sphinx -sphinx==4.1.2 \ - --hash=sha256:3092d929cd807926d846018f2ace47ba2f3b671b309c7a89cd3306e80c826b13 \ - --hash=sha256:46d52c6cee13fec44744b8c01ed692c18a640f6910a725cbb938bc36e8d64544 - # via - # -r requirements.in - # sphinx-copybutton - # sphinx-rtd-theme - # sphinx-tabs - # sphinxcontrib-httpdomain - # sphinxext-rediraffe sphinx-copybutton==0.4.0 \ --hash=sha256:4340d33c169dac6dd82dce2c83333412aa786a42dd01a81a8decac3b130dc8b0 \ --hash=sha256:8daed13a87afd5013c3a9af3575cc4d5bec052075ccd3db243f895c07a689386 @@ -637,6 +640,16 @@ sphinx-tabs==3.2.0 \ --hash=sha256:1e1b1846c80137bd81a78e4a69b02664b98b1e1da361beb30600b939dfc75065 \ --hash=sha256:33137914ed9b276e6a686d7a337310ee77b1dae316fdcbce60476913a152e0a4 # via -r requirements.in +sphinx==4.1.2 \ + --hash=sha256:3092d929cd807926d846018f2ace47ba2f3b671b309c7a89cd3306e80c826b13 \ + --hash=sha256:46d52c6cee13fec44744b8c01ed692c18a640f6910a725cbb938bc36e8d64544 + # via + # -r requirements.in + # sphinx-copybutton + # sphinx-rtd-theme + # sphinx-tabs + # sphinxcontrib-httpdomain + # sphinxext-rediraffe sphinxcontrib-applehelp==1.0.2 \ --hash=sha256:806111e5e962be97c29ec4c1e7fe277bfd19e9652fb1a4392105b43e01af885a \ --hash=sha256:a072735ec80e7675e3f432fcae8610ecf509c5f1869d17e2eecff44389cdbc58 @@ -710,7 +723,9 @@ wrapt==1.12.1 \ yapf==0.31.0 \ --hash=sha256:408fb9a2b254c302f49db83c59f9aa0b4b0fd0ec25be3a5c51181327922ff63d \ --hash=sha256:e3a234ba8455fe201eaa649cdac872d590089a18b661e39bbac7020978dd9c2e - # via -r requirements.in + # via + # -r requirements.in + # envoy.code-format.python-check yarl==1.6.3 \ --hash=sha256:00d7ad91b6583602eb9c1d085a2cf281ada267e9a197e8b7cae487dadbfa293e \ --hash=sha256:0355a701b3998dcd832d0dc47cc5dedf3874f966ac7f870e0f3a6788d802d434 \ diff --git a/tools/code_format/BUILD b/tools/code_format/BUILD index ba9de5fce8557..11416d9ca8415 100644 --- a/tools/code_format/BUILD +++ b/tools/code_format/BUILD @@ -1,6 +1,6 @@ load("@base_pip3//:requirements.bzl", "requirement") +load("@rules_python//python:defs.bzl", "py_binary") load("//bazel:envoy_build_system.bzl", "envoy_package") -load("//tools/base:envoy_python.bzl", "envoy_py_binary") licenses(["notice"]) # Apache 2 @@ -12,14 +12,11 @@ exports_files([ "envoy_build_fixer.py", ]) -envoy_py_binary( - name = "tools.code_format.python_check", +py_binary( + name = "python_check", + srcs = ["python_check.py"], deps = [ - "//tools/base:aio", - "//tools/base:checker", - "//tools/base:utils", - requirement("flake8"), - requirement("pep8-naming"), - requirement("yapf"), + "@envoy_repo", + requirement("envoy.code_format.python_check"), ], ) diff --git a/tools/code_format/python_check.py b/tools/code_format/python_check.py index e3a00f45b0cb5..c7e54f13b22f2 100755 --- a/tools/code_format/python_check.py +++ b/tools/code_format/python_check.py @@ -13,126 +13,29 @@ # python requires: flake8, yapf # -import argparse import pathlib import sys from functools import cached_property -from typing import Iterable, List, Optional, Tuple -from flake8.main.application import Application as Flake8Application # type:ignore +import abstracts -import yapf # type:ignore +from envoy.code_format import python_check -from tools.base import aio, checker, utils +import envoy_repo -FLAKE8_CONFIG = '.flake8' -YAPF_CONFIG = '.style.yapf' -# TODO(phlax): add checks for: -# - isort - - -class PythonChecker(checker.AsyncChecker): - checks = ("flake8", "yapf") - - @property - def diff_file_path(self) -> Optional[pathlib.Path]: - return pathlib.Path(self.args.diff_file) if self.args.diff_file else None +@abstracts.implementer(python_check.APythonChecker) +class EnvoyPythonChecker: @cached_property - def flake8_app(self) -> Flake8Application: - flake8_app = Flake8Application() - flake8_app.initialize(self.flake8_args) - return flake8_app - - @property - def flake8_args(self) -> Tuple[str, ...]: - return ("--config", str(self.flake8_config_path), str(self.path)) - - @property - def flake8_config_path(self) -> pathlib.Path: - return self.path.joinpath(FLAKE8_CONFIG) - - @property - def recurse(self) -> bool: - """Flag to determine whether to apply checks recursively""" - return self.args.recurse - - @property - def yapf_config_path(self) -> pathlib.Path: - return self.path.joinpath(YAPF_CONFIG) - - @property - def yapf_files(self) -> List[str]: - return yapf.file_resources.GetCommandLineFiles( - self.args.paths, - recursive=self.recurse, - exclude=yapf.file_resources.GetExcludePatternsForDir(str(self.path))) - - def add_arguments(self, parser: argparse.ArgumentParser) -> None: - super().add_arguments(parser) - parser.add_argument( - "--recurse", - "-r", - choices=["yes", "no"], - default="yes", - help="Recurse path or paths directories") - parser.add_argument( - "--diff-file", default=None, help="Specify the path to a diff file with fixes") - - async def check_flake8(self) -> None: - """Run flake8 on files and/or repo""" - errors: List[str] = [] - with utils.buffered(stdout=errors, mangle=self._strip_lines): - self.flake8_app.run_checks() - self.flake8_app.report() - if errors: - self.error("flake8", errors) - - async def check_yapf(self) -> None: - """Run flake8 on files and/or repo""" - futures = aio.concurrent(self.yapf_format(python_file) for python_file in self.yapf_files) - - async for (python_file, (reformatted, encoding, changed)) in futures: - self.yapf_result(python_file, reformatted, changed) - - async def on_check_run(self, check: str) -> None: - if check not in self.failed and check not in self.warned: - self.succeed(check, [check]) - - async def on_checks_complete(self) -> int: - if self.diff_file_path and self.has_failed: - result = await aio.async_subprocess.run(["git", "diff", "HEAD"], - cwd=self.path, - capture_output=True) - self.diff_file_path.write_bytes(result.stdout) - return await super().on_checks_complete() - - async def yapf_format(self, python_file: str) -> tuple: - return python_file, yapf.yapf_api.FormatFile( - python_file, - style_config=str(self.yapf_config_path), - in_place=self.fix, - print_diff=not self.fix) - - def yapf_result(self, python_file: str, reformatted: str, changed: bool) -> None: - if not changed: - return self.succeed("yapf", [python_file]) - if self.fix: - return self.warn("yapf", [f"{python_file}: reformatted"]) - if reformatted: - return self.warn("yapf", [f"{python_file}: diff\n{reformatted}"]) - self.error("yapf", [python_file]) - - def _strip_line(self, line: str) -> str: - return line[len(str(self.path)) + 1:] if line.startswith(f"{self.path}/") else line - - def _strip_lines(self, lines: Iterable[str]) -> List[str]: - return [self._strip_line(line) for line in lines if line] + def path(self) -> pathlib.Path: + if self.args.paths: + return pathlib.Path(self.args.paths[0]) + return pathlib.Path(envoy_repo.PATH) -def main(*args: str) -> Optional[int]: - return PythonChecker(*args).run() +def main(*args) -> int: + return EnvoyPythonChecker(*args).run() if __name__ == "__main__": diff --git a/tools/code_format/tests/test_python_check.py b/tools/code_format/tests/test_python_check.py deleted file mode 100644 index 7cf39577d4bb7..0000000000000 --- a/tools/code_format/tests/test_python_check.py +++ /dev/null @@ -1,384 +0,0 @@ -import types -from contextlib import contextmanager -from unittest.mock import AsyncMock, patch, MagicMock, PropertyMock - -import pytest - -from tools.code_format import python_check - - -def test_python_checker_constructor(): - checker = python_check.PythonChecker("path1", "path2", "path3") - assert checker.checks == ("flake8", "yapf") - assert checker.args.paths == ['path1', 'path2', 'path3'] - - -@pytest.mark.parametrize("diff_path", ["", None, "PATH"]) -def test_python_diff_path(patches, diff_path): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - "pathlib", - ("PythonChecker.args", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_plib, m_args): - m_args.return_value.diff_file = diff_path - assert checker.diff_file_path == (m_plib.Path.return_value if diff_path else None) - - if diff_path: - assert ( - list(m_plib.Path.call_args) - == [(m_args.return_value.diff_file, ), {}]) - else: - assert not m_plib.Path.called - - -def test_python_flake8_app(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - ("PythonChecker.flake8_args", dict(new_callable=PropertyMock)), - "Flake8Application", - prefix="tools.code_format.python_check") - - with patched as (m_flake8_args, m_flake8_app): - assert checker.flake8_app == m_flake8_app.return_value - - assert ( - list(m_flake8_app.call_args) - == [(), {}]) - assert ( - list(m_flake8_app.return_value.initialize.call_args) - == [(m_flake8_args.return_value,), {}]) - - -def test_python_flake8_args(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - ("PythonChecker.flake8_config_path", dict(new_callable=PropertyMock)), - ("PythonChecker.path", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_flake8_config, m_path): - assert ( - checker.flake8_args - == ('--config', - str(m_flake8_config.return_value), - str(m_path.return_value))) - - -def test_python_flake8_config_path(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - ("PythonChecker.path", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_path, ): - assert checker.flake8_config_path == m_path.return_value.joinpath.return_value - - assert ( - list(m_path.return_value.joinpath.call_args) - == [(python_check.FLAKE8_CONFIG, ), {}]) - - -def test_python_yapf_config_path(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - ("PythonChecker.path", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_path, ): - assert checker.yapf_config_path == m_path.return_value.joinpath.return_value - - assert ( - list(m_path.return_value.joinpath.call_args) - == [(python_check.YAPF_CONFIG, ), {}]) - - -def test_python_yapf_files(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - - patched = patches( - ("PythonChecker.args", dict(new_callable=PropertyMock)), - ("PythonChecker.path", dict(new_callable=PropertyMock)), - "yapf.file_resources.GetCommandLineFiles", - "yapf.file_resources.GetExcludePatternsForDir", - prefix="tools.code_format.python_check") - - with patched as (m_args, m_path, m_yapf_files, m_yapf_exclude): - assert checker.yapf_files == m_yapf_files.return_value - - assert ( - list(m_yapf_files.call_args) - == [(m_args.return_value.paths,), - {'recursive': m_args.return_value.recurse, - 'exclude': m_yapf_exclude.return_value}]) - assert ( - list(m_yapf_exclude.call_args) - == [(str(m_path.return_value),), {}]) - - -def test_python_add_arguments(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - add_mock = patch("tools.code_format.python_check.checker.AsyncChecker.add_arguments") - m_parser = MagicMock() - - with add_mock as m_add: - checker.add_arguments(m_parser) - - assert ( - list(m_add.call_args) - == [(m_parser,), {}]) - assert ( - list(list(c) for c in m_parser.add_argument.call_args_list) - == [[('--recurse', '-r'), - {'choices': ['yes', 'no'], - 'default': 'yes', - 'help': 'Recurse path or paths directories'}], - [('--diff-file',), - {'default': None, 'help': 'Specify the path to a diff file with fixes'}]]) - - -@pytest.mark.asyncio -@pytest.mark.parametrize("errors", [[], ["err1", "err2"]]) -async def test_python_check_flake8(patches, errors): - checker = python_check.PythonChecker("path1", "path2", "path3") - - patched = patches( - "utils.buffered", - "PythonChecker.error", - "PythonChecker._strip_lines", - ("PythonChecker.flake8_app", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - @contextmanager - def mock_buffered(stdout=None, mangle=None): - yield - stdout.extend(errors) - - with patched as (m_buffered, m_error, m_mangle, m_flake8_app): - m_buffered.side_effect = mock_buffered - assert not await checker.check_flake8() - - assert ( - list(m_buffered.call_args) - == [(), {'stdout': errors, 'mangle': m_mangle}]) - assert ( - list(m_flake8_app.return_value.run_checks.call_args) - == [(), {}]) - assert ( - list(m_flake8_app.return_value.report.call_args) - == [(), {}]) - - if errors: - assert ( - list(m_error.call_args) - == [('flake8', ['err1', 'err2']), {}]) - else: - assert not m_error.called - - -def test_python_check_recurse(): - checker = python_check.PythonChecker("path1", "path2", "path3") - args_mock = patch( - "tools.code_format.python_check.PythonChecker.args", - new_callable=PropertyMock) - - with args_mock as m_args: - assert checker.recurse == m_args.return_value.recurse - assert "recurse" not in checker.__dict__ - - -@pytest.mark.asyncio -async def test_python_check_yapf(patches): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - "aio", - ("PythonChecker.yapf_format", dict(new_callable=MagicMock)), - "PythonChecker.yapf_result", - ("PythonChecker.yapf_files", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - files = ["file1", "file2", "file3"] - - async def concurrent(iters): - assert isinstance(iters, types.GeneratorType) - for i, format_result in enumerate(iters): - yield (format_result, (f"REFORMAT{i}", f"ENCODING{i}", f"CHANGED{i}")) - - with patched as (m_aio, m_yapf_format, m_yapf_result, m_yapf_files): - m_yapf_files.return_value = files - m_aio.concurrent.side_effect = concurrent - assert not await checker.check_yapf() - - assert ( - list(list(c) for c in m_yapf_format.call_args_list) - == [[(file,), {}] for file in files]) - assert ( - list(list(c) for c in m_yapf_result.call_args_list) - == [[(m_yapf_format.return_value, f"REFORMAT{i}", f"CHANGED{i}"), {}] for i, _ in enumerate(files)]) - - -@pytest.mark.asyncio -@pytest.mark.parametrize("errors", [[], ["check2", "check3"], ["check1", "check3"]]) -@pytest.mark.parametrize("warnings", [[], ["check4", "check5"], ["check1", "check5"]]) -async def test_python_on_check_run(patches, errors, warnings): - checker = python_check.PythonChecker("path1", "path2", "path3") - checkname = "check1" - patched = patches( - "PythonChecker.succeed", - ("PythonChecker.name", dict(new_callable=PropertyMock)), - ("PythonChecker.failed", dict(new_callable=PropertyMock)), - ("PythonChecker.warned", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_succeed, m_name, m_failed, m_warned): - m_failed.return_value = errors - m_warned.return_value = warnings - assert not await checker.on_check_run(checkname) - - if checkname in warnings or checkname in errors: - assert not m_succeed.called - else: - assert ( - list(m_succeed.call_args) - == [(checkname, [checkname]), {}]) - - -@pytest.mark.asyncio -@pytest.mark.parametrize("diff_path", ["", "DIFF1"]) -@pytest.mark.parametrize("failed", [True, False]) -async def test_python_on_checks_complete(patches, diff_path, failed): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - "aio", - ("checker.AsyncChecker.on_checks_complete", dict(new_callable=AsyncMock)), - ("PythonChecker.diff_file_path", dict(new_callable=PropertyMock)), - ("PythonChecker.has_failed", dict(new_callable=PropertyMock)), - ("PythonChecker.path", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_aio, m_super, m_diff, m_failed, m_path): - m_aio.async_subprocess.run = AsyncMock() - if not diff_path: - m_diff.return_value = None - m_failed.return_value = failed - assert await checker.on_checks_complete() == m_super.return_value - - if diff_path and failed: - assert ( - list(m_aio.async_subprocess.run.call_args) - == [(['git', 'diff', 'HEAD'],), - dict(capture_output=True, cwd=m_path.return_value)]) - assert ( - list(m_diff.return_value.write_bytes.call_args) - == [(m_aio.async_subprocess.run.return_value.stdout,), {}]) - else: - assert not m_aio.async_subprocess.run.called - - assert ( - list(m_super.call_args) - == [(), {}]) - - -@pytest.mark.asyncio -@pytest.mark.parametrize("fix", [True, False]) -async def test_python_yapf_format(patches, fix): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - "yapf.yapf_api.FormatFile", - ("PythonChecker.yapf_config_path", dict(new_callable=PropertyMock)), - ("PythonChecker.fix", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_format, m_config, m_fix): - m_fix.return_value = fix - assert await checker.yapf_format("FILENAME") == ("FILENAME", m_format.return_value) - - assert ( - list(m_format.call_args) - == [('FILENAME',), - {'style_config': str(m_config.return_value), - 'in_place': fix, - 'print_diff': not fix}]) - assert ( - list(list(c) for c in m_fix.call_args_list) - == [[(), {}], [(), {}]]) - - -@pytest.mark.parametrize("reformatted", ["", "REFORMAT"]) -@pytest.mark.parametrize("fix", [True, False]) -@pytest.mark.parametrize("changed", [True, False]) -def test_python_yapf_result(patches, reformatted, fix, changed): - checker = python_check.PythonChecker("path1", "path2", "path3") - patched = patches( - "PythonChecker.succeed", - "PythonChecker.warn", - "PythonChecker.error", - ("PythonChecker.fix", dict(new_callable=PropertyMock)), - prefix="tools.code_format.python_check") - - with patched as (m_succeed, m_warn, m_error, m_fix): - m_fix.return_value = fix - checker.yapf_result("FILENAME", reformatted, changed) - - if not changed: - assert ( - list(m_succeed.call_args) - == [('yapf', ['FILENAME']), {}]) - assert not m_warn.called - assert not m_error.called - assert not m_fix.called - return - assert not m_succeed.called - if fix: - assert not m_error.called - assert len(m_warn.call_args_list) == 1 - assert ( - list(m_warn.call_args) - == [('yapf', [f'FILENAME: reformatted']), {}]) - return - if reformatted: - assert not m_error.called - assert len(m_warn.call_args_list) == 1 - assert ( - list(m_warn.call_args) - == [('yapf', [f'FILENAME: diff\n{reformatted}']), {}]) - return - assert not m_warn.called - assert ( - list(m_error.call_args) - == [('yapf', ['FILENAME']), {}]) - - -def test_python_strip_lines(): - checker = python_check.PythonChecker("path1", "path2", "path3") - strip_mock = patch("tools.code_format.python_check.PythonChecker._strip_line") - lines = ["", "foo", "", "bar", "", "", "baz", "", ""] - - with strip_mock as m_strip: - assert ( - checker._strip_lines(lines) - == [m_strip.return_value] * 3) - - assert ( - list(list(c) for c in m_strip.call_args_list) - == [[('foo',), {}], [('bar',), {}], [('baz',), {}]]) - - -@pytest.mark.parametrize("line", ["REMOVE/foo", "REMOVE", "bar", "other", "REMOVE/baz", "baz"]) -def test_python_strip_line(line): - checker = python_check.PythonChecker("path1", "path2", "path3") - path_mock = patch( - "tools.code_format.python_check.PythonChecker.path", - new_callable=PropertyMock) - - with path_mock as m_path: - m_path.return_value = "REMOVE" - assert ( - checker._strip_line(line) - == line[7:] if line.startswith(f"REMOVE/") else line) - - -def test_python_checker_main(command_main): - command_main( - python_check.main, - "tools.code_format.python_check.PythonChecker") From 7aeadc43eea8a796d3c9e3d10e585e00e9234f0c Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Sat, 11 Sep 2021 20:20:16 +0100 Subject: [PATCH 2/4] tooling: Use upstream dependency.pip_check Signed-off-by: Ryan Northey --- ci/do_ci.sh | 2 +- tools/base/requirements.in | 1 + tools/base/requirements.txt | 6 + tools/dependency/BUILD | 11 +- tools/dependency/pip_check.py | 82 ++-------- tools/dependency/tests/test_pip_check.py | 195 ----------------------- 6 files changed, 25 insertions(+), 272 deletions(-) delete mode 100644 tools/dependency/tests/test_pip_check.py diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 7f10e6612ea02..7e1350525a603 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -459,7 +459,7 @@ elif [[ "$CI_TARGET" == "deps" ]]; then "${ENVOY_SRCDIR}"/ci/check_repository_locations.sh # Run pip requirements tests - bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/dependency:pip_check "${ENVOY_SRCDIR}" + bazel run "${BAZEL_BUILD_OPTIONS[@]}" //tools/dependency:pip_check exit 0 elif [[ "$CI_TARGET" == "cve_scan" ]]; then diff --git a/tools/base/requirements.in b/tools/base/requirements.in index 004cbfd280086..b12ec7e8bc1b1 100644 --- a/tools/base/requirements.in +++ b/tools/base/requirements.in @@ -4,6 +4,7 @@ coloredlogs coverage envoy.base.utils envoy.code_format.python_check>=0.0.4 +envoy.dependency.pip_check>=0.0.4 envoy.distribution.release envoy.distribution.verify envoy.gpg.sign diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index 481dbdd1c0fc9..9e737332415a1 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -11,6 +11,7 @@ abstracts==0.0.12 \ # envoy.abstract.command # envoy.base.utils # envoy.code-format.python-check + # envoy.dependency.pip-check # envoy.github.abstract # envoy.github.release aio.functional==0.0.9 \ @@ -269,6 +270,7 @@ envoy.base.checker==0.0.2 \ --hash=sha256:2ac81efa20fd01fff644ff7dc7fadeac1c3e4dbb6210881ac7a7919ec0e048d8 # via # envoy.code-format.python-check + # envoy.dependency.pip-check # envoy.distribution.distrotest # envoy.distribution.verify envoy.base.runner==0.0.4 \ @@ -283,12 +285,16 @@ envoy.base.utils==0.0.8 \ # via # -r requirements.in # envoy.code-format.python-check + # envoy.dependency.pip-check # envoy.distribution.distrotest # envoy.github.release # envoy.gpg.sign envoy.code-format.python-check==0.0.4 \ --hash=sha256:5e166102d1f873f0c14640bcef87b46147cbad1cb68888c977acfde7fce96e04 # via -r requirements.in +envoy.dependency.pip-check==0.0.4 \ + --hash=sha256:3213d77959f65c3c97e9b5d74cb14c02bc02dae64bac2e7c3cb829a2f4e5e40e + # via -r requirements.in envoy.distribution.distrotest==0.0.3 \ --hash=sha256:c094adbd959eb1336f93afc00aedb7ee4e68e8252e2365be816a6f9ede8a3de7 # via envoy.distribution.verify diff --git a/tools/dependency/BUILD b/tools/dependency/BUILD index 2909e36a8b8a9..b1e0af18c2aea 100644 --- a/tools/dependency/BUILD +++ b/tools/dependency/BUILD @@ -1,6 +1,6 @@ load("@rules_python//python:defs.bzl", "py_binary", "py_library") load("//bazel:envoy_build_system.bzl", "envoy_package") -load("//tools/base:envoy_python.bzl", "envoy_py_binary") +load("@base_pip3//:requirements.bzl", "requirement") licenses(["notice"]) # Apache 2 @@ -41,10 +41,11 @@ py_binary( ], ) -envoy_py_binary( - name = "tools.dependency.pip_check", +py_binary( + name = "pip_check", + srcs = ["pip_check.py"], deps = [ - "//tools/base:checker", - "//tools/base:utils", + "@envoy_repo", + requirement("envoy.dependency.pip_check"), ], ) diff --git a/tools/dependency/pip_check.py b/tools/dependency/pip_check.py index 91a8456fc2854..0a0ac98f96853 100755 --- a/tools/dependency/pip_check.py +++ b/tools/dependency/pip_check.py @@ -11,89 +11,29 @@ # ./tools/dependency/pip_check.py -h # +import pathlib import sys from functools import cached_property -from typing import Iterable, Set -from tools.base import checker, utils +import abstracts -DEPENDABOT_CONFIG = ".github/dependabot.yml" -REQUIREMENTS_FILENAME = "requirements.txt" +from envoy.dependency import pip_check -# TODO(phlax): add checks for: -# - requirements can be installed together -# - pip-compile formatting +import envoy_repo -class PipConfigurationError(Exception): - pass - - -class PipChecker(checker.Checker): - checks = ("dependabot",) - _dependabot_config = DEPENDABOT_CONFIG - _requirements_filename = REQUIREMENTS_FILENAME - - @cached_property - def config_requirements(self) -> set: - """Set of configured pip dependabot directories""" - return set( - update['directory'] - for update in self.dependabot_config["updates"] - if update["package-ecosystem"] == "pip") +@abstracts.implementer(pip_check.APipChecker) +class EnvoyPipChecker: @cached_property - def dependabot_config(self) -> dict: - """Parsed dependabot config""" - result = utils.from_yaml(self.path.joinpath(self.dependabot_config_path)) - if not isinstance(result, dict): - raise PipConfigurationError( - f"Unable to parse dependabot config: {self.dependabot_config_path}") - return result - - @property - def dependabot_config_path(self) -> str: - return self._dependabot_config - - @cached_property - def requirements_dirs(self) -> Set[str]: - """Set of found directories in the repo containing requirements.txt""" - return set( - f"/{f.parent.relative_to(self.path)}" for f in self.path.glob("**/*") - if f.name == self.requirements_filename) - - @property - def requirements_filename(self) -> str: - return self._requirements_filename - - def check_dependabot(self) -> None: - """Check that dependabot config matches requirements.txt files found in repo""" - missing_dirs = self.config_requirements.difference(self.requirements_dirs) - missing_config = self.requirements_dirs.difference(self.config_requirements) - correct = self.requirements_dirs.intersection(self.config_requirements) - if correct: - self.dependabot_success(correct) - if missing_dirs: - self.dependabot_errors( - missing_dirs, - f"Missing {self.requirements_filename} dir, specified in dependabot config") - if missing_config: - self.dependabot_errors( - missing_config, - f"Missing dependabot config for {self.requirements_filename} in dir") - - def dependabot_success(self, correct: Iterable) -> None: - self.succeed( - "dependabot", - ([f"{self.requirements_filename}: {dirname}" for dirname in sorted(correct)])) - - def dependabot_errors(self, missing: Iterable, msg: str) -> None: - for dirname in sorted(missing): - self.error("dependabot", [f"{msg}: {dirname}"]) + def path(self) -> pathlib.Path: + if self.args.paths: + return pathlib.Path(self.args.paths[0]) + return pathlib.Path(envoy_repo.PATH) def main(*args) -> int: - return PipChecker(*args).run() + return EnvoyPipChecker(*args).run() if __name__ == "__main__": diff --git a/tools/dependency/tests/test_pip_check.py b/tools/dependency/tests/test_pip_check.py deleted file mode 100644 index 0c3458626cc89..0000000000000 --- a/tools/dependency/tests/test_pip_check.py +++ /dev/null @@ -1,195 +0,0 @@ -from unittest.mock import MagicMock, patch, PropertyMock - -import pytest - -from tools.dependency import pip_check - - -def test_pip_checker_constructor(): - checker = pip_check.PipChecker("path1", "path2", "path3") - assert checker.checks == ("dependabot",) - assert checker.dependabot_config_path == pip_check.DEPENDABOT_CONFIG == ".github/dependabot.yml" - assert checker.requirements_filename == pip_check.REQUIREMENTS_FILENAME == "requirements.txt" - assert checker.args.paths == ['path1', 'path2', 'path3'] - - -def test_pip_checker_config_requirements(): - checker = pip_check.PipChecker("path1", "path2", "path3") - - config_mock = patch( - "tools.dependency.pip_check.PipChecker.dependabot_config", - new_callable=PropertyMock) - - with config_mock as m_config: - m_config.return_value.__getitem__.return_value = [ - {"package-ecosystem": "pip", "directory": "dir1"}, - {"package-ecosystem": "not-pip", "directory": "dir2"}, - {"package-ecosystem": "pip", "directory": "dir3"}] - assert checker.config_requirements == {'dir1', 'dir3'} - assert ( - list(m_config.return_value.__getitem__.call_args) - == [('updates',), {}]) - - -@pytest.mark.parametrize("isdict", [True, False]) -def test_pip_checker_dependabot_config(patches, isdict): - checker = pip_check.PipChecker("path1", "path2", "path3") - patched = patches( - "utils", - ("PipChecker.path", dict(new_callable=PropertyMock)), - prefix="tools.dependency.pip_check") - - with patched as (m_utils, m_path): - if isdict: - m_utils.from_yaml.return_value = {} - - if isdict: - assert checker.dependabot_config == m_utils.from_yaml.return_value - else: - with pytest.raises(pip_check.PipConfigurationError) as e: - checker.dependabot_config - - assert ( - e.value.args[0] - == f'Unable to parse dependabot config: {checker.dependabot_config_path}') - - assert ( - list(m_path.return_value.joinpath.call_args) - == [(checker._dependabot_config, ), {}]) - assert ( - list(m_utils.from_yaml.call_args) - == [(m_path.return_value.joinpath.return_value,), {}]) - - -def test_pip_checker_requirements_dirs(patches): - checker = pip_check.PipChecker("path1", "path2", "path3") - dummy_glob = [ - "FILE1", "FILE2", "FILE3", - "REQUIREMENTS_FILE", "FILE4", - "REQUIREMENTS_FILE", "FILE5"] - patched = patches( - ("PipChecker.requirements_filename", dict(new_callable=PropertyMock)), - ("PipChecker.path", dict(new_callable=PropertyMock)), - prefix="tools.dependency.pip_check") - expected = [] - - with patched as (m_reqs, m_path): - m_reqs.return_value = "REQUIREMENTS_FILE" - _glob = [] - - for fname in dummy_glob: - _mock = MagicMock() - _mock.name = fname - if fname == "REQUIREMENTS_FILE": - expected.append(_mock) - _glob.append(_mock) - - m_path.return_value.glob.return_value = _glob - assert checker.requirements_dirs == {f"/{f.parent.relative_to.return_value}" for f in expected} - - for exp in expected: - assert ( - list(exp.parent.relative_to.call_args) - == [(m_path.return_value,), {}]) - assert "requirements_dirs" in checker.__dict__ - - -TEST_REQS = ( - (set(), set()), - (set(["A", "B"]), set()), - (set(["A", "B"]), set(["B", "C"])), - (set(["A", "B", "C"]), set(["A", "B", "C"])), - (set(), set(["B", "C"]))) - - -@pytest.mark.parametrize("requirements", TEST_REQS) -def test_pip_checker_check_dependabot(patches, requirements): - config, dirs = requirements - checker = pip_check.PipChecker("path1", "path2", "path3") - - patched = patches( - ("PipChecker.config_requirements", dict(new_callable=PropertyMock)), - ("PipChecker.requirements_dirs", dict(new_callable=PropertyMock)), - ("PipChecker.requirements_filename", dict(new_callable=PropertyMock)), - "PipChecker.dependabot_success", - "PipChecker.dependabot_errors", - prefix="tools.dependency.pip_check") - - with patched as (m_config, m_dirs, m_fname, m_success, m_errors): - m_config.return_value = config - m_dirs.return_value = dirs - assert not checker.check_dependabot() - - if config & dirs: - assert ( - list(m_success.call_args) - == [(config & dirs, ), {}]) - else: - assert not m_success.called - - if config - dirs: - assert ( - [(config - dirs, f"Missing {m_fname.return_value} dir, specified in dependabot config"), {}] - in list(list(c) for c in m_errors.call_args_list)) - - if dirs - config: - assert ( - [(dirs - config, f"Missing dependabot config for {m_fname.return_value} in dir"), {}] - in list(list(c) for c in m_errors.call_args_list)) - - if not config - dirs and not dirs - config: - assert not m_errors.called - - -def test_pip_checker_dependabot_success(patches): - checker = pip_check.PipChecker("path1", "path2", "path3") - succeed_mock = patch - success = set(["C", "D", "B", "A"]) - - patched = patches( - "PipChecker.succeed", - ("PipChecker.requirements_filename", dict(new_callable=PropertyMock)), - prefix="tools.dependency.pip_check") - - with patched as (m_succeed, m_fname): - checker.dependabot_success(success) - - assert ( - list(m_succeed.call_args) - == [('dependabot', - [f"{m_fname.return_value}: {x}" for x in sorted(success)]), {}]) - - -def test_pip_checker_dependabot_errors(patches): - checker = pip_check.PipChecker("path1", "path2", "path3") - succeed_mock = patch - errors = set(["C", "D", "B", "A"]) - MSG = "ERROR MESSAGE" - - patched = patches( - "PipChecker.error", - ("PipChecker.name", dict(new_callable=PropertyMock)), - prefix="tools.dependency.pip_check") - - with patched as (m_error, m_name): - checker.dependabot_errors(errors, MSG) - - assert ( - list(list(c) for c in list(m_error.call_args_list)) - == [[('dependabot', [f'ERROR MESSAGE: {x}']), {}] for x in sorted(errors)]) - - -def test_pip_checker_main(): - class_mock = patch("tools.dependency.pip_check.PipChecker") - - with class_mock as m_class: - assert ( - pip_check.main("arg0", "arg1", "arg2") - == m_class.return_value.run.return_value) - - assert ( - list(m_class.call_args) - == [('arg0', 'arg1', 'arg2'), {}]) - assert ( - list(m_class.return_value.run.call_args) - == [(), {}]) From a6b50ebb31d927a3dd011b52102a23e7d6f95862 Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Wed, 15 Sep 2021 14:59:01 +0100 Subject: [PATCH 3/4] add upstream information Signed-off-by: Ryan Northey --- tools/code_format/python_check.py | 21 +++++++++++++++++---- tools/dependency/pip_check.py | 21 ++++++++++++++++++--- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/tools/code_format/python_check.py b/tools/code_format/python_check.py index c7e54f13b22f2..ff67b12d6935b 100755 --- a/tools/code_format/python_check.py +++ b/tools/code_format/python_check.py @@ -4,13 +4,26 @@ # # with bazel: # -# bazel run //tools/code_format:python_check -- -h +# $ bazel //tools/code_format:python_check -- -h # -# alternatively, if you have the necessary python deps available +# $ bazel //tools/code_format:python_check # -# PYTHONPATH=. ./tools/code_format/python_check.py -h +# with pip: # -# python requires: flake8, yapf +# $ pip install envoy.code_format.python_check +# $ envoy.code_format.python_check -h +# +# usage with pip requires a path, eg +# +# $ envoy.code_format.python_check . +# +# The upstream lib is maintained here: +# +# https://github.com/envoyproxy/pytooling/tree/main/envoy.code_format.python_check +# +# Please submit issues/PRs to the pytooling repo: +# +# https://github.com/envoyproxy/pytooling # import pathlib diff --git a/tools/dependency/pip_check.py b/tools/dependency/pip_check.py index 0a0ac98f96853..03e839e995186 100755 --- a/tools/dependency/pip_check.py +++ b/tools/dependency/pip_check.py @@ -4,11 +4,26 @@ # # with bazel: # -# bazel //tools/dependency:pip_check -- -h +# $ bazel //tools/dependency:pip_check -- -h # -# alternatively, if you have the necessary python deps available +# $ bazel //tools/dependency:pip_check # -# ./tools/dependency/pip_check.py -h +# with pip: +# +# $ pip install envoy.dependency.pip_check +# $ envoy.dependency.pip_check -h +# +# usage with pip requires a path, eg +# +# $ envoy.dependency.pip_check . +# +# The upstream lib is maintained here: +# +# https://github.com/envoyproxy/pytooling/tree/main/envoy.dependency.pip_check +# +# Please submit issues/PRs to the pytooling repo: +# +# https://github.com/envoyproxy/pytooling # import pathlib From d80c901f35ad061270c28131806f040f5039b40c Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Wed, 15 Sep 2021 15:33:24 +0100 Subject: [PATCH 4/4] bazel-typo Signed-off-by: Ryan Northey --- tools/code_format/python_check.py | 4 ++-- tools/dependency/pip_check.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/code_format/python_check.py b/tools/code_format/python_check.py index ff67b12d6935b..135b7e9fd3ffe 100755 --- a/tools/code_format/python_check.py +++ b/tools/code_format/python_check.py @@ -4,9 +4,9 @@ # # with bazel: # -# $ bazel //tools/code_format:python_check -- -h +# $ bazel run //tools/code_format:python_check -- -h # -# $ bazel //tools/code_format:python_check +# $ bazel run //tools/code_format:python_check # # with pip: # diff --git a/tools/dependency/pip_check.py b/tools/dependency/pip_check.py index 03e839e995186..4da2ac5f8fb4e 100755 --- a/tools/dependency/pip_check.py +++ b/tools/dependency/pip_check.py @@ -4,9 +4,9 @@ # # with bazel: # -# $ bazel //tools/dependency:pip_check -- -h +# $ bazel run //tools/dependency:pip_check -- -h # -# $ bazel //tools/dependency:pip_check +# $ bazel run //tools/dependency:pip_check # # with pip: #