test: type hints in Python tests

kiminuo commented at 7:15 pm on February 25, 2020: contributor

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.

Notes:

–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. 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:
```
0_opcode_instances = []  # type: List[CScriptOp]
```
to
```
0_opcode_instances:List[CScriptOp] = [] 
```
for type hints that are not function parameters and function return types.

Useful resources:

MarcoFalke commented at 7:23 pm on February 25, 2020: member

Is there a way to make python crash if the type hint is incorrect? In that case they seem useful, otherwise they might get outdated and cause more confusion.

DrahtBot added the label Tests on Feb 25, 2020

kiminuo commented at 7:48 pm on February 25, 2020: contributor

There is Mypy - a static type checker for Python 3 and Python 2.7.

Example

0PS C:\temp> cat test.py
1#!/usr/bin/env python3
2
3s: str
4s = 1
5PS C:\temp> python -m mypy test.py
6test.py:4: error: Incompatible types in assignment (expression has type "int", variable has type "str")
7Found 1 error in 1 file (checked 1 source file)

Links

DrahtBot commented at 10:23 pm on February 25, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in test/functional/test_framework/test_framework.py:93 in 79410242b5 outdated

89@@ -90,6 +90,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
90 
91     This class also contains various public and private helper methods."""
92 
93+    chain: str

fanquake commented at 2:34 am on February 26, 2020:

We can’t use these annotations while we are still supporting Python 3.5.

kiminuo commented at 6:20 am on February 26, 2020:

Yes, that’s unfortunately true. However, Python 3.5 EOL is 09/2020. Is that a sufficient reason to bump Python version, or what is the policy for upgrade here?

fanquake commented at 2:41 am on February 26, 2020: member

Concept ACK - assuming the types are actually checked/enforced in some way, documentation is written for contributors etc. As Marco mentioned, shotgunning type annotations all over the code base without anything actually using them is likely to just lead them them being out of date and/or confusing.

I did look at doing this for our test framework at one point, but think I came to the conclusion it wasn’t quite worth it while we are still supporting 3.5+, given you are basically limited to function param and return type annotations. However maybe we can require 3.6+ at some point in the near future?

I did end up implementing some types + type checking via mypy for the HWI project in https://github.com/bitcoin-core/HWI/pull/203, so you could check that out for some example usage.

fanquake renamed this:
~~Type hints in Python tests~~
test: type hints in Python tests
on Feb 26, 2020

kiminuo force-pushed on Feb 26, 2020

kiminuo commented at 9:37 am on February 26, 2020: contributor

Python 3.5 EOL is 09/2020. Maybe Python may be bumped to 3.6 in not too distant future. What is the policy for Python upgrade?
Should a specific version of mypy be installed?

practicalswift commented at 12:04 pm on February 26, 2020: contributor

Concept ACK on checking type hints using mypy and gradually adding type hints to our Python code base :)

kiminuo force-pushed on Feb 26, 2020

MarcoFalke commented at 2:39 pm on February 26, 2020: member

https://packages.ubuntu.com/xenial/python3 is still supported and not EOL until April next year, so we can’t bump to python 3.6 yet.

kiminuo commented at 3:47 pm on February 26, 2020: contributor

@MarcoFalke So Ubuntu provides support for Python 3.5 even after Python developers do not support the software? I thought that this is relevant source of information: https://devguide.python.org/#status-of-python-branches (saying EOF is 2020-09-13)

laanwj commented at 5:04 pm on February 26, 2020: member

Agree that they need to be checked in the CI, but very much concept ACK, thanks for doing this.

kiminuo force-pushed on Feb 26, 2020

kiminuo force-pushed on Feb 27, 2020

kiminuo requested review from fanquake on Feb 27, 2020

kiminuo marked this as ready for review on Feb 27, 2020

elichai commented at 3:09 pm on March 1, 2020: contributor

Concept ACK cool to hear about MyPy, tested it locally and it’s pretty nice except for the need to ignore it on all imports. maybe adding this flag is better? https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports

Example, changed:

0diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py
1--- a/test/functional/feature_cltv.py
2+++ b/test/functional/feature_cltv.py
3-def cltv_validate(node, tx, height):
4+def cltv_validate(node, tx: int, height):

0 $ mypy test/functional/feature_cltv.py 
1test/functional/test_framework/script.py:19: error: Need type annotation for 'OPCODE_NAMES' (hint: "OPCODE_NAMES: Dict[<type>, <type>] = ...")
2test/functional/test_framework/script.py:25: error: Need type annotation for '_opcode_instances' (hint: "_opcode_instances: List[<type>] = ...")
3test/functional/test_framework/test_framework.py:599: error: No library stub file for module 'zmq'
4test/functional/test_framework/test_framework.py:599: note: (Stub files are from https://github.com/python/typeshed)
5test/functional/feature_cltv.py:41: error: "int" has no attribute "vin"
6test/functional/feature_cltv.py:42: error: "int" has no attribute "nLockTime"
7Found 5 errors in 3 files (checked 1 source file)

kiminuo commented at 7:56 pm on March 1, 2020: contributor

@elichai I’m slightly more in favor of not using that ignore flag because:

https://github.com/python/typeshed seems very active so somebody may actually add typings for zmq to typeshed which would be ideal I guess.
It does not seem that number of # type: ignores needed for module imports will be high - correct me if I’m wrong.

EDIT: I have changed my mind and I have added --ignore-missing-imports mypy flag to make the commit easier to review and less obtrusive for developers at the beginning.

DrahtBot added the label Needs rebase on Mar 11, 2020

kiminuo force-pushed on Mar 12, 2020

DrahtBot removed the label Needs rebase on Mar 12, 2020

DrahtBot added the label Needs rebase on Mar 17, 2020

kiminuo force-pushed on Mar 17, 2020

kiminuo force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

kiminuo force-pushed on May 29, 2020

troygiorshev commented at 1:10 pm on June 1, 2020: contributor

Concept ACK. The combination of Type Hinting and mypy is the single best improvement to ever occur to the python ecosystem IMO. However we’ll need to go about this gradually. This may be a useful guide: https://mypy.readthedocs.io/en/stable/existing_code.html

in test/functional/data/invalid_txs.py:24 in b466ecb697 outdated

20@@ -21,6 +21,7 @@
21 """
22 import abc
23 
24+from typing import Optional

MarcoFalke commented at 1:13 pm on June 1, 2020:

This means that mypy is a requirement to run the tests, right?

MarcoFalke commented at 1:15 pm on June 1, 2020:

Ah no, this is the standard library: https://docs.python.org/3/library/typing.html

kiminuo commented at 1:39 pm on June 1, 2020:

Right

MarcoFalke commented at 1:15 pm on June 1, 2020: member

Concept ACK. Fine with me.

in ci/lint/04_install.sh:12 in b466ecb697 outdated

 8@@ -9,6 +9,7 @@ export LC_ALL=C
 9 travis_retry pip3 install codespell==1.15.0
10 travis_retry pip3 install flake8==3.7.8
11 travis_retry pip3 install yq
12+travis_retry pip3 install mypy

MarcoFalke commented at 1:57 pm on June 1, 2020:

Can this be set to a specific version or is this assumed to be a stable program? We had trillion of issues with flake8 and codespell being updated from down under, so I’d like to avoid any such mishaps in the future.

kiminuo commented at 2:08 pm on June 1, 2020:

Looks like there is a new release every month or so: https://github.com/python/mypy/releases (it should be stable enough though as they seem to fix bugs and improve it, they don’t redesign it every month)

So it makes sense to set: travis_retry pip3 install mypy==0.770

kiminuo commented at 6:34 pm on June 1, 2020:

@MarcoFalke Done

MarcoFalke approved

MarcoFalke commented at 2:00 pm on June 1, 2020: member

ACK, just one question.

kiminuo force-pushed on Jun 1, 2020

MarcoFalke commented at 11:24 pm on June 1, 2020: member

ACK 600ba915f84129fb74f39c5eafd2a28ef4841fa0

in test/functional/test_framework/test_framework.py:662 in 600ba915f8 outdated

658@@ -656,7 +659,7 @@ def _initialize_chain_clean(self):
659     def skip_if_no_py3_zmq(self):
660         """Attempt to import the zmq package and skip the test if the import fails."""
661         try:
662-            import zmq  # noqa
663+            import zmq  # type: ignore  # noqa

