From bd7e530f010d43816bb05d6f1590d1cd36cdaa2c Mon Sep 17 00:00:00 2001 From: Kiminuo Date: Tue, 25 Feb 2020 20:05:48 +0100 Subject: [PATCH] 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 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. Useful resources: * https://docs.python.org/3.5/library/typing.html * https://www.python.org/dev/peps/pep-0484/ --- .gitignore | 1 + ci/lint/04_install.sh | 1 + test/functional/README.md | 6 ++- test/functional/data/invalid_txs.py | 3 +- test/functional/p2p_segwit.py | 52 +++++++++---------- test/functional/test_framework/script.py | 5 +- .../test_framework/test_framework.py | 5 +- test/functional/test_runner.py | 2 +- test/lint/lint-python.sh | 3 ++ 9 files changed, 45 insertions(+), 33 deletions(-) diff --git a/.gitignore b/.gitignore index 5b9ec1f355a5b..23b609026503d 100644 --- a/.gitignore +++ b/.gitignore @@ -127,6 +127,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 8b2d609504d5c..26b576c1ae26f 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -9,6 +9,7 @@ export LC_ALL=C travis_retry pip3 install codespell==1.15.0 travis_retry pip3 install flake8==3.7.8 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 004e0afb1dcac..aff5f714f2b85 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 `'{}'.format(x)` for string formatting, not `'%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 ae5721bec2b1e..6e72db1d966c0 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -21,6 +21,7 @@ """ import abc +from typing import Optional from test_framework.messages import ( COutPoint, CTransaction, @@ -56,7 +57,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/p2p_segwit.py b/test/functional/p2p_segwit.py index 8a989097b4b59..8803086213116 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -295,7 +295,7 @@ def func_wrapper(self, *args, **kwargs): return func_wrapper - @subtest + @subtest # type: ignore def test_non_witness_transaction(self): """See if sending a regular transaction works, and create a utxo to use in later tests.""" # Mine a block with an anyone-can-spend coinbase, @@ -324,7 +324,7 @@ def test_non_witness_transaction(self): self.utxo.append(UTXO(tx.sha256, 0, 49 * 100000000)) self.nodes[0].generate(1) - @subtest + @subtest # type: ignore def test_unnecessary_witness_before_segwit_activation(self): """Verify that blocks with witnesses are rejected before activation.""" @@ -355,7 +355,7 @@ def test_unnecessary_witness_before_segwit_activation(self): self.utxo.pop(0) self.utxo.append(UTXO(tx.sha256, 0, tx.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_block_relay(self): """Test that block requests to NODE_WITNESS peer are with MSG_WITNESS_FLAG. @@ -451,7 +451,7 @@ def test_block_relay(self): self.old_node.announce_tx_and_wait_for_getdata(block4.vtx[0]) assert block4.sha256 not in self.old_node.getdataset - @subtest + @subtest # type: ignore def test_v0_outputs_arent_spendable(self): """Test that v0 outputs aren't spendable before segwit activation. @@ -533,7 +533,7 @@ def test_v0_outputs_arent_spendable(self): self.utxo.pop(0) self.utxo.append(UTXO(txid, 2, value)) - @subtest + @subtest # type: ignore def test_getblocktemplate_before_lockin(self): txid = int(self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1), 16) @@ -559,7 +559,7 @@ def test_getblocktemplate_before_lockin(self): self.nodes[0].generate(1) self.sync_blocks() - @subtest + @subtest # type: ignore def test_witness_tx_relay_before_segwit_activation(self): # Generate a transaction that doesn't require a witness, but send it @@ -601,7 +601,7 @@ def test_witness_tx_relay_before_segwit_activation(self): self.utxo.pop(0) self.utxo.append(UTXO(tx_hash, 0, tx_value)) - @subtest + @subtest # type: ignore def test_standardness_v0(self): """Test V0 txout standardness. @@ -679,7 +679,7 @@ def test_standardness_v0(self): self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue)) assert_equal(len(self.nodes[1].getrawmempool()), 0) - @subtest + @subtest # type: ignore def advance_to_segwit_active(self): """Mine enough blocks to activate segwit.""" assert not softfork_active(self.nodes[0], 'segwit') @@ -690,7 +690,7 @@ def advance_to_segwit_active(self): assert softfork_active(self.nodes[0], 'segwit') self.segwit_active = True - @subtest + @subtest # type: ignore def test_p2sh_witness(self): """Test P2SH wrapped witness programs.""" @@ -759,7 +759,7 @@ def test_p2sh_witness(self): self.utxo.pop(0) self.utxo.append(UTXO(spend_tx.sha256, 0, spend_tx.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_witness_commitments(self): """Test witness commitments. @@ -849,7 +849,7 @@ def test_witness_commitments(self): self.utxo.pop(0) self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_block_malleability(self): # Make sure that a block that has too big a virtual size @@ -889,7 +889,7 @@ def test_block_malleability(self): block.vtx[0].wit.vtxinwit[0].scriptWitness.stack = [ser_uint256(0)] test_witness_block(self.nodes[0], self.test_node, block, accepted=True) - @subtest + @subtest # type: ignore def test_witness_block_size(self): # TODO: Test that non-witness carrying blocks can't exceed 1MB # Skipping this test for now; this is covered in p2p-fullblocktest.py @@ -967,7 +967,7 @@ def test_witness_block_size(self): self.utxo.pop(0) self.utxo.append(UTXO(block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue)) - @subtest + @subtest # type: ignore def test_submit_block(self): """Test that submitblock adds the nonce automatically when possible.""" block = self.build_next_block() @@ -1003,7 +1003,7 @@ def test_submit_block(self): # Tip should not advance! assert self.nodes[0].getbestblockhash() != block_2.hash - @subtest + @subtest # type: ignore def test_extra_witness_data(self): """Test extra witness data in a transaction.""" @@ -1076,7 +1076,7 @@ def test_extra_witness_data(self): self.utxo.pop(0) self.utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_max_witness_push_length(self): """Test that witness stack can only allow up to 520 byte pushes.""" @@ -1113,7 +1113,7 @@ def test_max_witness_push_length(self): self.utxo.pop() self.utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_max_witness_program_length(self): """Test that witness outputs greater than 10kB can't be spent.""" @@ -1161,7 +1161,7 @@ def test_max_witness_program_length(self): self.utxo.pop() self.utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_witness_input_length(self): """Test that vin length must match vtxinwit length.""" @@ -1243,7 +1243,7 @@ def serialize_with_witness(self): self.utxo.pop() self.utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_tx_relay_after_segwit_activation(self): """Test transaction relay after segwit activation. @@ -1336,7 +1336,7 @@ def test_tx_relay_after_segwit_activation(self): self.utxo.pop(0) self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_segwit_versions(self): """Test validity of future segwit version transactions. @@ -1418,7 +1418,7 @@ def test_segwit_versions(self): # Add utxo to our list self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_premature_coinbase_witness_spend(self): block = self.build_next_block() @@ -1453,7 +1453,7 @@ def test_premature_coinbase_witness_spend(self): test_witness_block(self.nodes[0], self.test_node, block2, accepted=True) self.sync_blocks() - @subtest + @subtest # type: ignore def test_uncompressed_pubkey(self): """Test uncompressed pubkey validity in segwit transactions. @@ -1558,7 +1558,7 @@ def test_uncompressed_pubkey(self): test_witness_block(self.nodes[0], self.test_node, block, accepted=True) self.utxo.append(UTXO(tx5.sha256, 0, tx5.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_signature_version_1(self): key = ECKey() @@ -1740,7 +1740,7 @@ def test_signature_version_1(self): for i in range(len(tx.vout)): self.utxo.append(UTXO(tx.sha256, i, tx.vout[i].nValue)) - @subtest + @subtest # type: ignore def test_non_standard_witness_blinding(self): """Test behavior of unnecessary witnesses in transactions does not blind the node for the transaction""" @@ -1794,7 +1794,7 @@ def test_non_standard_witness_blinding(self): self.utxo.pop(0) self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue)) - @subtest + @subtest # type: ignore def test_non_standard_witness(self): """Test detection of non-standard P2WSH witness""" pad = chr(1).encode('latin-1') @@ -1894,7 +1894,7 @@ def test_non_standard_witness(self): self.utxo.pop(0) - @subtest + @subtest # type: ignore def test_upgrade_after_activation(self): """Test the behavior of starting up a segwit-aware node after the softfork has activated.""" @@ -1916,7 +1916,7 @@ def test_upgrade_after_activation(self): assert_equal(self.nodes[0].getblock(block_hash), self.nodes[2].getblock(block_hash)) height -= 1 - @subtest + @subtest # type: ignore def test_witness_sigops(self): """Test sigop counting is correct inside witnesses.""" diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index 9102266456cb7..bc1b5b26fc58f 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -9,6 +9,7 @@ import hashlib import struct import unittest +from typing import List, Dict from .messages import ( CTransaction, @@ -21,7 +22,7 @@ ) MAX_SCRIPT_ELEMENT_SIZE = 520 -OPCODE_NAMES = {} +OPCODE_NAMES = {} # type: Dict[CScriptOp, str] def hash160(s): return hashlib.new('ripemd160', sha256(s)).digest() @@ -37,7 +38,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 5469f808d170e..8e0da5295df2f 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -91,6 +91,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 = 'regtest' @@ -407,7 +410,7 @@ def run_test(self): # 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 0812470b0c848..1517407700572 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -42,7 +42,7 @@ 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 86ac5a930fc92..b9aa6c799b03e 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 @@ -96,3 +97,5 @@ PYTHONWARNINGS="ignore" flake8 --ignore=B,C,E,F,I,N,W --select=$(IFS=","; echo " echo "$@" fi ) + +mypy --ignore-missing-imports $(git ls-files "test/functional/*.py") \ No newline at end of file