From 271acac3a72390b883f0ddb16249387908f425d2 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 3 Jun 2020 22:01:29 +0800 Subject: [PATCH] Merge #18210: test: type hints in Python tests bd7e530f010d43816bb05d6f1590d1cd36cdaa2c This PR adds initial support for type hints checking in python scripts. (Kiminuo) Pull request description: This PR adds initial support for type hints checking in python scripts. Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention." [Mypy](https://mypy.readthedocs.io/en/latest/index.html) is used in `lint-python.sh` to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error. **Notes:** * [--ignore-missing-imports](https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports) switch is passed on to `mypy` checker for now. The effect of this is that one does not need `# type: ignore` for `import zmq`. More information about import processing can be found [here](https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports). This can be changed in a follow-up PR, if it is deemed useful. * We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change: ```python _opcode_instances = [] # type: List[CScriptOp] ``` to ```python _opcode_instances:List[CScriptOp] = [] ``` for type hints that are **not** function parameters and function return types. **Useful resources:** * https://docs.python.org/3.5/library/typing.html * https://www.python.org/dev/peps/pep-0484/ ACKs for top commit: fanquake: ACK bd7e530f010d43816bb05d6f1590d1cd36cdaa2c - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat). MarcoFalke: ACK bd7e530f010d43816bb05d6f1590d1cd36cdaa2c fine with me Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a --- .gitignore | 1 + ci/lint/04_install.sh | 1 + test/functional/README.md | 6 ++++-- test/functional/data/invalid_txs.py | 3 ++- test/functional/test_framework/script.py | 5 +++-- test/functional/test_framework/test_framework.py | 5 ++++- test/functional/test_runner.py | 2 +- test/lint/lint-python.sh | 3 +++ 8 files changed, 19 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 6bd2bb3533..a56c40a4d1 100644 --- a/.gitignore +++ b/.gitignore @@ -128,6 +128,7 @@ linux-build win32-build test/config.ini test/cache/* +test/.mypy_cache/ !src/leveldb*/Makefile diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index e7758a8c57..40fe6c3fd3 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -14,6 +14,7 @@ travis_retry pip3 install codespell==1.17.1 travis_retry pip3 install flake8==3.8.3 travis_retry pip3 install vulture==2.3 travis_retry pip3 install yq +travis_retry pip3 install mypy==0.700 SHELLCHECK_VERSION=v0.6.0 curl -s "https://storage.googleapis.com/shellcheck/shellcheck-${SHELLCHECK_VERSION}.linux.x86_64.tar.xz" | tar --xz -xf - --directory /tmp/ diff --git a/test/functional/README.md b/test/functional/README.md index 11485ed03a..efb69a5234 100644 --- a/test/functional/README.md +++ b/test/functional/README.md @@ -26,10 +26,12 @@ don't have test cases for. The Travis linter also checks this, but [possibly not in all cases](https://github.com/bitcoin/bitcoin/pull/14884#discussion_r239585126). - See [the python lint script](/test/lint/lint-python.sh) that checks for violations that could lead to bugs and issues in the test code. +- Use [type hints](https://docs.python.org/3/library/typing.html) in your code to improve code readability + and to detect possible bugs earlier. - Avoid wildcard imports - Use a module-level docstring to describe what the test is testing, and how it is testing it. -- When subclassing the BitcoinTestFramwork, place overrides for the +- When subclassing the BitcoinTestFramework, place overrides for the `set_test_params()`, `add_options()` and `setup_xxxx()` methods at the top of the subclass, then locally-defined helper methods, then the `run_test()` method. - Use `f'{x}'` for string formatting in preference to `'{}'.format(x)` or `'%s' % x`. @@ -45,7 +47,7 @@ don't have test cases for. - `rpc` for tests for individual RPC methods or features, eg `rpc_listtransactions.py` - `tool` for tests for tools, eg `tool_wallet.py` - `wallet` for tests for wallet features, eg `wallet_keypool.py` -- use an underscore to separate words +- Use an underscore to separate words - exception: for tests for specific RPCs or command line options which don't include underscores, name the test after the exact RPC or argument name, eg `rpc_decodescript.py`, not `rpc_decode_script.py` - Don't use the redundant word `test` in the name, eg `interface_zmq.py`, not `interface_zmq_test.py` diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index 946afcc265..95114fdf12 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -21,6 +21,7 @@ Invalid tx cases not covered here can be found by running: """ import abc +from typing import Optional from test_framework.messages import ( COutPoint, CTransaction, @@ -39,7 +40,7 @@ class BadTxTemplate: __metaclass__ = abc.ABCMeta # The expected error code given by bitcoind upon submission of the tx. - reject_reason = "" + reject_reason = "" # type: Optional[str] # Only specified if it differs from mempool acceptance error. block_reject_reason = "" diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index 6ec17a11f0..51da54081c 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -8,6 +8,7 @@ This file is modified from python-bitcoinlib. """ import struct import unittest +from typing import List, Dict from .messages import ( CTransaction, @@ -19,7 +20,7 @@ from .messages import ( from .ripemd160 import ripemd160 MAX_SCRIPT_ELEMENT_SIZE = 520 -OPCODE_NAMES = {} +OPCODE_NAMES = {} # type: Dict[CScriptOp, str] def hash160(s): return ripemd160(sha256(s)) @@ -35,7 +36,7 @@ def bn2vch(v): # Serialize to bytes return encoded_v.to_bytes(n_bytes, 'little') -_opcode_instances = [] +_opcode_instances = [] # type: List[CScriptOp] class CScriptOp(int): """A single script opcode""" __slots__ = () diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index d094fe2d1f..53d1ac8f74 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -112,6 +112,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): This class also contains various public and private helper methods.""" + chain = None # type: str + setup_clean_chain = None # type: bool + def __init__(self): """Sets test framework defaults. Do not override this method. Instead, override the set_test_params() method""" self.chain: str = 'regtest' @@ -460,7 +463,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): # Public helper methods. These can be accessed by the subclass test scripts. - def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None, binary_cli=None, versions=None): + def add_nodes(self, num_nodes: int, extra_args=None, *, rpchost=None, binary=None, binary_cli=None, versions=None): """Instantiate TestNode objects. Should only be called once after the nodes have been specified in diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 73623c9d50..97cca9c626 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -43,7 +43,7 @@ except UnicodeDecodeError: if os.name != 'nt' or sys.getwindowsversion() >= (10, 0, 14393): if os.name == 'nt': import ctypes - kernel32 = ctypes.windll.kernel32 + kernel32 = ctypes.windll.kernel32 # type: ignore ENABLE_VIRTUAL_TERMINAL_PROCESSING = 4 STD_OUTPUT_HANDLE = -11 STD_ERROR_HANDLE = -12 diff --git a/test/lint/lint-python.sh b/test/lint/lint-python.sh index 8112b2b078..958e1df47d 100755 --- a/test/lint/lint-python.sh +++ b/test/lint/lint-python.sh @@ -7,6 +7,7 @@ # Check for specified flake8 warnings in python files. export LC_ALL=C +export MYPY_CACHE_DIR="${BASE_ROOT_DIR}/test/.mypy_cache" enabled=( E101 # indentation contains mixed spaces and tabs @@ -103,3 +104,5 @@ PYTHONWARNINGS="ignore" $FLAKECMD --ignore=B,C,E,F,I,N,W --select=$(IFS=","; ech echo "$@" fi ) + +mypy --ignore-missing-imports $(git ls-files "test/functional/*.py") \ No newline at end of file