Move our list of static files out of test/static-code (which is shared
with other projects). Add testlib to that list.
This reduces the cost of running mypy on a warm cache from 20s to ~0.3s,
while expanding checking to every single Python file in the source tree.
A overall `test/static-code` run (minus `node_modules`) decreases from
~24s to ~4s. The majority of that time is now in the json validity
checking rule (since it runs a separate python invocation for each
file).
Now that we gate our ruff updates via the tasks container, we can afford
to be a bit more future-oriented here.
This brings in a nice raft of fix-ups, including my personal favourite:
RUF003 Comment contains ambiguous `µ` (MICRO SIGN). Did you mean `μ` (GREEK SMALL LETTER MU)?
Sure... why not?
The majority of fixes here are from a single rule suggesting to rewrite
a and b or c
as
(a and b) or c
But in most (but not all) cases in our code we actually want to write
that as
b if a else c
I've also added some type annotations to help me make the right choice
in places where it wasn't immediately clear.
Lacking a proper requirements.txt file, we're at the mercy of whatever
got randomly uploaded to pypy last night, and we've been seeing a lot of
breaks lately.
We already run our ruff/mypy/etc. stuff as part of our unit tests
workflow, so let's stop running it in venv. The idea is that we should
pass against the current version of the tasks container (which will
probably shift at some point to being our pinned version of the tasks
container).
While we're at it, drop Python 3.7 (which the world seems to have
collectively decided to stop caring about) and add Python 3.13 (which
has been in Fedora 39 for a while already). Note: we do *not* drop
Python 3.6 — although it is no longer an officially supported release,
it's used in RHEL 7 and RHEL 8 and Fedora intends to keep packaging it.
New ruff warns to stderr that the old way is deprecated. That's enough
to convince test/static-code that an error is being reported.
Let's move this over.
Rework the packages loading code. The main changes:
- we don't attempt to do checksums anymore. This was taking a large
amount of time when loading the bridge and we never implemented it
properly anyway (since we weren't sending the `.checksum` fields in
the manifests)
- instead, we do scan the list of files in the package one time (at
first use, not load) and build our map of URL paths to filenames at
this point. This allows us to retain our efficient lookup code, but
requires us to do the actual file loading at the time of the request.
Because we're no longer storing the contents of the files in memory,
this is a substantial runtime memory usage reduction.
- drop all of the extra code we had for walking the paths in a
particular order. We just do (r)globs now.
- move all of the code for deciding which packages to load to a
separate class. We now load all of the manifests and evaluate them
(requirements, conditionals, priorities) and only the packages that
we actually intend to serve are then scanned. The new structure also
makes it easier for cockpit-beiboot to do its thing.
- the packages loading code now uses the actual cockpit version number
for requires comparisons instead of the previously hardcoded '300'.
Add an extra check if our version number is `0` (ie: running out of
git) and disable version checking in that case.
- clean up the relationship between `cockpit.packages` and
`cockpit.channels.packages`. Previously, `cockpit.packages` would
access properties on the channel object and call methods on it to
serve the content back. Now the channel requests the data from
`cockpit.packages` (which merely returns the result).
- enabled by the above: full typing throughout, and mypy is happy. We
have one tiny little thorn in that the packages channel is not
strictly capable of knowing that the router to which it's connected
has a `packages` attribute, but that's nothing that we weren't doing
already. Add a comment to draw attention to it.
- to the extent possible, we try to keep the state of the packages
channel away from the packages code proper. This led to an overhaul
of our `Content-Security-Policy` not to include the origin URLs in
the policy output. This is redundant anyway, since that's what
"'self'" is for. We do need to do one hack for websockets though,
until we can convince ourselves about browser support for the
standard. This hack is lifted to the channel level. Adjust tests
accordingly.
- with some small changes to our pyproject.toml, the two rewritten
files (`packages.py` and `channels/packages.py`) are now also passing
pylint, but we don't enable that yet, since everything else is
broken.
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
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.
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.
Add a conftest.py file to test/verify with a hook that allows us to load
our `check-*` files. This is a little bit complicated because the `-`
in the name and the lack of `.py` means that they're not recognized as
normal python modules, so we need to do some hacks (similar to the ones
we do in test/common/run-tests).
With this in place, it's now possible to run the integration test suite
via pytest with something like the following:
$ test/common/pixel-tests pull
$ test/image-prepare
$ test/common/pywrap -m pytest test/verify -n4
Note: the -n option is for specifying the level of parallelism and
requires the python3-pytest-xdist plugin to be installed.
We adjust our vulture config a bit to teach it about pytest plugins.
beipack prefers if python files don't contain `'''` because it uses this
for its own escaping of the file content. There's no ruff rule to
blanket-forbid the use of `'''` but all of the cases we have of `'''` in
src/cockpit happen to be docstrings, and there's a rule for that, so
let's turn it on.
This results in a lot of hints in docstrings in a lot of hits in other
files. I've done a blanket
sed -i s/\'\'\'/\"\"\"/g $(git grep -l "'''" src test tools)
and unsurprisingly, this hasn't caused any problems.
This gives mypy a bit of help in finding our testing API. Mostly, it
helps get rid of a lot of warnings like these in vim:
# E: Cannot find implementation or library stub for module
Without the parent hacks we can just treat the bots and testlib stuff as
normal third party imports.
This causes one small problem: ruff defaults to auto-detect files in the
current directory as first-party imports, which affects a couple of
imports in test/common/run-tests. Those are only detected first-party
if the files are present when we run ruff, which is not the case for
incremental changes checked by test/static-code when used as part of a
git hook. This wasn't a problem before, because we hardcoded the
modules. Disable ruff's automatic import checking (at the problem
toplevel) to avoid that problem in this case and potential future cases.
Instead of running ruff on *.py, with a large number of exemptions (all
of test/verify, test/common, tools/, etc) we now run it on all of the
Python files that test/static-code can find.
Previous commits have done some amount of code cleanups on the
newly-exposed code, but we deal with most remaining issues by using
per-directory ruff.toml files which let us adjust the configuration in a
subdirectory. We make sure to add those to test/static-code so that
they will be available for partial linting during hooks.
Add "FBT" to our ruff config, which forbids use of booleans as
positional arguments in function definitions and calls.
Do some clean-ups for that.
One little bit of Python syntax:
def func(x, *, y=None):
...
means that the following arguments can not be given positionally, and
must always by specified by name, like `y=value`.
Theoretically, we want to separate things into five buckets (stdlib,
third-party, vendored, cockpit, local-folder) but we also want to take a
more or less stock isort behaviour, so we do this:
In our testcases, we want the "local folder" bucket to refer to the mock
utility classes in the pytest/ directory, while first-party refers to
cockpit. Add __init__.py to test/ and test/pytest/ to ensure that we can
do relative imports (which we should have been doing already).
In src/ we make sure to always refer to our vendored packages by their
full name (`cockpit._vendor.*`) and to do relative imports from cockpit
itself. That ends up putting our vendored packages in the first-party
bucket and cockpit into local-folder, which is approximately correct.
The only issue here is that if we directly include vendored packages
from our testcases, they end up in the same bucket with cockpit itself.
That's not a big problem.
This enables checking for use of variable names which shadow builtins.
The check for class attributes is not reasonable (since it's almost
impossible to refer to class attributes as bare words), so disable it.
Drop our dependency on setuptools and write our own PEP 517 build
backend. This turns out to be relatively straightforward — only ~100
lines, and avoids the workarounds we had to do to prevent setuptools
from trying to write to the source tree. It's also dramatically
faster.
While we're at it, we can now fix a long-standing nag: we now have a
proper place to make sure that `modules/checkout` gets called before
building an sdist or a wheel. That means that you can run `tox` or `pip
install .` in a freshly checked-out tree and expect correct results.
We have to add a command-line interface to support wheel creation on
RHEL8, where pip is not yet PEP517-aware.
We don't implement any optional features of PEP 517 and we also don't
support editable installs.
The way we've been using vulture is pretty ineffectual. Cranking the
min-confidence all the way to 100 is skipping a lot of potentially
useful messages. We've had to do that because vulture does
whole-program analysis and we run test/static-code from our post-common
hook only on the files that changed.
Let's move our vulture configuration to pyproject.toml, removing the
minimum confidence level while we're at it, so we get a lot more
suggestions. We have to add some suppressions to deal with a lot of
those, but that's how the tool is intended to be used.
In order to avoid running vulture in the post-commit hook (where it only
has access to a subset of files) use an environment variable to signal
test/static-code that we're in this situation an skip vulture in that
case.
We effectively have two sets of configuration for vulture now:
- what happens when you run `vulture`: this basically runs on `src/`
and `test/pytest`, excluding the integration tests and testlib. We
do this because by default vulture only looks at *.py files, and our
integration tests are executable scripts without extensions. That
would lead vulture to falsely conclude that most of testlib is
unused. This is also how vulture will run from ALE, if enabled.
- when happens when you run `test-static/code`: in this case, we do
more aggressively scanning for python files based on #!-lines. In
this case we scan all files (except for fmf_metadata) which means
that the testlib appears (correctly) well-used.
Make a bunch of pure-whitespace changes to get us under the maximum line
length of 118. Modify the value for ruff in pyproject.toml.
For some of these cases, particularly in some of the long function call
signatures in mocktransport, I let black do its thing. It strongly
favours hanging indent for long argument lists and produced output like
this:
def function(
arg1: type,
arg2: type,
arg3: type,
) -> type:
body
I'm not super thrilled about that, but it has precedence in PEP-8.
https://github.com/psf/black/issues/1178 has some (heated) discussion
about the exact indentation level, but nobody seems to prefer visual
indent to hanging indent in this case.
Let's let it be.
Add some initial ruff configuration to pyproject.toml and install ruff
in the unit-tests container (via pypi, unfortunately). Add a hook to
pytest to run ruff, if it's available.
The initial configuration is chosen to avoid having errors straight
away. Fixes will follow.
Add ruff to test/static-code and make sure we send pyproject.toml into
the tmpdir when running from the post-commit (and pre-push) hooks.
ruff is still very much in development and it's missing a lot of
common-sense checks. We'll keep flake8 as well, for the time being.
The version of setuptools required to support pyproject.toml isn't
available in any current version of RHEL, so let's take a step back for
now and port our config to setup.cfg.
This lets us drop our setuptools dependency to 39.2.0, which is,
conveniently, the version available in RHEL8 as well as the minimum
version that supports dynamic version numbers (which we will soon use).
While we're at it, switch our package-finding logic to the
fully-automatic form, adding an __init__.py to cockpit.data to make sure
it gets included.
Include the test files in the sdist (and therefore SRPM).
This is how RPM expects to run our tests.
Use the setup from systemd_ctypes. The default test environment (site)
will use the system environment and install no additional depends.
Manually specifying an environment (like `-e py311`) will create a
virtual environment and install the correct dependencies.
The rpm macros seem to use the py311 environment name by default both
for collecting build dependencies (to turn them into RPM package
depends) and for running the tests themselves, so this works well.
This also allows us to easily run the tests under old Python versions
(like `tox -e py37`). Note, however, that we can't test Python 3.6
because we depend on a version of setuptools that's not available there.
Our PeerStateListener and SuperuserStartup (and DBusStartup and
ControlMessageStartup) objects had a strange and overlapping role which
was basically two things:
- notification of completion of a task, with possible notification of
failure, including reporting of exceptions
- an authentication hook
The first thing is basically something like a Future, which is
equivalent to an asynchronous function call. In some places, we were
even bridging this gap, such as via DBusStartup creating a future for
the (async) D-Bus call to wait on.
Let's push things away from these custom types and towards a more
unified approach of asynchronous function calls for startup of peers.
Failures are reported via exceptions and completion is reported by
returning.
For the authentication hook usecase, we can use the existing
ferny.InteractionResponder interface. It also works as an async
method call, so add some async helpers to CockpitServerProtocol for
sending/receiving authorize challenge/responses asynchronously.
Drop our own custom askpass code and use ferny's. Also drop all
handling of authorize control messages from Peer — this all happens via
the ferny InteractionAgent now. Our "pseudo" test helper needs a
rewrite to deal with that.
We also drop our send-stderr control message solution that we copied
from the C bridge and replace it with a solution which passes the
original stderr over the redirected stderr from the ferny agent.
Add a bunch of unit tests to check that peers behave properly in various
situations.
Instead of shipping our copy of systemd_ctypes as a top-level package
which might conflict with an independently-installed version, ship it as
a package called cockpit._vendor.systemd_ctypes.
Like the C bridge send a proper formatted `fail.html` back to the
client. This does not handle anything any other then 404's.
Co-authored-by: Jelle van der Waa <jvanderwaa@redhat.com>
Since we dropped the Makefile rule for creating cockpit-bridge.pyz, the
pybridge scenario has been broken.
Add some minimal pyproject.toml metadata required to build a wheel of
the Python package. Build this wheel on the outside, and inject it into
the VM via image-customize.
Note: we no longer bother with building the C parts of the package
anymore — we rely on the distro packages already present in the image.
Add a new testcase in test/pytest/test_server.py which knows how to run
the QUnit tests, including collection of coverage information.
Make sure we build the `test-server` and the webpacks before running
`pytest` from the Makefile. For running `pytest` directly, the html
tests will be skipped if the test-server is missing or if there are no
`dist/*/test-*.html` files.
Remove the `make pycheck` target, and drop the extra scenario from the
unit-tests workflow.
`pytest` now takes significantly longer to run. The pytest-xdist
plugin can help with this, and it works in combination with our other
plugins, pytest-timeout and pytest-cov.
For manual invocation of pytest, use something like the following:
pytest -n auto --cov --timeout 30
We run the tests non-parallel in the unit-tests container because
there's no real advantage to speeding this up in that case, and running
in parallel makes the log output more difficult to read.
This is a leftover from when we were considering using tox to drive the
pytest tests. Without a build system (which was removed during the
review process), this won't work anyway.
Enable branch coverage.
Skip displaying files with 100% coverage in the report.
Add the '# pragma: no cover' pattern to our excludes list. The docs
say:
Note that when using the `exclude_lines` option in a configuration
file, you are taking control of the entire list of regexes, so you
need to re-specify the default “pragma: no cover” match if you still
want it to apply.
Make a few other tweaks to the transports code to improve coverage,
turning some `if` statements into `assert`s, for example. We also don't
try to observe BlockingIOError directly after polling as ready to read
or write — that's a difficult case to trigger.
Allison Karlitskaya