Skip to content

Conversation

@CoMPaTech
Copy link
Owner

@CoMPaTech CoMPaTech commented Aug 15, 2025

Summary by CodeRabbit

  • New Features

    • Fetch device warnings.
    • Non-forced firmware update availability check.
    • Wireless role and mode metadata added to derived output.
  • Documentation

    • Changelog updated for v0.3.0.
  • Tests

    • Added tests for warnings and update-check responses and error cases.
  • Chores

    • Fixture generation now fails fast on JSON or processing errors.

@coderabbitai
Copy link

coderabbitai bot commented Aug 15, 2025

Warning

Rate limit exceeded

@CoMPaTech has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between bb4ed1c and b83197a.

📒 Files selected for processing (1)
  • requirements-test.txt (1 hunks)

Walkthrough

Adds DerivedWirelessRole and DerivedWirelessMode enums and fields to derived data, exposes them from airos8, adds warnings and update_check async endpoints with error handling, updates fixtures and tests, changes fixture-generation to re-raise errors, bumps version to 0.3.0 and updates CHANGELOG.

Changes

Cohort / File(s) Summary
Changelog & Versioning
CHANGELOG.md, pyproject.toml
New 0.3.0 changelog entry added; project version bumped 0.2.11 → 0.3.0.
Data Model
airos/data.py
Added enums DerivedWirelessRole (STATION, ACCESS_POINT) and DerivedWirelessMode (PTP, PTMP); added role and mode fields to Derived dataclass; minor distance field doc comments.
Core API (AirOS)
airos/airos8.py
Exported DerivedWirelessMode/DerivedWirelessRole; updated derived_data default/mapping for wireless modes; added _warnings_url and _update_check_url; added async warnings() and update_check() methods mirroring status()-style error handling.
Fixtures (Derived role/mode)
fixtures/...ap_ptmp_40mhz.json, fixtures/...sta_ptmp_40mhz.json, fixtures/...ap-ptp_30mhz.json, fixtures/...sta-ptp_30mhz.json, fixtures/...ap-ptp.json, fixtures/...sta-ptp.json, fixtures/...ap-ptp_8718_missing_gps.json
Added derived.mode ("point_to_point"/"point_to_multipoint") and derived.role ("access_point"/"station") to multiple device fixtures.
Fixtures (New endpoints)
fixtures/update_check_available.json, fixtures/warnings.json
Added example JSON responses for firmware update-check and warnings endpoints.
Script Behavior
script/generate_ha_fixture.py
Now re-raises json.JSONDecodeError and generic Exception after logging, causing fixture generation to abort on first failure.
Tests
tests/test_airos8.py
Added async tests for warnings() and update_check() covering success, invalid JSON, and not-connected cases; added aiofiles import and assertions covering error causes.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AirOS
  participant Device

  Client->>AirOS: warnings()
  AirOS->>Device: GET /api/warnings (+CSRF)
  Device-->>AirOS: JSON warnings
  AirOS-->>Client: Parsed dict or raises error

  Client->>AirOS: update_check()
  AirOS->>Device: POST /api/fw/update-check {}
  Device-->>AirOS: JSON update info
  AirOS-->>Client: Parsed dict or raises error
Loading
sequenceDiagram
  participant User
  participant Script as generate_ha_fixture.py
  participant FS as FileSystem

  User->>Script: run
  Script->>FS: read JSON fixture
  FS-->>Script: invalid JSON / error
  Script-->>User: log error, re-raise (abort)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Improvements #47: Overlaps changes to derived-data logic and types in airos/data.py and airos/airos8.py.
  • Upd #62: Alters AirOS.derived_data behavior in airos/airos8.py, intersecting with the mapping/export changes here.
  • Fixture generation for derived #36: Related to fixture generation and derived-data handling, touching script/generate_ha_fixture.py and fixtures.

Poem

