Add two-user integration tests for chat-module by at0m1x19 · Pull Request #24 · logos-co/logos-chat-module

at0m1x19 · 2026-05-05T21:36:27Z

Adds end-to-end integration test for two-user chat (test_two_users_chat) plus the integration-tests CI job.

tests/integration/:pytest harness (conftest.py, _helpers.py, test_two_users_chat.py) + fixtures (deterministic nodekey + chat configs with placeholder ENR resolved at runtime).
.github/workflows/ci.yml: new integration-tests job (ubuntu-only, needs: build-and-test, uploads container logs on failure).

…er removal; update CI to forward CLI output through pytest logs.

…o specific CLI rewrite SHA to support containerized daemons.

…t_arg` helper for consistent argument access.

…event handling.

… and remove redundant logic

fbarbu15

Thanks, left some comments

fbarbu15 · 2026-05-08T06:19:04Z

+
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.12"


since it's a new job, why not go with newer python ex 3.14?

Bumped to 3.14. Done together with logos-integration-test-framework (PR #4 there) which now covers 3.11/3.12/3.13/3.14.

fbarbu15 · 2026-05-08T06:19:48Z

+          # Once framework's pyproject.toml repins to a SHA that includes the
+          # `network=` kwarg, this whole block can become plain `pip install -r ...`.
+          pip install --no-deps -r requirements.txt
+          pip install pytest>=8.0


why not add pytest instead requirements.txt ?

Resolved by upstream re-pin: now that logos-integration-test-framework master tracks logoscore@master pip resolution is no longer in conflict.

The workaround (pip install --no-deps -r requirements.txt && pip install pytest>=8.0) is gone. CI now runs plain pip install -r requirements.txt.

fbarbu15 · 2026-05-08T06:39:19Z

@@ -0,0 +1,150 @@
+from __future__ import annotations


I would put this file into a dedicated libs/helper folder to not be mixed with other root files

fbarbu15 · 2026-05-08T06:40:10Z

+
+from _helpers import ChatUser, setup_chat_user
+
+FIXTURES_DIR = Path(__file__).parent / "fixtures"


can we move all constants to a separate/dedicated file?

and thus have all other constants in this file as well

Done — moved to tests/e2e/_constants.py

fbarbu15 · 2026-05-08T06:40:55Z

+    try:
+        (log_dir / f"{container_name}.log").write_text(_docker_logs(container_name))
+    except OSError:
+        pass


let's log a warning in this case

fbarbu15 · 2026-05-08T06:44:35Z

+    rest_host_port = pick_free_port()
+
+    cmd = [
+        "docker", "run", "-d", "--name", container_name,


any reason to not use python docker sdk?

subprocess.run(["docker", ...]) mirrors how LogoscoreDockerDaemon itself drives docker internally (also subprocess-based). Made it for consistency.

Also these docker calls are simple, SDK looks like overhead to me. It's easy to debug and we don't add one more dependency.

If calls become more complex we can switch any moment. WDYT?

yes, agreed

fbarbu15 · 2026-05-08T06:45:53Z

+    config_a = _read_fixture("chat-a.json").replace("__BOOTSTRAP_ENR__", nwaku_bootstrap)
+    config_b = _read_fixture("chat-b.json").replace("__BOOTSTRAP_ENR__", nwaku_bootstrap)
+
+    alice_name = f"logoscore-alice-{uuid.uuid4().hex[:8]}"


node_a and node_b or similar is better and more consistent

now it's saro/raya :)

daemon was chosen over node because LogoscoreDockerDaemon is a logoscore daemon process, while in our setup there's also a separate waku node. I thought it's a bit confusing.

fbarbu15 · 2026-05-08T06:54:31Z

+            a.client,
+            "newPrivateConversation",
+            b.intro_bundle,
+            hex_encode("hello-from-A-1"),


hello-from-A-1 should be a reusable var so you can assert against it at line 73

fbarbu15 · 2026-05-08T06:58:13Z

+        convo_id_a = extract_convo_id(
+            parse_json_field(wait_event(nc_a, "chatNewConversation", timeout=20.0), 0)
+        )
+        convo_id_b = extract_convo_id(


convo_id_b differs from convo_id_a ? I would expect them to match and in that case to add an extra assert convo_id_b == convo_id_a

They're different by design, libchat uses X3DH, where Initiator and Responder gets different local conversation ids

fbarbu15 · 2026-05-08T07:00:23Z

+        assert extract_message_content(parse_json_field(first_msg, 0)) == "hello-from-A-1"
+
+    # B → A reply
+    with subscribe(a.client, MODULE, "chatNewMessage") as nm_a:


those blocks b->a / a->b have a lot in common, could we refactor them into a reusable function to make the test shorter and more readable?

fbarbu15 · 2026-05-08T07:01:03Z

Thanks, left some comments

Also I see the CI job is failing, is there a bug / issue?

igor-sirotin

Noice, thank you!

igor-sirotin · 2026-05-08T20:16:42Z

+
+
+@pytest.fixture(scope="module")
+def chat_users(


This fixture looks very specific to test_two_users_can_chat. Like, if we'll want a test with 3 users and then with 5 users, we'll implement 2 more fixtures? 😄

I think such fixture should build 1 user only. And then alice/bob fixtures can be created ad-hoc.

igor-sirotin · 2026-05-08T20:20:58Z

+    alice_name = f"logoscore-alice-{uuid.uuid4().hex[:8]}"
+    bob_name = f"logoscore-bob-{uuid.uuid4().hex[:8]}"


Up to you, but we tend not use classic Alice/Bob/Charlie here, because it's not exactly the right case to use it. We use Saro/Raya/Pax: https://github.com/logos-messaging/specs/blob/master/informational/chat_cast.md

igor-sirotin · 2026-05-08T20:24:35Z

@@ -0,0 +1,139 @@
+# logos-chat-module integration tests


Wouldn't it not count as e2e tests, rather than integration? 🤔

igor-sirotin · 2026-05-08T20:25:24Z

+┌──────────── docker network "logoschat-it-<uuid>" ────────────┐
+│                                                              │
+│  ┌─────────────────┐   ┌──────────────┐   ┌──────────────┐   │
+│  │ nwaku-bootstrap │   │ logoscore-   │   │ logoscore-   │   │
+│  │ 172.30.0.10:60000   │ alice        │   │ bob          │   │
+│  │ deterministic   │   │ port=60002   │   │ port=60003   │   │
+│  │   ENR           │   │ staticPeers= │   │ staticPeers= │   │
+│  │                 │   │   [bootENR]  │   │   [bootENR]  │   │
+│  └─────────────────┘   └──────────────┘   └──────────────┘   │
+└──────────────────────────────────────────────────────────────┘
+            ▲                  │                  │
+            │     /vac/waku/relay/2.0.0  ─────────┘
+            │
+  pubsub: /waku/2/rs/2/1


suggestion: replace with a mermaid diagram? (up to you)

igor-sirotin · 2026-05-08T20:26:46Z

+    config_a = _read_fixture("chat-a.json").replace("__BOOTSTRAP_ENR__", nwaku_bootstrap)
+    config_b = _read_fixture("chat-b.json").replace("__BOOTSTRAP_ENR__", nwaku_bootstrap)


Why do we need static JSON files? Can we generate them on the fly?
This would be essential for further scaling of the tests

…and migrate to dynamic config generation

…accordingly in e2e tests

at0m1x19 · 2026-05-08T22:44:02Z

Thanks, left some comments

Also I see the CI job is failing, is there a bug / issue?

Was flaky cause of a regression in module-builder's codegen path. Should be fixed now.

…h `parse_event` and `parse_push_payload` for improved clarity and consistency.

…matting consistency.

…daemon version to prevent RPC/transport handshake issues.

…rtup arguments.

fbarbu15

LGTM, but I still see the CI job failing

…olve event-loop starvation issues.

at0m1x19 · 2026-05-13T18:17:24Z

Defer libchat-callback emits to fix initChat RPC hang on ubuntu-latest

This change defers every impl->emitEvent(...) call inside the twelve libchat C callbacks through a QMetaObject::invokeMethod(..., Qt::QueuedConnection) helper. A small QObject m_emitRouter member on ChatModuleImpl serves as the receiver so Qt drops pending queued metacalls when the impl is destroyed, preventing use-after-free on teardown.

Why this is needed. Every libchat callback is invoked synchronously from inside a libchat C API call (chat_new, chat_start, …) which itself is invoked synchronously from a LOGOS_METHOD on ChatModuleImpl. The plugin host's Q_INVOKABLE slot is still on the stack when the callback emits. While that synchronous chain runs the host thread cannot reach its next event-loop iteration — and that iteration is when QLocalSocket flushes the QRO reply packet back to the daemon. On slow runners (ubuntu-latest: 4 vCPU shared, x86_64), the flush starvation exceeds the caller's 20 s waitForFinished deadline and core_service.callModuleMethod returns METHOD_FAILED → exit 4.

Plugin semantics are unchanged: each LOGOS_METHOD still returns true synchronously, listeners still receive the result event — just a few milliseconds later, after the QRO dispatch stack has unwound.

Evidence.

Pre-fix on 87ea9ff: pytest ERRORs at setup_chat_user with exit 4 (MethodError) after ~20 s. The plugin's own [chat_module] ChatModuleImpl: Chat context created successfully log line is present at +5 ms — proving the plugin completed initChat and true was returned to the proxy. The reply never reaches the daemon's RemoteLogosObject::callMethod, which times out.
After this commit: test_two_users_can_chat PASSED in ~10.9 s on ubuntu-latest, matching local-Mac timings. Bisecting against test_basic_module_cpp.returnTrue (no re-entrant emit, otherwise identical CI environment) passes in ~1.1 s, confirming the QRO transport itself is healthy — the bug is the re-entrant emit pattern in this plugin.

Scope. This is a plugin-local mitigation. The same anti-pattern is latent in any module that emits Qt events synchronously from inside a Q_INVOKABLE-dispatched C callback; an SDK-side hardening is proposed at logos-co/logos-cpp-sdk#60 so that the workaround in deferredEmit here can be removed in a follow-up once the SDK fix lands.

igor-sirotin · 2026-05-20T15:03:34Z

@at0m1x19 is it ready to merge?

at0m1x19 · 2026-05-20T15:58:15Z

@at0m1x19 is it ready to merge?

@igor-sirotin it’s ready from my point of view. Before the merge, I think it’s worth taking a look at the latest commit together with this for someone who ones this repo.

Add two-user integration tests for chat-module

a0875de

at0m1x19 self-assigned this May 5, 2026

Egor Rachkovskii added 5 commits May 6, 2026 12:11

Reorder log-capture callbacks to ensure logs are saved before contain…

53b214a

…er removal; update CI to forward CLI output through pytest logs.

Improve diagnostics for module loading in integration tests; pin CI t…

8a4e18c

…o specific CLI rewrite SHA to support containerized daemons.

Refactor event payload handling in integration tests; introduce `even…

