Skip to content
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

tests: Enable mypy type checking in pre-commit and fix typing issues #1581

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

swick
Copy link
Contributor

@swick swick commented Jan 17, 2025

This gets is mypy in addition to ruff check and ruff format. I spent way too many hours trying to make things work with pre-commit and I don't think it's even possible. CI runs the check script directly, the git-hooks integration only when a commit changes something in tests.

githooks: Replace pre-commit tool with simple shell script

It's basically impossible to run mypy with pre-commit because it will
ignore the repo config and check all files changed the commit. The
dbusmock templates are special and mypy fails on them so we use the mypy
config to skip them.

This introduces a new script which checks the python tests and installs
all the required tools into a python virtual environment. It also
changes to pre-commit hook to call the script if any of the files in
tests changed and uses git stash push/pop to get the test a clean state
of the repo.

@swick
Copy link
Contributor Author

swick commented Jan 21, 2025

/cc @whot don't know who else could review this

@swick swick force-pushed the wip/pytest-mypy-improvements branch from c28d53f to e6b876c Compare January 24, 2025 12:42
Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

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

Isn't this throwing out the baby with the bathwater? pre-commit is relatively standard and if mypy is the issue here would it make more sense to just treat mypy special (e.g. even invoking it as part of meson test) and leave everything else that works to pre-commit?

Or like this example to wrap the mypy run into a pre-commit local script so we control mypy only. But I guess you found that one so - maybe running mypy as part of meson test is the easiest?

Oh, and adding this line to the top of really stubborn files that we cannot fix seems to help (see docs:

# mypy: disable-error-code="annotation-unchecked"

What were the specific unfixable issues you ran into here?

.githooks/check-tests-python.sh Outdated Show resolved Hide resolved
@swick swick force-pushed the wip/pytest-mypy-improvements branch from e6b876c to f114b39 Compare January 29, 2025 12:36
@swick
Copy link
Contributor Author

swick commented Jan 29, 2025

Alright, a fresh mind seems to have helped. Managed to make mypy work with pre-commit by using a bunch of mypy annotations.

@swick swick changed the title githooks: Replace pre-commit tool with simple shell script tests: Enable mypy type checking in pre-commit and fix typing issues Jan 30, 2025
@swick swick requested a review from whot January 30, 2025 11:24
tests/test_document_fuse.py Outdated Show resolved Hide resolved
@swick swick force-pushed the wip/pytest-mypy-improvements branch from f114b39 to 404ff7e Compare February 4, 2025 12:34
tests/test_document_fuse.py Outdated Show resolved Hide resolved
swick added 9 commits February 7, 2025 15:16
The code also had a bug where its using instanceof on a class instead of
an instance, but then also accidentally check for the negated of what it
was supposed to check. Found by mypy because it complained that the
variable might not have been of the right type.
The type is Response|None and accessing (None).response would be wrong,
so mypy correctly complains.
Response is of type Callable | Response at first but we use the Callable
to always get a Response. For mypy to understand what's going on, we
assign the response to a new variable res.
instead of the entire string which can contain other data, such as the
bus name.

Also adds an assert to ensure template is actually of type string which
makes mypy happy.
@swick swick force-pushed the wip/pytest-mypy-improvements branch from 404ff7e to 3668f12 Compare February 7, 2025 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

2 participants