A rabbit hops the dataline bright,
Two enums join the morning light.
Warnings whisper, updates sing—
JSON carrots in a ring.
If parsing stumbles, paws alert;
We raise the error, not the dirt. 🥕✨

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch upd

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 86.23188% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.15%. Comparing base (1787f88) to head (b83197a).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
airos/airos8.py 66.07% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
- Coverage   96.08%   95.15%   -0.93%     
==========================================
  Files           9        9              
  Lines        1353     1487     +134     
==========================================
+ Hits         1300     1415     +115     
- Misses         53       72      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (12)
CHANGELOG.md (1)

5-12: Changelog entry reads well; minor consistency tweak.

Nit: The first bullet ends with a period while the next two don’t. For consistency with the rest of the changelog, consider removing the period.

- - Implementation of `[AP|Sta]-[MODE]` to Enums.
+ - Implementation of `[AP|Sta]-[MODE]` to Enums
fixtures/update_check_available.json (2)

1-1: Pretty-print fixture for readability and consistent diffs.

All other fixtures are formatted; this one-line JSON is hard to review. Recommend pretty-printing it.

-{"checksum": "b1bea879a9f518f714ce638172e3a860", "version": "v8.7.19", "security": "", "date": "250811", "url": "https://dl.ubnt.com/firmwares/XC-fw/v8.7.19/WA.v8.7.19.48279.250811.0636.bin", "update": true, "changelog": "https://dl.ubnt.com/firmwares/XC-fw/v8.7.19/changelog.txt"}
+{
+  "checksum": "b1bea879a9f518f714ce638172e3a860",
+  "version": "v8.7.19",
+  "security": "",
+  "date": "250811",
+  "url": "https://dl.ubnt.com/firmwares/XC-fw/v8.7.19/WA.v8.7.19.48279.250811.0636.bin",
+  "update": true,
+  "changelog": "https://dl.ubnt.com/firmwares/XC-fw/v8.7.19/changelog.txt"
+}

1-1: Consider adding a “no update available” companion fixture.

Tests likely cover the positive case (update: true). For completeness and future-proofing, add a fixture representing update: false and a corresponding test.

Happy to draft the additional fixture and test update if you want.

airos/data.py (3)

144-149: New DerivedWirelessRole enum: LGTM

Values are well named and lowercased to match JSON. No issues spotted.

If useful, consider a slightly more descriptive class docstring (e.g., “Derived wireless role, independent of AP/station booleans”).


151-156: New DerivedWirelessMode enum: LGTM

Clear mapping to “point_to_point” and “point_to_multipoint”.

As with role, small docstring enhancement could help future contributors.


571-573: Consider backward-compatibility defaults and pre-deserialize mapping for new fields

Adding non-optional role and mode is a good evolution. However, this can break from_dict on older stored fixtures or consumer data that predates these fields. Two lightweight improvements:

  • Provide sensible defaults so missing keys won’t fail deserialization.
  • Optionally map from the existing booleans (station/access_point, ptp/ptmp) if role/mode are absent.

Proposed minimal diff (defaults only):

-    role: DerivedWirelessRole
-    mode: DerivedWirelessMode
+    role: DerivedWirelessRole = DerivedWirelessRole.STATION
+    mode: DerivedWirelessMode = DerivedWirelessMode.PTP

Additionally, if you want robust back-compat when deserializing older payloads that include the booleans but not the enums, add this helper inside Derived (outside the selected lines, shown as a regular code block):

@classmethod
def __pre_deserialize__(cls, d: dict[str, Any]) -> dict[str, Any]:
    # Only set if missing; prefer explicit values when present
    if "role" not in d:
        if d.get("access_point"):
            d["role"] = DerivedWirelessRole.ACCESS_POINT.value
        elif d.get("station"):
            d["role"] = DerivedWirelessRole.STATION.value
    if "mode" not in d:
        if d.get("ptmp"):
            d["mode"] = DerivedWirelessMode.PTMP.value
        elif d.get("ptp"):
            d["mode"] = DerivedWirelessMode.PTP.value
    return d

This keeps enum fields authoritative going forward while gracefully handling older inputs.

script/generate_ha_fixture.py (2)

56-59: Log message says “Skipping” but code now aborts; align the message and include traceback

You re-raise the JSONDecodeError (good for failing fast), but the log still says “Skipping”. Use “Aborting” and log the exception with a traceback for easier diagnosis.

Apply:

-            except json.JSONDecodeError:
-                _LOGGER.error("Skipping '%s': Not a valid JSON file.", filename)
-                raise
+            except json.JSONDecodeError:
+                _LOGGER.exception("Aborting on invalid JSON in '%s'.", filename)
+                raise

60-61: Also log unknown errors with traceback

For unexpected exceptions, prefer _LOGGER.exception so the stack trace is captured before re-raising.

Apply:

-            except Exception as e:
-                _LOGGER.error("Error processing '%s': %s", filename, e)
-                raise
+            except Exception as e:
+                _LOGGER.exception("Error processing '%s': %s", filename, e)
+                raise

Nit: you can also switch json.loads(source.read()) to json.load(source) for simplicity and slight performance. If you want that tweak, update Line 46:

source_data = json.load(source)
tests/test_airos8.py (1)

265-395: Add non-200 response tests for warnings() and update_check()

Coverage is solid for happy-path, invalid JSON, and not-connected. Consider adding tests for non-200 responses to fully exercise error handling parity with status().

Example additions:

+@pytest.mark.asyncio
+async def test_warnings_non_200_response() -> None:
+    """warnings() should raise connection error on non-200."""
+    mock_session = MagicMock()
+    airos_device = AirOS(
+        host="http://192.168.1.3", username="test", password="test", session=mock_session
+    )
+    airos_device.connected = True
+
+    mock_response = MagicMock()
+    mock_response.__aenter__.return_value = mock_response
+    mock_response.status = 500
+    mock_response.text = AsyncMock(return_value="Error")
+    with (
+        patch.object(airos_device.session, "get", return_value=mock_response),
+        # If you adopt the suggestion in airos8.py to align with status(), expect AirOSDeviceConnectionError:
+        pytest.raises(airos.exceptions.AirOSDeviceConnectionError),
+    ):
+        await airos_device.warnings()
+
+
+@pytest.mark.asyncio
+async def test_update_check_non_200_response() -> None:
+    """update_check() should raise connection error on non-200."""
+    mock_session = MagicMock()
+    airos_device = AirOS(
+        host="http://192.168.1.3", username="test", password="test", session=mock_session
+    )
+    airos_device.connected = True
+    airos_device.current_csrf_token = "mock-csrf-token"
+
+    mock_response = MagicMock()
+    mock_response.__aenter__.return_value = mock_response
+    mock_response.status = 503
+    mock_response.text = AsyncMock(return_value="Error")
+    with (
+        patch.object(airos_device.session, "post", return_value=mock_response),
+        # If you adopt the suggestion in airos8.py to align with status(), expect AirOSDeviceConnectionError:
+        pytest.raises(airos.exceptions.AirOSDeviceConnectionError),
+    ):
+        await airos_device.update_check()
airos/airos8.py (3)

401-431: Fix typos and align non-200 handling with status()

  • Typo: “fech” -> “fetch”.
  • Consider raising AirOSDeviceConnectionError on non-200 to mirror status() semantics and the PR’s error-handling parity intent.

Apply:

     async def warnings(self) -> dict[str, Any] | Any:
         """Get warnings."""
         if not self.connected:
             _LOGGER.error("Not connected, login first")
             raise AirOSDeviceConnectionError from None

         request_headers = {**self._common_headers}
         if self.current_csrf_token:
             request_headers["X-CSRF-ID"] = self.current_csRF_token

         # Formal call is '/api/warnings?_=1755249683586'
         try:
             async with self.session.get(
                 self._warnings_url,
                 headers=request_headers,
             ) as response:
                 response_text = await response.text()
                 if response.status == 200:
                     return json.loads(response_text)
-                log = f"Unable to fech warning status {response.status} with {response_text}"
+                log = f"Unable to fetch warnings status {response.status} with {response_text}"
                 _LOGGER.error(log)
-                raise AirOSDataMissingError from None
+                raise AirOSDeviceConnectionError from None
         except json.JSONDecodeError:
             _LOGGER.exception("JSON Decode Error in warning response")
             raise AirOSDataMissingError from None
         except (TimeoutError, aiohttp.ClientError) as err:
             _LOGGER.exception("Error during call to retrieve warnings: %s", err)
             raise AirOSDeviceConnectionError from err
         except asyncio.CancelledError:
             _LOGGER.info("Warning check task was cancelled")
             raise

433-465: Correct docstring/header casing/typos and consider error alignment

  • Docstring refers to “warnings” but this method checks for firmware updates.
  • Header key casing: prefer “Content-Type”.
  • Typos: “fech” -> “fetch”; log mentions “warning response” for update endpoint.
  • Consider raising AirOSDeviceConnectionError on non-200, consistent with status().

Apply:

-    async def update_check(self) -> dict[str, Any] | Any:
-        """Get warnings."""
+    async def update_check(self) -> dict[str, Any] | Any:
+        """Check for firmware updates."""
         if not self.connected:
             _LOGGER.error("Not connected, login first")
             raise AirOSDeviceConnectionError from None

         request_headers = {**self._common_headers}
         if self.current_csrf_token:
             request_headers["X-CSRF-ID"] = self.current_csrf_token
-        request_headers["Content-type"] = "application/json"
+        request_headers["Content-Type"] = "application/json"

         # Post without data
         try:
             async with self.session.post(
                 self._update_check_url,
                 headers=request_headers,
                 json={},
             ) as response:
                 response_text = await response.text()
                 if response.status == 200:
                     return json.loads(response_text)
-                log = f"Unable to fech update status {response.status} with {response_text}"
+                log = f"Unable to fetch update status {response.status} with {response_text}"
                 _LOGGER.error(log)
-                raise AirOSDataMissingError from None
+                raise AirOSDeviceConnectionError from None
         except json.JSONDecodeError:
-            _LOGGER.exception("JSON Decode Error in warning response")
+            _LOGGER.exception("JSON Decode Error in update-check response")
             raise AirOSDataMissingError from None
         except (TimeoutError, aiohttp.ClientError) as err:
             _LOGGER.exception("Error during call to retrieve update status: %s", err)
             raise AirOSDeviceConnectionError from err
         except asyncio.CancelledError:
-            _LOGGER.info("Warning update status task was cancelled")
+            _LOGGER.info("Firmware update check task was cancelled")
             raise

Note: since you pass json={}, aiohttp will set Content-Type automatically; the explicit header is optional.


401-465: Reduce duplication with a small helper for JSON-fetching endpoints

warnings() and update_check() share near-identical patterns (headers, status check, JSON parse, error mapping). Consider factoring into a private helper to keep error handling uniform.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between aa8ce26 and a3ac401.

📒 Files selected for processing (15)
  • CHANGELOG.md (1 hunks)
  • airos/airos8.py (5 hunks)
  • airos/data.py (5 hunks)
  • fixtures/airos_LiteBeam5AC_ap-ptp_30mhz.json (1 hunks)
  • fixtures/airos_LiteBeam5AC_sta-ptp_30mhz.json (1 hunks)
  • fixtures/airos_liteapgps_ap_ptmp_40mhz.json (1 hunks)
  • fixtures/airos_loco5ac_ap-ptp.json (1 hunks)
  • fixtures/airos_loco5ac_sta-ptp.json (1 hunks)
  • fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json (1 hunks)
  • fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json (1 hunks)
  • fixtures/update_check_available.json (1 hunks)
  • fixtures/warnings.json (1 hunks)
  • pyproject.toml (1 hunks)
  • script/generate_ha_fixture.py (1 hunks)
  • tests/test_airos8.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
airos/airos8.py (2)
airos/data.py (4)
  • AirOS8Data (576-593)
  • DerivedWirelessMode (151-155)
  • DerivedWirelessRole (144-148)
  • redact_data_smart (42-101)
airos/exceptions.py (2)
  • AirOSDeviceConnectionError (24-25)
  • AirOSDataMissingError (16-17)
tests/test_airos8.py (3)
tests/conftest.py (1)
  • airos_device (23-28)
airos/airos8.py (4)
  • AirOS (31-465)
  • status (263-322)
  • warnings (401-431)
  • update_check (433-465)
airos/exceptions.py (2)
  • AirOSDataMissingError (16-17)
  • AirOSDeviceConnectionError (24-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build and publish Python 🐍 distributions 📦 to TestPyPI
  • GitHub Check: Process test complexity
🔇 Additional comments (17)
pyproject.toml (1)

7-7: Version bump to 0.3.0 looks good.

Changelog and tests align with this bump; no concerns here.

fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json (2)

16-20: Derived role/mode additions are consistent with sta-ptmp.

mode: "point_to_multipoint" with role: "station" matches the wireless.mode sta-ptmp and existing booleans. Looks good.


16-20: derived.mode/role mapping verified — no change required

  • airos/data.py — DerivedWirelessRole uses "station"/"access_point" and DerivedWirelessMode uses "point_to_point"/"point_to_multipoint" (matches fixtures).
  • airos/airos8.py — AirOS.derived_data sets derived["role"]/derived["mode"] to the corresponding enum members.

Fixtures and enum values align; deserialization will match these strings to the enums. No fix needed.

fixtures/airos_liteapgps_ap_ptmp_40mhz.json (2)

16-20: Derived role/mode additions are consistent with ap-ptmp.

mode: "point_to_multipoint" with role: "access_point" matches the wireless.mode ap-ptmp and booleans.


16-20: Cross-check enum mapping for these values.

Same mapping concern as the station fixture: confirm mashumaro parsing recognizes these new strings for the derived enums.

Use the same script shared on the other fixture comment to validate mappings in code.

fixtures/warnings.json (1)

1-2: Fixture looks good and aligns with new warnings endpoint.

Structure matches the expected shape (top-level warning flags + nested firmware object). Should be sufficient for exercising parsing and error paths in tests.

airos/data.py (3)

367-367: Distance field doc clarified: good improvement

“distance: int // In meters” increases clarity. Thanks for adding units.


425-425: Distance field doc clarified here as well

Consistent with Remote.distance.


463-463: Distance field doc clarified here as well

Consistent unit annotation across classes.

fixtures/airos_LiteBeam5AC_sta-ptp_30mhz.json (1)

16-20: Derived role/mode additions are consistent with wireless.mode

Derived.mode = point_to_point and Derived.role = station align with wireless.mode = sta-ptp. Good consistency.

Please confirm all other fixtures received the same role/mode additions to avoid deserialization failures when using the new enums across the board.

fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json (1)

16-20: Derived role/mode additions are consistent with wireless.mode

Derived.mode = point_to_point and Derived.role = access_point align with wireless.mode = ap-ptp.

fixtures/airos_LiteBeam5AC_ap-ptp_30mhz.json (1)

16-20: Consistent derived metadata for AP-PTP

The added derived.mode and derived.role values match the AP-PTP wireless mode and new enums. No issues spotted.

fixtures/airos_loco5ac_sta-ptp.json (1)

16-20: Correct derived metadata for STA-PTP

mode="point_to_point" and role="station" align with wireless.mode="sta-ptp" and the enums added in airos/data.py. Looks good.

tests/test_airos8.py (1)

261-262: Good assertion of underlying cause for deserialization failure

Asserting the MissingField as the cause ensures we catch the exact mashumaro error routed through AirOSKeyDataMissingError.

airos/airos8.py (3)

14-19: Publicly exposing Derived enums is fine

Re-exporting DerivedWirelessMode and DerivedWirelessRole from data is appropriate and keeps the public surface cohesive.


62-63: New endpoints initialization looks correct

Endpoints match the API paths described, and placement in init aligns with existing URL attributes.


211-213: Derived role/mode mapping is sensible and defaults are coherent

Defaults to STATION/PTP and overrides per wireless.mode are correct for ap-ptmp, sta-ptmp, ap-ptp, and sta-ptp cases. This integrates cleanly with existing derived flags.

Also applies to: 221-223, 226-227, 230-231

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
tests/test_airos8.py (6)

11-11: Consider dropping aiofiles in tests and use Path.read_text for simpler, more robust fixture loading.

Avoids an extra dev dependency, reduces async complexity in tests, and makes the fixture path explicit and stable.

Apply this diff:

@@
-import aiofiles
+from pathlib import Path
@@
-    async with aiofiles.open("fixtures/warnings.json") as f:
-        content = await f.read()
-        mock_response_data = json.loads(content)
-    mock_response.text = AsyncMock(return_value=json.dumps(mock_response_data))
+    content = (
+        Path(__file__).resolve().parent.parent / "fixtures" / "warnings.json"
+    ).read_text(encoding="utf-8")
+    mock_response_data = json.loads(content)
+    mock_response.text = AsyncMock(return_value=content)
@@
-    async with aiofiles.open("fixtures/update_check_available.json") as f:
-        content = await f.read()
-        mock_response_data = json.loads(content)
-    mock_response.text = AsyncMock(return_value=json.dumps(mock_response_data))
+    content = (
+        Path(__file__).resolve().parent.parent / "fixtures" / "update_check_available.json"
+    ).read_text(encoding="utf-8")
+    mock_response_data = json.loads(content)
+    mock_response.text = AsyncMock(return_value=content)

Also applies to: 281-285, 332-336


31-33: Fix misleading comment: function returns False, not None.

-        # We expect a return of None as the CSRF token is missing
+        # We expect a return of False as the CSRF token is missing

107-112: Avoid brittle references to source line numbers in comments.

These go stale quickly and add maintenance burden.

-    # This will directly test the 'if not interfaces:' branch (line 206)
+    # This directly tests the 'if not interfaces:' branch

337-341: Assert request payload and CSRF header in update_check() test; avoid hardcoding version string.

This verifies the behavior that matters (json payload and X-CSRF-ID propagation) and reduces test brittleness against fixture changes.

-    with patch.object(airos_device.session, "post", return_value=mock_response):
-        result = await airos_device.update_check()
-        assert result["version"] == "v8.7.19"
-        assert result["update"] is True
+    with patch.object(airos_device.session, "post", return_value=mock_response) as mock_post:
+        result = await airos_device.update_check()
+        # Avoid hardcoding version; assert matches fixture
+        assert result["version"] == mock_response_data["version"]
+        assert result["update"] is True
+        # Verify request formation
+        mock_post.assert_called_once()
+        _, kwargs = mock_post.call_args
+        assert kwargs.get("json") == {}
+        headers = kwargs.get("headers") or {}
+        assert headers.get("X-CSRF-ID") == "mock-csrf-token"

265-397: Add missing coverage for non-200 and network exceptions in warnings() and update_check().

Current tests cover happy path and invalid JSON. Consider adding:

  • Non-200 responses raise AirOSDataMissingError (per implementation summary).
  • aiohttp.ClientError maps to AirOSDeviceConnectionError.
  • asyncio.CancelledError is propagated (not wrapped).

You can append something like this near the end of the file:

@pytest.mark.asyncio
async def test_warnings_non_200_raises_data_missing() -> None:
    mock_session = MagicMock()
    airos_device = AirOS("http://192.168.1.3", "test", "test", mock_session)
    airos_device.connected = True

    mock_response = MagicMock()
    mock_response.__aenter__.return_value = mock_response
    mock_response.status = 500
    mock_response.text = AsyncMock(return_value="Error")

    with (
        patch.object(airos_device.session, "get", return_value=mock_response),
        pytest.raises(airos.exceptions.AirOSDataMissingError),
    ):
        await airos_device.warnings()


@pytest.mark.asyncio
async def test_warnings_client_error_raises_connection_error() -> None:
    mock_session = MagicMock()
    airos_device = AirOS("http://192.168.1.3", "test", "test", mock_session)
    airos_device.connected = True

    with (
        patch.object(airos_device.session, "get", side_effect=aiohttp.ClientError),
        pytest.raises(airos.exceptions.AirOSDeviceConnectionError),
    ):
        await airos_device.warnings()


@pytest.mark.asyncio
async def test_update_check_non_200_raises_data_missing() -> None:
    mock_session = MagicMock()
    airos_device = AirOS("http://192.168.1.3", "test", "test", mock_session)
    airos_device.connected = True
    airos_device.current_csrf_token = "mock-csrf-token"

    mock_response = MagicMock()
    mock_response.__aenter__.return_value = mock_response
    mock_response.status = 500
    mock_response.text = AsyncMock(return_value="Error")

    with (
        patch.object(airos_device.session, "post", return_value=mock_response),
        pytest.raises(airos.exceptions.AirOSDataMissingError),
    ):
        await airos_device.update_check()


@pytest.mark.asyncio
async def test_update_check_client_error_raises_connection_error() -> None:
    mock_session = MagicMock()
    airos_device = AirOS("http://192.168.1.3", "test", "test", mock_session)
    airos_device.connected = True
    airos_device.current_csrf_token = "mock-csrf-token"

    with (
        patch.object(airos_device.session, "post", side_effect=aiohttp.ClientError),
        pytest.raises(airos.exceptions.AirOSDeviceConnectionError),
    ):
        await airos_device.update_check()


@pytest.mark.asyncio
async def test_update_check_cancelled_error_propagates() -> None:
    mock_session = MagicMock()
    airos_device = AirOS("http://192.168.1.3", "test", "test", mock_session)
    airos_device.connected = True
    airos_device.current_csrf_token = "mock-csrf-token"

    with patch.object(airos_device.session, "post", side_effect=asyncio.CancelledError):
        with pytest.raises(asyncio.CancelledError):
            await airos_device.update_check()

If you want, I can consolidate these into parametrized tests.


269-276: Use spec’ed mocks for better type safety and earlier failures.

Spec’ing the mocks to aiohttp.ClientSession (and ClientResponse where applicable) catches typos in attribute/method names at test time.

For example:

-    mock_session = MagicMock()
+    mock_session = MagicMock(spec=aiohttp.ClientSession)

Similarly, for responses:

mock_response = MagicMock(spec=aiohttp.ClientResponse)
mock_response.__aenter__.return_value = mock_response
mock_response.status = 200
mock_response.text = AsyncMock(return_value="...")

Also applies to: 295-302, 319-326, 346-352, 371-377, 388-393

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a3ac401 and bb4ed1c.

📒 Files selected for processing (1)
  • tests/test_airos8.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_airos8.py (3)
tests/conftest.py (1)
  • airos_device (23-28)
airos/airos8.py (5)
  • AirOS (31-465)
  • status (263-322)
  • warnings (401-431)
  • update_check (433-465)
  • AirOS (26-386)
airos/exceptions.py (2)
  • AirOSDataMissingError (16-17)
  • AirOSDeviceConnectionError (24-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run mypy
🔇 Additional comments (1)
tests/test_airos8.py (1)

261-263: LGTM: Asserting the underlying cause preserves error provenance.

Validating that AirOSKeyDataMissingError chains MissingField via cause is solid and prevents regression in error semantics.

@sonarqubecloud
Copy link

@CoMPaTech CoMPaTech merged commit a41436e into main Aug 15, 2025
11 of 14 checks passed
@CoMPaTech CoMPaTech deleted the upd branch August 15, 2025 11:13
This was referenced Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants