Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ jobs:
- "3.11"
- "3.14"
runs-on: ubuntu-latest
services:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of attaching the emqx service container directly to the main test matrix job (which pulls the Docker image and waits for healthchecks on every Python version in the matrix), we could isolate this:

Two great options here (both will run in parallel):

  1. A separate parallel job in ci.yml (e.g. test-mqtt with the emqx service running on a single Python version). Top-level jobs run in parallel by default, so it won't add any sequential wait time to the main test suite while keeping the matrix lean.
  2. A separate workflow file (e.g. .github/workflows/e2e.yml or mqtt.yml). This cleanly decouples broker E2E testing from the core unit test workflow and can optionally use path filtering (e.g. running when roborock/mqtt/** or tests/e2e/** are touched).

Either approach avoids spinning up duplicate EMQX containers across the Python matrix and clearly isolates broker infrastructure issues from unit test failures.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for separating broker tests? Why should we not test each Python version here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its fine to test multiple python versions still.

Some tests depend on the service, and other tests do not, so the idea would be to spin up the service and run the tests that do use it separate from the tests that do not use it.

emqx:
image: emqx/emqx:6.2.3
ports:
- 1888:1883
options: >-
--health-cmd "/opt/emqx/bin/emqx ctl status"
--health-interval 5s
--health-timeout 25s
--health-retries 5
steps:
- uses: actions/checkout@v6
- name: Set up uv
Expand Down
8 changes: 7 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,17 @@ pre-commit run --all-files

We use `pytest` for testing. Please ensure all tests pass and add new tests for your changes.

MQTT tests require a real EMQX broker at `127.0.0.1:1888`. Start it locally with

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep pytest (or uv run pytest) as the standard instruction for general testing, and document Docker Compose specifically under the pytest -m mqtt_broker section for running the real-broker test suite.

Docker Compose before running the tests. CI provides the broker as a GitHub Actions service.

```bash
# Run tests
docker compose up -d --wait
pytest
docker compose down
```

To run only the real broker tests, use `pytest -m mqtt_broker`.

## Pull Requests

1. **Create a branch** for your changes.
Expand Down
10 changes: 10 additions & 0 deletions compose.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
services:
emqx:
image: emqx/emqx:6.2.3
ports:
- "127.0.0.1:1888:1883"
healthcheck:
test: ["CMD", "/opt/emqx/bin/emqx", "ctl", "status"]
interval: 5s
timeout: 25s
retries: 5
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ dev = [
"syrupy>=4.9.1,<6",
"pdoc>=15.0.4,<17",
"pytest-cov>=7.0.0",
"zmqtt>=0.2.0,<0.3.0",
]

[tool.hatch.build.targets.sdist]
Expand Down Expand Up @@ -120,6 +121,7 @@ module = ["roborock.map.proto.*"]
ignore_errors = true

[tool.pytest.ini_options]
markers = ["mqtt_broker: tests requiring a real MQTT broker"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mqtt_broker marker is a great way to tag these tests.

To keep the default developer experience frictionless when running uv run pytest locally without Docker, we should:

  1. Configure addopts = "-m 'not mqtt_broker'" in pyproject.toml so standard pytest runs the unit tests, and pytest -m mqtt_broker targets this broker suite.
  2. Ensure all connection and message waits in these tests have tight, bounded timeouts so they fail fast with clear diagnostics if the broker is unresponsive.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The default pytest run may work fine locally, but it fails in CI.
  2. The pytest default timeout of 30 seconds is applied to each test.

Contributing guide contains tests setup guide

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1 I would assume on CI we'd still run them (e.g. with an additional pytest command to run the tests.

asyncio_mode = "auto"
asyncio_default_fixture_loop_scope = "function"
timeout = 30
Expand Down
240 changes: 240 additions & 0 deletions tests/e2e/test_mqtt_broker.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
"""Test the MQTT session and channel over TCP with a real broker."""

import asyncio
import json
from collections.abc import AsyncGenerator
from uuid import uuid4

import pytest
from zmqtt import MQTTClientV5, QoS, ReconnectConfig, create_client

from roborock.data import UserData
from roborock.devices.transport.mqtt_channel import MqttChannel
from roborock.mqtt.roborock_session import create_lazy_mqtt_session, create_mqtt_session
from roborock.mqtt.session import MqttParams, MqttQos, MqttSession, MqttSessionException
from roborock.protocol import MessageParser
from roborock.roborock_message import RoborockMessage, RoborockMessageProtocol
from tests.mock_data import LOCAL_KEY, USER_DATA

pytestmark = pytest.mark.mqtt_broker


@pytest.fixture(name="mqtt_params")
def mqtt_params_fixture() -> MqttParams:
"""Use the broker exposed by Docker Compose or GitHub Actions."""
return MqttParams(
host="127.0.0.1",
port=1888,
tls=False,
username="username",
password="password",
timeout=5.0,
)


@pytest.fixture(name="session")
async def session_fixture(mqtt_params: MqttParams) -> AsyncGenerator[MqttSession, None]:
"""Create and close the production MQTT session."""
session = await create_mqtt_session(mqtt_params)
try:
assert session.connected
yield session
finally:
await session.close()


@pytest.fixture(name="peer")
async def peer_fixture(mqtt_params: MqttParams) -> AsyncGenerator[MQTTClientV5, None]:
"""Represent a device using an independent MQTT client."""
async with create_client(
mqtt_params.host,
mqtt_params.port,
version="5.0",
mqtt_connect_timeout=mqtt_params.timeout,
reconnect=ReconnectConfig(enabled=False),
) as peer:
yield peer


@pytest.fixture(name="topic")
def topic_fixture() -> str:
"""Isolate each test's traffic on the shared broker."""
return f"roborock-tests/{uuid4().hex}"


async def test_receive_message(session: MqttSession, peer: MQTTClientV5, topic: str) -> None:
"""Deliver an encrypted Roborock response to the session callback."""
messages: asyncio.Queue[bytes] = asyncio.Queue()
await session.subscribe(topic, messages.put_nowait)
response = RoborockMessage(
protocol=RoborockMessageProtocol.RPC_RESPONSE,
payload=b'{"result":"ok"}',
seq=123,
)
payload = MessageParser.build(response, local_key=LOCAL_KEY, prefixed=False)

await peer.publish(topic, payload)
received = await asyncio.wait_for(messages.get(), timeout=5)

assert received == payload
parsed, remaining = MessageParser.parse(received, local_key=LOCAL_KEY)
assert not remaining
assert len(parsed) == 1
assert parsed[0].protocol == response.protocol
assert parsed[0].seq == response.seq
assert parsed[0].payload == response.payload


@pytest.mark.parametrize("qos", list(MqttQos))
async def test_publish_message(session: MqttSession, peer: MQTTClientV5, topic: str, qos: MqttQos) -> None:
"""Deliver the full payload and requested QoS to another MQTT client."""
payload = MessageParser.build(
RoborockMessage(protocol=RoborockMessageProtocol.RPC_REQUEST, payload=b'{"method":"get_status"}'),
local_key=LOCAL_KEY,
prefixed=False,
)
async with peer.subscribe(topic, qos=QoS.EXACTLY_ONCE) as subscription:
await session.publish(topic, payload, qos=qos)
received = await asyncio.wait_for(subscription.get_message(), timeout=5)

assert received.topic == topic
assert received.payload == payload
assert received.qos == qos


async def test_subscriber_lifecycle(session: MqttSession, peer: MQTTClientV5, topic: str) -> None:
"""Fan out messages, remove callbacks, and reuse an idle subscription."""
first: asyncio.Queue[bytes] = asyncio.Queue()
second: asyncio.Queue[bytes] = asyncio.Queue()
unsub_first = await session.subscribe(topic, first.put_nowait)
unsub_second = await session.subscribe(topic, second.put_nowait)

await peer.publish(topic, b"both")
assert await asyncio.wait_for(first.get(), timeout=5) == b"both"
assert await asyncio.wait_for(second.get(), timeout=5) == b"both"

unsub_first()
await peer.publish(topic, b"second only")
assert await asyncio.wait_for(second.get(), timeout=5) == b"second only"
assert first.empty()

unsub_second()
await session.subscribe(topic, first.put_nowait)
await peer.publish(topic, b"first again")
assert await asyncio.wait_for(first.get(), timeout=5) == b"first again"
assert first.empty()
assert second.empty()


async def test_topic_routing(session: MqttSession, peer: MQTTClientV5, topic: str) -> None:
"""Keep messages for different devices separate in a shared session."""
first: asyncio.Queue[bytes] = asyncio.Queue()
second: asyncio.Queue[bytes] = asyncio.Queue()
await session.subscribe(f"{topic}/first", first.put_nowait)
await session.subscribe(f"{topic}/second", second.put_nowait)

await peer.publish(f"{topic}/first", b"first")
await peer.publish(f"{topic}/second", b"second")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test_restart_restores_subscriptions:

await session.restart()
async with asyncio.timeout(20):
    while session.connected:
        await asyncio.sleep(0.01)
    while not session.connected:
        await asyncio.sleep(0.01)

If the background reconnect task completes very rapidly before the first loop iteration executes, while session.connected: could theoretically spin until timeout. It may be worth ensuring this polling loop handles fast transitions robustly or utilizes an event/status notification if available.

assert await asyncio.wait_for(first.get(), timeout=5) == b"first"
assert await asyncio.wait_for(second.get(), timeout=5) == b"second"
assert first.empty()
assert second.empty()


async def test_restart_restores_subscriptions(session: MqttSession, peer: MQTTClientV5, topic: str) -> None:
"""Restore message delivery after the session reconnects."""
messages: asyncio.Queue[bytes] = asyncio.Queue()
await session.subscribe(topic, messages.put_nowait)
await peer.publish(topic, b"before restart")
assert await asyncio.wait_for(messages.get(), timeout=5) == b"before restart"

await session.restart()
async with asyncio.timeout(20):
while session.connected:
await asyncio.sleep(0.01)
while not session.connected:
await asyncio.sleep(0.01)

await peer.publish(topic, b"after restart")
assert await asyncio.wait_for(messages.get(), timeout=5) == b"after restart"
async with peer.subscribe(topic) as subscription:
await session.publish(topic, b"outbound after restart")
received = await asyncio.wait_for(subscription.get_message(), timeout=5)
assert received.payload == b"outbound after restart"


async def test_close(session: MqttSession, topic: str) -> None:
"""Close a connected session and reject further publications."""
await session.close()

assert not session.connected
with pytest.raises(MqttSessionException):
await session.publish(topic, b"closed")


async def test_lazy_session_subscribe(mqtt_params: MqttParams, peer: MQTTClientV5, topic: str) -> None:
"""Connect a lazy session on its first subscription and receive a message."""
session = await create_lazy_mqtt_session(mqtt_params)
assert not session.connected

messages: asyncio.Queue[bytes] = asyncio.Queue()
await session.subscribe(topic, messages.put_nowait)
assert session.connected

await peer.publish(topic, b"inbound")
assert await asyncio.wait_for(messages.get(), timeout=5) == b"inbound"

await session.close()
assert not session.connected


async def test_lazy_session_publish(mqtt_params: MqttParams, peer: MQTTClientV5, topic: str) -> None:
"""Connect a lazy session on its first publication and deliver a message."""
session = await create_lazy_mqtt_session(mqtt_params)
assert not session.connected

async with peer.subscribe(topic) as subscription:
await session.publish(topic, b"outbound")
assert session.connected
received = await asyncio.wait_for(subscription.get_message(), timeout=5)
assert received.payload == b"outbound"

await session.close()
assert not session.connected


async def test_channel_request_response(session: MqttSession, mqtt_params: MqttParams, peer: MQTTClientV5) -> None:
"""Exchange encrypted commands and responses on the device's Roborock topics."""
user_data = UserData.from_dict(USER_DATA)
duid = uuid4().hex
channel = MqttChannel(session, duid, LOCAL_KEY, user_data.rriot, mqtt_params)
request_topic = f"rr/m/i/{user_data.rriot.u}/{mqtt_params.username}/{duid}"
response_topic = f"rr/m/o/{user_data.rriot.u}/{mqtt_params.username}/{duid}"
messages: asyncio.Queue[RoborockMessage] = asyncio.Queue()
unsub = await channel.subscribe(messages.put_nowait)
async with peer.subscribe(request_topic) as subscription:
command = RoborockMessage(
protocol=RoborockMessageProtocol.RPC_REQUEST,
payload=json.dumps({"dps": {"101": json.dumps({"id": 123, "method": "get_status"})}}).encode(),
seq=456,
)
await channel.publish(command)
request = await asyncio.wait_for(subscription.get_message(), timeout=5)
parsed, remaining = MessageParser.parse(request.payload, local_key=LOCAL_KEY)
assert request.topic == request_topic
assert not remaining
assert len(parsed) == 1
assert parsed[0].protocol == RoborockMessageProtocol.RPC_REQUEST
assert parsed[0].seq == 456
assert parsed[0].payload == command.payload

response = RoborockMessage(
protocol=RoborockMessageProtocol.RPC_RESPONSE,
payload=json.dumps({"dps": {"102": json.dumps({"id": 123, "result": [{"state": 8}]})}}).encode(),
)
await peer.publish(response_topic, MessageParser.build(response, local_key=LOCAL_KEY, prefixed=False))
received = await asyncio.wait_for(messages.get(), timeout=5)
assert received.protocol == response.protocol
assert received.payload == response.payload
unsub()
11 changes: 11 additions & 0 deletions uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.