chore(tests): Cover mqtt e2e scenarios for MqttSession

[borisalekseev]

I wrote only e2e tests according to the conversation at #928

[allenporter]

Thank you @borisalekseev for following up on the discussion from #928 and putting together this real-broker E2E test suite! (Note: I am currently experimenting with agentic code review to help review PRs, so please let me know if anything in the feedback looks off).

These tests are well-structured, keep traffic isolated with per-test UUID topics, and effectively exercise the behavioral contracts of both MqttSession and MqttChannel without modifying production dependencies.

I have left a few inline comments on:

1. Ensuring broker tests have bounded timeouts and are tagged so standard local pytest runs stay fast and offline-friendly.
2. Decoupling the EMQX service container into a separate parallel job or standalone workflow.

[allenporter]

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.

[borisalekseev]

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

[allenporter]

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.

[allenporter]

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.

[borisalekseev]

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

[allenporter]

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

[allenporter]

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.

[allenporter]

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.