Chore: align sync/async clients, update stk_push tests and add 100-call STK push stability tests#127
Open
watersRand wants to merge 7 commits intoByte-Barn:masterfrom
Open
Chore: align sync/async clients, update stk_push tests and add 100-call STK push stability tests#127watersRand wants to merge 7 commits intoByte-Barn:masterfrom
watersRand wants to merge 7 commits intoByte-Barn:masterfrom
Conversation
… stk_push tests to use both session and no session in addition to 100 stk_pushes. closes#116
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds tenacity-backed retry and error-mapping to the async Mpesa HTTP client, introduces low-level retry-wrapped Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Caller"
participant Client as "MpesaAsyncHttpClient"
participant Retry as "Tenacity (RetryPolicy)"
participant Httpx as "httpx.AsyncClient"
participant API as "Mpesa API"
Caller->>Client: post(path, json, headers)
Client->>Client: join base + path -> abs_url
Client->>Retry: call async_raw_post(abs_url, json, headers, timeout)
Retry->>Httpx: POST abs_url, json, headers, timeout
Httpx->>API: HTTP POST request
API-->>Httpx: HTTP response / error
Httpx-->>Retry: return response or raise
Retry-->>Client: final response or raise after retries
Client->>Client: handle_request_error(response) or map retry failure
alt success
Client-->>Caller: return parsed JSON
else failure
Client-->>Caller: raise MpesaApiException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
mpesakit/http_client/mpesa_async_http_client.py (2)
130-136: Redundant URL resolution:urljoin+httpx.AsyncClient(base_url=…)overlap.
async_raw_postandasync_raw_getbuild a full absolute URL viaurljoin(self.base_url, url)and pass it toself._client.post(full_url, …). Sinceself._clientwas created withbase_url=self.base_url(line 113), httpx would also resolve relative URLs on its own — but that capability is never used because you always supply an absolute URL. Thebase_urlargument in__init__is effectively dead.Pick one resolution strategy: either rely on
httpx.AsyncClient(base_url=…)and pass relative paths, or dropbase_urlfrom the client constructor and keepurljoin.Also applies to: 179-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 130 - 136, The code redundantly resolves URLs by calling urljoin(self.base_url, url) in async_raw_post and async_raw_get while also instantiating httpx.AsyncClient with base_url=self.base_url in __init__, so either remove the client base_url and keep urljoin or stop calling urljoin and pass relative paths to self._client; pick one: I suggest deleting urljoin usage in async_raw_post and async_raw_get and pass the relative url directly to self._client.post/get so httpx resolves against base_url, and remove any unused import of urljoin; update references to self._client, async_raw_post, async_raw_get, and __init__ accordingly.
155-170:MpesaApiExceptionfrom retry exhaustion bypasses thisexcept— by design, but worth a comment.When retries are exhausted,
handle_retry_exceptionraisesMpesaApiException, which is not anhttpx.RequestErrororValueError, so it flies past thisexceptclause. Theexceptonly catches non-retried transport errors and JSON parse failures. The same applies toget()at lines 216-224. This is correct behavior, but a brief inline comment would clarify the intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 155 - 170, Add a brief inline comment in the async post and get request flows (around the try/except that calls async_raw_post/async_raw_get, handle_request_error, and returns response.json()) explaining that MpesaApiException raised by handle_retry_exception on retry exhaustion is intentionally not caught by the except (which only handles httpx.RequestError and ValueError for non-retried transport or JSON errors); place this comment immediately above the except block so future maintainers understand the design intent and why MpesaApiException will propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 316-339: Fix the docstring typo in test_stk_push_retry_async
("asnycClient" → "asyncClient") and update the test to actually exercise the
retry behavior: when patching httpx.AsyncClient.post (mock name
mock_async_post), use an AsyncMock side_effect that simulates transient failures
for the first N attempts (e.g., raise an httpx.ConnectError or return a response
with is_success=False/status_code!=200) and then returns a successful
mock_response, run the client.post loop and assert that each logical call
eventually returns ResponseCode "0" while also asserting
mock_async_post.await_count is greater than the number of logical calls (showing
retries occurred); reference MpesaAsyncHttpClient and test_stk_push_retry_async
to locate and update the test.
- Around line 165-184: The test named test_stk_push_retry is misleading and
never exercises the retry decorator because the mock response is always
successful; rename it to test_stk_push_100_calls_success (and the async
counterpart) and fix typos in the docstring ("multiple") and variable name
(succcess_count → success_count); add a new test (e.g.,
test_stk_push_retries_on_transient_error) that uses the same MpesaHttpClient
instantiation and patches httpx.Client.post with a side_effect sequence like
[httpx.TimeoutException(), mock_response] (or httpx.ConnectError), call
client.post once, then assert the result is the successful response, assert
mock_post.call_count == 2, and verify retry behavior (and mirror for async test
using httpx.AsyncClient.post).
- Around line 31-35: The fixture mock_http_client currently instantiates a real
MpesaHttpClient inside MagicMock(spec=...) which creates an actual httpx.Client;
change it to use the class as the spec (MagicMock(spec=MpesaHttpClient)) to
avoid creating real network resources, and if tests need the parameterized
use_session value attach it to the returned mock (e.g., mock.use_session =
use_session) so downstream tests can observe the flag without instantiating a
real client.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 130-136: The code redundantly resolves URLs by calling
urljoin(self.base_url, url) in async_raw_post and async_raw_get while also
instantiating httpx.AsyncClient with base_url=self.base_url in __init__, so
either remove the client base_url and keep urljoin or stop calling urljoin and
pass relative paths to self._client; pick one: I suggest deleting urljoin usage
in async_raw_post and async_raw_get and pass the relative url directly to
self._client.post/get so httpx resolves against base_url, and remove any unused
import of urljoin; update references to self._client, async_raw_post,
async_raw_get, and __init__ accordingly.
- Around line 155-170: Add a brief inline comment in the async post and get
request flows (around the try/except that calls async_raw_post/async_raw_get,
handle_request_error, and returns response.json()) explaining that
MpesaApiException raised by handle_retry_exception on retry exhaustion is
intentionally not caught by the except (which only handles httpx.RequestError
and ValueError for non-retried transport or JSON errors); place this comment
immediately above the except block so future maintainers understand the design
intent and why MpesaApiException will propagate.
Comment on lines
165
to
184
| @pytest.mark.parametrize("use_session", [True, False]) | ||
| def test_stk_push_retry(use_session): | ||
| """Test that mutliple simulation sync of stk_push is successful.""" | ||
| client = MpesaHttpClient(env="sandbox", use_session=use_session) | ||
|
|
||
| mock_response = MagicMock(spec=httpx.Response) | ||
| mock_response.is_success = True | ||
| mock_response.status_code = 200 | ||
| mock_response.json.return_value = {"MerchantRequestID": "12345", "ResponseCode": "0"} | ||
|
|
||
| with patch.object(httpx.Client, "post", return_value=mock_response) as mock_post: | ||
| succcess_count= 0 | ||
|
|
||
| for _ in range(100): | ||
| result = client.post("/test", json={}, headers={}) | ||
| if result["ResponseCode"] == "0": | ||
| succcess_count += 1 | ||
|
|
||
| assert succcess_count == 100 | ||
| assert mock_post.call_count == 100 |
Contributor
There was a problem hiding this comment.
Test name is misleading and retry logic is never exercised.
test_stk_push_retry (and the async counterpart) only fires 100 successful calls — the mock never raises TimeoutException or ConnectError, so the @retry decorator is never triggered. This tests throughput/stability, not retry behavior. Consider:
- Renaming to
test_stk_push_100_calls_success(or similar) to reflect what's actually tested. - Adding a separate test that injects transient failures (e.g.,
side_effect=[TimeoutException(...), mock_response]) to verify the retry path.
Also, a couple of typos:
- Line 167:
"mutliple"→"multiple" - Line 176:
succcess_count→success_count
Proposed fix for typos
- """Test that mutliple simulation sync of stk_push is successful."""
+ """Test that multiple simulated sync STK push calls are successful."""
client = MpesaHttpClient(env="sandbox", use_session=use_session)
mock_response = MagicMock(spec=httpx.Response)
mock_response.is_success = True
mock_response.status_code = 200
mock_response.json.return_value = {"MerchantRequestID": "12345", "ResponseCode": "0"}
with patch.object(httpx.Client, "post", return_value=mock_response) as mock_post:
- succcess_count= 0
+ success_count = 0
- for _ in range(100):
- result = client.post("/test", json={}, headers={})
- if result["ResponseCode"] == "0":
- succcess_count += 1
+ for _ in range(100):
+ result = client.post("/test", json={}, headers={})
+ if result["ResponseCode"] == "0":
+ success_count += 1
- assert succcess_count == 100
+ assert success_count == 100
assert mock_post.call_count == 100🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/mpesa_express/test_stk_push.py` around lines 165 - 184, The test
named test_stk_push_retry is misleading and never exercises the retry decorator
because the mock response is always successful; rename it to
test_stk_push_100_calls_success (and the async counterpart) and fix typos in the
docstring ("multiple") and variable name (succcess_count → success_count); add a
new test (e.g., test_stk_push_retries_on_transient_error) that uses the same
MpesaHttpClient instantiation and patches httpx.Client.post with a side_effect
sequence like [httpx.TimeoutException(), mock_response] (or httpx.ConnectError),
call client.post once, then assert the result is the successful response, assert
mock_post.call_count == 2, and verify retry behavior (and mirror for async test
using httpx.AsyncClient.post).
Comment on lines
316
to
339
| @pytest.mark.asyncio | ||
| async def test_stk_push_retry_async(): | ||
| """Test that multiple simulation of asnycClient stk_push is successful.""" | ||
| client = MpesaAsyncHttpClient() | ||
|
|
||
| mock_response = MagicMock(spec=httpx.Response) | ||
| mock_response.is_success = True | ||
| mock_response.status_code = 200 | ||
| mock_response.json.return_value = {"MerchantRequestID": "12345", "ResponseCode": "0"} | ||
|
|
||
| async_mock = AsyncMock(return_value=mock_response) | ||
|
|
||
| with patch("httpx.AsyncClient.post", async_mock) as mock_async_post: | ||
|
|
||
|
|
||
| success_count = 0 | ||
|
|
||
| for _ in range(100): | ||
| result = await client.post("/test", json={}, headers={}) | ||
| if result["ResponseCode"] == "0": | ||
| success_count += 1 | ||
|
|
||
| assert success_count == 100 | ||
| assert mock_async_post.await_count == 100 |
Contributor
There was a problem hiding this comment.
Same issues as the sync counterpart: misleading name, not testing retry, typo.
Line 318 docstring: "asnycClient" → "asyncClient". Same as the sync test, this only exercises the happy path 100 times — no transient failures are injected to trigger the retry decorator.
Proposed fix for typo and formatting
`@pytest.mark.asyncio`
async def test_stk_push_retry_async():
- """Test that multiple simulation of asnycClient stk_push is successful."""
+ """Test that multiple simulated async STK push calls are successful."""
client = MpesaAsyncHttpClient()
mock_response = MagicMock(spec=httpx.Response)
@@ ..
async_mock = AsyncMock(return_value=mock_response)
with patch("httpx.AsyncClient.post", async_mock) as mock_async_post:
-
-
success_count = 0
for _ in range(100):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.asyncio | |
| async def test_stk_push_retry_async(): | |
| """Test that multiple simulation of asnycClient stk_push is successful.""" | |
| client = MpesaAsyncHttpClient() | |
| mock_response = MagicMock(spec=httpx.Response) | |
| mock_response.is_success = True | |
| mock_response.status_code = 200 | |
| mock_response.json.return_value = {"MerchantRequestID": "12345", "ResponseCode": "0"} | |
| async_mock = AsyncMock(return_value=mock_response) | |
| with patch("httpx.AsyncClient.post", async_mock) as mock_async_post: | |
| success_count = 0 | |
| for _ in range(100): | |
| result = await client.post("/test", json={}, headers={}) | |
| if result["ResponseCode"] == "0": | |
| success_count += 1 | |
| assert success_count == 100 | |
| assert mock_async_post.await_count == 100 | |
| `@pytest.mark.asyncio` | |
| async def test_stk_push_retry_async(): | |
| """Test that multiple simulated async STK push calls are successful.""" | |
| client = MpesaAsyncHttpClient() | |
| mock_response = MagicMock(spec=httpx.Response) | |
| mock_response.is_success = True | |
| mock_response.status_code = 200 | |
| mock_response.json.return_value = {"MerchantRequestID": "12345", "ResponseCode": "0"} | |
| async_mock = AsyncMock(return_value=mock_response) | |
| with patch("httpx.AsyncClient.post", async_mock) as mock_async_post: | |
| success_count = 0 | |
| for _ in range(100): | |
| result = await client.post("/test", json={}, headers={}) | |
| if result["ResponseCode"] == "0": | |
| success_count += 1 | |
| assert success_count == 100 | |
| assert mock_async_post.await_count == 100 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/mpesa_express/test_stk_push.py` around lines 316 - 339, Fix the
docstring typo in test_stk_push_retry_async ("asnycClient" → "asyncClient") and
update the test to actually exercise the retry behavior: when patching
httpx.AsyncClient.post (mock name mock_async_post), use an AsyncMock side_effect
that simulates transient failures for the first N attempts (e.g., raise an
httpx.ConnectError or return a response with is_success=False/status_code!=200)
and then returns a successful mock_response, run the client.post loop and assert
that each logical call eventually returns ResponseCode "0" while also asserting
mock_async_post.await_count is greater than the number of logical calls (showing
retries occurred); reference MpesaAsyncHttpClient and test_stk_push_retry_async
to locate and update the test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
mpesakit/http_client/mpesa_async_http_client.py (2)
76-94:retry_enabled(enabled=False)path is dead code — both decorators always passenabled=True.The
if not enabled: return Falsebranch is never executed. Either remove theenabledparameter and inlinebase_retrydirectly, or use it (e.g., tie it to a constructor flag for test overriding):♻️ Simplified form
-def retry_enabled(enabled: bool): - base_retry = retry_if_exception_type( - httpx.TimeoutException - ) | retry_if_exception_type(httpx.ConnectError) - - def _retry(retry_state): - if not enabled: - return False - return base_retry(retry_state) - - return _retry +_RETRY_PREDICATE = ( + retry_if_exception_type(httpx.TimeoutException) + | retry_if_exception_type(httpx.ConnectError) +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 76 - 94, The retry_enabled function contains dead code because the enabled parameter is never toggled; update it by removing the unused enabled parameter and return the composed retry predicate (base_retry) directly, or alternatively make enabled a configurable flag passed from the HTTP client constructor and wire that flag into retry_enabled so the _retry function actually checks a real setting; locate the retry_enabled factory, the base_retry variable and the inner _retry closure and either inline base_retry as the returned predicate or use a constructor property to control the enabled branch.
113-114:base_urlonhttpx.AsyncClientis unused — all requests already pass absolute URLs.
async_raw_postandasync_raw_getboth buildfull_url = urljoin(self.base_url, url)and hand the absolute URL directly toself._client.post(full_url, ...)/self._client.get(full_url, ...). When httpx receives an absolute URL it ignores the client's ownbase_url, sohttpx.AsyncClient(base_url=self.base_url)has no effect. Either remove thebase_urlkwarg and keepurljoin, or removeurljoinand let httpx resolve paths againstbase_url.♻️ Option A — drop redundant base_url from AsyncClient
- self._client = httpx.AsyncClient(base_url=self.base_url) + self._client = httpx.AsyncClient()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 113 - 114, The AsyncClient is constructed with base_url but all calls in async_raw_post and async_raw_get pass absolute URLs (full_url = urljoin(self.base_url, url)), so the client's base_url is unused; remove the base_url kwarg when creating self._client (change self._client = httpx.AsyncClient(base_url=self.base_url) to self._client = httpx.AsyncClient()) and keep the existing urljoin usage in async_raw_post and async_raw_get so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 123-128: The tenacity retry parameters are hardcoded via
decorators on MpesaAsyncHttpClient methods (the decorators at the top of the
POST/GET methods around lines shown), causing long sleeps in tests; remove the
class-level `@retry` decorators from those methods and instead perform retries
programmatically inside the method bodies using tenacity.Retrying/AsyncRetrying,
driven by injectable class-level attributes (e.g.
MpesaAsyncHttpClient._retry_enabled, _retry_wait, _retry_stop,
_retry_error_callback, _retry_before_sleep) with the same defaults as before;
update the POST and GET method implementations to call
AsyncRetrying(...).call/iterate and reference those attributes so tests can
override them (e.g. set MpesaAsyncHttpClient._retry_wait = wait_none() and
_retry_stop = stop_after_attempt(1)).
- Around line 37-44: The MpesaError.raw_response is receiving mixed types (dict
from handle_request_error vs str/None from post()/get()), so normalize it to a
single type (choose string) across code paths: in handle_request_error convert
response_data (dict) to a JSON string (or str()) before passing as raw_response
to MpesaError/MpesaApiException, and in post() and get() ensure the except
blocks pass raw_response as response.text or str(response_text) (or None)
consistently; make the same change in the synchronous client
(mpesakit/http_client/mpesa_http_client.py) and update any tests or callers
expecting a dict accordingly, referencing the functions/methods
handle_request_error, post, get, MpesaError and MpesaApiException.
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 31-34: The fixture mock_http_client is parameterized with
params=[True, False] but never reads request.param, so tests run twice without
effect; fix by either removing the params list from the fixture definition or by
consuming request.param inside mock_http_client (e.g., after creating
MagicMock(spec=MpesaHttpClient) set a flag/attribute on the mock like
mock.use_session = request.param or otherwise branch behavior based on
request.param) so the parameterization actually changes the returned mock's
behavior.
- Around line 316-338: The test test_async_stk_push_multiple_times leaks an
httpx.AsyncClient because MpesaAsyncHttpClient() is created but never closed;
update the test to ensure the client is closed by either awaiting
client.aclose() after the loop or by using an async context manager (async with
MpesaAsyncHttpClient() as client) around the loop so the underlying AsyncClient
is torn down; ensure the existing patch for httpx.AsyncClient.post (async_mock)
still applies inside the context so mock_async_post.await_count remains
accurate.
---
Duplicate comments:
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 164-183: The test function test_stk_push_multiple_times contains
typos and inconsistent indentation: rename the docstring word "mutliple" to
"multiple" and rename the variable succcess_count to success_count everywhere
(including the increment and asserts); also normalize indentation inside the
with patch.object(...) block so the with body is indented by 4 spaces and the
for loop and its body follow standard 4-space nesting (ensure the loop, the call
client.post from MpesaHttpClient, the conditional increment of success_count,
and the final asserts use consistent 4-space indentation); verify
mock_post.call_count usage remains correct.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 76-94: The retry_enabled function contains dead code because the
enabled parameter is never toggled; update it by removing the unused enabled
parameter and return the composed retry predicate (base_retry) directly, or
alternatively make enabled a configurable flag passed from the HTTP client
constructor and wire that flag into retry_enabled so the _retry function
actually checks a real setting; locate the retry_enabled factory, the base_retry
variable and the inner _retry closure and either inline base_retry as the
returned predicate or use a constructor property to control the enabled branch.
- Around line 113-114: The AsyncClient is constructed with base_url but all
calls in async_raw_post and async_raw_get pass absolute URLs (full_url =
urljoin(self.base_url, url)), so the client's base_url is unused; remove the
base_url kwarg when creating self._client (change self._client =
httpx.AsyncClient(base_url=self.base_url) to self._client = httpx.AsyncClient())
and keep the existing urljoin usage in async_raw_post and async_raw_get so
behavior remains unchanged.
Comment on lines
37
to
44
| raise MpesaApiException( | ||
| MpesaError( | ||
| error_code=f"HTTP_{response.status_code}", | ||
| error_message=error_message, | ||
| status_code=response.status_code, | ||
| raw_response=response_data, | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(mpesa|error)" | head -20Repository: Byte-Barn/mpesakit
Length of output: 836
🏁 Script executed:
cat -n mpesakit/http_client/mpesa_async_http_client.py | wc -lRepository: Byte-Barn/mpesakit
Length of output: 65
🏁 Script executed:
# First, let's look at the MpesaError class definition
rg -n "class MpesaError" --type py -A 15Repository: Byte-Barn/mpesakit
Length of output: 1021
🏁 Script executed:
# Now let's examine the three error handling locations
cat -n mpesakit/http_client/mpesa_async_http_client.py | sed -n '30,50p'Repository: Byte-Barn/mpesakit
Length of output: 869
🏁 Script executed:
# Check lines around 154-170 and 211-224
cat -n mpesakit/http_client/mpesa_async_http_client.py | sed -n '150,175p'Repository: Byte-Barn/mpesakit
Length of output: 1150
🏁 Script executed:
cat -n mpesakit/http_client/mpesa_async_http_client.py | sed -n '205,230p'Repository: Byte-Barn/mpesakit
Length of output: 1289
🏁 Script executed:
# Find where raw_response is used downstream
rg -n "raw_response" --type py -B 2 -A 2Repository: Byte-Barn/mpesakit
Length of output: 5253
raw_response field receives inconsistent types across error paths.
handle_request_error passes raw_response=response_data (a dict), while the except blocks in post() and get() pass raw_response=getattr(response, "text", None) (a str or None). Any downstream code that reads MpesaError.raw_response will encounter different types depending on the error path, which will cause AttributeError/TypeError at runtime if the field is assumed to be one type.
Normalize to a consistent type (e.g., always str or always dict):
🛠 Proposed fix for `handle_request_error`
raise MpesaApiException(
MpesaError(
error_code=f"HTTP_{response.status_code}",
error_message=error_message,
status_code=response.status_code,
- raw_response=response_data,
+ raw_response=response.text,
)
)Also applies to synchronous equivalents in mpesakit/http_client/mpesa_http_client.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| raise MpesaApiException( | |
| MpesaError( | |
| error_code=f"HTTP_{response.status_code}", | |
| error_message=error_message, | |
| status_code=response.status_code, | |
| raw_response=response_data, | |
| ) | |
| ) | |
| raise MpesaApiException( | |
| MpesaError( | |
| error_code=f"HTTP_{response.status_code}", | |
| error_message=error_message, | |
| status_code=response.status_code, | |
| raw_response=response.text, | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 37 - 44, The
MpesaError.raw_response is receiving mixed types (dict from handle_request_error
vs str/None from post()/get()), so normalize it to a single type (choose string)
across code paths: in handle_request_error convert response_data (dict) to a
JSON string (or str()) before passing as raw_response to
MpesaError/MpesaApiException, and in post() and get() ensure the except blocks
pass raw_response as response.text or str(response_text) (or None) consistently;
make the same change in the synchronous client
(mpesakit/http_client/mpesa_http_client.py) and update any tests or callers
expecting a dict accordingly, referencing the functions/methods
handle_request_error, post, get, MpesaError and MpesaApiException.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
124-132:reraise=Trueis redundant and misleading alongsideretry_error_callback.When
retry_error_callbackis provided, it is invoked instead of raisingRetryError. Sincehandle_retry_exceptionalways raisesMpesaApiException, thereraise=Trueparameter is never reached and creates a false impression that the originalhttpxexception can surface. Remove it to keep the intent clear.🛠 Proposed fix
return AsyncRetrying( retry=retry_enabled(enabled=self._retry_enabled_flag), wait=self._wait_strategy, stop=self._stop_strategy, retry_error_callback=handle_retry_exception, before_sleep=before_sleep_log(logger, logging.WARNING), - reraise=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 124 - 132, Remove the misleading reraise=True from the AsyncRetrying construction in _build_retrying: the method AsyncRetrying(..., retry_error_callback=handle_retry_exception, ...) already routes errors to handle_retry_exception (which raises MpesaApiException), so delete the reraise argument to avoid implying the original httpx exception will be re-raised; keep AsyncRetrying, retry_enabled(...), _wait_strategy, _stop_strategy, retry_error_callback=handle_retry_exception, before_sleep=before_sleep_log(logger, logging.WARNING) and reraise omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 183-200: The async_raw_get function can exit the async for loop
without returning (like async_raw_post), causing mypy to complain; after the
async for attempt loop in async_raw_get (which uses _build_retrying and awaits
_client.get), add an explicit raise (e.g., raise RuntimeError or a more specific
httpx exception) indicating the GET failed after retries so the function always
either returns a Response or raises an error. Ensure the raised error mirrors
the behavior/failure type used in async_raw_post for consistency.
- Around line 136-147: The async_raw_post function's async for loop using
self._build_retrying() can conceptually fall through without returning, causing
Mypy to flag a missing return; after the loop add an explicit unreachable
sentinel (e.g., raise RuntimeError("unreachable: retry exhausted") or
AssertionError) to satisfy the type checker and document the invariant,
referencing async_raw_post and the retry iterator returned by _build_retrying to
indicate why the raise is safe.
---
Duplicate comments:
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 165-184: In test_stk_push_multiple_times fix the typos and
normalize indentation: correct the docstring text "mutliple" to "multiple",
rename the variable succcess_count to success_count everywhere (initialization
and increments and assertions), and reformat the with-block so the body uses
consistent 4-space indentation and the for-loop and its body are properly
indented; the test function name and httpx.Client.post patch
(patch.object(httpx.Client, "post", ...)) should remain unchanged.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 124-132: Remove the misleading reraise=True from the AsyncRetrying
construction in _build_retrying: the method AsyncRetrying(...,
retry_error_callback=handle_retry_exception, ...) already routes errors to
handle_retry_exception (which raises MpesaApiException), so delete the reraise
argument to avoid implying the original httpx exception will be re-raised; keep
AsyncRetrying, retry_enabled(...), _wait_strategy, _stop_strategy,
retry_error_callback=handle_retry_exception,
before_sleep=before_sleep_log(logger, logging.WARNING) and reraise omitted.
Comment on lines
136
to
147
| async def async_raw_post( | ||
| self, url: str, json: Dict[str, Any], headers: Dict[str, str], timeout: int = 10 | ||
| ) -> httpx.Response: | ||
| """Low-level POST request - may raise httpx exceptions.""" | ||
| full_url = urljoin(self.base_url, url) | ||
|
|
||
| async for attempt in self._build_retrying(): | ||
| with attempt: | ||
| return await self._client.post( | ||
| full_url, json=json, headers=headers, timeout=timeout | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Pipeline failure: Mypy [return] — missing return statement in async_raw_post.
After the async for loop there is no explicit return or raise, so Mypy (correctly) flags this as a missing return path. At runtime tenacity's retry_error_callback always raises before the loop can exhaust silently, but Mypy can't infer that. Add an unreachable sentinel raise after the loop to satisfy the type checker and document the invariant.
🛠 Proposed fix
async def async_raw_post(
self, url: str, json: Dict[str, Any], headers: Dict[str, str], timeout: int = 10
) -> httpx.Response:
"""Low-level POST request - may raise httpx exceptions."""
full_url = urljoin(self.base_url, url)
async for attempt in self._build_retrying():
with attempt:
return await self._client.post(
full_url, json=json, headers=headers, timeout=timeout
)
+
+ raise RuntimeError("async_raw_post: unreachable – retry_error_callback should have raised")🧰 Tools
🪛 GitHub Actions: Code Quality
[error] 136-136: Mypy: Missing return statement. [return]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 136 - 147, The
async_raw_post function's async for loop using self._build_retrying() can
conceptually fall through without returning, causing Mypy to flag a missing
return; after the loop add an explicit unreachable sentinel (e.g., raise
RuntimeError("unreachable: retry exhausted") or AssertionError) to satisfy the
type checker and document the invariant, referencing async_raw_post and the
retry iterator returned by _build_retrying to indicate why the raise is safe.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
111-111:wait_strategyandstop_strategyparameters missing type hints.Both parameters accept any tenacity wait/stop strategy but lack annotations, reducing IDE support and type-checking coverage. The tenacity-exported types are
WaitBaseTandStopBaseT.♻️ Suggested improvement
+from tenacity import WaitBaseT, StopBaseT ... - def __init__(self, env: str = "sandbox", retry_enabled_flag: bool = True, wait_strategy=None, stop_strategy=None): + def __init__( + self, + env: str = "sandbox", + retry_enabled_flag: bool = True, + wait_strategy: Optional[WaitBaseT] = None, + stop_strategy: Optional[StopBaseT] = None, + ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` at line 111, The __init__ signature in mpesa_async_http_client.py is missing type hints for wait_strategy and stop_strategy; import tenacity's WaitBaseT and StopBaseT and annotate the parameters as wait_strategy: Optional[WaitBaseT] = None and stop_strategy: Optional[StopBaseT] = None (add Optional import from typing), leaving defaults as None, and update the function signature in the MpesaAsyncHttpClient.__init__ to use these types so IDEs and type-checkers recognize tenacity strategies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 179-187: The generic except block that maps all
httpx.RequestError/ValueError to REQUEST_FAILED must be changed to preserve
specific error codes when timeouts or connection errors occur: in the except
(httpx.RequestError, ValueError) as e handler (used by the request/send path and
in get()), inspect the exception type (isinstance(e, httpx.TimeoutException) ->
use error_code "REQUEST_TIMEOUT"; isinstance(e, httpx.ConnectError) ->
"CONNECTION_ERROR"; else -> "REQUEST_FAILED"), construct the MpesaError with the
appropriate error_message and include status_code/text from getattr(response,
...) if present, then raise MpesaApiException from e; ensure this change is
applied to both get() and the other request-handling block so retry_disabled
paths keep specific codes and preserve exception chaining and existing use of
MpesaError/MpesaApiException.
- Around line 125-133: The _build_retrying method currently passes reraise=True
to AsyncRetrying which is dead code when retry_error_callback is provided;
remove the reraise=True argument from the AsyncRetrying call in
mpesa_async_http_client.py so behavior matches the sync version
(mpesa_http_client.py) and relies on handle_retry_exception to map/raise
MpesaApiException; keep all other parameters (retry, wait, stop,
retry_error_callback=handle_retry_exception, before_sleep) unchanged.
---
Duplicate comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 202-211: In async_raw_get, initialize a local variable (e.g.,
response = None) before the retry loop and, after the await
self._client.get(...) inside the with attempt: block, assign to that variable
and explicitly break out of the loop on success (matching the async_raw_post
pattern) so the later if response is None: guard works and the behavior is
consistent and obvious; locate async_raw_get and mirror the response
initialization/assignment and break logic used in async_raw_post.
- Around line 143-153: The variable response is not initialized before the async
for loop in the method that calls self._build_retrying(), so if the loop body
never runs an UnboundLocalError can occur; initialize response = None just
before the loop (the block that uses async for attempt in
self._build_retrying()) so the subsequent if response is None check is valid, or
alternatively remove the redundant guard if you rely on handle_retry_exception
to always raise when retries are exhausted; reference _build_retrying(), the
async for loop, and the await self._client.post(...) call when making the
change.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Line 111: The __init__ signature in mpesa_async_http_client.py is missing type
hints for wait_strategy and stop_strategy; import tenacity's WaitBaseT and
StopBaseT and annotate the parameters as wait_strategy: Optional[WaitBaseT] =
None and stop_strategy: Optional[StopBaseT] = None (add Optional import from
typing), leaving defaults as None, and update the function signature in the
MpesaAsyncHttpClient.__init__ to use these types so IDEs and type-checkers
recognize tenacity strategies.
Comment on lines
+125
to
+133
| def _build_retrying(self): | ||
| return AsyncRetrying( | ||
| retry=retry_enabled(enabled=self._retry_enabled_flag), | ||
| wait=self._wait_strategy, | ||
| stop=self._stop_strategy, | ||
| retry_error_callback=handle_retry_exception, | ||
| before_sleep=before_sleep_log(logger, logging.WARNING), | ||
| reraise=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find project dependency files to identify tenacity version
find . -type f \( -name "requirements.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" \) | head -20Repository: Byte-Barn/mpesakit
Length of output: 78
🏁 Script executed:
# Read the specific file being reviewed
head -150 mpesakit/http_client/mpesa_async_http_client.py | tail -50Repository: Byte-Barn/mpesakit
Length of output: 1975
🏁 Script executed:
# Get the full context of the _build_retrying method and surrounding code
sed -n '110,145p' mpesakit/http_client/mpesa_async_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 1545
🌐 Web query:
tenacity library reraise=True retry_error_callback precedence behavior
💡 Result:
In tenacity, when retries are exhausted (i.e., the stop condition is met), retry_error_callback takes precedence over reraise=True:
- If
retry_error_callbackis set, Tenacity schedules only that callback and returns its value as the function result; it does not raiseRetryErroror re-raise the last exception (unless your callback raises). This happens because the stop-handling path checksretry_error_callbackfirst andreturns before reaching thereraise/RetryErrorlogic. [1] reraise=Trueonly applies in the no-callback case: Tenacity constructsretry_error_cls(last_attempt)(defaultRetryError), and ifreraiseis true it callsretry_exc.reraise()to re-raise the last attempt’s exception; otherwise it raisesRetryError. [1][2]
So the effective precedence is:
retry_error_callback(if provided) ⇒ return callback value (or raise whatever the callback raises)- else if
reraise=True⇒ raise last attempt exception - else ⇒ raise
RetryError[1]
Sources: [1] [2]
🏁 Script executed:
# Find where handle_retry_exception is defined
rg "def handle_retry_exception" --type py -A 10Repository: Byte-Barn/mpesakit
Length of output: 1988
🏁 Script executed:
# Check imports in the file to understand where handle_retry_exception comes from
head -30 mpesakit/http_client/mpesa_async_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 851
🏁 Script executed:
# Search for handle_retry_exception usage and definition across the codebase
rg "handle_retry_exception" --type py -B 2 -A 5Repository: Byte-Barn/mpesakit
Length of output: 3175
Remove reraise=True—it is dead code that conflicts with the callback precedence.
Tenacity's retry_error_callback takes precedence over reraise=True. When a callback is provided, the reraise parameter is ignored and the callback's return value (or raised exception) is used instead. Your handle_retry_exception callback correctly maps httpx.TimeoutException and httpx.ConnectError to MpesaApiException, so the exception wrapping works as intended—but reraise=True never executes and merely creates confusion.
The synchronous counterpart in mpesa_http_client.py does not include this parameter. Remove reraise=True from _build_retrying() to match the sync version and clarify intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 125 - 133, The
_build_retrying method currently passes reraise=True to AsyncRetrying which is
dead code when retry_error_callback is provided; remove the reraise=True
argument from the AsyncRetrying call in mpesa_async_http_client.py so behavior
matches the sync version (mpesa_http_client.py) and relies on
handle_retry_exception to map/raise MpesaApiException; keep all other parameters
(retry, wait, stop, retry_error_callback=handle_retry_exception, before_sleep)
unchanged.
Comment on lines
+179
to
+187
| except (httpx.RequestError, ValueError) as e: | ||
| raise MpesaApiException( | ||
| MpesaError( | ||
| error_code="REQUEST_FAILED", | ||
| error_message=f"HTTP request failed: {str(e)}", | ||
| status_code=None, | ||
| raw_response=None, | ||
| error_message="HTTP request failed.", | ||
| status_code=getattr(response, "status_code", None), | ||
| raw_response=getattr(response, "text", None), | ||
| ) | ||
| ) | ||
| ) from e |
Contributor
There was a problem hiding this comment.
REQUEST_TIMEOUT/CONNECTION_ERROR codes are silently downgraded to REQUEST_FAILED when retries are disabled.
When retry_enabled_flag=False, retry_enabled() returns False for every failed attempt. Tenacity's return_last_value path re-raises the original httpx.TimeoutException/httpx.ConnectError directly — handle_retry_exception is only invoked when the stop condition triggers, not when retry returns False. The except (httpx.RequestError, ValueError) block then catches the raw exception and maps it uniformly to REQUEST_FAILED, losing the more specific codes.
The same issue affects get() (lines 237–245). Consider mapping the exception types explicitly in the except handler to preserve specificity:
🛠 Proposed fix
- except (httpx.RequestError, ValueError) as e:
+ except httpx.TimeoutException as e:
raise MpesaApiException(
MpesaError(
- error_code="REQUEST_FAILED",
- error_message="HTTP request failed.",
+ error_code="REQUEST_TIMEOUT",
+ error_message=str(e),
status_code=getattr(response, "status_code", None),
raw_response=getattr(response, "text", None),
)
) from e
+ except httpx.ConnectError as e:
+ raise MpesaApiException(
+ MpesaError(
+ error_code="CONNECTION_ERROR",
+ error_message="Failed to connect to M-Pesa API.",
+ status_code=getattr(response, "status_code", None),
+ raw_response=getattr(response, "text", None),
+ )
+ ) from e
+ except (httpx.RequestError, ValueError) as e:
+ raise MpesaApiException(
+ MpesaError(
+ error_code="REQUEST_FAILED",
+ error_message="HTTP request failed.",
+ status_code=getattr(response, "status_code", None),
+ raw_response=getattr(response, "text", None),
+ )
+ ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 179 - 187, The
generic except block that maps all httpx.RequestError/ValueError to
REQUEST_FAILED must be changed to preserve specific error codes when timeouts or
connection errors occur: in the except (httpx.RequestError, ValueError) as e
handler (used by the request/send path and in get()), inspect the exception type
(isinstance(e, httpx.TimeoutException) -> use error_code "REQUEST_TIMEOUT";
isinstance(e, httpx.ConnectError) -> "CONNECTION_ERROR"; else ->
"REQUEST_FAILED"), construct the MpesaError with the appropriate error_message
and include status_code/text from getattr(response, ...) if present, then raise
MpesaApiException from e; ensure this change is applied to both get() and the
other request-handling block so retry_disabled paths keep specific codes and
preserve exception chaining and existing use of MpesaError/MpesaApiException.
Member
|
Apologies @watersRand , seems coderabbit is too chatty with low/manageable reviews that could be easier done with humans. I'll have to disable it 🤝 I'll review your PR as soon as I can. Again apologies for any inconvenience. |
Contributor
Author
|
Not a problem @RafaelJohn9 ,coderabbit opened my eyes to inconsistencies in my codebase tools.In this pr,l had intended to match the async and sync clients, add and update the stk_push 100 times with both session and no session .Its a good tool ,good challenge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR aligns retry behavior between
MpesaHttpClient(sync) andMpesaAsyncHttpClient(async) to ensure consistent exception handling and retry logic.Closes #116Changes include:
awaiton raw calls).Type of Change
How Has This Been Tested?
Existing retry tests (sync & async).
New tests simulating 100 successful STK push calls:
use_session=Trueuse_session=FalseVerified retry behavior with simulated
TimeoutExceptionandConnectError.All unit tests pass locally.
Checklist
Summary by CodeRabbit
New Features
Tests
Documentation