refactor(test): API call methods and enhance test coverage #17
Conversation
WalkthroughThis update refactors internal API call methods in Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant ApiLib as ApiLib
participant Server as Test REST API
Test->>ApiLib: _call(method, path, ...)
ApiLib->>Server: HTTP request
Server-->>ApiLib: HTTP status code, response
ApiLib-->>Test: (status, response) tuple
Note over ApiLib: If status is 2xx, response is returned<br/>If not, returns None
Possibly related PRs
Poem
✨ Finishing Touches
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
🔭 Outside diff range comments (1)
api_lib/api_lib.py (1)
59-113: Critical behavioral change: Retry logic removed.The removal of retry logic represents a significant behavioral change. The method now returns
(None, None)immediately on any exception instead of attempting retries, which could impact application reliability.Please verify that the removal of retry logic is intentional and that calling code can handle immediate failures. Also, there's a potential logging format issue flagged by static analysis.
Let me check if there are other places in the codebase that might depend on the retry behavior:
#!/bin/bash # Search for any documentation or comments mentioning retry behavior rg -i "retry|retries" --type py -A 3 -B 3 # Search for any error handling that might expect retries ast-grep --pattern 'try { $$$ _call_api_with_timeout($$$) $$$ } catch { $$$ }'
🧹 Nitpick comments (3)
test/config/rest_api.py (1)
42-45: Consider the impact of the 30-second sleep.While the slow endpoint is useful for testing timeout behavior, the 30-second sleep duration could significantly slow down test execution if not properly managed with appropriate timeouts in tests.
Consider adding a comment to clarify the intended use case:
@app.route("/slow_endpoint", methods=["GET"]) def slow_endpoint(): + # Simulates a slow API response for timeout testing time.sleep(30) return jsonify({"success": True}), 200api_lib/api_lib.py (1)
28-57: Method refactoring looks good, but consider exception handling.The renaming from
__callto_calland the simplified return of status code and data is a good improvement. However, the generic exception handling at lines 54-55 is unnecessary.Remove the redundant generic exception handling:
try: data: dict = await res.json() except aiohttp.ContentTypeError: data: str = await res.text() -except Exception as e: - raise etest/test_api_lib.py (1)
2-2: Consider the implications of testing private methods.While the direct testing of
_calland_call_api_with_timeoutprovides good coverage, testing private methods (those with underscore prefix) can make tests more fragile to internal refactoring. Consider whether some of these tests could focus on the public API instead.That said, given this appears to be a library where these methods represent core functionality, the current approach provides valuable coverage and aligns well with the refactoring objectives.
Also applies to: 9-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.coveragerc(1 hunks)api_lib/api_lib.py(4 hunks)test/config/rest_api.py(2 hunks)test/conftest.py(1 hunks)test/test_api_lib.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/conftest.py (1)
test/config/rest_api.py (1)
run_server(50-52)
api_lib/api_lib.py (2)
api_lib/objects/request.py (2)
as_dict(18-23)RequestData(16-35)api_lib/method.py (1)
Method(4-9)
🪛 Pylint (3.3.7)
api_lib/api_lib.py
[refactor] 59-59: Too many arguments (6/5)
(R0913)
[refactor] 59-59: Too many positional arguments (6/5)
(R0917)
[error] 94-102: Too many arguments for logging format string
(E1205)
🔇 Additional comments (9)
.coveragerc (1)
6-7: Good addition to coverage exclusions.Excluding logging statements from coverage reports is a best practice, as these lines typically don't require explicit testing and can artificially inflate coverage metrics without meaningful test value.
test/config/rest_api.py (1)
14-25: Good simplification of test endpoints.The new
/returns_listendpoint provides a clean, simple response for testing list handling functionality.api_lib/api_lib.py (1)
125-133: Status code handling implementation is correct.The updated logic to use HTTP status codes instead of boolean flags provides better granularity and aligns well with the refactored
_call_api_with_timeoutmethod.test/conftest.py (1)
11-11: Good change to relative import.The switch from absolute to relative import follows Python packaging best practices and maintains consistency with the package structure.
test/test_api_lib.py (5)
9-14: Good direct testing of internal API calls.Testing the
_callmethod directly provides good coverage of the core functionality and allows for precise status code verification.
30-33: Excellent timeout test implementation.The test correctly verifies that the timeout functionality works as expected, returning
(None, None)when the request times out. The 2-second timeout against the 30-second endpoint is well-designed for fast test execution.
54-55: Correct exception handling for unreachable endpoints.The change from expecting
RuntimeErrortoClientConnectorErroris more precise and aligns with the refactored error handling that no longer wraps connection errors.
60-61: Status return type change handled correctly.The test correctly expects an integer status code instead of a boolean, reflecting the API refactoring changes.
83-92: Good addition of granular error testing.These tests provide excellent coverage for both
try_req(returningNoneon failure) andreq(raisingRuntimeErroron failure) scenarios.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores