test: add a python version of test-server #20706
Conversation
allisonkarlitskaya
added
the
no-test
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
2383815
to
913ab48
Compare
|
So, I think this PR is a bit misplaced. It seems like a good chunk of our qunit tests are aiming to test specific behaviours of The real reason I did this was to help me develop and test new features in the bridge and/or So: I think we should merge this soonish, but leave the existing C After that, I think a reasonable next step would be to try to use |
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
913ab48
to
692597d
Compare
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
692597d
to
8f954de
Compare
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
8f954de
to
551f3f7
Compare
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
551f3f7
to
5ea399d
Compare
allisonkarlitskaya
changed the title
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
5ea399d
to
8b13dbb
Compare
hmm. |
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
8b13dbb
to
6b8b56e
Compare
This is roughly a replacement for test-server. It's an outline for what a simple cockpit-ws in Python might look like (albeit without any of the necessary authentication code). It's already useful enough to run the majority of the existing QUnit, which means we can now run most unit tests without needing to build any C code.
allisonkarlitskaya
force-pushed
the
test-server.py
branch
from
6b8b56e
to
740518c
Compare
| @@ -0,0 +1,49 @@ | |||
| import os | |||
There was a problem hiding this comment.
Could this file move to test/pytest/ ? Would be a bit less clutter-y.
| ['dbus-daemon', f'--config-file={dbus_config}', '--print-pid'], stdout=subprocess.PIPE | ||
| ) | ||
| except FileNotFoundError: | ||
| yield None # no dbus-daemon? Don't patch. |
There was a problem hiding this comment.
IMO that should hard-fail with the FileNotFoundError. It's actively dangerous if you run an unit test on your production machine (which is common practice), and you don't have dbus-daemon installed (dbus-broker has long been the default in Fedora -- my system doesn't have it installed either).
A test which wants to use this won't work correctly without dbus-daemon -- it should either skip itself or handle the exception.
There was a problem hiding this comment.
I added this because it's missing from toxbox...
| return | ||
|
|
||
| pid = int(dbus_daemon.stdout) | ||
| os.environ['DBUS_SESSION_BUS_ADDRESS'] = dbus_addr |
There was a problem hiding this comment.
We never test the system bus in our tests? ok 😁
(Not actionable, just curious)
There was a problem hiding this comment.
We actually do test the system bus.
The story of this whole thing goes something like so:
- for the test-server cases we export these mock objects on the user's session bus
- but if we're running more than one copy of the tests then we're gonna get in trouble because we try to own the same name twice
- that includes the case where we run the tests in parallel under xdist (ie:
pytest -n) - so, solution: per-session mock session dbus
There's a wrinkle, though: we need to make sure that we set this environment variable before the first time that systemd_ctypes get used in the process, otherwise it will set up its "default user dbus" shared instance thing before this variable is set. That's why this lives in the toplevel, although it's not like anything in verify/ uses this either...
Some of the other tests already talk to the system D-Bus but don't do anything like owning names on it. I think I did that back in the day because the system bus was more reliably available in our various test environments.
| @@ -0,0 +1,492 @@ | |||
| # This file is part of Cockpit. | |||
There was a problem hiding this comment.
This has a __main__(), so needs a shebang. (Why didn't our battery of linters not flag that?) GH doesn't show if the file is executable.
There was a problem hiding this comment.
The intent is to run this like spy -m test.pytest.mockwebserver which will hit the __name__ == '__main__' case and doesn't require the #!. If you run it directly then it won't be able to find its relative imports, much less the bits of cockpit it imports.
This could use docs :)
| @@ -22,6 +24,14 @@ | |||
| 'base1/test-websocket.html', | |||
| } | |||
|
|
|||
| MOCK_WEBSERVER_XFAIL = { | |||
There was a problem hiding this comment.
This is really nice as a TODO list!
very very broken at present, but definitely enough to show promise...
Update: the rationale on this PR has changed a bit, so I'll write about the updated idea here. The main reason for that change is that in the course of getting various qunit tests working with the new server it became clear that part of what the tests are trying to exercise is cockpit-ws itself. That makes sense because although
test-serveris a separate binary, it uses quite a lot of the same internal implementation as the existingcockpit-ws, so the code that the two of them share gets exercised by the tests.Moving to a situation where we have a version of test-server in Python which we use to run the tests, but still run the C-based
cockpit-wsin production would reduce the meaningfulness of these tests.The core reason why I started this work remains the same though: after a bunch of recent hacking on
cockpit.js(andpkg/lib/cockpit/) I started to want a better developer experience for that. This is also why I developed the coverage stuff.Specifically: this work makes it possible to run qunit tests without needing to build
test-server: justvendor/checkout,./build.jsandpytest -k test-fsinfo.html. The new server is significantly faster to run, as it runs the bridge code in-process (reducing startup times).There are a few missing features (like missing dbus mock endpoints and so on) vs. the C-based test-server. 5 test files are marked as xfail when running under the new server. There's probably some low-hanging fruit there, but I wanted to get this merged up before it rots too much.