From 96c7594d29c5f8cb92b8a2026dddf24d0e2fdce8 Mon Sep 17 00:00:00 2001 From: Dmitry Ivanov Date: Thu, 10 Jun 2021 22:53:15 +0300 Subject: [PATCH] Enable some kind of gradual typing in test_runner (#222) It's not realistic to enable full-blown type checks within test_runner's codebase, since the amount of warnings revealed by mypy is overwhelming. Tests are supposed to be easy to use, so we can't cripple everybody's workflow for the sake of imaginary benefit. Ultimately, the purpose of this attempt is three-fold: * Facilitate code navigation when paired with python-language-server. * Make method signatures apparent to a fellow programmer. * Occasionally catch some obvious type errors. --- test_runner/Pipfile | 1 + test_runner/Pipfile.lock | 80 ++++++++++- test_runner/README.md | 1 + test_runner/batch_others/test_createuser.py | 2 +- test_runner/fixtures/utils.py | 10 +- test_runner/fixtures/zenith_fixtures.py | 148 ++++++++++++-------- test_runner/setup.cfg | 16 +++ 7 files changed, 197 insertions(+), 61 deletions(-) diff --git a/test_runner/Pipfile b/test_runner/Pipfile index 7f33320c13..923aba9ad3 100644 --- a/test_runner/Pipfile +++ b/test_runner/Pipfile @@ -10,6 +10,7 @@ psycopg2 = "*" [dev-packages] yapf = "*" flake8 = "*" +mypy = "*" [requires] # we need at least 3.6, but pipenv doesn't allow to say this directly diff --git a/test_runner/Pipfile.lock b/test_runner/Pipfile.lock index 3ecb23fbef..31da752ece 100644 --- a/test_runner/Pipfile.lock +++ b/test_runner/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "c0de1f00310b584a272dda58386939d53098e49d14db518804a9e56269bfec1d" + "sha256": "2cc3a75a3846ba41f9dc08903061ac587a4f0fb6864ae2073f871a944533dfe4" }, "pipfile-spec": 6, "requires": { @@ -117,6 +117,41 @@ ], "version": "==0.6.1" }, + "mypy": { + "hashes": [ + "sha256:0d0a87c0e7e3a9becdfbe936c981d32e5ee0ccda3e0f07e1ef2c3d1a817cf73e", + "sha256:25adde9b862f8f9aac9d2d11971f226bd4c8fbaa89fb76bdadb267ef22d10064", + "sha256:28fb5479c494b1bab244620685e2eb3c3f988d71fd5d64cc753195e8ed53df7c", + "sha256:2f9b3407c58347a452fc0736861593e105139b905cca7d097e413453a1d650b4", + "sha256:33f159443db0829d16f0a8d83d94df3109bb6dd801975fe86bacb9bf71628e97", + "sha256:3f2aca7f68580dc2508289c729bd49ee929a436208d2b2b6aab15745a70a57df", + "sha256:499c798053cdebcaa916eef8cd733e5584b5909f789de856b482cd7d069bdad8", + "sha256:4eec37370483331d13514c3f55f446fc5248d6373e7029a29ecb7b7494851e7a", + "sha256:552a815579aa1e995f39fd05dde6cd378e191b063f031f2acfe73ce9fb7f9e56", + "sha256:5873888fff1c7cf5b71efbe80e0e73153fe9212fafdf8e44adfe4c20ec9f82d7", + "sha256:61a3d5b97955422964be6b3baf05ff2ce7f26f52c85dd88db11d5e03e146a3a6", + "sha256:674e822aa665b9fd75130c6c5f5ed9564a38c6cea6a6432ce47eafb68ee578c5", + "sha256:7ce3175801d0ae5fdfa79b4f0cfed08807af4d075b402b7e294e6aa72af9aa2a", + "sha256:9743c91088d396c1a5a3c9978354b61b0382b4e3c440ce83cf77994a43e8c521", + "sha256:9f94aac67a2045ec719ffe6111df543bac7874cee01f41928f6969756e030564", + "sha256:a26f8ec704e5a7423c8824d425086705e381b4f1dfdef6e3a1edab7ba174ec49", + "sha256:abf7e0c3cf117c44d9285cc6128856106183938c68fd4944763003decdcfeb66", + "sha256:b09669bcda124e83708f34a94606e01b614fa71931d356c1f1a5297ba11f110a", + "sha256:cd07039aa5df222037005b08fbbfd69b3ab0b0bd7a07d7906de75ae52c4e3119", + "sha256:d23e0ea196702d918b60c8288561e722bf437d82cb7ef2edcd98cfa38905d506", + "sha256:d65cc1df038ef55a99e617431f0553cd77763869eebdf9042403e16089fe746c", + "sha256:d7da2e1d5f558c37d6e8c1246f1aec1e7349e4913d8fb3cb289a35de573fe2eb" + ], + "index": "pypi", + "version": "==0.812" + }, + "mypy-extensions": { + "hashes": [ + "sha256:090fedd75945a69ae91ce1303b5824f428daf5a028d2f6ab8a299250a846f15d", + "sha256:2d82818f5bb3e369420cb3c4060a7970edba416647068eb4c5343488a6c604a8" + ], + "version": "==0.4.3" + }, "pycodestyle": { "hashes": [ "sha256:514f76d918fcc0b55c6680472f0a37970994e07bbb80725808c17089be302068", @@ -133,6 +168,49 @@ "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.3.1" }, + "typed-ast": { + "hashes": [ + "sha256:01ae5f73431d21eead5015997ab41afa53aa1fbe252f9da060be5dad2c730ace", + "sha256:067a74454df670dcaa4e59349a2e5c81e567d8d65458d480a5b3dfecec08c5ff", + "sha256:0fb71b8c643187d7492c1f8352f2c15b4c4af3f6338f21681d3681b3dc31a266", + "sha256:1b3ead4a96c9101bef08f9f7d1217c096f31667617b58de957f690c92378b528", + "sha256:2068531575a125b87a41802130fa7e29f26c09a2833fea68d9a40cf33902eba6", + "sha256:209596a4ec71d990d71d5e0d312ac935d86930e6eecff6ccc7007fe54d703808", + "sha256:2c726c276d09fc5c414693a2de063f521052d9ea7c240ce553316f70656c84d4", + "sha256:398e44cd480f4d2b7ee8d98385ca104e35c81525dd98c519acff1b79bdaac363", + "sha256:52b1eb8c83f178ab787f3a4283f68258525f8d70f778a2f6dd54d3b5e5fb4341", + "sha256:5feca99c17af94057417d744607b82dd0a664fd5e4ca98061480fd8b14b18d04", + "sha256:7538e495704e2ccda9b234b82423a4038f324f3a10c43bc088a1636180f11a41", + "sha256:760ad187b1041a154f0e4d0f6aae3e40fdb51d6de16e5c99aedadd9246450e9e", + "sha256:777a26c84bea6cd934422ac2e3b78863a37017618b6e5c08f92ef69853e765d3", + "sha256:95431a26309a21874005845c21118c83991c63ea800dd44843e42a916aec5899", + "sha256:9ad2c92ec681e02baf81fdfa056fe0d818645efa9af1f1cd5fd6f1bd2bdfd805", + "sha256:9c6d1a54552b5330bc657b7ef0eae25d00ba7ffe85d9ea8ae6540d2197a3788c", + "sha256:aee0c1256be6c07bd3e1263ff920c325b59849dc95392a05f258bb9b259cf39c", + "sha256:af3d4a73793725138d6b334d9d247ce7e5f084d96284ed23f22ee626a7b88e39", + "sha256:b36b4f3920103a25e1d5d024d155c504080959582b928e91cb608a65c3a49e1a", + "sha256:b9574c6f03f685070d859e75c7f9eeca02d6933273b5e69572e5ff9d5e3931c3", + "sha256:bff6ad71c81b3bba8fa35f0f1921fb24ff4476235a6e94a26ada2e54370e6da7", + "sha256:c190f0899e9f9f8b6b7863debfb739abcb21a5c054f911ca3596d12b8a4c4c7f", + "sha256:c907f561b1e83e93fad565bac5ba9c22d96a54e7ea0267c708bffe863cbe4075", + "sha256:cae53c389825d3b46fb37538441f75d6aecc4174f615d048321b716df2757fb0", + "sha256:dd4a21253f42b8d2b48410cb31fe501d32f8b9fbeb1f55063ad102fe9c425e40", + "sha256:dde816ca9dac1d9c01dd504ea5967821606f02e510438120091b84e852367428", + "sha256:f2362f3cb0f3172c42938946dbc5b7843c2a28aec307c49100c8b38764eb6927", + "sha256:f328adcfebed9f11301eaedfa48e15bdece9b519fb27e6a8c01aa52a17ec31b3", + "sha256:f8afcf15cc511ada719a88e013cec87c11aff7b91f019295eb4530f96fe5ef2f", + "sha256:fb1bbeac803adea29cedd70781399c99138358c26d05fcbd23c13016b7f5ec65" + ], + "version": "==1.4.3" + }, + "typing-extensions": { + "hashes": [ + "sha256:0ac0f89795dd19de6b97debb0c6af1c70987fd80a2d62d1958f7e56fcc31b497", + "sha256:50b6f157849174217d0656f99dc82fe932884fb250826c18350e159ec6cdf342", + "sha256:779383f6086d90c99ae41cf0ff39aac8a7937a9283ce0a414e5dd782f4c94a84" + ], + "version": "==3.10.0.0" + }, "yapf": { "hashes": [ "sha256:408fb9a2b254c302f49db83c59f9aa0b4b0fd0ec25be3a5c51181327922ff63d", diff --git a/test_runner/README.md b/test_runner/README.md index 8309900271..303acb8de0 100644 --- a/test_runner/README.md +++ b/test_runner/README.md @@ -99,5 +99,6 @@ Before submitting a patch, please consider: * Writing a couple of docstrings to clarify the reasoning behind a new test. * Running `flake8` (or a linter of your choice, e.g. `pycodestyle`) and fixing possible defects, if any. * Formatting the code with `yapf -r -i .` (TODO: implement an opt-in pre-commit hook for that). +* (Optional) Typechecking the code with `mypy .`. Currently this mostly affects `fixtures/zenith_fixtures.py`. The tools can be installed with `pipenv install --dev`. diff --git a/test_runner/batch_others/test_createuser.py b/test_runner/batch_others/test_createuser.py index 15ff2ff63d..649559e162 100644 --- a/test_runner/batch_others/test_createuser.py +++ b/test_runner/batch_others/test_createuser.py @@ -17,7 +17,7 @@ def test_createuser(zenith_cli, pageserver, postgres, pg_bin): with conn.cursor() as cur: # Cause a 'relmapper' change in the original branch - cur.execute('CREATE USER testuser with password %s', ('testpwd',)) + cur.execute('CREATE USER testuser with password %s', ('testpwd', )) cur.execute('CHECKPOINT') diff --git a/test_runner/fixtures/utils.py b/test_runner/fixtures/utils.py index 4ab283a974..6214503af5 100644 --- a/test_runner/fixtures/utils.py +++ b/test_runner/fixtures/utils.py @@ -1,13 +1,15 @@ import os import subprocess +from typing import Any, List -def get_self_dir(): + +def get_self_dir() -> str: """ Get the path to the directory where this script lives. """ return os.path.dirname(os.path.abspath(__file__)) -def mkdir_if_needed(path): +def mkdir_if_needed(path: str) -> None: """ Create a directory if it doesn't already exist Note this won't try to create intermediate directories. @@ -18,7 +20,7 @@ def mkdir_if_needed(path): os.mkdir(path) -def subprocess_capture(capture_dir, cmd, **kwargs): +def subprocess_capture(capture_dir: str, cmd: List[str], **kwargs: Any) -> None: """ Run a process and capture its output Output will go to files named "cmd_NNN.stdout" and "cmd_NNN.stderr" @@ -42,7 +44,7 @@ def subprocess_capture(capture_dir, cmd, **kwargs): _global_counter = 0 -def global_counter(): +def global_counter() -> int: """ A really dumb global counter. This is useful for giving output files a unique number, so if we run the diff --git a/test_runner/fixtures/zenith_fixtures.py b/test_runner/fixtures/zenith_fixtures.py index 628447e2ae..fee626b616 100644 --- a/test_runner/fixtures/zenith_fixtures.py +++ b/test_runner/fixtures/zenith_fixtures.py @@ -4,6 +4,8 @@ import pytest import shutil import subprocess +from typing import Any, Callable, Dict, Iterator, List, Optional, TypeVar, Literal + from .utils import (get_self_dir, mkdir_if_needed, subprocess_capture) """ This file contains pytest fixtures. A fixture is a test resource that can be @@ -23,15 +25,18 @@ Don't import functions from this file, or pytest will emit warnings. Instead put directly-importable functions into utils.py or another separate file. """ +Env = Dict[str, str] +Fn = TypeVar('Fn', bound=Callable[..., Any]) + DEFAULT_OUTPUT_DIR = 'test_output' DEFAULT_POSTGRES_DIR = 'tmp_install' -def determine_scope(fixture_name, config): +def determine_scope(fixture_name: str, config: Any) -> str: return 'session' -def zenfixture(func): +def zenfixture(func: Fn) -> Fn: """ This is a python decorator for fixtures with a flexible scope. @@ -43,15 +48,14 @@ def zenfixture(func): stored in a directory called "shared". """ - if os.environ.get('TEST_SHARED_FIXTURES') is None: - scope = 'function' - else: - scope = 'session' + scope: Literal['session', 'function'] = \ + 'function' if os.environ.get('TEST_SHARED_FIXTURES') is None else 'session' + return pytest.fixture(func, scope=scope) @pytest.fixture(autouse=True, scope='session') -def safety_check(): +def safety_check() -> None: """ Ensure that no unwanted daemons are running before we start testing. """ # does not use -c as it is not supported on macOS @@ -71,7 +75,7 @@ class ZenithCli: We also store an environment that will tell the CLI to operate on a particular ZENITH_REPO_DIR. """ - def __init__(self, binpath, repo_dir, pg_distrib_dir): + def __init__(self, binpath: str, repo_dir: str, pg_distrib_dir: str): assert os.path.isdir(binpath) self.binpath = binpath self.bin_zenith = os.path.join(binpath, 'zenith') @@ -79,7 +83,7 @@ class ZenithCli: self.env['ZENITH_REPO_DIR'] = repo_dir self.env['POSTGRES_DISTRIB_DIR'] = pg_distrib_dir - def run(self, arguments): + def run(self, arguments: List[str]) -> Any: """ Run "zenith" with the specified arguments. @@ -93,6 +97,7 @@ class ZenithCli: """ assert type(arguments) == list + args = [self.bin_zenith] + arguments print('Running command "{}"'.format(' '.join(args))) return subprocess.run(args, @@ -104,41 +109,58 @@ class ZenithCli: @zenfixture -def zenith_cli(zenith_binpath, repo_dir, pg_distrib_dir): +def zenith_cli(zenith_binpath: str, repo_dir: str, pg_distrib_dir: str) -> ZenithCli: return ZenithCli(zenith_binpath, repo_dir, pg_distrib_dir) class ZenithPageserver: """ An object representing a running pageserver. """ - def __init__(self, zenith_cli): + def __init__(self, zenith_cli: ZenithCli): self.zenith_cli = zenith_cli self.running = False - # Initialize the repository, i.e. run "zenith init" - def init(self): - self.zenith_cli.run(['init']) + def init(self) -> 'ZenithPageserver': + """ + Initialize the repository, i.e. run "zenith init". + Returns self. + """ + + self.zenith_cli.run(['init']) + return self + + def start(self) -> 'ZenithPageserver': + """ + Start the page server. + Returns self. + """ - # Start the page server - def start(self): self.zenith_cli.run(['start']) self.running = True + return self - # Stop the page server - def stop(self): - self.zenith_cli.run(['stop']) - self.running = True + def stop(self) -> 'ZenithPageserver': + """ + Stop the page server. + Returns self. + """ + + if self.running: + self.zenith_cli.run(['stop']) + self.running = False + + return self # The page server speaks the Postgres FE/BE protocol, so you can connect # to it with any Postgres client, and run special commands. This function # returns a libpq connection string for connecting to it. - def connstr(self): + def connstr(self) -> str: username = getpass.getuser() conn_str = 'host={} port={} dbname=postgres user={}'.format('localhost', 64000, username) return conn_str @zenfixture -def pageserver(zenith_cli): +def pageserver(zenith_cli: ZenithCli) -> Iterator[ZenithPageserver]: """ The 'pageserver' fixture provides a Page Server that's up and running. @@ -151,12 +173,12 @@ def pageserver(zenith_cli): test called 'test_foo' would create and use branches with the 'test_foo' prefix. """ - ps = ZenithPageserver(zenith_cli) - ps.init() - ps.start() + ps = ZenithPageserver(zenith_cli).init().start() # For convenience in tests, create a branch from the freshly-initialized cluster. zenith_cli.run(["branch", "empty", "main"]) + yield ps + # After the yield comes any cleanup code we need. print('Starting pageserver cleanup') ps.stop() @@ -164,43 +186,45 @@ def pageserver(zenith_cli): class Postgres: """ An object representing a running postgres daemon. """ - def __init__(self, zenith_cli, repo_dir, instance_num): + def __init__(self, zenith_cli: ZenithCli, repo_dir: str, instance_num: int): self.zenith_cli = zenith_cli self.instance_num = instance_num self.running = False self.username = getpass.getuser() self.host = 'localhost' - self.port = 55431 + instance_num + self.port = 55431 + instance_num # TODO: find a better way self.repo_dir = repo_dir - self.branch = None + self.branch: Optional[str] = None # dubious, see asserts below # path to conf is /pgdatadirs//postgresql.conf - def create(self, branch, config_lines=None): + def create(self, branch: str, config_lines: Optional[List[str]] = None) -> 'Postgres': """ Create the pg data directory. Returns self. """ + if not config_lines: + config_lines = [] + self.zenith_cli.run(['pg', 'create', branch]) self.branch = branch - if config_lines is None: - config_lines = [] self.config(config_lines) return self - def start(self): + def start(self) -> 'Postgres': """ Start the Postgres instance. Returns self. """ + assert self.branch is not None self.zenith_cli.run(['pg', 'start', self.branch]) self.running = True return self - def config(self, lines): + def config(self, lines: List[str]) -> 'Postgres': """ Add lines to postgresql.conf. Lines should be an array of valid postgresql.conf rows. @@ -216,28 +240,31 @@ class Postgres: return self - def stop(self): + def stop(self) -> 'Postgres': """ Stop the Postgres instance if it's running. Returns self. """ if self.running: + assert self.branch is not None self.zenith_cli.run(['pg', 'stop', self.branch]) + self.running = False return self - def stop_and_destroy(self): + def stop_and_destroy(self) -> 'Postgres': """ Stop the Postgres instance, then destroy it. Returns self. """ + assert self.branch is not None self.zenith_cli.run(['pg', 'stop', '--destroy', self.branch]) return self - def create_start(self, branch, config_lines=None): + def create_start(self, branch: str, config_lines: Optional[List[str]] = None) -> 'Postgres': """ Create a Postgres instance, then start it. Returns self. @@ -247,41 +274,49 @@ class Postgres: return self - def connstr(self, dbname='postgres', username=None): + def connstr(self, dbname: str = 'postgres', username: Optional[str] = None) -> str: """ Build a libpq connection string for the Postgres instance. """ conn_str = 'host={} port={} dbname={} user={}'.format(self.host, self.port, dbname, - username or self.username) + (username or self.username)) return conn_str class PostgresFactory: """ An object representing multiple running postgres daemons. """ - def __init__(self, zenith_cli, repo_dir): + def __init__(self, zenith_cli: ZenithCli, repo_dir: str): self.zenith_cli = zenith_cli self.host = 'localhost' self.repo_dir = repo_dir self.num_instances = 0 - self.instances = [] + self.instances: List[Postgres] = [] + + def create_start(self, + branch: str = "main", + config_lines: Optional[List[str]] = None) -> Postgres: - def create_start(self, branch="main", config_lines=None): pg = Postgres(self.zenith_cli, self.repo_dir, self.num_instances + 1) self.num_instances += 1 self.instances.append(pg) + return pg.create_start(branch, config_lines) - def stop_all(self): + def stop_all(self) -> 'PostgresFactory': for pg in self.instances: pg.stop() + return self + @zenfixture -def postgres(zenith_cli, repo_dir): +def postgres(zenith_cli: ZenithCli, repo_dir: str) -> Iterator[PostgresFactory]: pgfactory = PostgresFactory(zenith_cli, repo_dir) + yield pgfactory + # After the yield comes any cleanup code we need. print('Starting postgres cleanup') pgfactory.stop_all() @@ -289,25 +324,25 @@ def postgres(zenith_cli, repo_dir): class PgBin: """ A helper class for executing postgres binaries """ - def __init__(self, log_dir, pg_distrib_dir): + def __init__(self, log_dir: str, pg_distrib_dir: str): self.log_dir = log_dir self.pg_install_path = pg_distrib_dir self.pg_bin_path = os.path.join(self.pg_install_path, 'bin') self.env = os.environ.copy() self.env['LD_LIBRARY_PATH'] = os.path.join(self.pg_install_path, 'lib') - def _fixpath(self, command): + def _fixpath(self, command: List[str]) -> None: if '/' not in command[0]: command[0] = os.path.join(self.pg_bin_path, command[0]) - def _build_env(self, env_add): + def _build_env(self, env_add: Optional[Env]) -> Env: if env_add is None: return self.env env = self.env.copy() env.update(env_add) return env - def run(self, command, env=None, cwd=None): + def run(self, command: List[str], env: Optional[Env] = None, cwd: Optional[str] = None) -> None: """ Run one of the postgres binaries. @@ -326,7 +361,10 @@ class PgBin: env = self._build_env(env) subprocess.run(command, env=env, cwd=cwd, check=True) - def run_capture(self, command, env=None, cwd=None): + def run_capture(self, + command: List[str], + env: Optional[Env] = None, + cwd: Optional[str] = None) -> None: """ Run one of the postgres binaries, with stderr and stdout redirected to a file. @@ -340,12 +378,12 @@ class PgBin: @zenfixture -def pg_bin(test_output_dir, pg_distrib_dir): +def pg_bin(test_output_dir: str, pg_distrib_dir: str) -> PgBin: return PgBin(test_output_dir, pg_distrib_dir) @zenfixture -def base_dir(): +def base_dir() -> str: """ find the base directory (currently this is the git root) """ base_dir = os.path.normpath(os.path.join(get_self_dir(), '../..')) @@ -354,7 +392,7 @@ def base_dir(): @zenfixture -def top_output_dir(base_dir): +def top_output_dir(base_dir: str) -> str: """ Compute the top-level directory for all tests. """ env_test_output = os.environ.get('TEST_OUTPUT') @@ -367,7 +405,7 @@ def top_output_dir(base_dir): @zenfixture -def test_output_dir(request, top_output_dir): +def test_output_dir(request: Any, top_output_dir: str) -> str: """ Compute the working directory for an individual test. """ if os.environ.get('TEST_SHARED_FIXTURES') is None: @@ -385,7 +423,7 @@ def test_output_dir(request, top_output_dir): @zenfixture -def repo_dir(request, test_output_dir): +def repo_dir(request: Any, test_output_dir: str) -> str: """ Compute the test repo_dir. @@ -398,7 +436,7 @@ def repo_dir(request, test_output_dir): @zenfixture -def zenith_binpath(base_dir): +def zenith_binpath(base_dir: str) -> str: """ Find the zenith binaries. """ env_zenith_bin = os.environ.get('ZENITH_BIN') @@ -412,7 +450,7 @@ def zenith_binpath(base_dir): @zenfixture -def pg_distrib_dir(base_dir): +def pg_distrib_dir(base_dir: str) -> str: """ Find the postgres install. """ env_postgres_bin = os.environ.get('POSTGRES_DISTRIB_DIR') diff --git a/test_runner/setup.cfg b/test_runner/setup.cfg index c2c1aa2176..578cb28efc 100644 --- a/test_runner/setup.cfg +++ b/test_runner/setup.cfg @@ -10,3 +10,19 @@ max-line-length = 100 [yapf] based_on_style = pep8 column_limit = 100 + +[mypy] +# some tests don't typecheck when this flag is set +check_untyped_defs = false + +disallow_incomplete_defs = false +disallow_untyped_calls = false +disallow_untyped_decorators = false +disallow_untyped_defs = false +strict = true + +[mypy-psycopg2.*] +ignore_missing_imports = true + +[mypy-pytest.*] +ignore_missing_imports = true