[WIP] Corellium serial driver#413
Conversation
WalkthroughThis update introduces websocket console support for the Corellium driver. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CorelliumDriver
participant CorelliumConsole
participant ApiClient
participant CorelliumAPI
User->>CorelliumDriver: Request serial console connection
CorelliumDriver->>CorelliumConsole: Initialize serial driver
CorelliumConsole->>ApiClient: get_instance_console_id(instance, console_name)
ApiClient->>CorelliumAPI: GET /instances/{id}
CorelliumAPI-->>ApiClient: Instance details (with consoles)
ApiClient-->>CorelliumConsole: Console ID
CorelliumConsole->>ApiClient: get_instance_console_url(instance, console_id)
ApiClient->>CorelliumAPI: GET /instances/{id}/console?type={console_id}
CorelliumAPI-->>ApiClient: { "url": "wss://..." }
ApiClient-->>CorelliumConsole: Console URL
CorelliumConsole->>WebsocketNetwork: Connect to websocket URL
WebsocketNetwork->>User: Established stream
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (16)
packages/jumpstarter-driver-corellium/examples/exporter.yml (1)
19-19: Document the Optionalconsole_nameConfiguration Field
Sinceconsole_nameis optional and impacts which Corellium console is selected, consider enhancing the documentation or example README to explain its purpose, valid values, and default behavior when omitted.packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (3)
211-212: Fix trailing whitespaceThere's trailing whitespace in this class docstring line.
- A PySerial subclass to handle + A PySerial subclass to handle🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Fix trailing whitespaceThere's trailing whitespace at the end of this line.
- parent: Corellium + parent: Corellium🧰 Tools
🪛 Ruff (0.8.2)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
208-243: Document the purpose of the CorelliumSerial class more clearlyThe class docstring is incomplete and doesn't fully explain what this class does. Consider expanding the docstring to clarify that this class handles serial communication over websockets for Corellium devices.
- A PySerial subclass to handle + A PySerial subclass to handle Corellium serial communication over websockets. + + This class dynamically retrieves the websocket URL for the console from + the Corellium API and uses it to establish a serial connection.🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (5)
2-3: Remove unused importsThe
asyncio,AsyncMock, andpatchimports are not used in this test file.import os -import asyncio -from unittest.mock import AsyncMock, patch + import pytest🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
6-8: Remove commented and unused importsThe
# import websocketsline appears to be a leftover comment, and the importedwebsocketsfrom.apiis not used.import pytest -# import websockets -from .api import ApiClient, websockets +from .api import ApiClient from .exceptions import CorelliumApiException from .types import Device, Instance, Project, Session🧰 Tools
🪛 Ruff (0.8.2)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
196-196: Fix trailing whitespaceThere's trailing whitespace at the end of this line.
- data = fixture('http/get-instance-200.json') + data = fixture('http/get-instance-200.json')🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Fix trailing whitespaceThere's trailing whitespace at the end of this line.
- data = fixture('http/get-instance-console-url-200.json') + data = fixture('http/get-instance-console-url-200.json')🧰 Tools
🪛 Ruff (0.8.2)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
188-204: Improve test case verification for console IDThe test for
get_instance_console_id_okcould be improved by asserting that the API client method was called with the correct parameters. This would ensure that the test is validating the right behavior.Consider using a mock to verify the actual API call was made with expected parameters:
from unittest.mock import patch @pytest.mark.parametrize( 'console_name,console_id', [ ('Console 1', 'port-1-cons',), ('Console 10', None,), ('Console 2', 'port-2-cons',), ]) def test_get_instance_console_id_ok(requests_mock, console_name, console_id): data = fixture('http/get-instance-200.json') instance = Instance(id='d59db33d-27bd-4b22-878d-49e4758a648e') requests_mock.get(f'https://api-host/api/v1/instances/{instance.id}', status_code=200, text=data) api = ApiClient('api-host', 'api-token') api.session = Session('session-token', '2022-03-20T01:50:10.000Z') current = api.get_instance_console_id(instance, console_name) assert console_id == current🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (4)
1-4: Remove unused imports
AsyncGeneratorandwebsocketsare imported but not used in this file.-from typing import AsyncGenerator, Optional +from typing import Optional import requests -import websockets🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
4-4:
websocketsimported but unusedRemove unused import:
websockets(F401)
166-167: Fix malformed docstringThere's an extra single quote at the end of the docstring for
destroy_instance.- '""" + """
192-193: Remove whitespace from blank lineThere's whitespace on this blank line.
if console['name'] == console_name: return console['id'] - + return None🧰 Tools
🪛 Ruff (0.8.2)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
195-207: Fix typo in docstring and improve error handlingThere's a typo in the docstring ("Get a a console URL"), and the method should handle the case where the 'url' key is missing from the response.
def get_instance_console_url(self, instance: Instance, console_id: str) -> Optional[str]: """ - Get a a console URL (websocket) to stream logs from. + Get a console URL (websocket) to stream logs from. """ try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}/console', params={'type': console_id.replace('port-', '')}) data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: raise CorelliumApiException(data.get('error', str(e))) from e - return data['url'] + if 'url' not in data: + raise CorelliumApiException("Response did not contain a URL") + return data['url']packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (3)
3-3: Remove unused importAny
Anyisn’t referenced anywhere in this module, so the import adds to cognitive noise and will be flagged by linters (see Ruff F401).-from typing import Any +# from typing import Any # ← removed – not used🧰 Tools
🪛 Ruff (0.8.2)
3-3:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
93-94: Doc‑string typoUse the actual method name (
connect_serial) and remove the hyphen:- Defaults to `self.connect-serial` in case no valid handler is found. + Defaults to `self.connect_serial` if no specialized handler matches.
85-86: Strip trailing / whitespace linesThere’s a blank line with spaces and a trailing space at the log line; these trigger linters (Ruff W293/W291).
🧰 Tools
🪛 Ruff (0.8.2)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py(4 hunks)packages/jumpstarter-driver-pyserial/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (3)
ApiClient(10-207)get_instance_console_id(176-193)get_instance_console_url(195-207)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/types.py (2)
Instance(51-56)Session(9-22)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/exceptions.py (1)
CorelliumApiException(7-10)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
3-3: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (8)
packages/jumpstarter-driver-pyserial/pyproject.toml (1)
18-18: Include WebSocket Support Dependency
The addition ofwebsockets>=15.0.1in the runtime dependencies correctly complements the new WebSocket-based serial transport.packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json (1)
1-3: Approve JSON Fixture for Successful Console URL Retrieval
Theget-instance-console-url-200.jsonfixture accurately models a successful API response containing the WebSocket URL.packages/jumpstarter-driver-corellium/pyproject.toml (2)
13-17: Add Core WebSocket and PySerial Dependencies
Includingjumpstarter-driver-pyserialandwebsockets>=15.0.1aligns this package with the enhanced serial‑over‑websocket functionality. Confirm compatibility across dependent driver versions.
23-23:Details
❓ Verification inconclusive
Enable Asynchronous Test Support
The addition ofpytest-asyncio>=0.25.3is necessary for the new async Corellium API tests. Please verify that tests use the@pytest.mark.asynciodecorator or pytest‑asyncio fixtures accordingly.
🏁 Script executed:
#!/bin/bash # Verify that async tests reference pytest-asyncio rg -R "pytest.mark.asyncio" -A2Length of output: 436
🏁 Script executed:
#!/bin/bash # Verify that async tests reference pytest-asyncio # Search for the pytest-asyncio marker decorator in test files rg "@pytest\.mark\.asyncio" -A2 .Length of output: 3269
#!/bin/bash # Verify that Corellium driver tests use pytest-asyncio echo "Searching for pytest.mark.asyncio in corellium tests..." rg "@pytest\.mark\.asyncio" -A2 packages/jumpstarter-driver-corellium echo "Searching for async def test in corellium tests..." rg "async def test" -A1 packages/jumpstarter-driver-corellium
#!/bin/bash # Verify that Corellium driver tests reference pytest-asyncio echo "Searching for pytest.mark.asyncio in corellium tests..." rg "@pytest\.mark\.asyncio" -A2 packages/jumpstarter-driver-corellium echo "Searching for async test definitions in corellium tests..." rg "async def test" -A1 packages/jumpstarter-driver-corellium
Verify pytest‑asyncio decorators in Corellium driver tests
I wasn’t able to locate any@pytest.mark.asynciousages in thepackages/jumpstarter-driver-corelliumtest files. Please confirm that all async tests for the Corellium API are marked correctly, for example:
- Add
@pytest.mark.asyncioabove eachasync def test_…function.- Or leverage the pytest‑asyncio
asynciofixture if preferred.Locations to inspect:
- packages/jumpstarter-driver-corellium/test_*.py
- Any other test modules under
packages/jumpstarter-driver-corelliumpackages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json (2)
1-5: Approve JSON Fixture for Console URL Error Response
Theget-instance-console-url-400.jsonfixture correctly simulates a 400 Invalid Type error from the Corellium API.
1-5:Details
✅ Verification successful
Verify Error Fixture Usage in Tests
Ensure that the newly added error fixture is referenced in the test suite to cover error‑handling paths.
🏁 Script executed:
#!/bin/bash # Check for usage of the error fixture in tests rg -R "get-instance-console-url-400.json"Length of output: 446
🏁 Script executed:
#!/bin/bash # Re-run search for the error fixture in the test suite using the correct rg syntax rg "get-instance-console-url-400.json"Length of output: 213
Error fixture usage verified
Theget-instance-console-url-400.jsonfixture is already referenced in tests at:
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
No further changes needed.packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (1)
236-237:Details
❌ Incorrect review comment
Fix incorrect expected URL in test
In
test_get_instance_console_url_ok, the expected URL seems to be incorrect. The line below is comparing against'wss://api-host/port-cons-1'but the fixture likely has a different URL. Double-check this value against the fixture data.
🏁 Script executed:
#!/bin/bash # Check the URL in the fixture file cat packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json | grep urlLength of output: 142
Ignore previous suggestion: expected URL is correct
The expected URL in
test_get_instance_console_url_ok('wss://api-host/port-cons-1'at lines 236–237) exactly matches the value inpackages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json. No change is needed.Likely an incorrect or invalid review comment.
packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json (1)
1-84: LGTM - Good test fixtureThe fixture provides a comprehensive representation of a Corellium instance response, including the necessary console information that will be used by the new API methods. The structure matches what would be expected from the Corellium API.
odra
force-pushed
the
corellium-serial-driver
branch
from
8e00bec to
ebaaf01
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
224-235:⚠️ Potential issueGuard against missing console ‑ same issue previously reported
The call sequence will raise
AttributeErrorwhenconsole_idisNonebecauseNonehas no.replace(). The exact concern was raised in an earlier review but is still unresolved.console_id = self.parent.api.get_instance_console_id(instance, self.parent.console_name) +if console_id is None: + raise ValueError( + f"Console '{self.parent.console_name}' not found on instance " + f"'{self.parent.device_name}'" + ) console_url = self.parent.api.get_instance_console_url(instance, console_id)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (1)
176-186:⚠️ Potential issue
instanceparameter should be anInstance, notstr(repeat finding)The method dereferences
instance.id, so passing a plain string will explode. This mirrors the unresolved comment in the previous review.- def get_instance_console_id(self, instance: str, console_name: str) -> Optional[str]: + def get_instance_console_id(self, instance: Instance, console_name: str) -> Optional[str]:Also update callers (tests & driver) accordingly.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
54-57: Duplicateclose()call
websocketsconnections are idempotent but callingclose()twice is unnecessary.- await self.conn.close() - await self.conn.close() + await self.conn.close()
107-110:⚠️ Potential issueSimplify
connect_wssto avoid redundant context & fix missing async‑resource handling
WebsocketSerialis already anAsyncResource; wrapping it in anotherasync withis superfluous and may double‑close the websocket.- async with websockets.connect(self.url) as websocket: - async with WebsocketSerial(conn=websocket) as stream: - yield stream + async with websockets.connect(self.url) as websocket: + yield WebsocketSerial(conn=websocket)
🧹 Nitpick comments (6)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
208-213: Remove trailing whitespace & polish doc‑stringThere are two
W291lint hits and the doc‑string is incomplete. Tighten this block and finish the sentence for clarity/readability.-@dataclass(kw_only=True) -class CorelliumSerial(PySerial): - """ - A PySerial subclass to handle - """ +@dataclass(kw_only=True) +class CorelliumSerial(PySerial): + """ + PySerial subclass that transparently connects to a Corellium + console (WebSocket) whose URL is discovered via the API. + """🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (2)
2-9: Prune unused imports to keep test file clean
asyncio,AsyncMock,patch, andwebsocketsare never referenced. They currently trigger RuffF401errors.-import asyncio -from unittest.mock import AsyncMock, patch -# import websockets -from .api import ApiClient, websockets +from .api import ApiClient🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
196-200: Strip trailing whitespaceRuff flags
W291here. Removing it eliminates noise in CI.- data = fixture('http/get-instance-200.json') + data = fixture('http/get-instance-200.json')🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
1-5: Drop unused imports
AsyncGeneratorandwebsocketsare never used in this module.-from typing import AsyncGenerator, Optional +from typing import Optional ... -import websockets🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
4-4:
websocketsimported but unusedRemove unused import:
websockets(F401)
165-168: Doc‑string typo breaks closing quoteThe triple‑quoted string for
destroy_instanceends with'""", leaving an unmatched single quote.- Does not return anything since Corellium's API return a HTTP 204 response. - '""" + Does not return anything since Corellium's API returns a HTTP‑204 response. + """packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
3-4: Remove unusedAnyimport-from typing import Any🧰 Tools
🪛 Ruff (0.8.2)
3-3:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py(4 hunks)packages/jumpstarter-driver-pyserial/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter-driver-corellium/pyproject.toml
- packages/jumpstarter-driver-pyserial/pyproject.toml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json
- packages/jumpstarter-driver-corellium/examples/exporter.yml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json
🧰 Additional context used
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
3-3: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
odra
force-pushed
the
corellium-serial-driver
branch
from
ebaaf01 to
61459d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
208-244:⚠️ Potential issueDataclass field ⇄ property name collision will raise
TypeErrorat import time
PySerialdeclares a dataclass field namedurl.
CorelliumSerial(itself a@dataclass) introduces a property with the same name.
During class creation,dataclassesdetects the overwrite and raises:TypeError: Cannot overwrite attribute 'url' in class 'CorelliumSerial'.Fix suggestion (one of):
-@dataclass(kw_only=True) -class CorelliumSerial(PySerial): +@dataclass(kw_only=True) +class CorelliumSerial(PySerial): + # Keep the original field but prevent it from clashing. + _dynamic_url: str = field(init=False, repr=False) - parent: Corellium + parent: Corellium + + def __post_init__(self): + super().__post_init__() + # Delay actual connection – do not rely on the bogus URL passed by the parent. + self._dynamic_url = '' - @property - def url(self) -> str: + @property + def url(self) -> str: ... - return console_url + self._dynamic_url = console_url + return console_url + + @_dynamic_url.setter + def url(self, _value: str) -> None: # noqa: D401 + # Disallow external mutation. + raise AttributeError("`url` is read‑only for CorelliumSerial")This eliminates the clash and clearly separates the internal backing field.
🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
♻️ Duplicate comments (3)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
233-235: Still missing guard for a non‑existent consolePrevious review already highlighted that
get_instance_console_idcan returnNone.
get_instance_console_urlwill then raise aTypeError. Please raise a clear, user‑facing exception whenconsole_id is None.packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
54-56: Remove duplicateclose()callThe
aclose()method callsself.conn.close()twice unnecessarily. Whilewebsocketshandles closing idempotently, one call is sufficient.- async def aclose(self): - await self.conn.close() - await self.conn.close() + async def aclose(self): + await self.conn.close()
103-109: Simplify WebSocket context handlingThe nested context managers can be simplified since
WebsocketSerialjust delegates to the websocket connection's close method.- async with websockets.connect(self.url) as websocket: - async with WebsocketSerial(conn=websocket) as stream: - yield stream + async with websockets.connect(self.url) as websocket: + yield WebsocketSerial(conn=websocket)This simplifies the code and eliminates the need for
WebsocketSerial.aclose()to callclose()at all since the outer context will handle it.
🧹 Nitpick comments (8)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
211-213: Trailing whitespace – lines flagged by Ruff (W291).
Please strip to keep the diff clean.🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (3)
1-5: Remove unused imports
AsyncGeneratorandwebsocketsare imported but never referenced; Ruff (F401) rightfully complains. Drop them to avoid dead code.-from typing import AsyncGenerator, Optional +from typing import Optional ... -import websockets🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
4-4:
websocketsimported but unusedRemove unused import:
websockets(F401)
195-207: Return type should bestr, notOptional[str]The function either returns
data['url']or raises – it never returnsNone.
Tighten the signature to improve type‑checking:-def get_instance_console_url(self, instance: Instance, console_id: str) -> Optional[str]: +def get_instance_console_url(self, instance: Instance, console_id: str) -> str:Also update the docstring accordingly.
192-193: Trailing whitespace (W293) – please remove.🧰 Tools
🪛 Ruff (0.8.2)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (2)
2-9: Prune unused test imports
asyncio,AsyncMock,patch, andwebsocketsaren’t used anywhere in this file; Ruff (F401) reports them.
Remove to keep tests lean:-import asyncio -from unittest.mock import AsyncMock, patch ... -# import websockets -from .api import ApiClient, websockets +from .api import ApiClient🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
196-199: Trailing whitespace in fixture loading blockLines 196 and 228 contain extra spaces that trigger Ruff
W291.
A quickpre-commit run --all-fileswill auto‑fix these.🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
3-3: Remove unused importThe
Anytype fromtypingis imported but not used anywhere in the code.-from typing import Any🧰 Tools
🪛 Ruff (0.8.2)
3-3:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
77-86: Clean up whitespace issuesThe implementation of the connect method looks good, but there are some whitespace issues that should be fixed.
async for stream in self.stream_handler(): yield stream - - self.logger.info("Disconnected from %s", self.url) + + self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py(4 hunks)packages/jumpstarter-driver-pyserial/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-driver-corellium/pyproject.toml
- packages/jumpstarter-driver-corellium/examples/exporter.yml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json
- packages/jumpstarter-driver-pyserial/pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/types.py (1)
Instance(51-56)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/exceptions.py (1)
CorelliumApiException(7-10)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
get_instance_console_id(176-193)get_instance_console_url(195-207)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/types.py (2)
Instance(51-56)Session(9-22)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/exceptions.py (1)
CorelliumApiException(7-10)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
3-3: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
88-101: Well-designed URL scheme handlerThe property-based approach for selecting the connection handler based on URL scheme is clean and extensible. This makes adding new connection types straightforward in the future.
111-129: Well-structured connection handlersThe specialized connection handlers for different URL schemes are well-implemented and documented. The consistent pattern makes the code easy to understand and maintain.
odra
force-pushed
the
corellium-serial-driver
branch
from
61459d7 to
5d9e56c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (2)
64-64: PySerial child is instantiated with an emptyurlbut still triggers the base-class machineryPassing an empty URL string could cause issues if
PySerial.__post_init__validates or attempts to use the URL before your override runs.Consider passing
Noneas a sentinel value, or check the PySerial implementation to ensure it won't prematurely validate the URL.- self.children['serial'] = CorelliumSerial(parent=self, url='', check_present=False) + self.children['serial'] = CorelliumSerial(parent=self, url=None, check_present=False)Run the following to verify this issue:
#!/bin/bash # Look at PySerial.__post_init__ to see if it attempts to validate the URL cat packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py | grep -A10 "__post_init__"
233-236:⚠️ Potential issueAdd error handling for a missing console
If the console specified by
console_namedoesn't exist,get_instance_console_idwill returnNone. This will lead to aTypeErrorwhen passingNonetoget_instance_console_url.console_id = self.parent.api.get_instance_console_id(instance, self.parent.console_name) + if console_id is None: + raise ValueError(f"Console '{self.parent.console_name}' does not exist on instance '{self.parent.device_name}'") console_url = self.parent.api.get_instance_console_url(instance, console_id)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
182-187:⚠️ Potential issueImprove error handling in exception handling blocks
If
requests.get()raises an exception beforeres.json()executes (e.g., network error),datais not yet assigned, causingdata.get('error', str(e))to raise anUnboundLocalErrorthat masks the real issue.try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}') data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: - raise CorelliumApiException(data.get('error', str(e))) from e + msg = str(e) + if 'data' in locals() and isinstance(data, dict): + msg = data.get('error', msg) + raise CorelliumApiException(msg) from e
200-205:⚠️ Potential issueImprove error handling in exception handling blocks
Apply the same error handling pattern to this method too.
try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}/console', params={'type': console_id.replace('port-', '')}) data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: - raise CorelliumApiException(data.get('error', str(e))) from e + msg = str(e) + if 'data' in locals() and isinstance(data, dict): + msg = data.get('error', msg) + raise CorelliumApiException(msg) from epackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
54-56: Remove duplicate close callThere are two identical calls to
await self.conn.close(). One call is sufficient sincewebsocketshandles closing idempotently.async def aclose(self): await self.conn.close() - await self.conn.close()
107-109: Simplify context handling in connect_wssNesting the
WebsocketSerialcontext inside thewebsockets.connect()context is redundant sinceWebsocketSerialmerely delegates to the sameconn.close(). You can yield it directly to avoid double-closing.async with websockets.connect(self.url) as websocket: - async with WebsocketSerial(conn=websocket) as stream: - yield stream + yield WebsocketSerial(conn=websocket)This change would also allow simplifying the
aclose()method inWebsocketSerialsince the outer context will handle closing the connection.
🧹 Nitpick comments (9)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
211-213: Complete the class docstring.The docstring for
CorelliumSerialis incomplete and doesn't explain the purpose of this class. Consider expanding it to describe how it interacts with Corellium's API to provide websocket serial connectivity.""" - A PySerial subclass to handle + A PySerial subclass to handle websocket console connections to Corellium instances. + + This class dynamically retrieves console URLs from Corellium's API based on the configured + console_name and provides a serial interface over websocket connections. """ parent: Corellium🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
2-8: Remove unused importsSeveral imports are not used in this file and should be removed to improve readability and avoid unnecessary dependencies.
import os -import asyncio -from unittest.mock import AsyncMock, patch + import pytest -# import websockets -from .api import ApiClient, websockets +from .api import ApiClient from .exceptions import CorelliumApiException from .types import Device, Instance, Project, Session🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
196-196: Remove trailing whitespace.Remove the trailing whitespace at the end of this line.
- data = fixture('http/get-instance-200.json') + data = fixture('http/get-instance-200.json')🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Remove trailing whitespace.Remove the trailing whitespace at the end of this line.
- data = fixture('http/get-instance-console-url-200.json') + data = fixture('http/get-instance-console-url-200.json')🧰 Tools
🪛 Ruff (0.8.2)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (3)
1-5: Remove unused importsSeveral imports are not used in the file and should be removed.
-from typing import AsyncGenerator, Optional +from typing import Optional import requests -import websockets + from .exceptions import CorelliumApiException🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
4-4:
websocketsimported but unusedRemove unused import:
websockets(F401)
196-198: Fix typo in docstringThere's a duplicated "a" in the docstring.
""" - Get a a console URL (websocket) to stream logs from. + Get a console URL (websocket) to stream logs from. """
192-193: Remove whitespace in blank lineRemove the trailing whitespace on the blank line.
if console['name'] == console_name: return console['id'] - + return None🧰 Tools
🪛 Ruff (0.8.2)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
3-6: Remove unused importThe import of
typing.Anyis not used and should be removed.-from typing import Any + import websockets from websockets.asyncio.client import ClientConnection as WSSClientConnection🧰 Tools
🪛 Ruff (0.8.2)
3-3:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
85-86: Remove whitespace in blank line and trailing whitespaceClean up the whitespace for better code readability.
yield stream - - self.logger.info("Disconnected from %s", self.url) + + self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(2 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py(4 hunks)packages/jumpstarter-driver-pyserial/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-driver-pyserial/pyproject.toml
- packages/jumpstarter-driver-corellium/examples/exporter.yml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json
- packages/jumpstarter-driver-corellium/pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
get_instance_console_id(176-193)get_instance_console_url(195-207)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/types.py (2)
Instance(51-56)Session(9-22)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/exceptions.py (1)
CorelliumApiException(7-10)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
213-213: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
3-3: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Trailing whitespace
Remove trailing whitespace
(W291)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
238-240:⚠️ Potential issueAdd error handling for missing console
The
urlproperty retrieves the console ID and URL, but if the console specified byconsole_namedoesn't exist,get_instance_console_idwill returnNone. This will lead to a potentialTypeErrorwhen passingNonetoget_instance_console_url.console_id = self.parent.api.get_instance_console_id(instance, self.parent.console_name) + if console_id is None: + raise ValueError(f"Console '{self.parent.console_name}' does not exist on instance '{self.parent.device_name}'") console_url = self.parent.api.get_instance_console_url(instance, console_id)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
106-108: Simplify WebSocket connection context managementNesting an
async with WebsocketSerial(...)inside thewebsockets.connect()context is redundant;WebsocketSerialsimply delegates toconn.close(). Consider simplifying this to avoid double-closing.- async with websockets.connect(self.url) as websocket: - async with WebsocketSerial(conn=websocket) as stream: - yield stream + async with websockets.connect(self.url) as websocket: + yield WebsocketSerial(conn=websocket)
🧹 Nitpick comments (7)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (3)
253-253: Fix typo in docstringThere's a typo in the docstring: "srreams" should be "streams".
- Create a websocket connection to `self.url` and srreams its output. + Create a websocket connection to `self.url` and streams its output.
247-247: Fix whitespace issuesThere are trailing whitespaces and whitespace in blank lines that should be removed.
url: str - + @exportstream @asynccontextmanager async def connect(self): ''' Create a websocket connection to `self.url` and srreams its output. ''' self.logger.info("Connecting to %s, baudrate: %d", self.url, self.baudrate) - + async with websockets.connect(self.url) as websocket: async with WebsocketClientStream(conn=websocket) as stream: yield stream - self.logger.info("Disconnected from %s", self.url) + self.logger.info("Disconnected from %s", self.url)Also applies to: 255-255, 260-260
🧰 Tools
🪛 Ruff (0.8.2)
247-247: Blank line contains whitespace
Remove whitespace from blank line
(W293)
254-259: Consider adding error handling for websocket connection failuresThe current implementation doesn't handle potential websocket connection issues. Consider adding error handling to make the driver more robust.
self.logger.info("Connecting to %s, baudrate: %d", self.url, self.baudrate) - async with websockets.connect(self.url) as websocket: - async with WebsocketClientStream(conn=websocket) as stream: - yield stream + try: + async with websockets.connect(self.url) as websocket: + async with WebsocketClientStream(conn=websocket) as stream: + yield stream + except websockets.exceptions.WebSocketException as e: + self.logger.error("WebSocket connection error: %s", str(e)) + raise ConnectionError(f"Failed to connect to {self.url}: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (2)
211-211: Fix incomplete docstringThe class docstring is incomplete and has trailing whitespace.
- A PySerial subclass to handle + A driver that provides console access to Corellium devices via WebSocket.🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
243-248: Remove or implement commented-out codeThe commented-out
url.setterseems unnecessary since you're creating a read-only property. Consider removing it completely rather than leaving commented code in the codebase.-# @url.setter -# def url(self, value: str) -> None: -# """ -# A workaround to not do anything, url is retrieved from Corellium's API. -# """ -# passpackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
84-85: Fix whitespace issuesThere are trailing whitespaces and whitespace in blank lines that should be removed.
async for stream in self.stream_handler(): yield stream - - self.logger.info("Disconnected from %s", self.url) + + self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
84-84: Blank line contains whitespace
Remove whitespace from blank line
(W293)
85-85: Trailing whitespace
Remove trailing whitespace
(W291)
92-92: Fix typo in docstringThere's a small typo in the docstring:
self.connect-serialshould beself.connect_serial(underscore instead of hyphen).- Defaults to `self.connect-serial` in case no valid handler is found. + Defaults to `self.connect_serial` in case no valid handler is found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py(2 hunks)packages/jumpstarter-driver-network/pyproject.toml(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-network/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-corellium/pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py (1)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (9)
client(71-72)send(27-28)send(45-46)receive(30-31)receive(48-49)send_eof(33-34)send_eof(51-52)aclose(36-38)aclose(54-55)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (3)
WebsocketNetwork(242-260)client(26-27)client(160-161)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
client(71-72)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (4)
get_project(56-71)get_instance(113-130)get_instance_console_id(176-193)get_instance_console_url(195-207)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py
247-247: Blank line contains whitespace
Remove whitespace from blank line
(W293)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
260-260: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
3-3: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
84-84: Blank line contains whitespace
Remove whitespace from blank line
(W293)
85-85: Trailing whitespace
Remove trailing whitespace
(W291)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py (1)
73-91: LGTM! WebsocketClientStream implementation looks solidThe
WebsocketClientStreamclass correctly implements theObjectStream[bytes]interface, mirroring similar patterns in the codebase.packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (2)
41-56: WebsocketSerial implementation looks goodThe
WebsocketSerialclass correctly implements theObjectStreaminterface with proper methods for sending and receiving data over WebSocket connections.
76-85: Good design for connection handler selectionThe refactoring to use a
stream_handlerproperty that selects the appropriate connection handler based on URL scheme is a clean and extensible approach. This makes it easy to add support for additional protocols in the future.Also applies to: 87-101
🧰 Tools
🪛 Ruff (0.8.2)
84-84: Blank line contains whitespace
Remove whitespace from blank line
(W293)
85-85: Trailing whitespace
Remove trailing whitespace
(W291)
odra
force-pushed
the
corellium-serial-driver
branch
from
9757e6c to
0c0efee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
242-260:⚠️ Potential issueMissing baudrate attribute will cause runtime error
The
WebsocketNetworkclass referencesself.baudratein theconnectmethod, but this attribute is not defined in the class, which will cause anAttributeErrorat runtime.Apply this diff to add the missing attribute:
class WebsocketNetwork(NetworkInterface, Driver): ''' Handles websocket connections from a given url. ''' url: str + baudrate: int = field(default=115200) @exportstream @asynccontextmanager🧰 Tools
🪛 Ruff (0.8.2)
247-247: Blank line contains whitespace
Remove whitespace from blank line
(W293)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
260-260: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
238-239:⚠️ Potential issueAdd error handling for missing console
The code doesn't handle the case where
get_instance_console_idreturnsNone(when the console doesn't exist), which would cause aTypeErrorwhen passingNonetoget_instance_console_url.Apply this diff to add error handling:
console_id = self.parent.api.get_instance_console_id(instance, self.parent.console_name) + if console_id is None: + raise ValueError(f"Console '{self.parent.console_name}' does not exist on instance '{self.parent.device_name}'") console_url = self.parent.api.get_instance_console_url(instance, console_id)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
182-187:⚠️ Potential issueFix potential UnboundLocalError in exception handling
If the request fails before
datais assigned, accessingdata.get('error', str(e))in the exception handler would cause anUnboundLocalError.Apply this diff to fix the issue:
try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}') data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: - raise CorelliumApiException(data.get('error', str(e))) from e + msg = str(e) + if 'data' in locals() and isinstance(data, dict): + msg = data.get('error', msg) + raise CorelliumApiException(msg) from e
199-205:⚠️ Potential issueFix potential UnboundLocalError in exception handling
Same issue as above: if the request fails before
datais assigned, accessingdata.get('error', str(e))in the exception handler would cause anUnboundLocalError.Apply this diff to fix the issue:
try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}/console', params={'type': console_id.replace('port-', '')}) data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: - raise CorelliumApiException(data.get('error', str(e))) from e + msg = str(e) + if 'data' in locals() and isinstance(data, dict): + msg = data.get('error', msg) + raise CorelliumApiException(msg) from e
🧹 Nitpick comments (10)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
252-252: Fix typo in docstringThere's a typo in the docstring: "srreams" should be "streams".
- Create a websocket connection to `self.url` and srreams its output. + Create a websocket connection to `self.url` and streams its output.
247-260: Clean up whitespace and formattingThere are several whitespace issues in this code segment that should be fixed.
url: str - + @exportstream @asynccontextmanager async def connect(self): ''' Create a websocket connection to `self.url` and srreams its output. ''' self.logger.info("Connecting to %s, baudrate: %d", self.url, self.baudrate) - + async with websockets.connect(self.url) as websocket: async with WebsocketClientStream(conn=websocket) as stream: yield stream - self.logger.info("Disconnected from %s", self.url) + self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
247-247: Blank line contains whitespace
Remove whitespace from blank line
(W293)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
260-260: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
2-8: Remove unused importsSeveral imports are never used in this file and should be removed:
asyncioAsyncMockandpatchfromunittest.mockwebsocketsfrom.apiimport os -import asyncio -from unittest.mock import AsyncMock, patch import pytest -# import websockets -from .api import ApiClient, websockets +from .api import ApiClient from .exceptions import CorelliumApiException from .types import Device, Instance, Project, Session🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
196-196: Remove trailing whitespaceRemove trailing whitespace from this line.
- data = fixture('http/get-instance-200.json') + data = fixture('http/get-instance-200.json')🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Remove trailing whitespaceRemove trailing whitespace from this line.
- data = fixture('http/get-instance-console-url-200.json') + data = fixture('http/get-instance-console-url-200.json')🧰 Tools
🪛 Ruff (0.8.2)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (2)
210-212: Complete the class docstringThe docstring for
CorelliumConsoleis incomplete and ends abruptly.""" - A PySerial subclass to handle + A PySerial subclass to handle console serial connections over websockets. + + This class extends WebsocketNetwork and dynamically retrieves the websocket + URL from the Corellium API to establish a serial connection. """🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
243-248: Remove commented-out codeThe commented-out
urlsetter method is unnecessary and should be removed. If you need to prevent setting the URL, consider using thepropertydecorator without implementing a setter.-# @url.setter -# def url(self, value: str) -> None: -# """ -# A workaround to not do anything, url is retrieved from Corellium's API. -# """ -# passpackages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (3)
197-197: Fix typo in docstringThere's a typo in the docstring: "Get a a console URL".
- Get a a console URL (websocket) to stream logs from. + Get a console URL (websocket) to stream logs from.
1-4: Remove unused importsThe imports
AsyncGeneratorandwebsocketsare never used in this file.-from typing import AsyncGenerator, Optional +from typing import Optional import requests -import websockets🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
4-4:
websocketsimported but unusedRemove unused import:
websockets(F401)
192-192: Remove whitespace from blank lineRemove whitespace from the blank line.
for console in data.get('consoles', []): if console['name'] == console_name: return console['id'] - + return None🧰 Tools
🪛 Ruff (0.8.2)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(2 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py(2 hunks)packages/jumpstarter-driver-network/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter-driver-corellium/examples/exporter.yml
- packages/jumpstarter-driver-network/pyproject.toml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json
- packages/jumpstarter-driver-corellium/pyproject.toml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json
- packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
get_instance_console_id(176-193)get_instance_console_url(195-207)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/types.py (2)
Instance(51-56)Session(9-22)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/exceptions.py (1)
CorelliumApiException(7-10)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py (1)
WebsocketClientStream(74-90)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(42-250)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
connect(55-69)packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver.py (1)
connect(77-80)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py
247-247: Blank line contains whitespace
Remove whitespace from blank line
(W293)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
260-260: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (9)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (4)
173-173: Fixture path correction looks goodThe fixture path correction from
get-instance-state-404.jsontoget-instance-404.jsonappears appropriate.
188-204: Good test coverage for console ID retrievalThe parameterized tests for
get_instance_console_idproperly verify both successful console ID retrieval and handling of non-existent console names.🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
226-238: Good test for console URL retrievalThis test correctly verifies that the URL is properly retrieved and formatted from the API response.
🧰 Tools
🪛 Ruff (0.8.2)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
241-259: Comprehensive error testingThe parameterized tests for error cases cover all relevant HTTP status codes (400, 403, 404) and error messages. This ensures the API client properly handles and reports errors.
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (3)
214-214: Baudrate setting looks goodThe
baudrateattribute with default value 115200 is correctly implemented. This properly addresses the issue in the parentWebsocketNetworkclass where this attribute was missing.
31-31: Good default console nameAdding the
console_namefield with a sensible default value provides good flexibility for configuration.
64-64: Good implementation of the serial driverAdding the
CorelliumConsoleas a child driver under the key 'serial' aligns well with the PR objective of supporting console serial connections over websockets.packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
176-181: Good implementation of console ID retrievalThe method to retrieve a console ID by name is well-implemented and handles the case where the console doesn't exist by returning
None.
195-196: Good implementation of console URL retrievalThe method to retrieve a console URL is well-implemented and correctly extracts the console type from the ID.
odra
force-pushed
the
corellium-serial-driver
branch
from
0c0efee to
91d9f27
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (1)
238-240:⚠️ Potential issueAdd error handling for missing console.
The code doesn't check if
console_idis None before passing it toget_instance_console_url(). If the specified console doesn't exist, this will cause a runtime error.console_id = self.parent.api.get_instance_console_id(instance, self.parent.console_name) + if console_id is None: + raise ValueError(f"Console '{self.parent.console_name}' not found for instance '{self.parent.device_name}'") console_url = self.parent.api.get_instance_console_url(instance, console_id)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
176-187: Fix potentialUnboundLocalErrorin exception handlingIf the request fails before
res.json()is executed,datawould be undefined in the except block, causing anUnboundLocalError.Apply the same fix pattern as previously suggested:
try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}') data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: - raise CorelliumApiException(data.get('error', str(e))) from e + msg = str(e) + if 'data' in locals() and isinstance(data, dict): + msg = data.get('error', msg) + raise CorelliumApiException(msg) from e
195-207: Fix potentialUnboundLocalErrorin exception handlingThe
get_instance_console_urlmethod has the same issue with exception handling as the previous method.Apply the same fix pattern here:
try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}/console', params={'type': console_id.replace('port-', '')}) data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: - raise CorelliumApiException(data.get('error', str(e))) from e + msg = str(e) + if 'data' in locals() and isinstance(data, dict): + msg = data.get('error', msg) + raise CorelliumApiException(msg) from e
🧹 Nitpick comments (12)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
153-161: Fix the unused variable and trailing whitespace.The test correctly verifies the WebsocketNetwork's connect method, but there are two minor issues:
- The variable
cis assigned but never used- There's trailing whitespace on lines 156 and 161
@pytest.mark.asyncio async def test_websocket_network_connect(): ws = AsyncMock() - ws.__aenter__.return_value = ws + ws.__aenter__.return_value = ws with patch("websockets.connect", return_value=ws) as m: client = WebsocketNetwork(url="ws://localhost/something") - async with client.connect() as c: - m.assert_called_once_with("ws://localhost/something") + async with client.connect() as _: + m.assert_called_once_with("ws://localhost/something")🧰 Tools
🪛 Ruff (0.8.2)
156-156: Trailing whitespace
Remove trailing whitespace
(W291)
160-160: Local variable
cis assigned to but never usedRemove assignment to unused variable
c(F841)
161-161: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
242-261: Fix typo in docstring and whitespace issues.The WebsocketNetwork implementation has a few minor issues:
- There's a typo in the connect method docstring
- There are whitespace issues in blank lines and trailing whitespace
@dataclass(kw_only=True) class WebsocketNetwork(NetworkInterface, Driver): ''' Handles websocket connections from a given url. ''' url: str - + @exportstream @asynccontextmanager async def connect(self): ''' - Create a websocket connection to `self.url` and srreams its output. + Create a websocket connection to `self.url` and streams its output. ''' self.logger.info("Connecting to %s", self.url) - + async with websockets.connect(self.url) as websocket: async with WebsocketClientStream(conn=websocket) as stream: yield stream - self.logger.info("Disconnected from %s", self.url) + self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
248-248: Blank line contains whitespace
Remove whitespace from blank line
(W293)
256-256: Blank line contains whitespace
Remove whitespace from blank line
(W293)
261-261: Trailing whitespace
Remove trailing whitespace
(W291)
251-261: Add error handling for websocket connection failures.The current implementation doesn't handle potential exceptions from the websocket connection. It's important to catch and properly log any connection errors.
@asynccontextmanager async def connect(self): ''' Create a websocket connection to `self.url` and srreams its output. ''' self.logger.info("Connecting to %s", self.url) - async with websockets.connect(self.url) as websocket: - async with WebsocketClientStream(conn=websocket) as stream: - yield stream + try: + async with websockets.connect(self.url) as websocket: + async with WebsocketClientStream(conn=websocket) as stream: + yield stream + except Exception as e: + self.logger.error("Failed to connect to %s: %s", self.url, str(e)) + raise self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
256-256: Blank line contains whitespace
Remove whitespace from blank line
(W293)
261-261: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (3)
2-8: Remove unused imports.There are several imports that aren't used in the module:
import os -import asyncio -from unittest.mock import AsyncMock, patch +# No unused imports import pytest -# import websockets -from .api import ApiClient, websockets +from .api import ApiClient from .exceptions import CorelliumApiException from .types import Device, Instance, Project, Session🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
196-204: Fix trailing whitespace in the test function.There's trailing whitespace in this line that should be removed.
@pytest.mark.parametrize( 'console_name,console_id', [ ('Console 1', 'port-1-cons',), ('Console 10', None,), ('Console 2', 'port-2-cons',), ]) def test_get_instance_console_id_ok(requests_mock, console_name, console_id): - data = fixture('http/get-instance-200.json') + data = fixture('http/get-instance-200.json') instance = Instance(id='d59db33d-27bd-4b22-878d-49e4758a648e') requests_mock.get(f'https://api-host/api/v1/instances/{instance.id}', status_code=200, text=data)🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-238: Fix trailing whitespace and verify URL in test.The test contains trailing whitespace and uses a hardcoded expected URL. Consider extracting the URL from the fixture for more maintainable tests.
def test_get_instance_console_url_ok(requests_mock): console_id = 'port-1-cons' - data = fixture('http/get-instance-console-url-200.json') + data = fixture('http/get-instance-console-url-200.json') instance = Instance(id='d59db33d-27bd-4b22-878d-49e4758a648e') requests_mock.get(f'https://api-host/api/v1/instances/{instance.id}/console?type=1-cons', status_code=200, text=data) api = ApiClient('api-host', 'api-token') api.session = Session('session-token', '2022-03-20T01:50:10.000Z') current = api.get_instance_console_url(instance, console_id) - expected = 'wss://api-host/port-cons-1' + # Extract expected URL from fixture instead of hardcoding + import json + expected = json.loads(data)['url'] assert expected == current🧰 Tools
🪛 Ruff (0.8.2)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (2)
208-214: Complete the class docstring.The docstring for CorelliumConsole is incomplete.
@dataclass(kw_only=True) class CorelliumConsole(WebsocketNetwork): """ - A PySerial subclass to handle + A driver to handle Corellium console connections over websockets. + + This class extends WebsocketNetwork to interface with Corellium's + console endpoints via websockets, allowing serial communication + through a PySerial client. """ parent: Corellium baudrate: int = field(default=115200)🧰 Tools
🪛 Ruff (0.8.2)
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
243-248: Remove or document commented-out code.The file contains commented-out code for a URL setter that doesn't do anything. Either remove it entirely or add a comment explaining why it's kept.
-# @url.setter -# def url(self, value: str) -> None: -# """ -# A workaround to not do anything, url is retrieved from Corellium's API. -# """ -# pass + # Note: No setter for url is implemented as the URL should always + # be dynamically fetched from the Corellium API at runtimepackages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (4)
1-4: Remove unused importsThe imports
AsyncGeneratorandwebsocketsare currently not being used in this file. While they're likely needed for the websocket functionality implemented elsewhere in the PR, they should be removed from this file unless they'll be used here.-from typing import AsyncGenerator, Optional +from typing import Optional import requests -import websockets🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
4-4:
websocketsimported but unusedRemove unused import:
websockets(F401)
192-192: Remove whitespace from blank lineLine 192 contains whitespace in a blank line.
for console in data.get('consoles', []): if console['name'] == console_name: return console['id'] - + return None🧰 Tools
🪛 Ruff (0.8.2)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
197-198: Fix typo in docstringThere's a small typo in the method docstring.
""" -Get a a console URL (websocket) to stream logs from. +Get a console URL (websocket) to stream logs from. """
176-193: Enhance error handling for non-existing consolesThe method returns
Nonewhen a console with the specified name doesn't exist, but this might not be the most informative way to handle this scenario. Consider whether raising a specific exception with a clear message would be more helpful when a requested console doesn't exist.def get_instance_console_id(self, instance: Instance, console_name: str) -> Optional[str]: """ Retrieve an instance's console id by its name. Return None if it does not exist. """ try: res = self.req.get(f'{self.baseurl}/v1/instances/{instance.id}') data = res.json() res.raise_for_status() except requests.exceptions.RequestException as e: msg = str(e) if 'data' in locals() and isinstance(data, dict): msg = data.get('error', msg) raise CorelliumApiException(msg) from e for console in data.get('consoles', []): if console['name'] == console_name: return console['id'] + # Optional enhancement: raise specific exception when console doesn't exist + # raise CorelliumApiException(f"Console '{console_name}' not found for instance {instance.id}") return None🧰 Tools
🪛 Ruff (0.8.2)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(2 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py(2 hunks)packages/jumpstarter-driver-network/pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json
- packages/jumpstarter-driver-network/pyproject.toml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json
- packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py
- packages/jumpstarter-driver-corellium/pyproject.toml
- packages/jumpstarter-driver-corellium/examples/exporter.yml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
WebsocketNetwork(243-261)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (4)
get_project(56-71)get_instance(113-130)get_instance_console_id(176-193)get_instance_console_url(195-207)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py
156-156: Trailing whitespace
Remove trailing whitespace
(W291)
160-160: Local variable c is assigned to but never used
Remove assignment to unused variable c
(F841)
161-161: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
211-211: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py
248-248: Blank line contains whitespace
Remove whitespace from blank line
(W293)
256-256: Blank line contains whitespace
Remove whitespace from blank line
(W293)
261-261: Trailing whitespace
Remove trailing whitespace
(W291)
odra
force-pushed
the
corellium-serial-driver
branch
from
91d9f27 to
fb26ff9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver_test.py (1)
176-204:⚠️ Potential issueFix incorrect patch for console URL method.
There's an issue in the test function where you're patching the same method twice. Line 201 is incorrectly patching
get_instance_console_idagain instead of patchingget_instance_console_url.- patch.object(root._api, 'get_instance_console_id', + patch.object(root._api, 'get_instance_console_url', **mock_data.get('get_instance_console_url', {'return_value': 'ws://mock'}))):Also, note that the static analysis tool flags line 203 as a "useless expression" but this is a false positive. The expression
console.urlis intentionally used without assignment because the test is verifying that accessing this property raises an exception, which is captured by thepytest.raisescontext manager.🧰 Tools
🪛 Ruff (0.8.2)
203-203: Found useless expression. Either assign it to a variable or remove it.
(B018)
🧹 Nitpick comments (5)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (1)
1-1: Unused imports should be removedThe imported
AsyncGeneratorfromtypingandwebsocketsmodule are not used in this file.-from typing import AsyncGenerator, Optional +from typing import Optional import requests -import websocketsAlso applies to: 4-4
🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.AsyncGeneratorimported but unusedRemove unused import:
typing.AsyncGenerator(F401)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (1)
1-8: Unused imports should be removedSeveral imported modules aren't used in the file:
asyncio,AsyncMock,patch, andwebsocketsfrom.api.import os -import asyncio -from unittest.mock import AsyncMock, patch import pytest # import websockets -from .api import ApiClient, websockets +from .api import ApiClient from .exceptions import CorelliumApiException from .types import Device, Instance, Project, Session🧰 Tools
🪛 Ruff (0.8.2)
2-2:
asyncioimported but unusedRemove unused import:
asyncio(F401)
3-3:
unittest.mock.AsyncMockimported but unusedRemove unused import
(F401)
3-3:
unittest.mock.patchimported but unusedRemove unused import
(F401)
8-8:
.api.websocketsimported but unusedRemove unused import:
.api.websockets(F401)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
153-161: Unused variable in async testThe local variable
cin the context manager is assigned but never used, and there's trailing whitespace in the code.@pytest.mark.asyncio async def test_websocket_network_connect(): ws = AsyncMock() - ws.__aenter__.return_value = ws + ws.__aenter__.return_value = ws with patch("websockets.connect", return_value=ws) as m: client = WebsocketNetwork(url="ws://localhost/something") - async with client.connect() as c: - m.assert_called_once_with("ws://localhost/something") + async with client.connect(): + m.assert_called_once_with("ws://localhost/something")🧰 Tools
🪛 Ruff (0.8.2)
156-156: Trailing whitespace
Remove trailing whitespace
(W291)
160-160: Local variable
cis assigned to but never usedRemove assignment to unused variable
c(F841)
161-161: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
252-254: Fix typo in docstringThere's a typo in the docstring where "srreams" should be "streams".
''' - Create a websocket connection to `self.url` and srreams its output. + Create a websocket connection to `self.url` and streams its output. '''
255-261: LGTM! Well-implemented connect method with proper loggingThe connect method properly establishes the websocket connection and wraps it in a WebsocketClientStream.
Remove trailing whitespace and whitespace in blank lines:
self.logger.info("Connecting to %s", self.url) - + async with websockets.connect(self.url) as websocket: async with WebsocketClientStream(conn=websocket) as stream: yield stream - self.logger.info("Disconnected from %s", self.url) + self.logger.info("Disconnected from %s", self.url)🧰 Tools
🪛 Ruff (0.8.2)
256-256: Blank line contains whitespace
Remove whitespace from blank line
(W293)
261-261: Trailing whitespace
Remove trailing whitespace
(W291)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
packages/jumpstarter-driver-corellium/examples/exporter.yml(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json(1 hunks)packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json(1 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py(9 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py(3 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py(4 hunks)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver_test.py(2 hunks)packages/jumpstarter-driver-corellium/pyproject.toml(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py(2 hunks)packages/jumpstarter-driver-network/pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/jumpstarter-driver-corellium/examples/exporter.yml
- packages/jumpstarter-driver-network/pyproject.toml
- packages/jumpstarter-driver-corellium/pyproject.toml
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-200.json
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-console-url-400.json
- packages/jumpstarter-driver-network/jumpstarter_driver_network/streams/websocket.py
- packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py
- packages/jumpstarter-driver-corellium/fixtures/http/get-instance-200.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/exceptions.py (1)
CorelliumApiException(7-10)packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/types.py (1)
Instance(51-56)
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver_test.py
203-203: Found useless expression. Either assign it to a variable or remove it.
(B018)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py
1-1: typing.AsyncGenerator imported but unused
Remove unused import: typing.AsyncGenerator
(F401)
4-4: websockets imported but unused
Remove unused import: websockets
(F401)
53-53: Blank line contains whitespace
Remove whitespace from blank line
(W293)
71-71: Blank line contains whitespace
Remove whitespace from blank line
(W293)
208-208: Local variable msgerr is assigned to but never used
Remove assignment to unused variable msgerr
(F841)
215-215: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import
(F401)
3-3: unittest.mock.patch imported but unused
Remove unused import
(F401)
8-8: .api.websockets imported but unused
Remove unused import: .api.websockets
(F401)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py
248-248: Blank line contains whitespace
Remove whitespace from blank line
(W293)
256-256: Blank line contains whitespace
Remove whitespace from blank line
(W293)
261-261: Trailing whitespace
Remove trailing whitespace
(W291)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py
156-156: Trailing whitespace
Remove trailing whitespace
(W291)
160-160: Local variable c is assigned to but never used
Remove assignment to unused variable c
(F841)
161-161: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (10)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api.py (2)
42-54: LGTM! Improved error handling with data initializationThe addition of
data = Nonebefore the API call and the improved error handling pattern prevents potentialUnboundLocalErrorwhen the request fails before JSON data is obtained.🧰 Tools
🪛 Ruff (0.8.2)
53-53: Blank line contains whitespace
Remove whitespace from blank line
(W293)
218-234: LGTM! Well-implemented console URL retrievalThe method correctly handles the console ID transformation by removing the "port-" prefix and properly formats the API request.
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/corellium/api_test.py (5)
173-173: LGTM! Fixed fixture path referenceThe test now correctly uses the
get-instance-404.jsonfixture for testing instance deletion error handling.
187-205: LGTM! Comprehensive test coverage for console ID retrievalThe parameterized test cases cover important scenarios like found and not found console IDs.
🧰 Tools
🪛 Ruff (0.8.2)
196-196: Trailing whitespace
Remove trailing whitespace
(W291)
206-224: LGTM! Thorough error handling tests for console ID retrievalTests for both authorization errors (403) and not found errors (404) ensure robust error handling.
226-239: LGTM! Validates console URL retrieval functionalityThe test correctly verifies that the API method returns the expected websocket URL.
🧰 Tools
🪛 Ruff (0.8.2)
228-228: Trailing whitespace
Remove trailing whitespace
(W291)
241-259: LGTM! Comprehensive error case testing for console URLTests cover all important error cases: invalid type (400), authorization (403), and not found (404).
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
242-248: LGTM! New WebsocketNetwork class implementationThe
WebsocketNetworkclass properly implements the required interfaces for network connectivity.🧰 Tools
🪛 Ruff (0.8.2)
248-248: Blank line contains whitespace
Remove whitespace from blank line
(W293)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver_test.py (2)
7-7: LGTM: Appropriate import added.The import of
CorelliumConsoleis necessary for the new test functions being added.
159-174: LGTM: Well-structured test for the URL retrieval feature.This test properly verifies that the console URL can be retrieved successfully and follows the same pattern as other tests in the file. The test mocks all necessary API calls and validates the expected output.
odra
force-pushed
the
corellium-serial-driver
branch
3 times, most recently
from
a380e97 to
c30f850
Compare
Signed-off-by: Leonardo Rossetti <lrossett@redhat.com>
NickCao
force-pushed
the
corellium-serial-driver
branch
from
c30f850 to
e8061d7
Compare
3 participants
Changes
Testing
Create a file like this (set proper values for project_id and instance name):
Start the jmp shell:
Power it on and start the shell:
It should start to stream the remote instance into your local console.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores