diff --git a/.circleci/config.yml b/.circleci/config.yml index c94dd20ff0..38534393ac 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -194,8 +194,14 @@ jobs: pipenv install --dev - run: name: Run yapf to ensure code format + when: always working_directory: test_runner command: pipenv run yapf --recursive --diff . + - run: + name: Run mypy to check types + when: always + working_directory: test_runner + command: pipenv run mypy . run-pytest: #description: "Run pytest" diff --git a/test_runner/Pipfile b/test_runner/Pipfile index a98acc5718..246a3d419f 100644 --- a/test_runner/Pipfile +++ b/test_runner/Pipfile @@ -14,11 +14,14 @@ asyncpg = "*" cached-property = "*" [dev-packages] -flake8 = "*" -mypy = "*" # Behavior may change slightly between versions. These are run continuously, # so we pin exact versions to avoid suprising breaks. Update if comfortable. yapf = "==0.31.0" +mypy = "==0.910" +# Non-pinned packages follow. +flake8 = "*" +types-requests = "*" +types-psycopg2 = "*" [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 75fc17ffad..289639a2ee 100644 --- a/test_runner/Pipfile.lock +++ b/test_runner/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "3645ae8d2dcf55bd2a54963c44cfeedf577f3b289d1077365214a80a7f36e643" + "sha256": "48eef51e8eb3c7f0d7284879dd68a83d9b0d9dc835f7a3ff0452b4ddfe9c560a" }, "pipfile-spec": 6, "requires": { @@ -369,6 +369,22 @@ "markers": "python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==0.10.2" }, + "types-psycopg2": { + "hashes": [ + "sha256:77ed80f2668582654623e04fb3d741ecce93effcc39c929d7e02f4a917a538ce", + "sha256:98a6e0e9580cd7eb4bd4d20f7c7063d154b2589a2b90c0ce4e3ca6085cde77c6" + ], + "index": "pypi", + "version": "==2.9.1" + }, + "types-requests": { + "hashes": [ + "sha256:b279284e51f668e38ee12d9665e4d789089f532dc2a0be4a1508ca0efd98ba9e", + "sha256:ba1d108d512e294b6080c37f6ae7cb2a2abf527560e2b671d1786c1fc46b541a" + ], + "index": "pypi", + "version": "==2.25.11" + }, "typing-extensions": { "hashes": [ "sha256:49f75d16ff11f1cd258e1b988ccff82a3ca5570217d7ad8c5f48205dd99a677e", diff --git a/test_runner/README.md b/test_runner/README.md index cdbf7e988d..fe37fdbcb2 100644 --- a/test_runner/README.md +++ b/test_runner/README.md @@ -95,13 +95,16 @@ Python destructors, e.g. `__del__()` aren't recommended for cleanup. ### Code quality -We force code formatting via yapf: +We force code formatting via yapf and type hints via mypy: 1. Install `yapf` and other tools (`flake8`, `mypy`) with `pipenv install --dev`. 1. Reformat all your code by running `pipenv run yapf -ri .` in the `test_runner/` directory. +1. Ensure there are no type errors by running `pipenv run mypy .` in the `test_runner/` directory. 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. -* (Optional) Typechecking the code with `mypy .`. Currently this mostly affects `fixtures/zenith_fixtures.py`. +* Adding more type hints to your code to avoid `Any`, especially: + * For fixture parameters, they are not automatically deduced. + * For function arguments and return values. diff --git a/test_runner/batch_others/test_createdropdb.py b/test_runner/batch_others/test_createdropdb.py index 5fe103496d..edaea5d220 100644 --- a/test_runner/batch_others/test_createdropdb.py +++ b/test_runner/batch_others/test_createdropdb.py @@ -87,12 +87,14 @@ def test_dropdb( pg_before.connect(dbname='foodb').close() # Test that database subdir exists on the branch before drop + assert pg_before.pgdata_dir dbpath = pathlib.Path(pg_before.pgdata_dir) / 'base' / str(dboid) log.info(dbpath) assert os.path.isdir(dbpath) == True # Test that database subdir doesn't exist on the branch after drop + assert pg_after.pgdata_dir dbpath = pathlib.Path(pg_after.pgdata_dir) / 'base' / str(dboid) log.info(dbpath) diff --git a/test_runner/batch_others/test_pageserver_api.py b/test_runner/batch_others/test_pageserver_api.py index 407b78fff6..e7674491c2 100644 --- a/test_runner/batch_others/test_pageserver_api.py +++ b/test_runner/batch_others/test_pageserver_api.py @@ -3,7 +3,8 @@ from uuid import uuid4 import pytest import psycopg2 import requests -from fixtures.zenith_fixtures import ZenithPageserver, ZenithPageserverHttpClient +from fixtures.zenith_fixtures import ZenithCli, ZenithPageserver, ZenithPageserverHttpClient +from typing import cast pytest_plugins = ("fixtures.zenith_fixtures") @@ -53,7 +54,7 @@ def test_branch_list_psql(pageserver: ZenithPageserver, zenith_cli): conn.close() -def test_tenant_list_psql(pageserver: ZenithPageserver, zenith_cli): +def test_tenant_list_psql(pageserver: ZenithPageserver, zenith_cli: ZenithCli): res = zenith_cli.run(["tenant", "list"]) res.check_returncode() tenants = sorted(map(lambda t: t.split()[0], res.stdout.splitlines())) @@ -74,7 +75,7 @@ def test_tenant_list_psql(pageserver: ZenithPageserver, zenith_cli): cur.execute('tenant_list') # compare tenants list - new_tenants = sorted(map(lambda t: t['id'], json.loads(cur.fetchone()[0]))) + new_tenants = sorted(map(lambda t: cast(str, t['id']), json.loads(cur.fetchone()[0]))) assert sorted([pageserver.initial_tenant, tenant1]) == new_tenants diff --git a/test_runner/batch_others/test_readonly_node.py b/test_runner/batch_others/test_readonly_node.py index cc6c11caad..7f964470f3 100644 --- a/test_runner/batch_others/test_readonly_node.py +++ b/test_runner/batch_others/test_readonly_node.py @@ -84,6 +84,6 @@ def test_readonly_node(zenith_cli, pageserver: ZenithPageserver, postgres: Postg # Create node at pre-initdb lsn try: zenith_cli.run(["pg", "start", "test_branch_preinitdb", "test_readonly_node@0/42"]) - assert false, "compute node startup with invalid LSN should have failed" + assert False, "compute node startup with invalid LSN should have failed" except Exception: print("Node creation with pre-initdb LSN failed (as expected)") diff --git a/test_runner/batch_others/test_zenith_cli.py b/test_runner/batch_others/test_zenith_cli.py index a328f1a607..91e9d5085a 100644 --- a/test_runner/batch_others/test_zenith_cli.py +++ b/test_runner/batch_others/test_zenith_cli.py @@ -1,19 +1,24 @@ import json import uuid +from psycopg2.extensions import cursor as PgCursor from fixtures.zenith_fixtures import ZenithCli, ZenithPageserver +from typing import cast pytest_plugins = ("fixtures.zenith_fixtures") -def helper_compare_branch_list(page_server_cur, zenith_cli, initial_tenant: str): +def helper_compare_branch_list(page_server_cur: PgCursor, + zenith_cli: ZenithCli, + initial_tenant: str): """ Compare branches list returned by CLI and directly via API. Filters out branches created by other tests. """ page_server_cur.execute(f'branch_list {initial_tenant}') - branches_api = sorted(map(lambda b: b['name'], json.loads(page_server_cur.fetchone()[0]))) + branches_api = sorted( + map(lambda b: cast(str, b['name']), json.loads(page_server_cur.fetchone()[0]))) branches_api = [b for b in branches_api if b.startswith('test_cli_') or b in ('empty', 'main')] res = zenith_cli.run(["branch"]) @@ -32,7 +37,7 @@ def helper_compare_branch_list(page_server_cur, zenith_cli, initial_tenant: str) assert branches_api == branches_cli == branches_cli_with_tenant_arg -def test_cli_branch_list(pageserver: ZenithPageserver, zenith_cli): +def test_cli_branch_list(pageserver: ZenithPageserver, zenith_cli: ZenithCli): page_server_conn = pageserver.connect() page_server_cur = page_server_conn.cursor() @@ -58,9 +63,10 @@ def test_cli_branch_list(pageserver: ZenithPageserver, zenith_cli): assert 'test_cli_branch_list_nested' in branches_cli -def helper_compare_tenant_list(page_server_cur, zenith_cli: ZenithCli): +def helper_compare_tenant_list(page_server_cur: PgCursor, zenith_cli: ZenithCli): page_server_cur.execute(f'tenant_list') - tenants_api = sorted(map(lambda t: t['id'], json.loads(page_server_cur.fetchone()[0]))) + tenants_api = sorted( + map(lambda t: cast(str, t['id']), json.loads(page_server_cur.fetchone()[0]))) res = zenith_cli.run(["tenant", "list"]) assert res.stderr == '' diff --git a/test_runner/fixtures/benchmark_fixture.py b/test_runner/fixtures/benchmark_fixture.py index f41d66674d..7bfb698984 100644 --- a/test_runner/fixtures/benchmark_fixture.py +++ b/test_runner/fixtures/benchmark_fixture.py @@ -54,16 +54,10 @@ in the test initialization, or measure disk usage after the test query. """ -# All the results are collected in this list, as a tuple: -# (test_name: str, metric_name: str, metric_value: float, unit: str) -# + # TODO: It would perhaps be better to store the results as additional # properties in the pytest TestReport objects, to make them visible to # other pytest tools. -global zenbenchmark_results -zenbenchmark_results = [] - - class ZenithBenchmarkResults: """ An object for recording benchmark results. """ def __init__(self): @@ -77,6 +71,10 @@ class ZenithBenchmarkResults: self.results.append((test_name, metric_name, metric_value, unit)) +# Will be recreated in each session. +zenbenchmark_results: ZenithBenchmarkResults = ZenithBenchmarkResults() + + # Session scope fixture that initializes the results object @pytest.fixture(autouse=True, scope='session') def zenbenchmark_global(request) -> Iterator[ZenithBenchmarkResults]: @@ -137,6 +135,7 @@ class ZenithBenchmarker: matches = re.search(r'^pageserver_disk_io_bytes{io_operation="write"} (\S+)$', all_metrics, re.MULTILINE) + assert matches return int(round(float(matches.group(1)))) def get_peak_mem(self, pageserver) -> int: @@ -147,6 +146,7 @@ class ZenithBenchmarker: all_metrics = pageserver.http_client().get_metrics() # See comment in get_io_writes() matches = re.search(r'^pageserver_maxrss_kb (\S+)$', all_metrics, re.MULTILINE) + assert matches return int(round(float(matches.group(1)))) def get_timeline_size(self, repo_dir: str, tenantid: str, timelineid: str): diff --git a/test_runner/fixtures/zenith_fixtures.py b/test_runner/fixtures/zenith_fixtures.py index 2ea321a0af..a5149e39ff 100644 --- a/test_runner/fixtures/zenith_fixtures.py +++ b/test_runner/fixtures/zenith_fixtures.py @@ -192,7 +192,7 @@ class ZenithCli: self.env['ZENITH_REPO_DIR'] = repo_dir self.env['POSTGRES_DISTRIB_DIR'] = pg_distrib_dir - def run(self, arguments: List[str]) -> Any: + def run(self, arguments: List[str]) -> 'subprocess.CompletedProcess[str]': """ Run "zenith" with the specified arguments. @@ -249,12 +249,14 @@ class ZenithPageserverHttpClient(requests.Session): def check_status(self): self.get(f"http://localhost:{self.port}/v1/status").raise_for_status() - def branch_list(self, tenant_id: uuid.UUID) -> List[Dict]: + def branch_list(self, tenant_id: uuid.UUID) -> List[Dict[Any, Any]]: res = self.get(f"http://localhost:{self.port}/v1/branch/{tenant_id.hex}") res.raise_for_status() - return res.json() + res_json = res.json() + assert isinstance(res_json, list) + return res_json - def branch_create(self, tenant_id: uuid.UUID, name: str, start_point: str) -> Dict: + def branch_create(self, tenant_id: uuid.UUID, name: str, start_point: str) -> Dict[Any, Any]: res = self.post(f"http://localhost:{self.port}/v1/branch", json={ 'tenant_id': tenant_id.hex, @@ -262,17 +264,23 @@ class ZenithPageserverHttpClient(requests.Session): 'start_point': start_point, }) res.raise_for_status() - return res.json() + res_json = res.json() + assert isinstance(res_json, dict) + return res_json - def branch_detail(self, tenant_id: uuid.UUID, name: str) -> Dict: + def branch_detail(self, tenant_id: uuid.UUID, name: str) -> Dict[Any, Any]: res = self.get(f"http://localhost:{self.port}/v1/branch/{tenant_id.hex}/{name}", ) res.raise_for_status() - return res.json() + res_json = res.json() + assert isinstance(res_json, dict) + return res_json - def tenant_list(self) -> List[Dict]: + def tenant_list(self) -> List[Dict[Any, Any]]: res = self.get(f"http://localhost:{self.port}/v1/tenant") res.raise_for_status() - return res.json() + res_json = res.json() + assert isinstance(res_json, list) + return res_json def tenant_create(self, tenant_id: uuid.UUID): res = self.post( @@ -361,20 +369,23 @@ class PageserverPort: class ZenithPageserver(PgProtocol): - """ An object representing a running pageserver. """ - def __init__(self, zenith_cli: ZenithCli, repo_dir: str, port: PageserverPort): + """ + An object representing a running pageserver. + + Initializes the repository via `zenith init` and starts page server. + """ + def __init__(self, + zenith_cli: ZenithCli, + repo_dir: str, + port: PageserverPort, + enable_auth=False): super().__init__(host='localhost', port=port.pg) self.zenith_cli = zenith_cli self.running = False - self.initial_tenant = None + self.initial_tenant: str = cast(str, None) # Will be fixed by self.start() below self.repo_dir = repo_dir self.service_port = port # do not shadow PgProtocol.port which is just int - def init(self, enable_auth: bool = False) -> 'ZenithPageserver': - """ - Initialize the repository, i.e. run "zenith init". - Returns self. - """ cmd = [ 'init', f'--pageserver-pg-port={self.service_port.pg}', @@ -383,13 +394,8 @@ class ZenithPageserver(PgProtocol): if enable_auth: cmd.append('--enable-auth') self.zenith_cli.run(cmd) - return self - def repo_dir(self) -> str: - """ - Return path to repository dir - """ - return self.zenith_cli.repo_dir + self.start() # Required, otherwise self.initial_tenant is of wrong type def start(self) -> 'ZenithPageserver': """ @@ -401,7 +407,11 @@ class ZenithPageserver(PgProtocol): self.zenith_cli.run(['start']) self.running = True # get newly created tenant id - self.initial_tenant = self.zenith_cli.run(['tenant', 'list']).stdout.split()[0] + current_tenant = self.zenith_cli.run(['tenant', 'list']).stdout.split()[0] + if self.initial_tenant is None: + self.initial_tenant = current_tenant + else: + assert self.initial_tenant == current_tenant return self def stop(self, immediate=False) -> 'ZenithPageserver': @@ -461,8 +471,7 @@ def pageserver(zenith_cli: ZenithCli, repo_dir: str, By convention, the test branches are named after the tests. For example, test called 'test_foo' would create and use branches with the 'test_foo' prefix. """ - ps = ZenithPageserver(zenith_cli=zenith_cli, repo_dir=repo_dir, - port=pageserver_port).init().start() + ps = ZenithPageserver(zenith_cli=zenith_cli, repo_dir=repo_dir, port=pageserver_port) # For convenience in tests, create a branch from the freshly-initialized cluster. zenith_cli.run(["branch", "empty", "main"]) @@ -516,7 +525,7 @@ class PgBin: command: List[str], env: Optional[Env] = None, cwd: Optional[str] = None, - **kwargs: Any) -> None: + **kwargs: Any) -> str: """ Run one of the postgres binaries, with stderr and stdout redirected to a file. @@ -537,8 +546,10 @@ def pg_bin(test_output_dir: str, pg_distrib_dir: str) -> PgBin: @pytest.fixture def pageserver_auth_enabled(zenith_cli: ZenithCli, repo_dir: str, pageserver_port: PageserverPort): - with ZenithPageserver(zenith_cli=zenith_cli, repo_dir=repo_dir, - port=pageserver_port).init(enable_auth=True).start() as ps: + with ZenithPageserver(zenith_cli=zenith_cli, + repo_dir=repo_dir, + port=pageserver_port, + enable_auth=True) as ps: # For convenience in tests, create a branch from the freshly-initialized cluster. zenith_cli.run(["branch", "empty", "main"]) yield ps @@ -623,6 +634,7 @@ class Postgres(PgProtocol): def pg_data_dir_path(self) -> str: """ Path to data directory """ + assert self.node_name path = pathlib.Path('pgdatadirs') / 'tenants' / self.tenant_id / self.node_name return os.path.join(self.repo_dir, path) @@ -693,9 +705,9 @@ class Postgres(PgProtocol): """ assert self.node_name is not None - assert self.tenant_id is not None self.zenith_cli.run( ['pg', 'stop', '--destroy', self.node_name, f'--tenantid={self.tenant_id}']) + self.node_name = None return self @@ -793,29 +805,6 @@ class PostgresFactory: config_lines=config_lines, ) - def config(self, - node_name: str = "main", - tenant_id: Optional[str] = None, - wal_acceptors: Optional[str] = None, - config_lines: Optional[List[str]] = None) -> Postgres: - - pg = Postgres( - zenith_cli=self.zenith_cli, - repo_dir=self.repo_dir, - pg_bin=self.pg_bin, - tenant_id=tenant_id or self.initial_tenant, - port=self.port_distributor.get_port(), - ) - - self.num_instances += 1 - self.instances.append(pg) - - return pg.config( - node_name=node_name, - wal_acceptors=wal_acceptors, - config_lines=config_lines, - ) - def stop_all(self) -> 'PostgresFactory': for pg in self.instances: pg.stop() @@ -849,7 +838,7 @@ def postgres(zenith_cli: ZenithCli, pgfactory.stop_all() -def read_pid(path: Path): +def read_pid(path: Path) -> int: """ Read content of file into number """ return int(path.read_text()) @@ -955,7 +944,9 @@ class WalAcceptor: cur.execute("JSON_CTRL " + request_json) all = cur.fetchall() log.info(f"JSON_CTRL response: {all[0][0]}") - return json.loads(all[0][0]) + res = json.loads(all[0][0]) + assert isinstance(res, dict) + return res def http_client(self): return WalAcceptorHttpClient(port=self.port.http) @@ -1211,6 +1202,7 @@ def check_restored_datadir_content(zenith_cli: ZenithCli, subprocess.check_call(cmd, shell=True) # list files we're going to compare + assert pg.pgdata_dir pgdata_files = list_files_to_compare(pg.pgdata_dir) restored_files = list_files_to_compare(restored_dir_path) @@ -1237,7 +1229,7 @@ def check_restored_datadir_content(zenith_cli: ZenithCli, subprocess.run("xxd -b {} > {}.hex ".format(f1, f1), shell=True) subprocess.run("xxd -b {} > {}.hex ".format(f2, f2), shell=True) - cmd = ['diff {}.hex {}.hex'.format(f1, f2)] - subprocess.run(cmd, stdout=stdout_f, shell=True) + cmd = 'diff {}.hex {}.hex'.format(f1, f2) + subprocess.run([cmd], stdout=stdout_f, shell=True) assert (mismatch, error) == ([], []) diff --git a/test_runner/setup.cfg b/test_runner/setup.cfg index cff4c7f86e..9c37fa7299 100644 --- a/test_runner/setup.cfg +++ b/test_runner/setup.cfg @@ -22,7 +22,11 @@ disallow_untyped_decorators = false disallow_untyped_defs = false strict = true -[mypy-psycopg2.*] +[mypy-asyncpg.*] +# There is some work in progress, though: https://github.com/MagicStack/asyncpg/pull/577 +ignore_missing_imports = true + +[mypy-cached_property.*] ignore_missing_imports = true [mypy-pytest.*]