-
Notifications
You must be signed in to change notification settings - Fork 9
Feat/async #122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/async #122
Conversation
…out API with unit tests.
…API with unit tests.
…h unit tests for testing and updated unit tests name from Bill Integration to Bill Manager.
…e of bill manager unit tests
… API with unit tests
… API with unit tests.
📝 WalkthroughWalkthroughAdds asynchronous counterparts (Async*) for many synchronous mpesakit clients, updates package exports and tests, and ensures synchronous methods return response models. Async clients use AsyncHttpClient and AsyncTokenManager and mirror existing request/response flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant TM as TokenManager
participant HC as HttpClient
participant API as Remote API
Caller->>TM: get_token() / await get_token()
TM-->>Caller: Bearer <token>
Caller->>HC: POST endpoint, headers: Authorization: Bearer <token>, body
HC-->>API: HTTP POST
API-->>HC: JSON response
HC-->>Caller: response_data
Caller->>Caller: ResponseModel(**response_data)
Caller-->>Caller: return ResponseModel
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@mpesakit/bill_manager/bill_manager.py`:
- Around line 161-165: The ValueError raised in _ensure_app_key currently
references "BillManager" but should reference "AsyncBillManager"; update the
exception message in the _ensure_app_key method to mention AsyncBillManager
(e.g., "app_key must be set for this operation. You must pass it when
initializing AsyncBillManager.") so error messages correctly identify the class.
In `@tests/unit/b2b_express_checkout/test_b2b_express_checkout.py`:
- Around line 141-152: The fixtures use MagicMock for async behaviors which
causes await issues; change mock_async_token_manager and mock_async_http_client
to use AsyncMock (from unittest.mock) with spec=AsyncTokenManager and
spec=AsyncHttpClient respectively, and set
mock_async_token_manager.get_token.return_value = "async_test_token" so await
mock.get_token() returns the string correctly; update both fixture definitions
(mock_async_token_manager and mock_async_http_client) to instantiate AsyncMock
instead of MagicMock.
In `@tests/unit/c2b/test_c2b.py`:
- Around line 423-429: The async success test's response_data uses the
misspelled key "OriginatorCoversationID"; update that key to the correct
"OriginatorConversationID" in the response_data dict in the async register URL
success test so it matches the sync test, leaving the typo-handling covered by
test_async_register_url_handles_typo_field; only change the key name in the
response_data object (keep other fields unchanged).
In `@tests/unit/dynamic_qr_code/test_dynamic_qr_code.py`:
- Around line 186-284: Two assertions use assert_called_once() on AsyncMock
objects; replace them with assert_awaited_once() to ensure the coroutines were
awaited. In test_async_generate_dynamic_qr_success replace
mock_async_http_client.post.assert_called_once() with
mock_async_http_client.post.assert_awaited_once(), and in
test_async_generate_dynamic_qr_token_manager_called replace
mock_async_token_manager.get_token.assert_called_once() with
mock_async_token_manager.get_token.assert_awaited_once().
🧹 Nitpick comments (7)
mpesakit/account_balance/account_balance.py (1)
1-5: Consider updating the module docstring to mention async support.The docstring only mentions
HttpClientbut the module now also provides async functionality viaAsyncHttpClient. A minor documentation update would improve discoverability.📝 Suggested docstring update
"""Account Balance: Handles M-Pesa Account Balance API interactions. This module provides functionality to query account balance and handle result/timeout notifications -using the M-Pesa API. Requires a valid access token for authentication and uses the HttpClient for HTTP requests. +using the M-Pesa API. Requires a valid access token for authentication. Supports both synchronous +(HttpClient) and asynchronous (AsyncHttpClient) HTTP requests. """tests/unit/account_balance/test_account_balance.py (1)
226-237: Consider usingAsyncMockfor async method mocking.
MagicMockcan technically be awaited in Python 3.8+, butAsyncMockprovides clearer intent and proper async semantics. This makes the tests more robust and explicit about async behavior.♻️ Suggested refactor
+from unittest.mock import AsyncMock, MagicMock -from unittest.mock import MagicMock `@pytest.fixture` def mock_async_token_manager(): """Mock AsyncTokenManager for testing.""" - mock = MagicMock(spec=AsyncTokenManager) - mock.get_token.return_value = "test_async_token" + mock = MagicMock(spec=AsyncTokenManager) + mock.get_token = AsyncMock(return_value="test_async_token") return mock `@pytest.fixture` def mock_async_http_client(): """Mock AsyncHttpClient for testing.""" - return MagicMock(spec=AsyncHttpClient) + mock = MagicMock(spec=AsyncHttpClient) + mock.post = AsyncMock() + mock.get = AsyncMock() + return mockmpesakit/b2b_express_checkout/b2b_express_checkout.py (1)
55-87: LGTM! Async implementation correctly mirrors the sync version.The
AsyncB2BExpressCheckoutclass properly usesawaitfor both token retrieval and HTTP POST operations. The implementation is consistent with the synchronous counterpart.Consider adding the API documentation link in the docstring for consistency with the sync class (Line 21 has
https://developer.safaricom.co.ke/APIs/B2BExpressCheckout).mpesakit/reversal/reversal.py (1)
53-83: Consider DRYing the request construction across sync/async.
BothReversal.reverseandAsyncReversal.reversebuild the same URL, headers, and payload. A small shared helper (pure function) would reduce drift risk.tests/unit/reversal/test_reversal.py (1)
363-403: Strengthen async tests to assert awaited token use + header value.
This makes the tests catch accidental removal ofawaitand verifies token propagation.✅ Suggested test tightening
@@ - mock_async_token_manager.get_token.assert_called_once() + mock_async_token_manager.get_token.assert_awaited_once() @@ - assert "Authorization" in call_args[1]["headers"] + assert "Authorization" in call_args[1]["headers"] + assert call_args[1]["headers"]["Authorization"] == "Bearer test_token_async"Please confirm your minimum Python version supports
AsyncMock.assert_awaited_once()(Py 3.8+).tests/unit/b2c/test_b2c.py (1)
477-487: Misleading test name: tests token error, not HTTP error.The test
test_async_send_payment_http_erroractually tests a token retrieval error (mock_async_token_manager.get_token.side_effect), not an HTTP error. Consider renaming totest_async_send_payment_token_errorfor clarity.💡 Suggested rename
`@pytest.mark.asyncio` -async def test_async_send_payment_http_error( +async def test_async_send_payment_token_error( async_b2c, mock_async_http_client, mock_async_token_manager ): - """Test that async B2C payment handles HTTP errors gracefully.""" + """Test that async B2C payment handles token errors gracefully.""" request = valid_b2c_request() mock_async_token_manager.get_token.side_effect = Exception("Token error")tests/unit/mpesa_express/test_stk_push.py (1)
186-219: Preferassert_awaited_*on AsyncMock assertions.
assert_called_once()won’t catch a missingawait; usingassert_awaited_once()makes these async tests more robust.♻️ Suggested test tightening
-async def test_async_push_success(async_stk_push, mock_async_http_client): +async def test_async_push_success(async_stk_push, mock_async_http_client, mock_async_token_manager): ... - mock_async_http_client.post.assert_called_once() + mock_async_http_client.post.assert_awaited_once() + mock_async_token_manager.get_token.assert_awaited_once() ... -async def test_async_query_success(async_stk_push, mock_async_http_client): +async def test_async_query_success(async_stk_push, mock_async_http_client, mock_async_token_manager): ... - mock_async_http_client.post.assert_called_once() + mock_async_http_client.post.assert_awaited_once() + mock_async_token_manager.get_token.assert_awaited_once()Also applies to: 222-249
…when app key is not passed
…added stricter assertion to response header
… and added stricter assertions
There was a problem hiding this 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
🤖 Fix all issues with AI agents
In `@tests/unit/account_balance/test_account_balance.py`:
- Around line 273-277: Replace the synchronous Mock assertions with
AsyncMock-await assertions: change
mock_async_http_client.post.assert_called_once() to
mock_async_http_client.post.assert_awaited_once(), and use
mock_async_http_client.post.await_args (or await_args_list[0] if necessary)
instead of call_args to extract the awaited call's args/kwargs; then assert the
request path and headers (Authorization and Content-Type) against those
await_args to ensure the coroutine was actually awaited.
♻️ Duplicate comments (1)
tests/unit/dynamic_qr_code/test_dynamic_qr_code.py (1)
237-237: Replaceassert_called_once()withassert_awaited_once()for AsyncMock.
assert_called_once()only verifies the mock was called but doesn't confirm the coroutine was awaited. Useassert_awaited_once()for proper async verification.Proposed fix
- mock_async_http_client.post.assert_called_once() + mock_async_http_client.post.assert_awaited_once()
🧹 Nitpick comments (5)
tests/unit/b2c/test_b2c.py (1)
451-475: Add mock invocation assertions for parity with sync test.The synchronous
test_send_payment_successverifies thatmock_http_client.post.assert_called_once()was invoked and checks theAuthorizationheader. This async test should similarly verify that the mocked async methods were awaited, ensuring the client correctly called the dependencies.♻️ Suggested addition after line 474
assert response.ResponseDescription == response_data["ResponseDescription"] + + mock_async_token_manager.get_token.assert_awaited_once() + mock_async_http_client.post.assert_awaited_once() + args, kwargs = mock_async_http_client.post.call_args + assert args[0] == "/mpesa/b2c/v3/paymentrequest" + assert kwargs["headers"]["Authorization"] == "Bearer test_token_async"tests/unit/b2b_express_checkout/test_b2b_express_checkout.py (2)
163-181: Consider verifying HTTP client call arguments.The test validates response parsing but doesn't verify that
mock_async_http_client.postwas called with the correct URL, headers (including the Authorization token), and request body. Adding such assertions would strengthen the test by ensuring the integration between the token manager and HTTP client.Also, line 174 redundantly sets
get_token.return_valuewhich is already configured in the fixture (line 145).♻️ Optional enhancement
mock_async_http_client.post.return_value = response_data - mock_async_token_manager.get_token.return_value = "async_test_token" response = await async_b2b_express_checkout.ussd_push(request) assert isinstance(response, B2BExpressCheckoutResponse) assert response.is_successful() is True assert response.code == response_data["code"] assert response.status == response_data["status"] + mock_async_http_client.post.assert_called_once() + call_args = mock_async_http_client.post.call_args + assert call_args[0][0] == "/v1/ussdpush/get-msisdn" + assert "Bearer async_test_token" in call_args[1]["headers"]["Authorization"]
197-212: Consider extending assertion to verify token usage.The test verifies
get_tokenwas called, but doesn't confirm the token was actually used in the HTTP request headers. Consider adding an assertion to verify the Authorization header contains the expected token value.Line 208 also redundantly sets
get_token.return_valuealready configured in the fixture.♻️ Optional enhancement
mock_async_http_client.post.return_value = response_data - mock_async_token_manager.get_token.return_value = "async_test_token" await async_b2b_express_checkout.ussd_push(request) mock_async_token_manager.get_token.assert_called_once() + # Verify token was used in the HTTP request + call_kwargs = mock_async_http_client.post.call_args[1] + assert call_kwargs["headers"]["Authorization"] == "Bearer async_test_token"tests/unit/mpesa_express/test_stk_push.py (1)
256-292: LGTM!The error handling tests correctly verify that exceptions propagate through the async methods. Using
pytest.raiseswithawaitinside the context manager is the proper pattern for async exception testing.Optional enhancement: Consider adding tests for more specific exception types (e.g.,
MpesaApiException) if the production code raises domain-specific exceptions for different failure scenarios.tests/unit/reversal/test_reversal.py (1)
382-403: Tighten AsyncMock assertions and verify JSON payload.
Prefer asserting the awaited call and validating the serialized request body to make this test more precise.♻️ Proposed update
- assert mock_async_http_client.post.called - call_args = mock_async_http_client.post.call_args - assert call_args[0][0] == "/mpesa/reversal/v1/request" - assert "Authorization" in call_args[1]["headers"] - assert call_args[1]["headers"]["Authorization"] == "Bearer test_token_async" - assert call_args[1]["headers"]["Content-Type"] == "application/json" + mock_async_http_client.post.assert_awaited_once() + args, kwargs = mock_async_http_client.post.call_args + assert args[0] == "/mpesa/reversal/v1/request" + assert kwargs["json"] == request.model_dump(by_alias=True) + assert kwargs["headers"]["Authorization"] == "Bearer test_token_async" + assert kwargs["headers"]["Content-Type"] == "application/json"
… call with appropriate args.
Description
This PR introduces comprehensive async support across the entire M-Pesa SDK by implementing asynchronous versions of all major API clients. This enables non-blocking operations for high-throughput applications and modern async/await Python applications.
The implementation includes:
This change maintains full backward compatibility with existing synchronous code while providing async alternatives for improved performance in async contexts.
Fixes #58 #59 #60 #61 #62 #63 #64 #65 #66 #67 #68 #69 #70 #71
Type of Change
How Has This Been Tested?
AsyncMockChecklist
Additional Context
This async implementation lays the foundation for high-performance M-Pesa integrations in modern Python applications using frameworks like FastAPI, aiohttp, or any async web framework. The async clients follow the same interface patterns as their synchronous counterparts, making migration straightforward for users.
Future work could include:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.