Initial implementation of simradio-based testing with a multi-node simulated mesh#946
Open
ianmcorvidae wants to merge 11 commits into
Open
Initial implementation of simradio-based testing with a multi-node simulated mesh#946ianmcorvidae wants to merge 11 commits into
ianmcorvidae wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds meshtasticd-backed simulator test infrastructure, simulator-driven smoke tests, CI/pytest wiring, CLI modem preset flags, and a ChangesFirmware Simulator Test Infrastructure
Estimated code review effort: 4 (Complex) | ~75 minutes Node.deleteChannel Write Optimization
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant pytest
participant SimMesh
participant meshtasticd
GitHubActions->>pytest: run smokevirt or smokemesh
pytest->>SimMesh: start fixtures
SimMesh->>meshtasticd: launch simulator nodes
pytest-->>GitHubActions: report results
Related Issues: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #946 +/- ##
==========================================
+ Coverage 66.84% 67.38% +0.53%
==========================================
Files 25 25
Lines 4741 4752 +11
==========================================
+ Hits 3169 3202 +33
+ Misses 1572 1550 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
meshtastic/tests/test_smokevirt.py (2)
244-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused helper
_set_and_verify.This wrapper around
cli_then_verifyis defined but not called anywhere in the file; all tests callcli_then_verifydirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/tests/test_smokevirt.py` around lines 244 - 246, The helper _set_and_verify is unused and duplicates cli_then_verify, so remove the dead wrapper from the test file and update any remaining references if needed to call cli_then_verify directly. If you want to keep it for future use, add a real caller; otherwise delete the _set_and_verify function to avoid stale, misleading test helpers.
542-559: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer unpacking over list concatenation.
Ruff flags
["--pos-fields"] + _POS_FIELDS_INPUT(line 558);[*_POS_FIELDS_INPUT]-style unpacking is the idiomatic form.Suggested fix
cli_then_verify( firmware_node.port, - ["--pos-fields"] + _POS_FIELDS_INPUT, + ["--pos-fields", *_POS_FIELDS_INPUT], check, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/tests/test_smokevirt.py` around lines 542 - 559, The smoke test in test_smokevirt_pos_fields builds the CLI arguments with list concatenation, which Ruff prefers to be unpacked idiomatically. Update the cli_then_verify call in test_smokevirt_pos_fields to pass the --pos-fields arguments using unpacking with _POS_FIELDS_INPUT instead of ["--pos-fields"] + _POS_FIELDS_INPUT, keeping the rest of the test logic unchanged.Source: Linters/SAST tools
.github/workflows/ci.yml (1)
84-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated poetry setup vs the
validatejob.The
poetry install/version-check steps mirror thevalidatejob almost verbatim (differing only by--all-extras --with dev). Consider extracting a reusable composite action or workflow template if more jobs adopt this pattern, though it's not urgent for two jobs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 84 - 95, The Python/Poetry setup in the CI workflow duplicates the same install/version-check flow used by the validate job, so consolidate the shared setup behind a reusable workflow, composite action, or a single common step sequence. Update the job that runs the meshtastic install to reuse that shared logic and keep only the job-specific Poetry options such as all-extras and dev, using the existing validate job and its Poetry install steps as the reference points.meshtastic/tests/test_smokemesh.py (1)
94-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
test_smokemesh_show_nodesmakes no assertions.The test only invokes
showNodes()on each interface without validating output content, so it only verifies the call doesn't raise — it wouldn't catch missing/incorrect node entries.♻️ Suggested improvement using capsys
`@pytest.mark.smokemesh` -def test_smokemesh_show_nodes(firmware_mesh): +def test_smokemesh_show_nodes(firmware_mesh, capsys): """showNodes should report the other nodes in the mesh.""" for i in range(3): iface = firmware_mesh.get_iface(i) iface.showNodes() + out = capsys.readouterr().out + assert out.strip(), f"node {i} showNodes produced no output"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/tests/test_smokemesh.py` around lines 94 - 99, test_smokemesh_show_nodes only calls showNodes() and never verifies behavior, so add assertions to confirm the printed output contains the expected node information. Update the test to use capsys around firmware_mesh.get_iface(i).showNodes() and assert the captured output includes the other mesh nodes, so the test validates content rather than just absence of exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 84-89: The checkout step in the CI workflow is persisting
credentials by default, which should be disabled for this job. Update the
actions/checkout usage in the workflow to set persist-credentials to false,
keeping the token from being available after checkout while the job continues
with the PPA and meshtasticd installation steps.
In `@meshtastic/tests/conftest.py`:
- Around line 34-65: SimMesh startup can fail after some meshtasticd processes
are already launched, leaving orphaned nodes because SimMesh.stop() exits early
before _started is set. Update SimMesh.start() and/or the startup path in
firmware_harness.py to track partially started nodes and invoke cleanup on any
failure from connect() or later initialization steps. Use the SimMesh.start,
SimMesh.stop, and connect flow to ensure already-started processes are stopped
even when startup does not complete.
In `@meshtastic/tests/firmware_harness.py`:
- Around line 193-212: Partial startup failures in
`firmware_harness.FirmwareMesh.start` leave previously launched nodes running
because `_started` stays false and `stop()` won’t clean them up. Wrap the
`node.start()`/`node.connect()` setup in `start()` with failure handling that
stops any nodes already started and clears their resources before re-raising.
Use the existing `FirmwareMesh.start`, `FirmwareMesh.stop`, and node lifecycle
methods to ensure partial initialization is always torn down.
In `@meshtastic/tests/test_node.py`:
- Around line 729-742: The test in Node.deleteChannel’s primary-slot rejection
path uses an equality check for the exception type, which triggers Ruff E721.
Update the assertion in test_delete_channel_rejects_primary to compare
pytest_wrapped_e.type to SystemExit with identity semantics instead of equality,
keeping the rest of the SystemExit validation unchanged.
In `@meshtastic/tests/test_smokemesh.py`:
- Around line 102-128: The smokemesh traceroute test is using a fixed sleep and
an unguarded traceroute payload lookup. In
test_smokemesh_traceroute_across_relay, replace the time.sleep delay with
PacketCollector.wait_for(1) so the test waits deterministically for traceroute
packets, and update the a_resp decoded traceroute access to use a guarded
assertion-friendly lookup so missing payloads fail through the test assertion
rather than a KeyError.
In `@meshtastic/tests/test_smokevirt.py`:
- Around line 394-432: `test_smokevirt_seturl_default` currently bypasses the
`--seturl` CLI path by calling `iface.localNode.setURL` directly, so it does not
test argument parsing or CLI behavior. Update the test to use `run_cli` for the
URL mutation, then reconnect or use a fresh interface to verify the resulting
URL via `firmware_node`/`iface.localNode.getURL`, keeping the existing
payload-decoding assertions intact.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 84-95: The Python/Poetry setup in the CI workflow duplicates the
same install/version-check flow used by the validate job, so consolidate the
shared setup behind a reusable workflow, composite action, or a single common
step sequence. Update the job that runs the meshtastic install to reuse that
shared logic and keep only the job-specific Poetry options such as all-extras
and dev, using the existing validate job and its Poetry install steps as the
reference points.
In `@meshtastic/tests/test_smokemesh.py`:
- Around line 94-99: test_smokemesh_show_nodes only calls showNodes() and never
verifies behavior, so add assertions to confirm the printed output contains the
expected node information. Update the test to use capsys around
firmware_mesh.get_iface(i).showNodes() and assert the captured output includes
the other mesh nodes, so the test validates content rather than just absence of
exceptions.
In `@meshtastic/tests/test_smokevirt.py`:
- Around line 244-246: The helper _set_and_verify is unused and duplicates
cli_then_verify, so remove the dead wrapper from the test file and update any
remaining references if needed to call cli_then_verify directly. If you want to
keep it for future use, add a real caller; otherwise delete the _set_and_verify
function to avoid stale, misleading test helpers.
- Around line 542-559: The smoke test in test_smokevirt_pos_fields builds the
CLI arguments with list concatenation, which Ruff prefers to be unpacked
idiomatically. Update the cli_then_verify call in test_smokevirt_pos_fields to
pass the --pos-fields arguments using unpacking with _POS_FIELDS_INPUT instead
of ["--pos-fields"] + _POS_FIELDS_INPUT, keeping the rest of the test logic
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: db37b9e9-10fb-4ad6-88db-88ae9c7eee05
📒 Files selected for processing (9)
.github/workflows/ci.ymlmeshtastic/node.pymeshtastic/tests/conftest.pymeshtastic/tests/firmware_harness.pymeshtastic/tests/fw_helpers.pymeshtastic/tests/test_node.pymeshtastic/tests/test_smokemesh.pymeshtastic/tests/test_smokevirt.pypytest.ini
Comment on lines
+34
to
+65
| @pytest.fixture(scope="function") | ||
| def firmware_node(): | ||
| """A single meshtasticd sim node for smokevirt tests. | ||
|
|
||
| Function-scoped so every test gets a freshly-erased node with no | ||
| state leaking from previous tests. This makes destructive commands | ||
| (``--reboot``, ``--set factory_reset true``) safe to run and lets | ||
| tests be order-independent. | ||
|
|
||
| Yields the SimNode instance. The node is booted with a fresh erased | ||
| config and listens on localhost at its TCP port. | ||
| """ | ||
| _skip_firmware_if_unavailable() | ||
| mesh = SimMesh(n_nodes=1, base_port=SINGLE_NODE_BASE_PORT) | ||
| mesh.start() | ||
| yield mesh.get_node(0) | ||
| mesh.stop() | ||
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
| def firmware_mesh(): | ||
| """A 3-node chain (A-B-C) meshtasticd sim mesh for smokemesh tests. | ||
|
|
||
| Yields the SimMesh instance. Nodes are connected and the SIMULATOR_APP | ||
| packet bridge is running. Node DB convergence is awaited. | ||
| """ | ||
| _skip_firmware_if_unavailable() | ||
| mesh = SimMesh(n_nodes=3, topology=CHAIN_TOPOLOGY) | ||
| mesh.start() | ||
| mesh.wait_for_convergence(timeout=30) | ||
| yield mesh | ||
| mesh.stop() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SimMesh.start/stop and SimNode.start/close for partial-failure cleanup handling
fd firmware_harness.py meshtastic/tests --exec cat -n {}Repository: meshtastic/python
Length of output: 14743
Handle partial startup cleanup in SimMesh.start()
mesh.start() can fail after some meshtasticd processes are already running, but SimMesh.stop() returns early until _started is set. Clean up already-started nodes inside firmware_harness.py so a failed connect() or later startup step doesn’t leave orphaned processes behind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshtastic/tests/conftest.py` around lines 34 - 65, SimMesh startup can fail
after some meshtasticd processes are already launched, leaving orphaned nodes
because SimMesh.stop() exits early before _started is set. Update
SimMesh.start() and/or the startup path in firmware_harness.py to track
partially started nodes and invoke cleanup on any failure from connect() or
later initialization steps. Use the SimMesh.start, SimMesh.stop, and connect
flow to ensure already-started processes are stopped even when startup does not
complete.
Comment on lines
+193
to
+212
| def start(self) -> None: | ||
| """Launch all nodes, connect, and start the packet bridge.""" | ||
| binary = find_meshtasticd() | ||
| if binary is None: | ||
| raise RuntimeError( | ||
| "meshtasticd not found. Set MESHTASTICD_BIN or install it on PATH." | ||
| ) | ||
|
|
||
| for node in self.nodes: | ||
| node.start(binary) | ||
|
|
||
| for node in self.nodes: | ||
| node.connect() | ||
| self._port_to_idx[node.port] = node.node_id | ||
|
|
||
| pub.subscribe(self._on_sim_packet, "meshtastic.receive.simulator") | ||
| self._started = True | ||
|
|
||
| if self.n_nodes > 1: | ||
| self._trigger_convergence() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Partial startup failure leaks meshtasticd processes.
If node.start() (or node.connect()) raises after earlier nodes have already launched, the exception propagates with self._started still False. The firmware_mesh/firmware_node fixtures only call mesh.stop() after yield, and stop() early-returns when _started is False, so the already-launched simulator processes (and their temp dirs) are never cleaned up and become orphans that also hold onto the TCP ports.
Wrap startup so any already-started nodes are torn down before re-raising.
🔧 Proposed cleanup on failure
- for node in self.nodes:
- node.start(binary)
-
- for node in self.nodes:
- node.connect()
- self._port_to_idx[node.port] = node.node_id
-
- pub.subscribe(self._on_sim_packet, "meshtastic.receive.simulator")
- self._started = True
+ try:
+ for node in self.nodes:
+ node.start(binary)
+
+ for node in self.nodes:
+ node.connect()
+ self._port_to_idx[node.port] = node.node_id
+ except Exception:
+ for node in self.nodes:
+ node.close()
+ raise
+
+ pub.subscribe(self._on_sim_packet, "meshtastic.receive.simulator")
+ self._started = True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def start(self) -> None: | |
| """Launch all nodes, connect, and start the packet bridge.""" | |
| binary = find_meshtasticd() | |
| if binary is None: | |
| raise RuntimeError( | |
| "meshtasticd not found. Set MESHTASTICD_BIN or install it on PATH." | |
| ) | |
| for node in self.nodes: | |
| node.start(binary) | |
| for node in self.nodes: | |
| node.connect() | |
| self._port_to_idx[node.port] = node.node_id | |
| pub.subscribe(self._on_sim_packet, "meshtastic.receive.simulator") | |
| self._started = True | |
| if self.n_nodes > 1: | |
| self._trigger_convergence() | |
| def start(self) -> None: | |
| """Launch all nodes, connect, and start the packet bridge.""" | |
| binary = find_meshtasticd() | |
| if binary is None: | |
| raise RuntimeError( | |
| "meshtasticd not found. Set MESHTASTICD_BIN or install it on PATH." | |
| ) | |
| try: | |
| for node in self.nodes: | |
| node.start(binary) | |
| for node in self.nodes: | |
| node.connect() | |
| self._port_to_idx[node.port] = node.node_id | |
| except Exception: | |
| for node in self.nodes: | |
| node.close() | |
| raise | |
| pub.subscribe(self._on_sim_packet, "meshtastic.receive.simulator") | |
| self._started = True | |
| if self.n_nodes > 1: | |
| self._trigger_convergence() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshtastic/tests/firmware_harness.py` around lines 193 - 212, Partial startup
failures in `firmware_harness.FirmwareMesh.start` leave previously launched
nodes running because `_started` stays false and `stop()` won’t clean them up.
Wrap the `node.start()`/`node.connect()` setup in `start()` with failure
handling that stops any nodes already started and clears their resources before
re-raising. Use the existing `FirmwareMesh.start`, `FirmwareMesh.stop`, and node
lifecycle methods to ensure partial initialization is always torn down.
Comment on lines
+102
to
+128
| @pytest.mark.smokemesh | ||
| def test_smokemesh_traceroute_across_relay(firmware_mesh): | ||
| """Traceroute from A to C should show route via B in both directions.""" | ||
| col_a = subscribe_traceroutes(firmware_mesh.get_iface(0)) | ||
| col_c = subscribe_traceroutes(firmware_mesh.get_iface(2)) | ||
| try: | ||
| src_a = firmware_mesh.get_node(0).node_num | ||
| dest_c = firmware_mesh.get_node(2).node_num | ||
| node_b = firmware_mesh.get_node(1).node_num | ||
|
|
||
| firmware_mesh.get_iface(0).sendTraceRoute(dest=dest_c, hopLimit=3) | ||
|
|
||
| time.sleep(2) | ||
|
|
||
| assert len(col_a.traceroutes) >= 1, "A did not receive traceroute response" | ||
| a_resp = col_a.traceroutes[0] | ||
| assert a_resp["from"] == dest_c, "response source should be C" | ||
|
|
||
| route = a_resp["decoded"]["traceroute"] | ||
| assert route.get("route") == [node_b], "forward route should be A→B→C" | ||
| assert route.get("routeBack") == [node_b], "return route should be C→B→A" | ||
|
|
||
| assert len(col_c.traceroutes) >= 1, "C did not receive traceroute request" | ||
| c_req = col_c.traceroutes[0] | ||
| assert c_req["from"] == src_a, "request source should be A" | ||
| finally: | ||
| unsubscribe_all("meshtastic.receive.traceroute") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect PacketCollector.wait_for signature and traceroute bridging in the harness
# to confirm whether wait_for works for traceroute topics and where the TRACEROUTE_APP
# packet bridging is implemented.
rg -n -A10 'def wait_for' meshtastic/tests/fw_helpers.py
rg -n -B2 -A15 'TRACEROUTE_APP|RouteDiscovery' meshtastic/tests/firmware_harness.pyRepository: meshtastic/python
Length of output: 639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate traceroute collector helpers and related tests.
rg -n -A6 -B4 'subscribe_traceroutes|traceroutes|wait_for\(' meshtastic/tests
printf '\n--- firmware_harness traceroute handling ---\n'
rg -n -A8 -B4 'TRACEROUTE_APP|RouteDiscovery|traceroute' meshtastic/tests/firmware_harness.py
printf '\n--- test_smokemesh context ---\n'
sed -n '1,220p' meshtastic/tests/test_smokemesh.pyRepository: meshtastic/python
Length of output: 9159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A12 -B6 'def sendTraceRoute|waitForTraceRoute|send_trace_route' meshtastic/tests fw_helpers.py meshtastic/tests/firmware_harness.pyRepository: meshtastic/python
Length of output: 2363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B4 'decoded"\]\["traceroute"|get\("decoded", \{\}\)\.get\("traceroute"|decoded.*traceroute' meshtastic/testsRepository: meshtastic/python
Length of output: 1324
Replace the fixed sleep with wait_for(1) here. PacketCollector already supports deterministic polling for traceroute packets, and a_resp["decoded"]["traceroute"] should use a guarded lookup so a missing payload fails with the assertion instead of a KeyError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshtastic/tests/test_smokemesh.py` around lines 102 - 128, The smokemesh
traceroute test is using a fixed sleep and an unguarded traceroute payload
lookup. In test_smokemesh_traceroute_across_relay, replace the time.sleep delay
with PacketCollector.wait_for(1) so the test waits deterministically for
traceroute packets, and update the a_resp decoded traceroute access to use a
guarded assertion-friendly lookup so missing payloads fail through the test
assertion rather than a KeyError.
Source: Pipeline failures
Comment on lines
394
to
432
| @pytest.mark.smokevirt | ||
| def test_smokevirt_ensure_ch_del_second_of_three_channels(): | ||
| """Test that when we delete the 2nd of 3 channels, that it deletes the correct channel.""" | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-add testing1" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput("meshtastic --host localhost --info") | ||
| assert re.match(r"Connected to radio", out) | ||
| assert re.search(r"SECONDARY", out, re.MULTILINE) | ||
| assert re.search(r"testing1", out, re.MULTILINE) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-add testing2" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput("meshtastic --host localhost --info") | ||
| assert re.match(r"Connected to radio", out) | ||
| assert re.search(r"testing2", out, re.MULTILINE) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-del --ch-index 1" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput("meshtastic --host localhost --info") | ||
| assert re.match(r"Connected to radio", out) | ||
| assert re.search(r"testing2", out, re.MULTILINE) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-del --ch-index 1" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| def test_smokevirt_seturl_default(firmware_node): | ||
| """--seturl applies a known channel URL.""" | ||
| url = "https://www.meshtastic.org/d/#CgUYAyIBAQ" | ||
|
|
||
| # Use the fixture's already-connected TCPInterface so the same | ||
| # connection handles the firmware restart after setURL. | ||
| if firmware_node.iface is None: | ||
| pytest.fail("fixture interface not connected") | ||
| iface = firmware_node.iface | ||
| iface.localNode.setURL(url) | ||
|
|
||
| @pytest.mark.smokevirt | ||
| def test_smokevirt_ensure_ch_del_third_of_three_channels(): | ||
| """Test that when we delete the 3rd of 3 channels, that it deletes the correct channel.""" | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-add testing1" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput("meshtastic --host localhost --info") | ||
| assert re.match(r"Connected to radio", out) | ||
| assert re.search(r"SECONDARY", out, re.MULTILINE) | ||
| assert re.search(r"testing1", out, re.MULTILINE) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-add testing2" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput("meshtastic --host localhost --info") | ||
| assert re.match(r"Connected to radio", out) | ||
| assert re.search(r"testing2", out, re.MULTILINE) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-del --ch-index 2" | ||
| ) | ||
| assert re.match(r"Connected to radio", out) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput("meshtastic --host localhost --info") | ||
| assert re.match(r"Connected to radio", out) | ||
| assert re.search(r"testing1", out, re.MULTILINE) | ||
| assert return_value == 0 | ||
| # pause for the radio | ||
| time.sleep(PAUSE_AFTER_COMMAND) | ||
| return_value, out = subprocess.getstatusoutput( | ||
| "meshtastic --host localhost --ch-del --ch-index 1" | ||
| time.sleep(2.0) | ||
|
|
||
| actual = iface.localNode.getURL() | ||
| assert "meshtastic.org" in actual, f"not a channel URL: {actual}" | ||
| # The firmware may reshape the URL and use slightly different base64 | ||
| # for the same data (trailing bits are ignored during decode), so we | ||
| # decode both and compare the protobuf payload prefix rather than | ||
| # doing a substring match on the encoded form. | ||
| _, _, frag = actual.partition("/#") | ||
| if not frag: | ||
| _, _, frag = actual.rpartition("#") | ||
| missing = len(frag) % 4 | ||
| if missing: | ||
| frag += "=" * (4 - missing) | ||
| actual_bytes: bytes = base64.urlsafe_b64decode(frag) | ||
|
|
||
| _, _, efrag = url.partition("/#") | ||
| missing = len(efrag) % 4 | ||
| if missing: | ||
| efrag += "=" * (4 - missing) | ||
| expected_bytes: bytes = base64.urlsafe_b64decode(efrag) | ||
|
|
||
| assert actual_bytes.startswith(expected_bytes), ( | ||
| f"URL payload mismatch:\n" | ||
| f" expected (hex): {expected_bytes.hex()}\n" | ||
| f" actual (hex): {actual_bytes.hex()}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
test_smokevirt_seturl_default never exercises the --seturl CLI flag.
The test's name/docstring claims to test --seturl, but it calls iface.localNode.setURL(url) directly on the fixture's Python interface — the CLI is never invoked. This means CLI argument parsing/handling for --seturl (which is explicitly tested via run_cli for the invalid-URL case right below) is not covered here, undermining the smoke-test's stated purpose of exercising the real CLI path against the simulator.
Suggested fix: invoke `run_cli` for the mutation, then verify via a fresh interface (consistent with the rest of the file)
`@pytest.mark.smokevirt`
def test_smokevirt_seturl_default(firmware_node):
"""--seturl applies a known channel URL."""
url = "https://www.meshtastic.org/d/#CgUYAyIBAQ"
- # Use the fixture's already-connected TCPInterface so the same
- # connection handles the firmware restart after setURL.
- if firmware_node.iface is None:
- pytest.fail("fixture interface not connected")
- iface = firmware_node.iface
- iface.localNode.setURL(url)
-
- time.sleep(2.0)
-
- actual = iface.localNode.getURL()
+ rc, out = run_cli(firmware_node.port, "--seturl", url)
+ assert rc == 0, out
+
+ def check(iface):
+ actual = iface.localNode.getURL()
+ ...
+ verify_state(firmware_node.port, check)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshtastic/tests/test_smokevirt.py` around lines 394 - 432,
`test_smokevirt_seturl_default` currently bypasses the `--seturl` CLI path by
calling `iface.localNode.setURL` directly, so it does not test argument parsing
or CLI behavior. Update the test to use `run_cli` for the URL mutation, then
reconnect or use a fresh interface to verify the resulting URL via
`firmware_node`/`iface.localNode.getURL`, keeping the existing payload-decoding
assertions intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meshtastic/__main__.py`:
- Around line 972-980: Fix the incorrect protobuf accessor in the channel preset
handling inside __main__.py: the LONG_MODERATE and LONG_TURBO branches are using
config_pb2.config instead of the valid config_pb2.Config used by the neighboring
preset cases. Update the setSimpleConfig calls in that block so they all
reference config_pb2.Config.LoRaConfig.ModemPreset consistently, matching the
smoke test expectations and avoiding AttributeError for the affected CLI flags.
- Line 1972: The help text for the modem preset option contains a typo in the
deprecation message. Update the string in the argument definition for the
VERY_LONG_SLOW preset so it says “Deprecated since 2.5 firmware.” instead of
“Deprecated sincce 2.5 firmware.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf9ca07f-07f4-45d3-99fc-6bae0c62bbc5
📒 Files selected for processing (2)
meshtastic/__main__.pymeshtastic/tests/test_smokevirt.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meshtastic/tests/test_smokevirt.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
74-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a job-level timeout to bound the whole
simradio_testingjob.Only the final pytest step has
timeout-minutes: 15; the PPA add/update/install and poetry install steps are unbounded. A hungapt-get update/PPA fetch (network flakiness against a third-party PPA) could tie up a runner well past the intended time budget.⏱️ Proposed fix
simradio_testing: runs-on: ubuntu-latest + timeout-minutes: 25 strategy: fail-fast: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 74 - 106, Add a job-level timeout to the simradio_testing workflow job so the entire matrix run is bounded, not just the final pytest step. Update the simradio_testing job definition in the CI workflow to enforce an overall timeout that covers the checkout, setup-python, poetry install, and meshtasticd PPA install steps, while keeping the existing Run firmware smoke tests step timeout as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 74-106: Add a job-level timeout to the simradio_testing workflow
job so the entire matrix run is bounded, not just the final pytest step. Update
the simradio_testing job definition in the CI workflow to enforce an overall
timeout that covers the checkout, setup-python, poetry install, and meshtasticd
PPA install steps, while keeping the existing Run firmware smoke tests step
timeout as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cf0571f-d7b0-4d62-8d64-691f55323075
📒 Files selected for processing (3)
.github/workflows/ci.ymlmeshtastic/__main__.pymeshtastic/tests/test_node.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meshtastic/tests/test_node.py
…ther than LONG_FAST properly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
meshtastic/tests/conftest.py (1)
75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent blind-except during iface close.
Ruff flags this try/except/pass with a bare
Exceptioncatch. It's a reasonable best-effort cleanup before reconnect, but swallowing the error entirely can hide real failures (e.g., a genuinely broken socket) and make flaky-test triage harder.♻️ Suggested logging
try: node.iface.close() - except Exception: # pylint: disable=broad-except - pass + except Exception: # pylint: disable=broad-except + logging.debug("Ignoring error while closing stale iface for reconnect", exc_info=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/tests/conftest.py` around lines 75 - 78, The cleanup in the node iface close path is swallowing all exceptions silently, which hides useful failure information. Update the try/except around node.iface.close() in conftest.py to log the caught exception with enough context before continuing, using the existing node/iface cleanup flow so reconnect behavior stays best-effort while preserving triage details.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meshtastic/tests/conftest.py`:
- Around line 49-54: The firmware_node fixture leaves mesh.get_node(0).iface
stale after set_region() because the TCP listener restarts, so reconnect the
node before yielding it. Update the fixture in conftest.py to mirror the
reconnect pattern used by firmware_mesh: after set_region() on the single node,
close the existing iface if needed and establish a fresh connection on
mesh.get_node(0) before yielding. This ensures tests using firmware_node get a
live iface rather than a dead socket.
---
Nitpick comments:
In `@meshtastic/tests/conftest.py`:
- Around line 75-78: The cleanup in the node iface close path is swallowing all
exceptions silently, which hides useful failure information. Update the
try/except around node.iface.close() in conftest.py to log the caught exception
with enough context before continuing, using the existing node/iface cleanup
flow so reconnect behavior stays best-effort while preserving triage details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1df6a34e-ec04-4270-aafa-b855681a38ef
📒 Files selected for processing (2)
meshtastic/tests/conftest.pymeshtastic/tests/fw_helpers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- meshtastic/tests/fw_helpers.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
meshtastic/tests/conftest.py (1)
56-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the swallowed exception on iface close.
Ruff flags this as
try-except-pass(S110) and blindexcept Exception(BLE001). Since this is cleanup before a freshconnect(), a failure here is unlikely to matter functionally, but silently swallowing it can mask real problems when a test intermittently fails.♻️ Proposed fix
if node.iface is not None: try: node.iface.close() - except Exception: # pylint: disable=broad-except - pass + except Exception: # pylint: disable=broad-except + logging.debug("Failed to close stale iface before reconnect", exc_info=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshtastic/tests/conftest.py` around lines 56 - 60, The cleanup in node.iface.close() currently swallows all exceptions, which triggers Ruff S110/BLE001 and hides useful failure details. Update the teardown logic in conftest.py so the except block logs the caught exception with context before continuing, using the existing node.iface cleanup path around node.iface.close(). Keep the cleanup non-fatal, but avoid a bare silent pass by recording the error through the test logger or equivalent available in this fixture.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@meshtastic/tests/conftest.py`:
- Around line 56-60: The cleanup in node.iface.close() currently swallows all
exceptions, which triggers Ruff S110/BLE001 and hides useful failure details.
Update the teardown logic in conftest.py so the except block logs the caught
exception with context before continuing, using the existing node.iface cleanup
path around node.iface.close(). Keep the cleanup non-fatal, but avoid a bare
silent pass by recording the error through the test logger or equivalent
available in this fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 827cd124-8220-49f7-8153-a6b09203383a
📒 Files selected for processing (1)
meshtastic/tests/conftest.py
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When merged this should fix #643 as well. Still adding a bit of stuff to make
smokevirtnot so useless.Summary by CodeRabbit