Increase test coverage to 98%: add comprehensive unit tests for client/device, retries, and export paths#4
Conversation
…verage to 98%. The tests cover various edge cases and error handling in client.py and device.py, including authentication flows, connection error retries, JSON parsing, and response handling. Additional tests ensure robustness against invalid inputs and unexpected server responses.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage improvements to achieve 95%+ code coverage, focusing on uncovered branches and edge cases in client.py and device.py. The version is also bumped to 2.0.2.
Key Changes
- Added 1,221 lines of new tests covering authentication helpers, JSON parsing fallback paths, connection error retry logic, export_data_zip edge cases, and device.py missing lines
- Improved branch coverage from 0% to 96.23% and line coverage from 82.6% to 98.05%
- Tests cover error handling paths, retry mechanisms, encryption/decryption edge cases, and helper function branches
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/unit/test_coverage_improvements.py | New comprehensive test file covering previously untested branches and edge cases |
| fmd_api/_version.py | Version bump to 2.0.2 |
| coverage.xml | Updated coverage metrics showing improved coverage |
| .coverage | Binary coverage data file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_device_get_history_decrypt_error(): | ||
| """Test Device.get_history handles decrypt errors (line 99-101).""" |
There was a problem hiding this comment.
The comment references "line 99-101" but these line numbers don't correspond to the actual device.py file being tested. The get_history method starts at line 69 in device.py. Update the comment to reference the correct line numbers or remove specific line references for clarity.
| """Test Device.get_history handles decrypt errors (line 99-101).""" | |
| """Test Device.get_history handles decrypt errors.""" |
|
|
||
| @pytest.mark.asyncio | ||
| async def test_device_download_photo_decode_error(): | ||
| """Test Device.download_photo handles decode errors (line 137-138).""" |
There was a problem hiding this comment.
The comment references "line 137-138" but the download_photo method in device.py starts at line 115. Update the comment to reference the correct line numbers or remove specific line references for clarity.
| """Test Device.download_photo handles decode errors (line 137-138).""" | |
| """Test Device.download_photo handles decode errors.""" |
|
|
||
| # With enough samples, should have some variation (not all the same) | ||
| # (This might fail in rare cases but is statistically very unlikely) | ||
| assert len(set(delays)) > 1 or delays[0] == 0 # Allow all zeros as edge case |
There was a problem hiding this comment.
This assertion is testing randomness which can be flaky. While the comment acknowledges this ("This might fail in rare cases but is statistically very unlikely"), such tests can cause intermittent CI failures. Consider using a fixed seed for the random number generator or mocking the random function to make the test deterministic.
| with tempfile.NamedTemporaryFile(suffix='.zip', delete=False) as tmp: | ||
| output_path = tmp.name | ||
|
|
||
| result = await client.export_data_zip(output_path, include_pictures=True) |
There was a problem hiding this comment.
Variable result is not used.
| result = await client.export_data_zip(output_path, include_pictures=True) | |
| await client.export_data_zip(output_path, include_pictures=True) |
| with tempfile.NamedTemporaryFile(suffix='.zip', delete=False) as tmp: | ||
| output_path = tmp.name | ||
|
|
||
| result = await client.export_data_zip(output_path, include_pictures=False) |
There was a problem hiding this comment.
Variable result is not used.
| result = await client.export_data_zip(output_path, include_pictures=False) | |
| await client.export_data_zip(output_path, include_pictures=False) |
Summary
fmd_api.clientandfmd_api.deviceWhat changed
tests/unit/test_coverage_improvements.py:coverage.xmlgenerated for CodecovWhy
Metrics
client.py~77%,device.py~94%)client.py~97%,device.py100%)Compatibility
How to validate locally (optional)
Notes and follow-ups
client.py(e.g., specific fallback/log-only segments):353–358,368,392–393,477–478,563,717–718aiohttpresponse mocking or small refactors to expose those branches for deterministic testing; can be tackled in a follow-up if desiredChecklist
coverage.xmlupdated for CI/Codecov