8000 test: Mypy next steps by am1r021 · Pull Request #19532 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
Skip to content

test: Mypy next steps #19532

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

test: Mypy next steps #19532

wants to merge 5 commits into from

Conversation

am1r021
Copy link
@am1r021 am1r021 commented Jul 15, 2020

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.

Amir Ghorbanian added 5 commits July 15, 2020 18:20
ignore zmq import error from mypy
92b8001
add __init__.py file to data directory
a86945c 
The data package needs this file to be identified as a package directory for import
generate stub files for test_framework package
7f9cba7 
Generate necessary stub files for mypy checks.
generate stub files for data package
14f756e 
Generate necessary stub files for mypy checks.
remove missing-imports flag and set stub path
e7f433e 
Remove --ignore-missing-imports flag from call to mypy and set MYPYPATH variable for stub file recognition.
@DrahtBot DrahtBot added the Tests label Jul 15, 2020
@troygiorshev troygiorshev mentioned this pull request Jul 16, 2020
Closed
3 tasks
adamjonas
adamjonas reviewed Jul 16, 2020
View reviewed changes
test/functional/test_framework/test_framework.py
@@ -714,7 +714,7 @@ def _initialize_chain_clean(self):
def skip_if_no_py3_zmq(self):
"""Attempt to import the zmq package and skip the test if the import fails."""
try:
import zmq # noqa
import zmq # type:ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line appears to be getting caught in the linter.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor
@troygiorshev troygiorshev Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's unfortunate :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor
troygiorshev commented Jul 16, 2020 •
edited
Loading

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

@am1r021
Copy link
Author
am1r021 commented Jul 18, 2020

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
Copy link
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
Copy link
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
Copy link
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:

[mypy]
check_untyped_defs = True
no_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
Copy link
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
Copy link
Member
laanwj commented Sep 8, 2020

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 DrahtBot mentioned this pull request Sep 10, 2020
Merged
@DrahtBot
Copy link
Contributor
DrahtBot commented Sep 10, 2020 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor

🐙 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
Copy link
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 fanquake closed this May 28, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adamjonas adamjonas adamjonas left review comments

@jnewbery jnewbery jnewbery left review comments

@troygiorshev troygiorshev troygiorshev left review comments

@kiminuo kiminuo kiminuo left review comments

Assignees
No one assigned
Labels
Needs rebase Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@am1r021 @troygiorshev @adaminsky @laanwj @DrahtBot @fanquake @adamjonas @jnewbery @kiminuo
0