-
Notifications
You must be signed in to change notification settings - Fork 37.7k
test: type hints in Python tests #18210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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. |
There is Mypy - a static type checker for Python 3 and Python 2.7. Example
LinksSee also: https://github.com/python/mypy |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bitcoin-core/HWI#203, so you could check that out for some example usage.
@@ -90,6 +90,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): | |||
|
|||
This class also contains various public and private helper methods.""" | |||
|
|||
chain: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use these annotations while we are still supporting Python 3.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
e70e2e9
to
f532426
Compare
|
Concept ACK on checking type hints using |
f532426
to
c23f7af
Compare
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. |
@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) |
Agree that they need to be checked in the CI, but very much concept ACK, thanks for doing this. |
65fa76b
to
150e950
Compare
Concept ACK cool to hear about Example, changed:
|
e2a07a0
to
fe58ea7
Compare
e475947
to
024027e
Compare
024027e
to
b466ecb
Compare
Concept ACK. The combination of Type Hinting and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK. Fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, just one question.
ci/lint/04_install.sh
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcoFalke Done
b466ecb
to
600ba91
Compare
ACK 600ba91 |
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/
600ba91
to
bd7e530
Compare
ACK bd7e530 fine with me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK bd7e530 - 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:
diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
index bc1b5b26f..206e3eebc 100644
--- a/test/functional/test_framework/script.py
+++ b/test/functional/test_framework/script.py
@@ -22,7 +22,7 @@ from .messages import (
)
MAX_SCRIPT_ELEMENT_SIZE = 520
-OPCODE_NAMES = {} # type: Dict[CScriptOp, str]
+OPCODE_NAMES = {} # type: Dict[CScriptOp, int]
def hash160(s):
test/functional/test_framework/script.py:251: error: Dict entry 0 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
test/functional/test_framework/script.py:252: error: Dict entry 1 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
test/functional/test_framework/script.py:253: error: Dict entry 2 has incompatible type "CScriptOp": "str"; expected "CScriptOp": "int"
diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 6e72db1d9..14dffcee4 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functional/data/invalid_txs.py
@@ -57,7 +57,7 @@ class BadTxTemplate:
__metaclass__ = abc.ABCMeta
# The expected error code given by bitcoind upon submission of the tx.
- reject_reason = "" # type: Optional[str]
+ reject_reason = "" # type: Optional[bool]
test/functional/data/invalid_txs.py:60: error: Incompatible types in assignment (expression has type "str", variable has type "Optional[bool]")
test/functional/data/invalid_txs.py:84: error: Incompatible types in assignment (expression has type "str", base class "BadTxTemplate" defined the type as "Optional[bool]")
bd7e530 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 bd7e530 - 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 bd7e530 fine with me Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a
I have linter warnings on master (4ede05d):
UPDATE: False positive warnings due to the outdated local
|
5d77549 doc: Add mypy to test dependencies (Hennadii Stepanov) 7dda912 test: Do not swallow flake8 exit code (Hennadii Stepanov) Pull request description: After #18210 the `flake8` exit code in `test/lint/lint-python.sh` just not used that makes the linter broken. This PR: - combines exit codes of `flake8` and `mypy` into the `test/lint/lint-python.sh` exit code - documents `mypy` as the test dependency ACKs for top commit: MarcoFalke: Approach ACK 5d77549, fine with me practicalswift: ACK 5d77549 Tree-SHA512: e948ba04dc4d73393967ebf3c6a26c40d428d33766382a0310fc64746cb7972e027bd62e7ea76898b742a656cf7d0fcda2fdd61560a21bfd7be249cea27f3d41
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you set 0.700 instead of 0.770 like you mentioned in this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I screwed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be bumped because version 700 can not be compiled with python3.8-dev
Summary: These tests no longer work, and they test trivial stuff that is already tested in every single existing functional test (starting and stopping nodes...) I noticed them when testing `mypy`, a lint tool that will be introduced when backporting [[bitcoin/bitcoin#18210 | core#18210]] See D237 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9271
Summary: > 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 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/ Note: Core still depended on python 3.5 when this was merged, so they had to use old style "type comments". With Python 3.6, it becomes possible to use the typehint syntax for variables and attributes. Unfortunately Postponed Evaluation of Annotations is only available starting from python 3.7, so for now e have to use quotes when annotating types (classes) that are only defined later in a module. This is a backport of [[bitcoin/bitcoin#18210 | core#18210]] Depends on D9270 Test Plan: `ninja check-functional` ``` sudo arc liberate arc lint --trace ``` Add an intentional tyephint mistake: ``` diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index c3d39d8..76179e742 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -23,7 +23,7 @@ from .messages import ( ) -MAX_SCRIPT_ELEMENT_SIZE = 520 +MAX_SCRIPT_ELEMENT_SIZE: bool = 520 OPCODE_NAMES: Dict["CScriptOp", str] = {} ``` Run `arc lint` and check that it catches the problem: ``` >>> Lint for test/functional/test_framework/script.py: Error () mypy found an issue: Incompatible types in assignment (expression has type "int", variable has type "bool") 23 ) 24 25 >>> 26 MAX_SCRIPT_ELEMENT_SIZE: bool = 520 ^ 27 OPCODE_NAMES: Dict["CScriptOp", str] = {} 28 29 ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9273
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:
mypy
checker for now. The effect of this is that one does not need# type: ignore
forimport zmq
. More information about import processing can be found here. This can be changed in a follow-up PR, if it is deemed useful.Useful resources: