test: add a python version of test-server #20706

allisonkarlitskaya · 2024-07-04T05:54:26Z

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-server is a separate binary, it uses quite a lot of the same internal implementation as the existing cockpit-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-ws in 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 (and pkg/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: just vendor/checkout, ./build.js and pytest -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.

test/pytest/testserver.py

test/pytest/dbustest.py

test/pytest/testserver.py

allisonkarlitskaya · 2024-07-09T12:50:20Z

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 cockpit-ws, so running them under a new test-server.py would mean that they're no longer testing the code that's running in production (the very large chunks of shared code between cockpit-ws and test-server).

The real reason I did this was to help me develop and test new features in the bridge and/or pkg/lib without needing to ./autogen.sh && make && make test-server. For those purposes, this work is already far enough: the testcases like -file and -fsinfo which are clearly aiming to exercise bridge and cockpit.js functionality are working nicely.

So: I think we should merge this soonish, but leave the existing C test-server in place, as it is. We could maybe additionally run some subset of the tests under the Python server to make sure nothing decays, but the main purpose would be for use as a manual development tool.

After that, I think a reasonable next step would be to try to use test-server.py as a basis for a C-compilation-free Cockpit Client...

test/pytest/mockwebserver.py

test/pytest/webdriver_bidi.py

test/pytest/mockwebserver.py

test/pytest/asyncbridge.py

test/pytest/mockwebserver.py

allisonkarlitskaya · 2024-08-28T20:18:08Z

E FileNotFoundError: [Errno 2] No such file or directory: 'dbus-daemon'

hmm.

martinpitt

Amazing work, thanks! So now pytest-cov runs both scenarios.

The one thing that I'm missing is to document this in HACKING.md. Right now this still says "Run ./test-server". Can you please add that py-only alternative and how to invoke it?

martinpitt · 2024-08-29T07:03:39Z

test/conftest.py

@@ -0,0 +1,49 @@
+import os


Could this file move to test/pytest/ ? Would be a bit less clutter-y.

This is still left open, but the problem with moving it to test/pytest/ is that this fixture would also run when running normal tests as it has autouse=True

What are "normal" tests? So far this is only for the alternative python test server, so presumably it shouldn't even run when using the C test server? Or perhaps it should, and this should be an independent commit that reeingineers the private test server for both QUnit test scenarios?

martinpitt · 2024-08-29T07:05:50Z

test/conftest.py

+            ['dbus-daemon', f'--config-file={dbus_config}', '--print-pid'], stdout=subprocess.PIPE
+        )
+    except FileNotFoundError:
+        yield None  # no dbus-daemon?  Don't patch.


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.

I added this because it's missing from toxbox...

Should it instead warn then?

I still think it ought to hard-fail. Accidentally talking to the real user bus instead of the private test one can have dramatic consequences, if/once we write unit tests with "this is my playground" in mind. I think that so far we don't -- all unit tests run against the normal existing user bus, right? I don't see any dbus server launch anywhere in main, in fact I don't even know how they would work in github workflows, the only dbus-run-session that I see is in flatpak-test.yml (and there's nothing similar anywhere else).

It could perhaps also try dbus-broker?

Or perhaps just leave that out for the time being?

test/conftest.py

martinpitt · 2024-08-29T07:13:52Z

test/pytest/mockwebserver.py

@@ -0,0 +1,492 @@
+# This file is part of Cockpit.


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.

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 :)

test/pytest/test_browser.py

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.

jelly

This is probably a bit too nitpicky from my side, especially seeing how long this is open.

What I do wonder is, do we want to kill mock_info("pybridge") it seems useless as we no longer have a C bridge in main.

jelly · 2025-02-03T16:11:33Z

test/conftest.py

@@ -0,0 +1,49 @@
+import os


This is still left open, but the problem with moving it to test/pytest/ is that this fixture would also run when running normal tests as it has autouse=True

jelly · 2025-02-03T16:11:53Z

test/conftest.py

+            ['dbus-daemon', f'--config-file={dbus_config}', '--print-pid'], stdout=subprocess.PIPE
+        )
+    except FileNotFoundError:
+        yield None  # no dbus-daemon?  Don't patch.


Should it instead warn then?

jelly · 2025-02-03T16:25:30Z

test/pytest/mockdbusservice.py

+
+    @systemd_ctypes.bus.Interface.Method('', 'i')
+    def request_signal_emission(self, which_one: int) -> None:
+        del which_one


what purpose does this have? There isn't anything consuming which_one as far as I can see.

And src/ws/mock-service.c also only returns a test_signal if which_one == 0, so technically the test harness behaviour changed

del of a function parameter is a convention for indicating to a linter that the variable is intentionally unused. it's like naming the argument with a _ at the start, except that it keeps the name of the parameter unmodified in case it gets passed as as kwarg (like which_one=...).

jelly · 2025-02-03T16:31:02Z

test/pytest/mockwebserver.py

+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+# https://github.com/astral-sh/ruff/issues/10980#issuecomment-2219615329


Nothing happened here sadly.

test/pytest/mockwebserver.py

jelly · 2025-02-03T16:41:09Z

test/pytest/mockwebserver.py

+@routes.get(r'/mock/info')
+async def mock_info(_request: web.Request) -> web.Response:
+    return web.json_response({
+        'pybridge': True,


Dumb unrelated question, why do we still have this? We only have a pybridge

Right, I sent #21583 to clean it up (that's the level of engineering I'm capable of today..)

jelly · 2025-02-03T16:46:18Z

test/pytest/mockwebserver.py

+async def mock_info(_request: web.Request) -> web.Response:
+    return web.json_response({
+        'pybridge': True,
+        'skip_slow_tests': 'COCKPIT_SKIP_SLOW_TESTS' in os.environ


This is used in one place and also confusing:

if (await mock_info("skip_slow_tests")) { assert.ok(true, "skipping on python bridge, not implemented"); return; }

The comment seems wildly inaccurate, the test is a bit slow but it finished in 10 seconds so imo not that bad.

> info: # test: parallel stress test Array(0) > warn: Test "parallel stress test" took longer than 3000ms, but no timeout was set. Set QUnit.config.testTimeout or call assert.timeout() to avoid a timeout in QUnit 3. ht tps://qunitjs.com/api/config/testTimeout/

One small issue

Fixed the timeout issue in https://github.com/cockpit-project/cockpit/compare/main...jelly:qunit-timeout-deprecated?expand=1

This applies to running under either valgrind or qemu software emulated builders.

jelly · 2025-02-03T16:51:32Z

test/pytest/mockwebserver.py

+
+@routes.get('/')
+async def index(_request: web.Request) -> web.Response:
+    cases = Path('qunit').rglob('test-*.html')


Hmm, am I silly? There is no qunit path? So I am a bit confused what this lists.

Like it's mostly under base1?

esbuild puts the built bundles of pkg/base1/test* into qunit//test.

jelly · 2025-02-03T16:53:58Z

test/pytest/mockwebserver.py

+
+
+async def main() -> None:
+    parser = argparse.ArgumentParser(description="Serve a single git repository via HTTP")


🤔 That could use an update :-)

Oops. Thanks for the catch.

jelly · 2025-02-03T16:57:20Z

test/pytest/test_browser.py

+    'base1/test-external.html',
+    'base1/test-http.html',
+    'base1/test-stream.html',
+    'shell/machines/test-machines.html',


So test-machines has cockpit.dbus(null, { bus: 'internal' }) I guess that's the system bus?

No. That's our internal fake D-Bus inside of the bridge. We have some internal endpoints like configuration, machines, etc. that we publish via D-Bus for convenience, rather than making them their own channel types.

martinpitt

There's still a lot left from my previous review, but I'd first like to understand where this is going.

It seems to me that what we don't want is a complete implementation of a web server which is just being used for unit tests, that feels like a dead end. As you said, much of this tests the actual cockpit ws functionality, so this would only make sense if this eventually evolves into the web server. But we don't have a design/architecture for that yet (see COCKPIT-1238 and in particular COCKPIT-1239), and also we can't use aiohttp for that. So much like test-server, it would make sense if the Python version was an extension/derivative of the official web server which then implements all the mock paths.

So perhaps it'd be prundent to queue this until after COCKPIT-1239?

martinpitt · 2025-02-04T08:07:39Z

HACKING.md


    ./test-server

 which will output a URL to connect to with a browser, such as
 <http://localhost:8765/qunit/base1/test-dbus.html>. Adjust the path for different
-tests and inspect the results there.
+tests and inspect the results there.  Although the QUnit tests are mostly about testing the client side API, they also indirectly test the webserver, and test-server is written using many of the same components as cockpit-ws.


Overly long line

martinpitt · 2025-02-04T08:10:56Z

test/conftest.py

+            ['dbus-daemon', f'--config-file={dbus_config}', '--print-pid'], stdout=subprocess.PIPE
+        )
+    except FileNotFoundError:
+        yield None  # no dbus-daemon?  Don't patch.


I still think it ought to hard-fail. Accidentally talking to the real user bus instead of the private test one can have dramatic consequences, if/once we write unit tests with "this is my playground" in mind. I think that so far we don't -- all unit tests run against the normal existing user bus, right? I don't see any dbus server launch anywhere in main, in fact I don't even know how they would work in github workflows, the only dbus-run-session that I see is in flatpak-test.yml (and there's nothing similar anywhere else).

It could perhaps also try dbus-broker?

Or perhaps just leave that out for the time being?

martinpitt · 2025-02-04T08:14:54Z

test/conftest.py

@@ -0,0 +1,49 @@
+import os


What are "normal" tests? So far this is only for the alternative python test server, so presumably it shouldn't even run when using the C test server? Or perhaps it should, and this should be an independent commit that reeingineers the private test server for both QUnit test scenarios?

martinpitt · 2025-02-04T08:24:55Z

test/pytest/mockwebserver.py

+import aiohttp
+from aiohttp import WSCloseCode, web


As discussed in our 1-1 yesterday and in https://issues.redhat.com/browse/COCKPIT-1238: for the top-level web server we can't use aiohttp as that doesn't support HTTP 2/3. The most prominent candidate would be fastapi or the underlying starlette. I wouldn't want our future cockpit-ws package to end up depending on and being written in two different frameworks.

It's probably easier to port it now while it's in its infancy than once we actually start rolling it out.

I'm not yet clear where this is going, though, I'll write something in the top-level review. This might be a follow-up.

martinpitt · 2025-02-04T08:26:21Z

test/pytest/mockwebserver.py

+
+@routes.get('/')
+async def index(_request: web.Request) -> web.Response:
+    cases = Path('qunit').rglob('test-*.html')


esbuild puts the built bundles of pkg/base1/test* into qunit//test.

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

github-advanced-security bot found potential problems Jul 4, 2024

View reviewed changes

test/pytest/testserver.py Fixed Show fixed Hide fixed

test/pytest/dbustest.py Fixed Show fixed Hide fixed

allisonkarlitskaya force-pushed the test-server.py branch from 2383815 to 913ab48 Compare July 8, 2024 14:21

github-advanced-security bot found potential problems Jul 8, 2024

View reviewed changes

test/pytest/testserver.py Fixed Show fixed Hide fixed

allisonkarlitskaya force-pushed the test-server.py branch from 913ab48 to 692597d Compare July 22, 2024 12:39

github-advanced-security bot found potential problems Jul 22, 2024

View reviewed changes

test/pytest/mockwebserver.py Dismissed Show resolved Hide resolved

test/pytest/mockwebserver.py Fixed Show fixed Hide fixed

test/pytest/webdriver_bidi.py Fixed Show fixed Hide fixed

allisonkarlitskaya force-pushed the test-server.py branch from 692597d to 8f954de Compare July 22, 2024 14:13

github-advanced-security bot found potential problems Jul 22, 2024

View reviewed changes

test/pytest/mockwebserver.py Fixed Show fixed Hide fixed

allisonkarlitskaya force-pushed the test-server.py branch from 8f954de to 551f3f7 Compare July 22, 2024 17:16

github-advanced-security bot found potential problems Jul 22, 2024

View reviewed changes

test/pytest/asyncbridge.py Fixed Show fixed Hide fixed

martinpitt mentioned this pull request Aug 10, 2024

test: Port from CDP to BiDi browser automation #20832

Merged

21 tasks

allisonkarlitskaya force-pushed the test-server.py branch from 551f3f7 to 5ea399d Compare August 27, 2024 15:36

allisonkarlitskaya requested a review from martinpitt August 27, 2024 15:36

allisonkarlitskaya changed the title ~~wip: port test-server to python~~ test: add a python version of test-server Aug 27, 2024

allisonkarlitskaya force-pushed the test-server.py branch from 5ea399d to 8b13dbb Compare August 28, 2024 19:55

github-advanced-security bot found potential problems Aug 28, 2024

View reviewed changes

test/pytest/mockwebserver.py Fixed Show resolved Hide resolved

test/pytest/mockwebserver.py Fixed Show fixed Hide fixed

allisonkarlitskaya force-pushed the test-server.py branch 2 times, most recently from 6b8b56e to 740518c Compare August 28, 2024 20:44

martinpitt requested changes Aug 29, 2024

View reviewed changes

allisonkarlitskaya force-pushed the test-server.py branch from 740518c to f8b4a22 Compare February 1, 2025 17:40

allisonkarlitskaya marked this pull request as ready for review February 1, 2025 17:46

allisonkarlitskaya requested review from jelly and martinpitt February 1, 2025 17:47

jelly reviewed Feb 3, 2025

View reviewed changes

martinpitt requested changes Feb 4, 2025

View reviewed changes

allisonkarlitskaya mentioned this pull request Feb 6, 2025

spike: Investigate Web Service Workers #21593

Open



		async def main() -> None:
		parser = argparse.ArgumentParser(description="Serve a single git repository via HTTP")

test: add a python version of test-server #20706

Are you sure you want to change the base?

test: add a python version of test-server #20706

Conversation

allisonkarlitskaya commented Jul 4, 2024 • edited Loading

allisonkarlitskaya commented Jul 9, 2024

allisonkarlitskaya commented Aug 28, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly 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

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

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

allisonkarlitskaya commented Jul 4, 2024 •

edited

Loading