Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds full implementations for REST API, MQTT client, WebSocket server, and Robot integrations (TCP + ROS2), an interactive dashboard, CodeRabbit config, CI/tooling changes, extensive docs, and comprehensive integration tests. App factories and clients expose new public types and binding utilities for event-driven coordination. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 39
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
.coderabbit.yaml(1 hunks)docs/api/integrations/api.md(2 hunks)docs/api/integrations/mqtt.md(2 hunks)docs/api/integrations/robot.md(1 hunks)docs/api/integrations/websocket.md(3 hunks)docs/roadmap.md(1 hunks)pyproject.toml(3 hunks)src/cnckit/integrations/api/__init__.py(1 hunks)src/cnckit/integrations/api/static/dashboard.html(1 hunks)src/cnckit/integrations/mqtt/__init__.py(2 hunks)src/cnckit/integrations/robot/__init__.py(1 hunks)src/cnckit/integrations/websocket/__init__.py(1 hunks)tests/integrations/test_api.py(1 hunks)tests/integrations/test_mqtt.py(1 hunks)tests/integrations/test_robot.py(1 hunks)tests/integrations/test_websocket.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: Documentation uses MkDocs with Material theme.
- Code examples should be runnable
- API references use mkdocstrings format (:::)
- Keep examples simple and focused
Files:
docs/api/integrations/websocket.mddocs/api/integrations/mqtt.mddocs/api/integrations/robot.mddocs/roadmap.mddocs/api/integrations/api.md
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Test files should follow pytest conventions.
- Use fixtures appropriately
- Mock external services, don't make real connections
- Test both success and error paths
- Integration tests should be marked with @pytest.mark.integration
Files:
tests/integrations/test_api.pytests/integrations/test_mqtt.pytests/integrations/test_robot.pytests/integrations/test_websocket.py
src/cnckit/integrations/**/*.py
⚙️ CodeRabbit configuration file
src/cnckit/integrations/**/*.py: These are optional integration modules with external dependencies.
- Each integration must validate its dependencies on import
- Must raise ImportError with install instructions if deps missing
- Should integrate with EventEmitter for event binding
- Check that optional deps are in pyproject.toml extras
Files:
src/cnckit/integrations/websocket/__init__.pysrc/cnckit/integrations/mqtt/__init__.pysrc/cnckit/integrations/api/__init__.pysrc/cnckit/integrations/robot/__init__.py
🧬 Code graph analysis (5)
tests/integrations/test_api.py (2)
src/cnckit/integrations/api/__init__.py (1)
create_app(221-535)tests/conftest.py (1)
sample_gcode_file(26-30)
tests/integrations/test_mqtt.py (6)
src/cnckit/integrations/mqtt/__init__.py (7)
MQTTTopics(38-85)topics(238-240)all_command_topics(83-85)create_message(109-128)job_to_dict(93-106)port(233-235)disconnect(280-289)src/cnckit/core/job.py (3)
Job(25-108)mark_completed(77-80)mark_failed(82-91)src/cnckit/core/events.py (4)
EventEmitter(28-144)Event(15-25)listeners(119-130)emit(86-117)src/cnckit/core/queue.py (1)
JobQueue(24-158)src/cnckit/core/machine.py (2)
Machine(427-547)simulate(467-469)src/cnckit/core/scheduler.py (2)
Scheduler(27-250)SchedulerState(19-24)
src/cnckit/integrations/websocket/__init__.py (3)
src/cnckit/core/events.py (5)
Event(15-25)EventEmitter(28-144)clear(132-144)on(55-68)off(70-84)src/cnckit/integrations/mqtt/__init__.py (3)
job_to_dict(93-106)create_message(109-128)port(233-235)src/cnckit/integrations/robot/__init__.py (4)
job_to_dict(79-87)create_message(57-76)port(166-168)stop(334-336)
src/cnckit/integrations/api/__init__.py (4)
src/cnckit/core/events.py (1)
EventEmitter(28-144)src/cnckit/core/queue.py (6)
JobQueue(24-158)mode(50-52)jobs(135-142)add_job(70-77)add(54-68)remove(115-129)src/cnckit/core/machine.py (26)
Machine(427-547)state(58-60)state(129-131)state(295-321)state(472-474)position(63-65)position(134-136)position(324-335)position(477-479)tool(68-70)tool(139-141)tool(338-341)tool(482-484)current_program(73-75)current_program(144-146)current_program(344-346)current_program(487-489)progress(78-80)progress(149-151)progress(349-368)progress(492-494)simulate(467-469)stop(90-92)stop(200-203)stop(403-405)stop(518-520)src/cnckit/core/scheduler.py (8)
Scheduler(27-250)state(76-78)current_job(81-83)events(86-88)start(90-105)pause(128-143)stop(107-126)tick(145-191)
src/cnckit/integrations/robot/__init__.py (2)
src/cnckit/core/events.py (5)
Event(15-25)EventEmitter(28-144)on(55-68)off(70-84)clear(132-144)src/cnckit/core/job.py (1)
Job(25-108)
🪛 markdownlint-cli2 (0.18.1)
docs/api/integrations/websocket.md
63-63: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
74-74: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
85-85: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
102-102: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
124-124: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
135-135: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
149-149: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
161-161: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/api/integrations/api.md
79-79: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
93-93: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
129-129: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
151-151: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
160-160: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
196-196: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (37)
docs/roadmap.md (1)
21-27: Status update accurately reflects Phase 2 completion.The roadmap has been updated correctly to mark Phase 2 (Optional Integrations) as complete. All five listed deliverables—REST API, MQTT, WebSocket, Robot integration, and web dashboard—are now checked, with the section header showing the ✅ completion indicator consistent with earlier phases.
pyproject.toml (4)
42-42: Add httpx for REST API testing.Appropriate addition to dev dependencies for testing FastAPI endpoints with async support.
137-137: Verify code quality with extensive per-file ignores.The integration code is exempted from several important checks: B008 (function-call-as-default-argument), PLR0915 (too-many-statements), and others. While reasonable for integration code with complex event handlers and dynamic imports, ensure the underlying implementation is robust and well-tested to compensate for these relaxations.
156-156: Mypy module override expansion aligns with integrations.Extended coverage to include
pydantic.*,uvicorn.*,starlette.*, andstd_msgs.*—third-party packages that often lack complete type stubs. This change is appropriate.
159-162: Based on my thorough verification of the codebase, I can now provide the rewritten review comment:
External library constraints justify the mypy override, but clarify that cnckit integrations code itself is properly typed.
The mypy override disables
disallow_untyped_decoratorsand suppresses"misc"and"attr-defined"errors due to unavoidable constraints from external libraries—not due to untyped code in cnckit itself:
disallow_untyped_decorators = false: Required because paho-mqtt and FastAPI libraries have untyped callback/decorator APIs. All cnckit route handlers (e.g.,def health() -> HealthResponse) and callback methods (e.g.,def _on_connect(...) -> None) are properly type-annotated. The suppression handles assignment of typed cnckit methods to untyped external library callbacks.disable_error_code = ["misc", "attr-defined"]: Needed for compatibility with external library type stubs (paho-mqtt, FastAPI, websockets) which report these errors due to their own incomplete typing. This is standard practice for optional integrations with untyped dependencies.Test coverage is adequate (2230 lines across four integration test modules), and no untyped decorators or functions exist in the integrations module itself.
src/cnckit/integrations/websocket/__init__.py (2)
15-23: LGTM!Dependency validation correctly raises
ImportErrorwith clear install instructions whenwebsocketsis unavailable. This follows the coding guidelines for integration modules.
122-163: LGTM!Clean initialization with sensible defaults. Properties provide read-only access to server state, and the asyncio lock ensures thread-safe client management.
src/cnckit/integrations/mqtt/__init__.py (3)
17-24: LGTM!Dependency validation correctly raises
ImportErrorwith clear install instructions whenpaho-mqttis unavailable. This follows the coding guidelines for integration modules.
197-206: LGTM!Good backward compatibility handling for paho-mqtt 1.x and 2.x. The try/except fallback ensures the client works across versions.
37-86: LGTM!Clean topic configuration using a dataclass with automatic topic name generation in
__post_init__. Theall_command_topicsproperty provides convenient access to subscription topics.src/cnckit/integrations/robot/__init__.py (3)
521-529: LGTM!ROS2 dependency validation in
__init__(rather than import-time) is appropriate here since the module provides two interfaces:TCPRobotClient(no ROS2 dependency) andROS2Interface(requires ROS2). This allows usingTCPRobotClientwithout having ROS2 installed.
250-267: LGTM!The response reading loop correctly handles:
- Timeout via socket.settimeout raising TimeoutError
- Connection closed by checking for empty chunk
- Message terminator detection with proper splitting
416-453: LGTM!Nice feature allowing optional automatic part loading/unloading based on CNC events. The implementation properly integrates with the EventEmitter and stores handlers for cleanup.
tests/integrations/test_api.py (3)
5-11: LGTM!Clean dependency detection pattern using try/except to set
FASTAPI_AVAILABLE. This enables the test file to be imported even when FastAPI isn't installed.
14-23: LGTM!Good test coverage for the ImportError path when FastAPI isn't installed. The test correctly skips when FastAPI is available since it cannot test the import failure scenario.
168-288: LGTM!Comprehensive test coverage for queue endpoints including:
- Empty queue state
- Adding jobs with valid/invalid paths
- Custom job names
- Multiple jobs
- Job removal (success and not-found cases)
Tests both success and error paths as required by coding guidelines.
.coderabbit.yaml (1)
96-105:mypyis a supported CodeRabbit tool — the configuration is valid.Verification confirms that
mypyis officially supported in CodeRabbit and can be configured exactly as shown in lines 99-100. The web search results for CodeRabbit's tools reference explicitly showmypyas a configurable tool withenabledandconfig_fileoptions. The configuration in your.coderabbit.yamlis correct.Likely an incorrect or invalid review comment.
tests/integrations/test_websocket.py (3)
1-73: Well-structured test file with proper pytest conventions.Good coverage of the WebSocket integration including:
- Conditional handling when
websocketsis not installed- Message formatting utilities
- Use of
@pytest.mark.skipiffor optional dependency tests
186-199: Good use of fixtures for test setup.The
server_with_mock_clientfixture properly sets up a mock client for testing broadcast functionality without real connections.
226-243: Good coverage of error handling in broadcast tests.Testing that disconnected clients are properly removed from the client set when
ConnectionClosedis raised is important for reliability.docs/api/integrations/mqtt.md (2)
13-36: Quick Start example is well-structured and runnable.Good integration example showing the complete setup flow with core components. The
simulate=Trueparameter ensures the example works without actual hardware.
45-48: API reference uses correct mkdocstrings format.The
:::syntax with options block follows MkDocs Material conventions.tests/integrations/test_mqtt.py (5)
1-31: Good test structure with proper dependency handling.The conditional import pattern for
paho-mqttand theTestMQTTWithoutPahoclass ensure tests work correctly whether or not the optional dependency is installed.
156-165: Well-designed fixture for mocked MQTT client.The
mock_clientfixture properly patches the paho client and sets up the connected state, providing clean test isolation.
280-297: Comprehensive fixture with full component setup.The
mock_client_with_schedulerfixture correctly sets up the entire component stack (Machine, JobQueue, EventEmitter, Scheduler) for testing command handling.
338-354: Good test for pause command from running state.Testing
pausefrom a running scheduler state by manually settingscheduler._statedemonstrates proper edge case coverage. This tests the actual pause behavior rather than the no-op case.
526-545: Good test for disconnect cleanup behavior.Testing that
disconnect()properly unbinds event handlers ensures no memory leaks or dangling callbacks.docs/api/integrations/websocket.md (2)
233-281: Comprehensive complete example with proper async patterns.The example correctly shows the async context manager usage, scheduler tick loop, and periodic state broadcasting pattern.
283-322: Helpful client examples for JavaScript and Python.The JavaScript example demonstrates proper WebSocket lifecycle handling with reconnection-friendly patterns (ping/pong). The Python example is concise and runnable.
docs/api/integrations/api.md (1)
308-345: Good example showing REST API + WebSocket integration.The combined example demonstrates how to run both servers together for real-time dashboard functionality. The async pattern with uvicorn.Server is correct.
tests/integrations/test_robot.py (4)
1-18: LGTM! Test imports and setup are well-organized.The imports are clean, using appropriate mocking utilities and pytest. The test file properly imports from the robot integration module.
243-243: Good: Tests properly verify mock wasn't called whenwait_response=False.The test correctly asserts that
recvis not called whenwait_response=False, ensuring the behavior matches the expected contract.
518-529: Good safety test for disconnected state.This test verifies that auto-load gracefully handles the disconnected state without raising an exception. This is important for preventing crashes when events fire before the robot is ready.
206-216: Based on my verification, I can now provide the final assessment:The fixture is functioning correctly; the mock socket persists after the patch context exits.
The review comment misunderstands Python's mocking behavior. When
client.connect()is called within thewith patch("socket.socket")block, the mock object is stored inself._socketas an instance variable. Although the patch context exits, the mock object reference remains valid inself._socket. All subsequent test calls (likeclient.send_command()) use this stored mock object, which is the same reference returned by the fixture and modified by tests viamock_socket.recv.return_value.None of the tests using
connected_clienttrigger socket recreation, and no reconnection occurs in these tests. The separatetest_reconnect()uses its own patch context and is not affected by this fixture design.docs/api/integrations/robot.md (2)
12-26: LGTM! Quick start example is clear and runnable.The example demonstrates the essential workflow: connect, send commands, get status, disconnect. This is well-structured and matches the API documented later.
239-241: I'll help you verify the scheduler state comparison in the documentation example. Let me search for theSchedulerStateenum definition to confirm the implementation.
<function_calls>
#!/bin/bashSearch for SchedulerState enum definition
find . -type f -name "*.py" | head -20
Find SchedulerState in the codebase
rg -n "class SchedulerState" -A 10 --type py
Also search for state value assignments/comparisons
rg -n ".state.value|SchedulerState" --type py -B 2 -A 2 | head -50
</function_calls><function_calls>
#!/bin/bashLook for scheduler-related files
fd "scheduler" --type f | head -20
Search for state enum or class definitions more broadly
rg -n "state.*=|stopped" --type py | grep -i scheduler | head -20
</function_calls><function_calls>
#!/bin/bashGet repository structure first
git ls-files | grep -E ".py$" | head -30
Find the actual scheduler implementation
rg -n "scheduler" --type py -l | head -10
</function_calls>src/cnckit/integrations/api/__init__.py (1)
15-24: LGTM! Dependency validation follows coding guidelines.The import-time try/except properly validates that FastAPI is available and raises a clear
ImportErrorwith installation instructions. This follows the coding guidelines for integration modules.
| def get_loop() -> asyncio.AbstractEventLoop | None: | ||
| """Get the running event loop, or None if not running.""" | ||
| try: | ||
| return asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| return None | ||
|
|
||
| # Define handlers that schedule async broadcasts | ||
| def on_job_started(job: Job) -> None: | ||
| loop = get_loop() | ||
| if loop is not None: | ||
| loop.create_task(self.broadcast_job_started(job)) | ||
|
|
||
| def on_job_completed(job: Job) -> None: | ||
| loop = get_loop() | ||
| if loop is not None: | ||
| loop.create_task(self.broadcast_job_completed(job)) | ||
|
|
||
| def on_job_failed(job: Job, error: str | None = None) -> None: | ||
| loop = get_loop() | ||
| if loop is not None: | ||
| loop.create_task(self.broadcast_job_failed(job, error)) | ||
|
|
||
| def on_queue_empty() -> None: | ||
| loop = get_loop() | ||
| if loop is not None: | ||
| loop.create_task(self.broadcast_queue_empty()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent failure when no event loop is running.
The event handlers silently do nothing if asyncio.get_running_loop() fails. This could make debugging difficult when events aren't being broadcast. Consider logging a warning to aid troubleshooting.
+ import logging
+ logger = logging.getLogger(__name__)
+
def get_loop() -> asyncio.AbstractEventLoop | None:
"""Get the running event loop, or None if not running."""
try:
return asyncio.get_running_loop()
except RuntimeError:
+ logger.debug("No running event loop; skipping broadcast")
return NoneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/cnckit/integrations/websocket/__init__.py around lines 388 to 414, the
event handlers silently return if no running asyncio loop is found; update each
handler (on_job_started, on_job_completed, on_job_failed, on_queue_empty) to log
a warning when get_loop() returns None so missing broadcasts are visible during
debugging — call the existing logger (e.g., self.logger or module logger) to
emit a concise warning that includes which handler ran and any identifying data
(job id or error text when available), then only create_task when loop is
present.
| @pytest.mark.skipif(not FASTAPI_AVAILABLE, reason="FastAPI not installed") | ||
| class TestHealthEndpoint: | ||
| """Tests for /health endpoint.""" |
There was a problem hiding this comment.
Add @pytest.mark.integration marker as per coding guidelines.
The coding guidelines specify that integration tests should be marked with @pytest.mark.integration. These API tests qualify as integration tests since they test the API endpoints with core components.
+@pytest.mark.integration
@pytest.mark.skipif(not FASTAPI_AVAILABLE, reason="FastAPI not installed")
class TestHealthEndpoint:
"""Tests for /health endpoint."""Apply the same marker to all test classes in this file.
🤖 Prompt for AI Agents
In tests/integrations/test_api.py around lines 26 to 28, the TestHealthEndpoint
class (and all other test classes in this file) are missing the required
@pytest.mark.integration marker; add @pytest.mark.integration above each test
class definition (in addition to existing markers like skipif) so every
integration test class in this file is decorated with @pytest.mark.integration
per the coding guidelines.
| def test_health_returns_200(self): | ||
| """Health endpoint returns 200 OK.""" | ||
| from cnckit.integrations.api import create_app | ||
|
|
||
| app = create_app(simulate=True) | ||
| client = TestClient(app) | ||
|
|
||
| response = client.get("/health") | ||
|
|
||
| assert response.status_code == 200 | ||
| data = response.json() | ||
| assert data["status"] == "healthy" | ||
| assert "timestamp" in data |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a pytest fixture for app and client creation.
The pattern of creating app = create_app(simulate=True) and client = TestClient(app) is repeated in every test method. Consider extracting this to a fixture for DRY and easier maintenance.
@pytest.fixture
def api_client():
"""Create API test client with simulated machine."""
from cnckit.integrations.api import create_app
app = create_app(simulate=True)
return TestClient(app)
# Then in tests:
def test_health_returns_200(self, api_client):
response = api_client.get("/health")
assert response.status_code == 200🤖 Prompt for AI Agents
In tests/integrations/test_api.py around lines 30 to 42, the test repeatedly
creates the app and TestClient inline; extract that setup into a pytest fixture
(e.g., api_client) that imports create_app(simulate=True), instantiates the
TestClient and returns it, then update tests to accept the fixture (replace
direct app/client creation with a parameter like api_client and call
api_client.get(...)); ensure the fixture is placed at module scope (or
conftest.py if shared) and imports pytest so tests remain DRY and maintainable.
| def test_import_raises_without_rclpy(self): | ||
| """ROS2Interface raises ImportError without rclpy.""" | ||
| # The ROS2Interface class checks on __init__, not import | ||
| from cnckit.integrations.robot import ROS2Interface | ||
|
|
||
| with ( | ||
| patch("builtins.__import__", side_effect=ImportError("No module")), | ||
| pytest.raises(ImportError, match="rclpy is required"), | ||
| ): | ||
| ROS2Interface() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import mock is overly broad and may not accurately test behavior.
Patching builtins.__import__ affects all imports, not just rclpy. The test imports ROS2Interface before patching, so the actual import-time dependency check isn't tested. The patch only affects imports that happen during ROS2Interface() instantiation.
Consider a more targeted approach:
def test_import_raises_without_rclpy(self):
"""ROS2Interface raises ImportError without rclpy."""
- # The ROS2Interface class checks on __init__, not import
from cnckit.integrations.robot import ROS2Interface
+ # Mock the specific import of rclpy modules
with (
- patch("builtins.__import__", side_effect=ImportError("No module")),
+ patch.dict("sys.modules", {"rclpy": None, "rclpy.node": None}),
pytest.raises(ImportError, match="rclpy is required"),
):
ROS2Interface()Or patch at the module level where the import happens.
🤖 Prompt for AI Agents
In tests/integrations/test_robot.py around lines 535-544, the current test
patches builtins.__import__ too broadly and imports ROS2Interface before the
patch, so it doesn't correctly simulate missing rclpy; change the test to either
(A) perform the import of ROS2Interface inside the patched context and make the
import patch targeted by wrapping the original __import__ and raising
ImportError only when the requested module name is 'rclpy', or (B) use
monkeypatch to remove or replace sys.modules['rclpy'] (e.g.,
monkeypatch.delitem(sys.modules, 'rclpy', raising=False) or
monkeypatch.setitem(sys.modules, 'rclpy', None)) before importing/reloading
cnckit.integrations.robot so the class __init__ runs without rclpy and raises
the expected ImportError; ensure the patching/reloading happens before calling
ROS2Interface().
| @pytest.mark.skipif(not WEBSOCKETS_AVAILABLE, reason="websockets not installed") | ||
| class TestWebSocketIntegration: | ||
| """Integration tests with real server (using available ports).""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_real_server_starts_and_stops(self): | ||
| """Real server can start and stop.""" | ||
| from cnckit.integrations.websocket import WebSocketServer | ||
|
|
||
| # Use a random high port to avoid conflicts | ||
| server = WebSocketServer(host="127.0.0.1", port=28765) | ||
|
|
||
| await server.start() | ||
| assert server.is_running is True | ||
|
|
||
| await server.stop() | ||
| assert server.is_running is False | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_real_client_connection(self): | ||
| """Real client can connect and receive messages.""" | ||
| from cnckit.integrations.websocket import WebSocketServer | ||
|
|
||
| server = WebSocketServer(host="127.0.0.1", port=28766) | ||
|
|
||
| async with server: | ||
| # Connect a client | ||
| async with websockets.connect("ws://127.0.0.1:28766") as client: | ||
| # Should receive welcome message | ||
| msg = await asyncio.wait_for(client.recv(), timeout=2.0) | ||
| data = json.loads(msg) | ||
|
|
||
| assert data["type"] == "connected" | ||
| assert server.client_count == 1 | ||
|
|
||
| # Give server time to detect disconnect | ||
| await asyncio.sleep(0.1) | ||
| assert server.client_count == 0 | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_broadcast_reaches_client(self): | ||
| """Broadcast message reaches connected client.""" | ||
| from cnckit.integrations.websocket import WebSocketServer | ||
|
|
||
| server = WebSocketServer(host="127.0.0.1", port=28767) | ||
|
|
||
| async with server, websockets.connect("ws://127.0.0.1:28767") as client: | ||
| # Consume welcome message | ||
| await client.recv() | ||
|
|
||
| # Broadcast a message | ||
| await server.broadcast_machine_state( | ||
| state="idle", | ||
| progress=0.0, | ||
| ) | ||
|
|
||
| # Client should receive it | ||
| msg = await asyncio.wait_for(client.recv(), timeout=2.0) | ||
| data = json.loads(msg) | ||
|
|
||
| assert data["type"] == "machine_state" | ||
| assert data["data"]["state"] == "idle" |
There was a problem hiding this comment.
Missing @pytest.mark.integration marker on integration tests.
Per coding guidelines, integration tests that make real connections should be marked with @pytest.mark.integration. The TestWebSocketIntegration class contains tests that start real servers and make actual WebSocket connections.
Apply this diff:
@pytest.mark.skipif(not WEBSOCKETS_AVAILABLE, reason="websockets not installed")
+@pytest.mark.integration
class TestWebSocketIntegration:
"""Integration tests with real server (using available ports)."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.skipif(not WEBSOCKETS_AVAILABLE, reason="websockets not installed") | |
| class TestWebSocketIntegration: | |
| """Integration tests with real server (using available ports).""" | |
| @pytest.mark.asyncio | |
| async def test_real_server_starts_and_stops(self): | |
| """Real server can start and stop.""" | |
| from cnckit.integrations.websocket import WebSocketServer | |
| # Use a random high port to avoid conflicts | |
| server = WebSocketServer(host="127.0.0.1", port=28765) | |
| await server.start() | |
| assert server.is_running is True | |
| await server.stop() | |
| assert server.is_running is False | |
| @pytest.mark.asyncio | |
| async def test_real_client_connection(self): | |
| """Real client can connect and receive messages.""" | |
| from cnckit.integrations.websocket import WebSocketServer | |
| server = WebSocketServer(host="127.0.0.1", port=28766) | |
| async with server: | |
| # Connect a client | |
| async with websockets.connect("ws://127.0.0.1:28766") as client: | |
| # Should receive welcome message | |
| msg = await asyncio.wait_for(client.recv(), timeout=2.0) | |
| data = json.loads(msg) | |
| assert data["type"] == "connected" | |
| assert server.client_count == 1 | |
| # Give server time to detect disconnect | |
| await asyncio.sleep(0.1) | |
| assert server.client_count == 0 | |
| @pytest.mark.asyncio | |
| async def test_broadcast_reaches_client(self): | |
| """Broadcast message reaches connected client.""" | |
| from cnckit.integrations.websocket import WebSocketServer | |
| server = WebSocketServer(host="127.0.0.1", port=28767) | |
| async with server, websockets.connect("ws://127.0.0.1:28767") as client: | |
| # Consume welcome message | |
| await client.recv() | |
| # Broadcast a message | |
| await server.broadcast_machine_state( | |
| state="idle", | |
| progress=0.0, | |
| ) | |
| # Client should receive it | |
| msg = await asyncio.wait_for(client.recv(), timeout=2.0) | |
| data = json.loads(msg) | |
| assert data["type"] == "machine_state" | |
| assert data["data"]["state"] == "idle" | |
| @pytest.mark.skipif(not WEBSOCKETS_AVAILABLE, reason="websockets not installed") | |
| @pytest.mark.integration | |
| class TestWebSocketIntegration: | |
| """Integration tests with real server (using available ports).""" | |
| @pytest.mark.asyncio | |
| async def test_real_server_starts_and_stops(self): | |
| """Real server can start and stop.""" | |
| from cnckit.integrations.websocket import WebSocketServer | |
| # Use a random high port to avoid conflicts | |
| server = WebSocketServer(host="127.0.0.1", port=28765) | |
| await server.start() | |
| assert server.is_running is True | |
| await server.stop() | |
| assert server.is_running is False | |
| @pytest.mark.asyncio | |
| async def test_real_client_connection(self): | |
| """Real client can connect and receive messages.""" | |
| from cnckit.integrations.websocket import WebSocketServer | |
| server = WebSocketServer(host="127.0.0.1", port=28766) | |
| async with server: | |
| # Connect a client | |
| async with websockets.connect("ws://127.0.0.1:28766") as client: | |
| # Should receive welcome message | |
| msg = await asyncio.wait_for(client.recv(), timeout=2.0) | |
| data = json.loads(msg) | |
| assert data["type"] == "connected" | |
| assert server.client_count == 1 | |
| # Give server time to detect disconnect | |
| await asyncio.sleep(0.1) | |
| assert server.client_count == 0 | |
| @pytest.mark.asyncio | |
| async def test_broadcast_reaches_client(self): | |
| """Broadcast message reaches connected client.""" | |
| from cnckit.integrations.websocket import WebSocketServer | |
| server = WebSocketServer(host="127.0.0.1", port=28767) | |
| async with server, websockets.connect("ws://127.0.0.1:28767") as client: | |
| # Consume welcome message | |
| await client.recv() | |
| # Broadcast a message | |
| await server.broadcast_machine_state( | |
| state="idle", | |
| progress=0.0, | |
| ) | |
| # Client should receive it | |
| msg = await asyncio.wait_for(client.recv(), timeout=2.0) | |
| data = json.loads(msg) | |
| assert data["type"] == "machine_state" | |
| assert data["data"]["state"] == "idle" |
🤖 Prompt for AI Agents
In tests/integrations/test_websocket.py around lines 451 to 512, the
TestWebSocketIntegration class is missing the required @pytest.mark.integration
marker; add @pytest.mark.integration to the class definition (in addition to the
existing @pytest.mark.skipif) so the group of tests is properly labeled as
integration tests for pytest selection. Ensure the marker is applied to the
class (not just individual methods) so all contained async tests inherit it.
Rename title_check to title and description_check to description to match the CodeRabbit schema v2 specification.
Fix MD031 markdown lint warnings by ensuring blank lines precede all fenced code blocks in the API documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/api/integrations/api.md (2)
78-78: Unresolved Markdown lint violations (MD031).Fenced code blocks should be surrounded by blank lines. Six code blocks still lack blank lines before them: lines 78, 93, 130, 153, 163, and 200. Some parsers, including kramdown, will not parse fenced code blocks that don't have blank lines before and after them.
Apply blank lines before each fenced code block (example for line 78):
**Response:** + ```json[scratchpad_end] -->
Also applies to: 93-93, 130-130, 153-153, 163-163, 200-200
257-293: Missingimport timein example code.The workflow example calls
time.sleep(1)on line 292 but does not import thetimemodule, violating the guideline that code examples should be runnable.Add the import statement after line 258:
```python import requests +import time BASE_URL = "http://localhost:8000"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.coderabbit.yaml(1 hunks)docs/api/integrations/api.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: Documentation uses MkDocs with Material theme.
- Code examples should be runnable
- API references use mkdocstrings format (:::)
- Keep examples simple and focused
Files:
docs/api/integrations/api.md
🔇 Additional comments (1)
.coderabbit.yaml (1)
80-93: Pre-merge check keys corrected from previous review.The
titleanddescriptionkeys are now properly aligned with the CodeRabbit schema. This resolves the previous feedback on incorrect key naming.
CodeRabbit only supports ruff, markdownlint, and github-checks as built-in tools. mypy type checking is handled via CI pipeline.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Implements all Phase 2 optional integration modules for CNC machine connectivity:
cnckit.integrations.api): FastAPI endpoints for remote monitoring and control, includes web dashboard at/dashboardcnckit.integrations.mqtt): Pub/sub client with paho-mqtt for messaging and automationcnckit.integrations.websocket): Real-time streaming server for live status updatescnckit.integrations.robot): TCPRobotClient for socket-based robots and ROS2Interface for ROS2 integrationKey Features
EventEmitterfor automatic event publishingInstallation
Breaking Changes
None - all integrations are optional and additive.
Checklist
pytest)ruff check)mypy)