Add separate workflow for type checking

[elParaguayo]

This would be my preferred way for running our mypy-dependent tests. However, I've no idea why mypy is failing (before even running the specific tests) but our pre-commit script shows no mypy errors.

[jwijenbergh]

Pre-commit runs a different mypy version maybe?

[jwijenbergh]

Pre-commit runs a different mypy version maybe?

Nvm looks like the same version

[elParaguayo]

@jwijenbergh so, the problem is actually that our pre-commit hook isn't working properly. It's running in a virtual environment that doesn't have multiple packages installed (dbus-next, pywlroots etc) and it ignores the missing libraries error.

If we add these to the environment then we actually see the errors!

[tych0]

Ok, I pushed my patch to your branch, but I'm actually wondering if we shouldn't run qtile check in CI right now (at least on all the migrations, probably we should do some small simple config just to verify it works). My reasoning is that we end up with stack traces like this:

------------------------------------------ Captured stdout call ------------------------------------------
<test.migrate.conftest.SourceCode object at 0x7f25a35b9870>
Saved!
Finished. Backup files have not been deleted.
Checking Qtile config at: /tmp/tmpbqfuvj4c/config.py
Checking if config is valid python...
Traceback (most recent call last):
  File "/home/tycho/packages/qtile/libqtile/scripts/check.py", line 122, in check_config
    config.load()
  File "/home/tycho/packages/qtile/libqtile/confreader.py", line 134, in load
    config = importlib.import_module(name)
  File "/usr/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/tmp/tmpbqfuvj4c/config.py", line 3, in <module>
    qtile.spawn("alacritty")
AttributeError: '_UndefinedQtile' object has no attribute 'spawn'

for e.g. the tests in remove_command_prefix.py, after I add the obvious from libqtile import qtile which is missing. This is because when we import a config, it actually runs all the code.

We could go through and make all the configs not actually call qtile.spawn() like I did in my patch, but I'd rather land something quickly so we can get these tests passing for packaging folks, and it looked like a fair amount of work to change everything.

it makes me wonder though: what happens if people have naked qtile object accesses in their configs now? presumably they can't actually run qtile check either. I guess either nobody has these accesses, or nobody uses qtile check.

[elParaguayo]

That example above should not be run. The Change tests should just be confirming that the text is equal without running the code. Check tests are the ones that need valid import.

[tych0]

That example above should not be run. The Change tests should just be confirming that the text is equal without running the code. Check tests are the ones that need valid import.

I don't think this branch is enough, then. The at least one failure from #4868 was a Check test. I think we need tests of that file to pass (i.e. we have to run it in CI and make whatever it runs pass) so that it will work when people run the tests as part of packaging.

[elParaguayo]

The Change tests should be run as part of the normal test suite. The Check tests need mypy.

Could be a mistake in how I've set up the tests so I'll take a look later.

[tych0]

The change tests are all skipped because the normal environment doesn't ship mypy; when I added it in #4869 we started getting the same failures that packagers do.

[elParaguayo]

Then I need to fix that. Change tests shouldn't need mypy.

[tych0]

Maybe it's as simple as:

diff --git a/test/migrate/test_check_migrations.py b/test/migrate/test_check_migrations.py
index a66f773f..4b0fd336 100644
--- a/test/migrate/test_check_migrations.py
+++ b/test/migrate/test_check_migrations.py
@@ -19,9 +19,9 @@
 import pytest

 from libqtile.scripts.migrations import MIGRATIONS, load_migrations
-from test.test_check import have_mypy, is_cpython
+from test.test_check import is_cpython

-pytestmark = pytest.mark.skipif(not is_cpython() or not have_mypy(), reason="needs mypy")
+pytestmark = pytest.mark.skipif(not is_cpython(), reason="needs cpython")

 migration_tests = []

to reproduce, then?

[tych0]

but qtile check runs stubtest (which IIUC is the slow part), and that comes from mypy, right? So isn't it a real dependency?

[elParaguayo]

qtile check skips all type checking if mypy and/or stubtest aren't available. In that scenario, it essentially just checks for syntax errors.

I don't think qtile check touches the migrations.

[tych0]

qtile check skips all type checking if mypy and/or stubtest aren't available. In that scenario, it essentially just checks for syntax errors.

i see, ok, that's the bit i was missing.

[github-actions]

This PR is stale because it has been open 90 days with no activity. Remove the status: stale label or comment, or this will be closed in 30 days.