9de51cf

…t_arg` helper for consistent argument access.

Refactor integration tests to use wait_event helper for consistent …

33d877a

…event handling.

Simplify integration test setup and event handling; refactor comments…

1095a14

… and remove redundant logic

at0m1x19 requested review from fbarbu15, igor-sirotin, iurimatias and jazzz May 7, 2026 22:55

at0m1x19 marked this pull request as ready for review May 7, 2026 22:57

Remove outdated troubleshooting steps from integration test README.

3ab2020

fbarbu15 suggested changes May 8, 2026

View reviewed changes

Add interface field to metadata to indicate legacy compatibility

cddfec8

igor-sirotin approved these changes May 8, 2026

View reviewed changes

Egor Rachkovskii added 2 commits May 8, 2026 23:10

Rename integration tests to e2e tests; refactor fixtures, update CI, …

49da1ef

…and migrate to dynamic config generation

Move _helpers and _constants to libs directory; update imports …

414bd45

…accordingly in e2e tests

Egor Rachkovskii added 5 commits May 9, 2026 00:34

Merge remote-tracking branch 'origin/master' into add_integration_tests

e60af30

Refactor event payload handling in e2e tests: replace event_arg wit…

8312176

…h `parse_event` and `parse_push_payload` for improved clarity and consistency.

Refactor Chat module comments: align TODO annotations and improve for…

efba1e3

…matting consistency.

Update CI workflow: align logos-logoscore-cli pin with smoke-image …

c36248f

…daemon version to prevent RPC/transport handshake issues.

Enable SDK RPC trace in e2e tests by adding --verbose to daemon sta…

87ea9ff

…rtup arguments.

at0m1x19 force-pushed the add_integration_tests branch from 87ea9ff to 1095a14 Compare May 10, 2026 13:05

fbarbu15 approved these changes May 11, 2026

View reviewed changes

at0m1x19 force-pushed the add_integration_tests branch from d1fe25f to 87ea9ff Compare May 11, 2026 13:34

Refactor Chat module: use deferred emission for event dispatch to res…

e3a9078

…olve event-loop starvation issues.

at0m1x19 merged commit 9b22b52 into master May 26, 2026
3 checks passed

igor-sirotin deleted the add_integration_tests branch May 26, 2026 20:16

igor-sirotin mentioned this pull request May 28, 2026

experiment: drop deferredEmit, rely on cpp-sdk #68 #29

Draft


		from _helpers import ChatUser, setup_chat_user

		FIXTURES_DIR = Path(__file__).parent / "fixtures"

		alice_name = f"logoscore-alice-{uuid.uuid4().hex[:8]}"
		bob_name = f"logoscore-bob-{uuid.uuid4().hex[:8]}"

		config_a = _read_fixture("chat-a.json").replace("__BOOTSTRAP_ENR__", nwaku_bootstrap)
		config_b = _read_fixture("chat-b.json").replace("__BOOTSTRAP_ENR__", nwaku_bootstrap)



		@pytest.fixture(scope="module")
		def chat_users(

Conversation

at0m1x19 commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbarbu15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

at0m1x19 May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

at0m1x19 May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbarbu15 commented May 8, 2026

Uh oh!

igor-sirotin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

at0m1x19 commented May 5, 2026 •

edited

Loading

at0m1x19 May 8, 2026 •

edited

Loading

at0m1x19 May 8, 2026 •

edited

Loading