python: lots of linting/pytest/tox fixes #19054

allisonkarlitskaya · 2023-07-03T15:00:08Z

make pytest work on Python 3.6
add mypy to test/static-code and run it also on test/pytest
drop flake8, use pycodestyle for missing ruff rules
improve tox configuration

martinpitt

Nice work!

martinpitt · 2023-07-03T15:07:18Z

test/static-code

+    command -v pycodestyle >/dev/null || skip 'no pycodestyle'
+    find_python_files | xargs -r -0 pycodestyle --select=E1,E2,E3


I'm not that happy about this. Having .flake8 had the big advantage that vim ALE and friends used the same configuration as the tests. pycodestyle does not have such a facility.

Could this work with opting into that subset of rules with flake8 instead? Then we also won't need to change the dependency again in all our projects, tasks container, etc. There's nothing wrong with flake8 per-se, is it?

Note: flake8 implements the rules in question internally by using pycodestyle. The flake8 package depends on pycodestyle (and a whole lot of other stuff), so if you have flake8 you already have pycodestyle.

We could also add a setup.cfg with the pycodestyle config in it...

We could also add a setup.cfg with the pycodestyle config in it...

That'd be great! I'm aware of the flake8 → pycodestyle dependency, I'm just worried about a more declarative config.

test/static-code

src/cockpit/superuser.py

pyproject.toml

martinpitt · 2023-07-03T15:18:03Z

.github/workflows/python-venv.yaml

+    runs-on: ubuntu-latest
+    permissions: {}
+    container:
+      image: registry.fedoraproject.org/fedora:38


Hmm -- :latest ? I know it's more prone to "spontaneous" failures when Fedora updates, but (1) venv already isolates most of that, and (2) we won't forget to actually keep it up to date.

Sure. Happy to let this fail on us in the future if something changes, rather than forgetting that it exists.

martinpitt

Thanks! Looks good by and large, just wrangling with the container now.

test/pytest/conftest.py

martinpitt · 2023-07-04T04:39:29Z

.github/workflows/tox.yaml

+    runs-on: ubuntu-latest
+    permissions: {}
+    container:
+      image: quay.io/cockpit/tasks:latest


Could we rather use the unittests container? I'm a bit hesitant to pull tox into the tasks container, as it's so annoyingly big:

total download size: 147 M Installed size: 721 M

This also caused the failure.

if you move it into the unit-tests container, then the whole workflow should move to unit-tests.yaml as a new job, and also into unit-tests-refresh.yaml. FTR, I'm also fine with your previous approach of just apt installing it.

I've created https://github.com/allisonkarlitskaya/toxbox

jelly · 2023-07-04T08:02:14Z

test/static-code

+        for pkg in systemd_ctypes ferny bei; do
+            test -e "src/cockpit/_vendor/${pkg}/__init__.py" || skip "no ${pkg}"
+        done
+        mypy --no-error-summary src/cockpit test/pytest


Should we disable mypy caching iirc that gave some issues when running it separate, or is that solved or do we already do that in pyproject.toml

The issue was being caused by an oddity in systemd_ctypes. That's resolved now. I don't see any reason to keep the cache disabled, since it saves quite a bit of time...

jelly · 2023-07-04T08:08:20Z

pyproject.toml

 isolated_build = True
-envlist = site
+labels =
+  venv = py311-lint, py3{6,7,8,9,10,11,12}-pytest


Yay for testing on 3.12! \o/ Ideally we should remember to automatically add 3.13 etc.

I don't think this is possible, unfortunately :(

Spend a few minutes researching this and it seems that there indeed isn't a "py-latest".

Our current flake8 config overlaps our ruff config. Since we want to slowly move away from flake8, enable only the pycodestyle rules that we don't get from ruff. Once ruff implements the outstanding rules, we can drop flake8.

I'm not sure why I ever used this name, but the tool is called "vulture".

We're actually really close to having working mypy including the unit tests in test/pytest. The biggest change here is that we stopped using the return value of Peer.do_connect_transport() a while ago. Properly change that to be `None` everywhere.

We might bring this back some day in a different form, but linting definitely doesn't belong in the same directory as the unit tests.

We only use this from one place anymore, so let's just open-code it instead. This has a side-effect of unbreaking some unit tests on older Python versions.

