Support alternate Speedtest plugins - #80
Conversation
📝 WalkthroughWalkthroughSpeedtest retrieval now uses ChangesSpeedtest showlog integration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Documentation build overview
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aiopnsense/speedtest.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
Anywithobjectto resolve static analysis warnings.The
Anytype hint triggers the RuffANN401rule for dynamically typed expressions and violates the repository guideline to minimizeAnyusage. Since the code safely verifies these arguments at runtime usingisinstance,objectis the standard, strict type hint for this scenario.
aiopnsense/speedtest.py#L96-96: replaceshow_log: Anywithshow_log: object.aiopnsense/speedtest.py#L124-124: replaceserver_text: Anywithserver_text: object.tests/test_speedtest.py#L226-228: replaceshow_log: Anywithshow_log: object.tests/test_speedtest.py#L4-4: remove thefrom typing import Anyimport if it is no longer used elsewhere in the file.🤖 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 `@aiopnsense/speedtest.py` at line 1, Replace the Any annotations with object for the show_log and server_text parameters in the speedtest methods and the test helper, preserving their existing isinstance validation. Remove the typing.Any import from the test module if no other references remain.Sources: Coding guidelines, 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.
Outside diff comments:
In `@aiopnsense/speedtest.py`:
- Line 1: Replace the Any annotations with object for the show_log and
server_text parameters in the speedtest methods and the test helper, preserving
their existing isinstance validation. Remove the typing.Any import from the test
module if no other references remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fafd34f4-25c9-41a9-a638-320f18629d67
📒 Files selected for processing (2)
aiopnsense/speedtest.pytests/test_speedtest.py
Keep server IDs and names separate while normalizing showlog rows. Add regressions for digit-leading names and empty server names.
Delete the unused combined server-field parser after showlog normalization began preserving server IDs and names separately.
Avoid stripping server identifiers and names twice while preserving existing type and empty-value handling.
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 `@aiopnsense/speedtest.py`:
- Around line 55-60: In the latest-result handling, remove the redundant
isinstance checks and assign server_id and server_name directly from
latest_result.get("server_id") and latest_result.get("server"). Preserve the
existing field mappings and rely on _parse_showlog_latest’s str-or-None
contract.
- Around line 119-130: The server ID and server name normalization in the
latest-result parsing block is unnecessarily verbose. Replace the conditional
assignments for server_id and server with concise inline expressions that
preserve the existing behavior: reject booleans and non-integer/string IDs,
stringify valid IDs, and convert empty or invalid server names to None.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0b20fea-8aa2-4baf-9bc4-aca523dce230
📒 Files selected for processing (2)
aiopnsense/speedtest.pytests/test_speedtest.py
Summary
Supports Speedtest plugin variants that omit the
showrecentendpoint while preserving the existing normalized client response.Allows for support of 2 Speedtest plugins using the existing aiopnsense endpoints:
What Changed
showlogendpoint as the source for the latest Speedtest resultshowstatfor full-history averagesWhy
The supported Speedtest plugins expose the same
showlogandshowstatendpoints, but only one exposesshowrecent. Using the common endpoint avoids plugin-specific detection and keeps the publicaiopnsenseresponse contract unchanged.Fixes travisghansen/hass-opnsense#654
Summary by CodeRabbit
Greptile Summary
This PR switches the Speedtest integration from the
showrecentendpoint (only present in some plugin variants) to the universally-availableshowlogendpoint, normalising the newest history row into the same public response shape that callers already expect._parse_showlog_latestextracts index-addressed fields from the structured list rows returned byshowlog, with correct bool/type guards for the server-ID field and an early-return for missing or short rows.get_speedtestandrun_speedtestnow gate onshowlogavailability instead ofshowrecent;showstatremains an optional secondary fetch._safe_list_get, and new parametrised cases cover malformed rows and server-field edge cases.Confidence Score: 5/5
This PR is safe to merge; the change is well-scoped and all edge cases are guarded.
The logic is straightforward — swap one endpoint for another, add a small row-parsing helper with proper type guards — and the tests cover normal, missing, and malformed payloads. No data-loss paths, no silent coercions, and the public response contract is preserved.
No files require special attention.
Important Files Changed
Reviews (3): Last reviewed commit: "Harden Speedtest log parsing" | Re-trigger Greptile