fanquake commented at 0:44 am on June 2, 2020:

The type: ignore here should no longer be required?

kiminuo commented at 6:09 am on June 2, 2020:

You are right. Fixed.

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/

bd7e530f01

kiminuo force-pushed on Jun 2, 2020

MarcoFalke commented at 10:27 am on June 2, 2020: member

ACK bd7e530f010d43816bb05d6f1590d1cd36cdaa2c fine with me

fanquake approved

fanquake commented at 9:28 am on June 3, 2020: member

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).

Sanity checked that the linter complains when you change things:

 0diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
 1index bc1b5b26f..206e3eebc 100644
 2--- a/test/functional/test_framework/script.py
 3+++ b/test/functional/test_framework/script.py
 4@@ -22,7 +22,7 @@ from .messages import (
 5 )
 6 
 7 MAX_SCRIPT_ELEMENT_SIZE = 520
 8-OPCODE_NAMES = {}  # type: Dict[CScriptOp, str]
 9+OPCODE_NAMES = {}  # type: Dict[CScriptOp, int]
10 
11 def hash160(s):

0test/functional/test_framework/script.py:251: error: Dict entry 0 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
1test/functional/test_framework/script.py:252: error: Dict entry 1 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
2test/functional/test_framework/script.py:253: error: Dict entry 2 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"

0diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
1index 6e72db1d9..14dffcee4 100644
2--- a/test/functional/data/invalid_txs.py
3+++ b/test/functional/data/invalid_txs.py
4@@ -57,7 +57,7 @@ class BadTxTemplate:
5     __metaclass__ = abc.ABCMeta
6 
7     # The expected error code given by bitcoind upon submission of the tx.
8-    reject_reason = ""  # type: Optional[str]
9+    reject_reason = ""  # type: Optional[bool]

0test/functional/data/invalid_txs.py:60: error: Incompatible types in assignment (expression has type "str", variable has type "Optional[bool]")
1test/functional/data/invalid_txs.py:84: error: Incompatible types in assignment (expression has type "str", base class "BadTxTemplate" defined the type as "Optional[bool]")

fanquake merged this on Jun 3, 2020

fanquake closed this on Jun 3, 2020

sidhujag referenced this in commit 08b5728354 on Jun 3, 2020

hebasto commented at 5:23 am on June 5, 2020: member

I have linter warnings on master (4ede05d421e7799e631bb83ed16799e8bae79eeb):

0$ python3 --version
1Python 3.6.9
2$ ./test/lint/lint-python.sh
3test/functional/data/invalid_txs.py:24:1: F401 'typing.Optional' imported but unused
4test/functional/test_framework/script.py:12:1: F401 'typing.List' imported but unused
5test/functional/test_framework/script.py:12:1: F401 'typing.Dict' imported but unused
6Success: no issues found in 168 source files

UPDATE: False positive warnings due to the outdated local flake8 version:

0$ pip3 show flake8 | grep Version
1Version: 3.5.0

fanquake referenced this in commit 17cfa52d38 on Jun 6, 2020

in ci/lint/04_install.sh:12 in bd7e530f01

 8@@ -9,6 +9,7 @@ export LC_ALL=C
 9 travis_retry pip3 install codespell==1.15.0
10 travis_retry pip3 install flake8==3.7.8
11 travis_retry pip3 install yq
12+travis_retry pip3 install mypy==0.700

ysangkok commented at 7:57 am on June 21, 2020:

why did you set 0.700 instead of 0.770 like you mentioned in this comment?

kiminuo commented at 7:59 am on June 21, 2020:

I screwed up.

MarcoFalke commented at 9:54 am on June 21, 2020:

I think this should be bumped because version 700 can not be compiled with python3.8-dev

kiminuo deleted the branch on Dec 15, 2020

Fabcien referenced this in commit 89048c5c13 on Feb 24, 2021

Fabcien referenced this in commit e98dd8358d on Feb 25, 2021

DrahtBot locked this on Feb 15, 2022

test: type hints in Python tests #18210

Example

Links

Conflicts