Add (very basic) test #10
Conversation
WalkthroughThe changes introduce new and updated test infrastructure for the Ubiquity AirOS Python module, including new test fixtures, parameterized async tests, and Bandit security scanning configuration. The main AirOS class is renamed and enhanced for better URL parsing and connection state tracking. Project metadata and test dependencies are updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Pytest
participant AirOS
participant aiohttp.Session
participant MockedHTTP
Tester->>Pytest: Run test_ap with mode param
Pytest->>conftest.py: Setup airos_device fixture
conftest.py->>aiohttp.Session: Create session
conftest.py->>AirOS: Instantiate AirOS with session and credentials
Pytest->>AirOS: Call login()
AirOS->>MockedHTTP: POST login request
MockedHTTP-->>AirOS: Return login response (cookies)
AirOS->>AirOS: Set connected = True
Pytest->>AirOS: Call status()
AirOS->>MockedHTTP: GET status request
MockedHTTP-->>AirOS: Return status JSON (from fixture)
AirOS-->>Pytest: Return status dict
Pytest->>aiohttp.Session: Close session
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 4
🧹 Nitpick comments (5)
requirements-test.txt (1)
4-4: Pin the new dependency to a major-minor version to keep CI deterministic
aioresponsesis added unpinned. A floating dependency means everypytestrun can pull in a newer release with breaking changes and silently break the test suite.- aioresponses + aioresponses>=0.7,<1.0(Adjust the upper bound to whatever semantic-versioning window you are comfortable with.)
tests/bandit.yaml (1)
3-21: Consider positive selection carefully – you’re opting-in only to these Bandit rulesBy listing explicit test IDs under
tests:you exclude every other Bandit check.
That may unintentionally drop high-impact rules such asB301(pickle) orB303/B304(hashlib-md5/sha1). If the intention is to skip these rules, use theskips:key instead; otherwise confirm that this curated allow-list is really what you want.tests/__init__.py (1)
1-1: Nit: file can be removed unless package initialisation is required
tests/is collected by pytest without needing an__init__.py; keeping the tree namespace-package avoids accidentally importing test helpers in production code.pyproject.toml (2)
7-7: Bump version in lock-step with tag / changelogThe project version is now
0.0.5. Make sure the release notes (CHANGELOG / Git tag) are updated in the same PR so that packaging automation doesn’t publish an undocumented version.
537-538: Switchingpytest.asyncio_modetoautocan mask event-loop misuse
autofalls back to a default loop when a fixture forgets to provide one, so genuine missing‐await errors may go unnoticed. If the earlier"strict"setting caught issues for you, double-check that the new async tests still fail when the loop handling is wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.pre-commit-config.yaml(1 hunks)airos/airos8.py(7 hunks)pyproject.toml(2 hunks)requirements-test.txt(1 hunks)tests/__init__.py(1 hunks)tests/bandit.yaml(1 hunks)tests/conftest.py(1 hunks)tests/test_stations.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/conftest.py (1)
airos/airos8.py (1)
AirOS(16-209)
airos/airos8.py (2)
tests/conftest.py (1)
base_url(10-12)airos/exceptions.py (2)
ConnectionFailedError(8-9)DataMissingError(12-13)
🔇 Additional comments (11)
tests/conftest.py (2)
9-12: LGTM! Simple and effective fixture.The base URL fixture provides a clean testing URL that's appropriate for unit tests.
15-21: Good async fixture with proper cleanup.The fixture correctly:
- Creates an aiohttp session with cookie jar
- Instantiates AirOS with appropriate test parameters
- Properly cleans up the session after yielding
The parameters match the updated AirOS constructor from
airos/airos8.py(lines 19-27).tests/test_stations.py (1)
43-51: Good test structure with proper mocking.The test correctly:
- Uses context managers for patching
- Tests both login and status operations
- Verifies the expected wireless mode from fixture data
The assertion on line 51 appropriately tests that the fixture data contains the expected mode.
airos/airos8.py (8)
16-16: Good class rename for clarity.Renaming from
AirOS8toAirOSmakes the class name more generic and maintainable.
25-25: Good addition of use_ssl parameter.The
use_sslparameter provides flexibility for testing and different deployment scenarios, as evidenced by its use in the test fixtures.
32-40: Robust URL parsing implementation.The URL parsing logic correctly:
- Handles both full URLs and hostnames
- Defaults to appropriate schemes based on
use_ssl- Extracts hostname properly using
urlparseThis is a significant improvement over hardcoded HTTPS assumptions.
65-65: Good addition of connection state tracking.The
connectedattribute provides a clean way to track connection state and enforce proper usage patterns.
164-165: Improved login method with state management.The changes correctly:
- Parse JSON directly from
response.text- Set
connected = Trueupon successful login- Maintain the same error handling logic
181-183: Proper connection state enforcement.The status method correctly checks connection state before proceeding, which prevents usage errors and provides clear feedback.
198-198: Cleaner JSON parsing.Direct parsing from
response.textis more straightforward than the previous implementation.
205-205: Better error logging with response text.Including the raw response text in error logs will help with debugging authentication issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_stations.py (1)
42-43: Still has inconsistent mock data structure.The mock response setup still has the same inconsistency issue from the previous review:
- Line 43:
mock_status_payloadwraps fixture data in{"mode": fixture_data}- Line 46:
mock_status_response.textcontains raw fixture data as JSON string- Line 48:
mock_status_response.json()returns the wrapped payloadSince the AirOS status method uses
json.loads(response.text), this inconsistency could cause confusion.Additionally, the fixture is loaded twice (lines 42 and 43), which is inefficient.
Apply this fix:
-fixture_data = await _read_fixture(mode) -mock_status_payload = {"mode": await _read_fixture(fixture=mode)} +fixture_data = await _read_fixture(mode) +mock_status_payload = fixture_data mock_status_response = MagicMock() mock_status_response.__aenter__.return_value = mock_status_response mock_status_response.text = json.dumps(fixture_data) mock_status_response.status = 200 mock_status_response.json = AsyncMock(return_value=mock_status_payload)Also applies to: 46-46, 48-48
🧹 Nitpick comments (2)
tests/test_stations.py (2)
28-28: Consider a more descriptive test function name.The function name
test_apis generic but tests both "ap-ptp" and "sta-ptp" modes. A more descriptive name would improve test readability.Consider renaming to something like
test_device_modesortest_ap_and_sta_modes:-async def test_ap(airos_device, base_url, mode): +async def test_device_modes(airos_device, base_url, mode):
54-58: Add more comprehensive test assertions.The test could benefit from additional assertions to verify the login state and provide more confidence in the test coverage.
Consider adding these assertions:
assert await airos_device.login() +assert airos_device.connected is True +assert airos_device.current_csrf_token == "test-csrf-token" status = await airos_device.status() # Verify the fixture returns the correct mode assert status.get("wireless", {}).get("mode") == mode +assert isinstance(status, dict)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_stations.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_stations.py (2)
tests/conftest.py (2)
airos_device(16-21)base_url(10-12)airos/airos8.py (2)
status(179-209)login(67-177)
🔇 Additional comments (1)
tests/test_stations.py (1)
13-24: LGTM! Error handling improvements implemented.The fixture reading function now includes proper error handling for file not found and JSON decode errors, addressing the previous review feedback. The path construction is also more robust.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation