Discovery fixes #34
Discovery fixes #34
Conversation
WalkthroughThe update modifies the callback invocation method in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airos/discovery.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Latest commit
airos/discovery.py
[warning] 62-62: RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited. This warning indicates a coroutine was not awaited properly.
🔇 Additional comments (1)
pyproject.toml (1)
7-7: LGTM! Proper patch version increment.The version bump from 0.2.0 to 0.2.1 correctly follows semantic versioning for a patch release that includes the behavioral change in the discovery protocol.
| if parsed_data: | ||
| # Schedule the user-provided callback, don't await to keep listener responsive | ||
| asyncio.create_task(self.callback(parsed_data)) # noqa: RUF006 | ||
| self.callback(parsed_data) |
There was a problem hiding this comment.
Critical: Async callback not properly invoked.
The callback is documented as asynchronous (line 36) but is now called directly without awaiting, causing the coroutine to never execute. The pipeline failure confirms this with "RuntimeWarning: coroutine was never awaited."
Choose one of these solutions:
Option 1: Restore async task scheduling (recommended)
- self.callback(parsed_data)
+ asyncio.create_task(self.callback(parsed_data))Option 2: Await the callback (blocks the listener)
- self.callback(parsed_data)
+ await self.callback(parsed_data)Option 3: Make callback synchronous (breaking change)
- def __init__(self, callback: Callable[[dict[str, Any]], None]) -> None:
+ def __init__(self, callback: Callable[[dict[str, Any]], None]) -> None:
"""Initialize AirosDiscoveryProtocol.
Args:
- callback: An asynchronous function to call when a device is discovered.
+ callback: A synchronous function to call when a device is discovered.
It should accept a dictionary containing device information.The first option maintains the original responsive behavior while properly handling async callbacks.
📝 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.
| self.callback(parsed_data) | |
| - self.callback(parsed_data) | |
| + asyncio.create_task(self.callback(parsed_data)) |
🧰 Tools
🪛 GitHub Actions: Latest commit
[warning] 62-62: RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited. This warning indicates a coroutine was not awaited properly.
🤖 Prompt for AI Agents
In airos/discovery.py at line 62, the async callback is called directly without
awaiting, causing the coroutine to never execute. To fix this, restore the async
task scheduling by using an appropriate method like asyncio.create_task to
schedule the callback instead of calling it directly. This ensures the callback
runs asynchronously without blocking the listener.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_discovery.py (1)
135-174: Comprehensive callback verification with good test structure.The test properly verifies:
- Callback is called exactly once
- Correct arguments are passed
- Key parsed data fields match expected values
- Dynamic IP address handling
The selective field checking approach is reasonable for maintainability, though you could consider validating all fields if the parsing logic is critical.
If you want to validate all fields comprehensively, consider this approach:
- # For robust testing, you might want to only check the critical fields, - # or ensure your `parse_airos_packet` is separately tested to be correct. - # Here, we'll assert a subset of key fields for simplicity. - assert ( - actual_parsed_data.get("mac_address") - == expected_parsed_data_with_current_ip["mac_address"] - ) - assert ( - actual_parsed_data.get("hostname") - == expected_parsed_data_with_current_ip["hostname"] - ) - assert ( - actual_parsed_data.get("ip_address") - == expected_parsed_data_with_current_ip["ip_address"] - ) - assert ( - actual_parsed_data.get("model") == expected_parsed_data_with_current_ip["model"] - ) - assert ( - actual_parsed_data.get("firmware_version") - == expected_parsed_data_with_current_ip["firmware_version"] - ) + # Validate all expected fields + for key, expected_value in expected_parsed_data_with_current_ip.items(): + assert actual_parsed_data.get(key) == expected_value, f"Field {key} mismatch"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(1 hunks)tests/test_discovery.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🔇 Additional comments (2)
tests/test_discovery.py (2)
133-133: LGTM: Direct callback invocation aligns with protocol changes.The change from patching
asyncio.create_taskto directly callingdatagram_receivedcorrectly reflects the synchronous callback invocation mentioned in the AI summary.
127-175: Clarify synchronous callback execution inAirosDiscoveryProtocol.datagram_received.We’ve confirmed in airos/discovery.py at the
datagram_receivedmethod that the callback is now invoked directly:if parsed_data: # Before: asyncio.create_task(self.callback(parseded_data)) self.callback(parsed_data)This change can impact:
- Error handling: Exceptions in your callback now bubble up immediately.
- Execution context: Callbacks run inline, potentially blocking the protocol loop.
- Concurrency: You lose the previous parallelism from
asyncio.create_task.Could you clarify:
- Is this intended as a breaking change?
- How should users now handle long-running or error-prone callbacks?
- Should we document this change (e.g., release notes or upgrade guide)?
| expected_parsed_data_for_mock_packet = { | ||
| "ip_address": "192.168.1.3", # This comes from the host_ip passed to datagram_received | ||
| "mac_address": "01:23:45:67:89:CD", # From 0x06 TLV | ||
| "hostname": "name", # From 0x0B TLV | ||
| "model": "NanoStation 5AC loco", # From 0x0C TLV | ||
| "firmware_version": "WA.V8.7.17", # From 0x03 TLV | ||
| "uptime_seconds": 3231, # From 0x0A TLV (0x0C9F in hex) | ||
| "ssid": "DemoSSID", # From 0x0D TLV | ||
| "full_model_name": "NanoStation 5AC loco", # From 0x14 TLV | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify uptime_seconds value consistency.
There's a discrepancy in the uptime_seconds value. The new dictionary shows 3231 (0x0C9F), but the existing test_parse_airos_packet_success expects 265375. This inconsistency could indicate either incorrect test data or different fixtures being used.
🏁 Script executed:
#!/bin/bash
# Description: Check the actual uptime value in the binary fixture
# Expected: Verify which uptime value is correct by examining the fixture
# Look for the fixture file and examine its contents
fd -t f "airos_sta_discovery_packet.bin" fixtures/
if [ $? -eq 0 ]; then
echo "Found fixture file. Checking TLV 0x0A (uptime) content..."
# Use hexdump to examine the binary content
hexdump -C fixtures/airos_sta_discovery_packet.bin | grep -A5 -B5 "0a"
fiLength of output: 403
I couldn’t inspect the TLV parsing logic with hexdump. Let’s locate and review the packet‐parsing implementation to understand how the uptime field is decoded and then re‐extract it from the fixture.
🏁 Script executed:
#!/bin/bash
# 1) Find and show the parse_airos_packet function with context
rg -n "def parse_airos_packet" -tpy
rg -A20 -B5 "def parse_airos_packet" -tpy
# 2) Within that function (or nearby), look for handling of TLV type 0x0A / "uptime"
rg -n "0x0A" -C5 -tpy
rg -n "uptime" -C5 -tpyLength of output: 9942
Update expected uptime_seconds to match the 4‐byte unpacked value
The mock‐packet dict in tests/test_discovery.py currently uses 3231 (0x0C9F), but the fixture generator packs a 4-byte big-endian integer (UPTIME_SECONDS = 265375, i.e. 0x00040C9F), and the existing test_parse_airos_packet_success asserts 265375. To keep the tests consistent, change:
tests/test_discovery.py
@@ Lines 12–21
- "uptime_seconds": 3231, # From 0x0A TLV (0x0C9F in hex)
+ "uptime_seconds": 265375, # From 0x0A TLV (0x00040C9F in hex)• Location: tests/test_discovery.py, around line 18.
• Ensures both test_datagram_received_calls_callback and test_parse_airos_packet_success align on the same unpacked uptime value.
📝 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.
| expected_parsed_data_for_mock_packet = { | |
| "ip_address": "192.168.1.3", # This comes from the host_ip passed to datagram_received | |
| "mac_address": "01:23:45:67:89:CD", # From 0x06 TLV | |
| "hostname": "name", # From 0x0B TLV | |
| "model": "NanoStation 5AC loco", # From 0x0C TLV | |
| "firmware_version": "WA.V8.7.17", # From 0x03 TLV | |
| "uptime_seconds": 3231, # From 0x0A TLV (0x0C9F in hex) | |
| "ssid": "DemoSSID", # From 0x0D TLV | |
| "full_model_name": "NanoStation 5AC loco", # From 0x14 TLV | |
| } | |
| # tests/test_discovery.py | |
| expected_parsed_data_for_mock_packet = { | |
| "ip_address": "192.168.1.3", # This comes from the host_ip passed to datagram_received | |
| "mac_address": "01:23:45:67:89:CD", # From 0x06 TLV | |
| "hostname": "name", # From 0x0B TLV | |
| "model": "NanoStation 5AC loco", # From 0x0C TLV | |
| "firmware_version": "WA.V8.7.17", # From 0x03 TLV | |
| "uptime_seconds": 265375, # From 0x0A TLV (0x00040C9F in hex) | |
| "ssid": "DemoSSID", # From 0x0D TLV | |
| "full_model_name": "NanoStation 5AC loco", # From 0x14 TLV | |
| } |
🤖 Prompt for AI Agents
In tests/test_discovery.py around line 18, update the expected value of
"uptime_seconds" in the mock packet dictionary from 3231 to 265375 to match the
4-byte big-endian integer unpacked value used in the fixture generator. This
change ensures consistency between the test_datagram_received_calls_callback and
test_parse_airos_packet_success tests.
1 participant
Summary by CodeRabbit
Chores
Bug Fixes