test: Mypy next steps

scorbajio commented at 10:39 pm on July 15, 2020: none

Generate stub files for data and test_framework packages. Set MYPYPATH variable to allow recognition of the aforementioned stub files while ignoring the zmq import that causes an error while running mypy. Fixes issue #19389.

ignore zmq import error from mypy 92b8001022

add __init__.py file to data directory

The data package needs this file to be identified as a package directory for import

a86945cb2b

generate stub files for test_framework package

Generate necessary stub files for mypy checks.

7f9cba79fc

generate stub files for data package

Generate necessary stub files for mypy checks.

14f756e787

remove missing-imports flag and set stub path

Remove --ignore-missing-imports flag from call to mypy and set MYPYPATH variable for stub file recognition.

e7f433e1e1

DrahtBot added the label Tests on Jul 15, 2020

in test/functional/test_framework/test_framework.py:717 in 92b8001022 outdated

713@@ -714,7 +714,7 @@ def _initialize_chain_clean(self):
714     def skip_if_no_py3_zmq(self):
715         """Attempt to import the zmq package and skip the test if the import fails."""
716         try:
717-            import zmq  # noqa
718+            import zmq  # type:ignore

adamjonas commented at 5:00 pm on July 16, 2020:

This line appears to be getting caught in the linter.

test/functional/test_framework/test_framework.py:717:13: F401 'zmq' imported but unused

troygiorshev commented at 6:04 pm on July 16, 2020:

As far as I can tell, we don’t be able to fix this short of disabling flake8 linting for the entire file. It’s a false positive, in this case the very act of importing the module is “using” it.

troygiorshev commented at 6:58 pm on July 16, 2020:

It’s worth noting that this brings us from 3 F401 warnings up to 4 F401 warnings.

jnewbery commented at 8:20 am on July 17, 2020:

is it not possible to keep the # noqa linter disable comment?

troygiorshev commented at 12:25 pm on July 17, 2020:

Unfortunately not. We need the type:ignore or else MyPy fails completely. I’ve tried things like putting them both on the same line but it looks like flake8 and MyPy just don’t play nicely together in this sort of conflict.

Worth noting maybe that pylint allows us to disable a particular linting error for an entire function (which would fix this problem), where AFAICT flake8 does not.

jnewbery commented at 12:50 pm on July 17, 2020:

that’s unfortunate :(

kiminuo commented at 8:21 pm on September 21, 2020:

Unfortunately not. We need the type:ignore or else MyPy fails completely. I’ve tried things like putting them both on the same line but it looks like flake8 and MyPy just don’t play nicely together in this sort of conflict.

Just to clarify: https://mypy.readthedocs.io/en/stable/common_issues.html#silencing-linters - this does not work? Is it what you have tried?

troygiorshev commented at 6:14 pm on July 16, 2020: contributor

This all looks great!

However, try as I might, I can’t get MyPy to ever actually tell me something is wrong. (i.e. I intentionally give a function an argument with the wrong type and MyPy still passes every file) Have you been able to?

Regarding your concern here that this isn’t python 3.5 compatible, I don’t think you have to worry. The only parts that aren’t compatible are the .pyi files, but that’s ok, they’re only used by MyPy. I’ve tested running lint-python.sh under python 3.5 and it doesn’t give any errors. (MyPy does, however, get confused and tell me that a couple lines are wrong when they aren’t. It doesn’t seem related though).

scorbajio commented at 4:59 pm on July 18, 2020: none

This all looks great!

However, try as I might, I can’t get MyPy to ever actually tell me something is wrong. (i.e. I intentionally give a function an argument with the wrong type and MyPy still passes every file) Have you been able to?

Regarding your concern here that this isn’t python 3.5 compatible, I don’t think you have to worry. The only parts that aren’t compatible are the .pyi files, but that’s ok, they’re only used by MyPy. I’ve tested running lint-python.sh under python 3.5 and it doesn’t give any errors. (MyPy does, however, get confused and tell me that a couple lines are wrong when they aren’t. It doesn’t seem related though).

Thanks for taking the time to review the code!

When I run the linter file (lint-python.py), Mypy doesn’t catch any errors, however, when I run mypy from the command line as such: MYPYPATH=/path/to/stub/files mypy test/functional/contains_mypy_errors.py incompatible type errors are properly caught and displayed. I haven’t been able to find too much on this unexpected behavior from Mypy other than this Stackoverflow post: https://stackoverflow.com/questions/55437746/how-do-i-correctly-set-mypypath-to-pick-up-stubs-for-mypy.

adaminsky commented at 8:08 pm on August 9, 2020: contributor

I’m new to mypy, but I found this issue which seems to address the problem that was previously encountered. It seems like the --namespace-packages flag will fix the issue, but it may be easier to just add the empty __init__.py file to test/functional/data which fixes the problem too.

To test this, I included the __init__.py file in the data directory and then mypy test/functional runs without any errors. Creating an obvious type error inside the data package also results in the error being correctly detected. This will make the generated stub files no longer necessary, so the type annotations can be made directly in the source files.

troygiorshev commented at 9:19 pm on August 10, 2020: contributor

@adaminsky thanks for looking at this! Great to see that we might not need the stub files after all! I don’t think we’re all the way there yet thought, creating test/functional/data/__init__.py is already the second commit of this PR :)

Did you try making a type error inside a test in test/functional/? I still can’t get those to be caught.

troygiorshev commented at 9:20 pm on August 10, 2020: contributor

I’ve tried changing MyPy’s configuration options, maybe that will do the trick. To try this too, create a config file, say, test/functional/data/mypy.ini with the following in it:

0[mypy]
1check_untyped_defs = True
2no_implicit_optional = True

Then running MyPy with mypy --config-file test/functional/data/mypy.ini test/functional gives tonnes of errors (finally 🎉) but I’m not sure that all of them are useful yet.

adaminsky commented at 11:50 pm on August 10, 2020: contributor

@troygiorshev I mislabeled the types in the BaseNode.__init__ function in test/functional/example_test.py and when I run mypy test/functional, it gives an error. I tried stuff like 1 + 'test' to create type errors, but that doesn’t work because Mypy assumes everything is untyped unless a function is fully annotated (Mypy doc). I think this is why running with your config file finds so many errors because the check_untyped_defs flag makes Mypy treat everything as typed.

laanwj commented at 9:00 am on September 8, 2020: member

I understand mypy is desirable for type checking, but I see a potential maintainability issue with the stub files: many changes to the test framework will now require updating these files as well. I’m not sure we should be putting this burden on every contributor.

DrahtBot commented at 9:45 am on September 10, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19572 (ZMQ: Create “sequence” notifier, enabling client-side mempool tracking by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Sep 23, 2020

DrahtBot commented at 12:04 pm on September 23, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

fanquake commented at 3:49 am on May 28, 2021: member

@Ghorbanian thanks for your work here, but I agree with @laanwj in regards to the maintenance overhead of adding these stubs. Since this PR was opened, there have been upstream changes which have negated the needs for some of the commits here, such as ignoring the zmq imports (the pyzmq package now comes with stubs).

I’m going to close this PR, and I think we can renew discussion in #19389, and potentially make some other mypy related changes in the mean time.

fanquake closed this on May 28, 2021

DrahtBot locked this on Aug 16, 2022

test: Mypy next steps #19532

Conflicts