This was our last remaining user of unittest.TestCase in test/pytest/. The unittest.IsolatedAsyncioTestCase API was added in Python 3.8, which clears one more hurdle for running unit tests against older Python versions. We continue to use unittest.mock instead of pytest-mock: it does what we need, causes us no problems, and is available in the standard library (instead of being an extra dependency that we'd need to install).

This feature isn't in any version of pytest-asyncio available for Python 3.6, so drop it from our config for now and add explicit decorators in the places where they were missing.

Having a side-effecting statement like this at the top-level of a random test file is very strange. pytest-asyncio gives us the ability to override the `event_loop` fixture, which not only gives us the ability to return a custom type of loop, but also essentially gives us the ability to write a co-routine automatically wrapping each individual async test case. That's powerful: we can manually create the loop without messing with global state, but we can also take this as an opportunity to do some of the same polyfills that systemd_ctypes.async_run() would do (which is even easier, given the `monkeypatch` fixture). We also take the chance to ensure that our code for tracking task and process leaks runs on every async testcase, instead of relying on our current ad-hoc approach of trying to remember to do it. This finds two process leaks (of bash) in the subprocess tests, so fix those up.

The tests are now running with all Python versions since 3.7.

It's now possible to run the test/pytest unittests under Python 3.6.

Copy the same sort of tox setup that systemd_ctypes is using, with some minor adjustments to testing dependencies and the like. The biggest change here is that instead of running each linter tool as a separate command, we run them all together via test/static-code. The main reason for that is that we have a lot of stand-alone executable Python scripts (chmod +x, and not ending in .py) that we also want to lint, and the linters don't know how to find those on their own. As in systemd_ctypes, typing `tox p` now gives a fairly nice experience. Note also: it's now possible to do a linter run with tools installed from PyPI using something like: tox -e py311-lint It's also possible to run pytest with older version of Python: pytest -e py36-pytest It's also possible to run all of the venv-based environments like: tox p -m venv

martinpitt

Thanks! The 3.6 compatibility test is really nice, and it allows us to drop our .github/workflows/pybridge-c8s.yml workflow (that can be a follow-up). I suppose we still can't run them in RHEL package builds due to missing python module dependencies, though?

test/pytest/conftest.py

martinpitt · 2023-07-04T12:47:44Z

.github/workflows/tox.yaml

+        uses: actions/checkout@v3
+
+      - name: Download toxbox
+        run: podman pull ghcr.io/allisonkarlitskaya/toxbox


Hmm, that container definition ought to be in cockpit.git containers/, or just be built on the fly. So that it can co-evolve, and not spontaneously break. But it's straightforward enough, and we can change it later if it causes problems.

Improve our code for detecting and displaying leaked processes. The display is improved by showing `ps f` output instead of pgrep output. That makes it easier to see the relationships of the processes as well as process state (zombie, etc.) Example output looks like this: test/pytest/conftest.py:48: Failed --------------------- Captured stdout teardown --------------------- PID TTY STAT TIME COMMAND 1477658 pts/7 S+ 0:00 /usr/bin/python3 -P /usr/bin/pytest 1477677 ? Zs 0:00 \_ [bash] <defunct> 1477799 pts/7 R+ 0:00 \_ ps f --pid=1477658 --ppid=1477658 ===================== short test summary info ====================== ERROR test/pytest/test_transport.py::TestSubprocessTransport::test_window_size - Failed: Some subprocesses still running! The detection is improved by using waitid(WNOWAIT) in order to detect leaked processes, instead of calling pgrep. This is more efficient, and avoids side-effects caused by spawning more subprocesses (ie: extra SIGCHLD handling). This found a bug, which we fix here. It also means that in the "working" case, we don't depend on external tools like `pgrep` or `ps` (although we use them in case of failures).

If the modules are already checked out, don't attempt to update them again. This doesn't work if the directory is read-only.

Run all of our python tests in all possible venvs on each PR.

allisonkarlitskaya · 2023-07-04T13:55:42Z

Thanks! The 3.6 compatibility test is really nice, and it allows us to drop our .github/workflows/pybridge-c8s.yml workflow (that can be a follow-up). I suppose we still can't run them in RHEL package builds due to missing python module dependencies, though?

I see nothing that would stop us from running this on RHEL, at least in a venv with the testing dependencies from PyPI...

martinpitt

🚀

allisonkarlitskaya requested review from jelly and martinpitt July 3, 2023 15:00

allisonkarlitskaya force-pushed the pycodestyle branch 3 times, most recently from 1a79edd to 8425108 Compare July 3, 2023 15:15

martinpitt requested changes Jul 3, 2023

View reviewed changes

allisonkarlitskaya force-pushed the pycodestyle branch 4 times, most recently from 502dfed to f6948a7 Compare July 3, 2023 18:00

martinpitt requested changes Jul 4, 2023

View reviewed changes

jelly reviewed Jul 4, 2023

View reviewed changes

allisonkarlitskaya force-pushed the pycodestyle branch 4 times, most recently from 68066b4 to a946072 Compare July 4, 2023 10:31

allisonkarlitskaya added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 4, 2023

allisonkarlitskaya force-pushed the pycodestyle branch 2 times, most recently from b8abf45 to a0d0cac Compare July 4, 2023 11:09

allisonkarlitskaya requested a review from martinpitt July 4, 2023 11:13

allisonkarlitskaya added 2 commits July 4, 2023 14:05

.flake8: limit to rules not covered by ruff

47e5971

Our current flake8 config overlaps our ruff config. Since we want to slowly move away from flake8, enable only the pycodestyle rules that we don't get from ruff. Once ruff implements the outstanding rules, we can drop flake8.

test/static-code: stop calling it pyvulture

e80f3ae

I'm not sure why I ever used this name, but the tool is called "vulture".

allisonkarlitskaya force-pushed the pycodestyle branch from a0d0cac to d462c82 Compare July 4, 2023 12:06

allisonkarlitskaya added 6 commits July 4, 2023 14:07

python: fix up various mypy issues

4941522

We're actually really close to having working mypy including the unit tests in test/pytest. The biggest change here is that we stopped using the return value of Peer.do_connect_transport() a while ago. Properly change that to be `None` everywhere.

test/static-code: enable mypy

7af6376

pytest: don't run linters anymore

79a1a8d

We might bring this back some day in a different form, but linting definitely doesn't belong in the same directory as the unit tests.

python: don't polyfill .send_fds()

ac11a3d

We only use this from one place anymore, so let's just open-code it instead. This has a side-effect of unbreaking some unit tests on older Python versions.

pytest: drop "auto" asyncio mode

048d805

This feature isn't in any version of pytest-asyncio available for Python 3.6, so drop it from our config for now and add explicit decorators in the places where they were missing.

allisonkarlitskaya added 4 commits July 4, 2023 14:07

test/pytest: various Python compatibility fixes

cb925a4

The tests are now running with all Python versions since 3.7.

pytest: make compatible with Python 3.6

02c39f1

It's now possible to run the test/pytest unittests under Python 3.6.

allisonkarlitskaya force-pushed the pycodestyle branch from d462c82 to 496abdd Compare July 4, 2023 12:07

martinpitt added the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Jul 4, 2023

martinpitt previously approved these changes Jul 4, 2023

View reviewed changes

allisonkarlitskaya added 3 commits July 4, 2023 15:51

modules/checkout: fix with read-only directories

1da01a6

If the modules are already checked out, don't attempt to update them again. This doesn't work if the directory is read-only.

.github/workflows: add a tox-venv test

d31f278

Run all of our python tests in all possible venvs on each PR.

allisonkarlitskaya dismissed martinpitt’s stale review via d31f278 July 4, 2023 13:51

allisonkarlitskaya force-pushed the pycodestyle branch from 496abdd to d31f278 Compare July 4, 2023 13:51

github-actions bot removed the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Jul 4, 2023

allisonkarlitskaya requested review from martinpitt and jelly July 4, 2023 13:54

martinpitt added the .github-changes Set by a reviewer just before landing to acknowledge that a PR changes github workflows label Jul 4, 2023

martinpitt approved these changes Jul 4, 2023

View reviewed changes

allisonkarlitskaya merged commit e830319 into main Jul 4, 2023
31 of 32 checks passed

allisonkarlitskaya deleted the pycodestyle branch July 4, 2023 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python: lots of linting/pytest/tox fixes #19054

python: lots of linting/pytest/tox fixes #19054

allisonkarlitskaya commented Jul 3, 2023

martinpitt left a comment

martinpitt Jul 3, 2023

allisonkarlitskaya Jul 3, 2023

martinpitt Jul 4, 2023

martinpitt Jul 3, 2023

allisonkarlitskaya Jul 3, 2023

martinpitt left a comment

martinpitt Jul 4, 2023

allisonkarlitskaya Jul 4, 2023

jelly Jul 4, 2023

allisonkarlitskaya Jul 4, 2023

jelly Jul 4, 2023

allisonkarlitskaya Jul 4, 2023

jelly Jul 4, 2023

martinpitt left a comment

martinpitt Jul 4, 2023

allisonkarlitskaya commented Jul 4, 2023

martinpitt left a comment

		command -v pycodestyle >/dev/null \|\| skip 'no pycodestyle'
		find_python_files \| xargs -r -0 pycodestyle --select=E1,E2,E3

python: lots of linting/pytest/tox fixes #19054

python: lots of linting/pytest/tox fixes #19054

Conversation

allisonkarlitskaya commented Jul 3, 2023

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jul 4, 2023

martinpitt left a comment

Choose a reason for hiding this comment