feat: Change http client to Niquests #600
Conversation
|
""" WalkthroughThis update migrates the project's HTTP client and testing infrastructure from Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Responses as responses (mock)
participant Niquests as niquests.AsyncSession
participant Server as Mocked Server
Test->>Responses: Activate/patch HTTP mocking
Test->>Niquests: Make HTTP request (async)
Niquests->>Responses: Intercept request
Responses->>Server: Simulate response or error
Server-->>Responses: Return mocked response
Responses-->>Niquests: Provide response object
Niquests-->>Test: Return response
Test->>Responses: Deactivate/unpatch mocking
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ 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: 8
🔭 Outside diff range comments (2)
test/ably/rest/restrequest_test.py (1)
142-212:⚠️ Potential issueSeveral fallback tests have been removed
The original file contained tests for various fallback scenarios (
test_503_status_fallback,test_httpx_timeout_fallback,test_503_status_fallback_on_publish, andtest_400_cloudfront_fallback) that appear to have been removed. This may reduce test coverage for fallback handling.Ensure that these fallback scenarios are still being tested, either by adding equivalent tests using the
responseslibrary or by adding comprehensive tests elsewhere. These tests covered important connection resilience features:
- Fallback on 503 status
- Fallback on timeout
- Fallback during publish operations
- CloudFront-specific fallback behavior
Consider restoring these tests with the updated mocking approach to maintain test coverage.
🧰 Tools
🪛 Ruff (0.11.9)
147-147: Undefined name
respx(F821)
148-148: Undefined name
respx(F821)
149-149: Undefined name
respx(F821)
153-153: Undefined name
httpx(F821)
154-154: Undefined name
httpx(F821)
168-168: Undefined name
respx(F821)
169-169: Undefined name
respx(F821)
170-170: Undefined name
respx(F821)
174-174: Undefined name
httpx(F821)
175-175: Undefined name
httpx(F821)
196-196: Undefined name
respx(F821)
197-197: Undefined name
respx(F821)
198-198: Undefined name
respx(F821)
202-202: Undefined name
httpx(F821)
203-203: Undefined name
httpx(F821)
ably/http/http.py (1)
88-106: 🛠️ Refactor suggestion
Response.to_native()does not cover plain‐text / empty content.
- If the server sends
text/plainwe currently raise an exception, even though some endpoints
(e.g./time) legitimately return plain text.- The
json()call can raiseValueError; we should wrap it intry/exceptso that a malformed
payload surfaces asAblyExceptionrather than an uncaught stdlib error.Consider:
elif content_type.startswith('application/json'): - return self.__response.json() + try: + return self.__response.json() + except ValueError as exc: + raise AblyException("Malformed JSON", 500, 50000) from exc + elif content_type.startswith('text/plain'): + return content.decode()
🧹 Nitpick comments (7)
pyproject.toml (1)
54-65: Minor TOML style / consistency nits.
[poetry.group.dev.dependencies]requires Poetry ≥ 1.2. Make sure our CI image has at least that; otherwise dependency resolution will silently fail.- The entry
flake8="^3.9.2"is missing a space around the equals sign, unlike the lines above and below. This is harmless to the parser but sticks out during reviews.-flake8="^3.9.2" +flake8 = "^3.9.2"test/ably/utils.py (2)
15-15: Name shadowing may confuse static analysers.The module import
import responsesis later shadowed insideassert_responses_typeby a local list
namedresponses(line 74). Although legal, this hinders readability and can trip tools such as
flake8 –-select=F401. Consider renaming the local list tologged_responses(or similar).
22-29: Duplicate helper code – DRY opportunity.
responses_add_empty_msg_pack()is implemented twice (sync & async). The bodies are identical, only
the class they live in differs. You can factor the logic into a small free function and call it from
both classes:+def _add_empty_msgpack(url, method="GET"): + responses.add( + method=method, + url=url, + body=msgpack.packb({}), + status=200, + content_type="application/x-msgpack", + ) + class BaseTestCase(unittest.TestCase): - def responses_add_empty_msg_pack(self, url, method='GET'): - responses.add( - method=method, - url=url, - body=msgpack.packb({}), - status=200, - content_type="application/x-msgpack" - ) + def responses_add_empty_msg_pack(self, url, method="GET"): + _add_empty_msgpack(url, method) ... class BaseAsyncTestCase(IsolatedAsyncioTestCase): - def responses_add_empty_msg_pack(self, url, method='GET'): - responses.add( - method=method, - url=url, - body=msgpack.packb({}), - status=200, - content_type="application/x-msgpack" - ) + def responses_add_empty_msg_pack(self, url, method="GET"): + _add_empty_msgpack(url, method)This keeps the file shorter and avoids future drift.
Also applies to: 43-50
ably/http/http.py (1)
200-205: String replacement to drop default ports is brittle.Manipulating the URL via
str.replace()can remove false positives (e.g.:443inside the path or
query). Safer to useurllib.parse:-from urllib.parse import urljoin +from urllib.parse import urljoin, urlparse, urlunparse ... base_url = "%s://%s:%d" % (self.preferred_scheme, host, self.preferred_port) - # remove redundant port in base_url - if self.preferred_scheme == "https" and self.preferred_port == 443: - base_url = base_url.replace(":443", "") - elif self.preferred_scheme == "http" and self.preferred_port == 80: - base_url = base_url.replace(":80", "") + parsed = urlparse(base_url) + if (parsed.scheme == "https" and parsed.port == 443) or ( + parsed.scheme == "http" and parsed.port == 80 + ): + parsed = parsed._replace(netloc=parsed.hostname) + base_url = urlunparse(parsed)test/ably/rest/restauth_test.py (2)
88-96: Suppress expected network errors more idiomaticallyThe
try/except Exception: passblocks swallow all exceptions and obscure unexpected failures.
Usecontextlib.suppress(Exception)(or even a narrower exception class) so that only the expected errors are ignored and the intent is clearer:from contextlib import suppress ... with mock.patch.object(AsyncSession, "send") as send_mock: with suppress(Exception): await ably.http.get("/time", skip_auth=False)Also applies to: 101-106, 113-118
🧰 Tools
🪛 Ruff (0.11.9)
89-92: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
389-390: Yoda-style condition hurts readabilityThe assertions
1 == count_calland similar Yoda comparisons read unnaturally.
Swap operands for the commonvariable == valueorder:-assert 1 == count_call +assert count_call == 1Same for the two
token_details.tokencomparisons below.
This keeps the tests idiomatic and avoids RuffSIM300warnings.🧰 Tools
🪛 Ruff (0.11.9)
389-389: Yoda condition detected
Rewrite as
count_call == 1(SIM300)
390-390: Yoda condition detected
Rewrite as
token_details.token == 'token_string'(SIM300)
test/conftest.py (1)
110-111: Direct assignment is clearer thansetattrfor constants
responses.mockandresponses._default_mockare constant attributes; plain assignment is simpler and silences RuffB010:-responses.mock = mock -responses._default_mock = _default_mock🧰 Tools
🪛 Ruff (0.11.9)
110-110: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
111-111: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)ably/http/http.py(4 hunks)ably/realtime/connectionmanager.py(2 hunks)ably/rest/auth.py(2 hunks)ably/scripts/unasync.py(2 hunks)ably/util/crypto.py(1 hunks)pyproject.toml(2 hunks)test/ably/realtime/realtimeauth_test.py(2 hunks)test/ably/rest/restauth_test.py(11 hunks)test/ably/rest/restchannelhistory_test.py(2 hunks)test/ably/rest/restchannelpublish_test.py(5 hunks)test/ably/rest/restchannelstatus_test.py(1 hunks)test/ably/rest/restcrypto_test.py(3 hunks)test/ably/rest/resthttp_test.py(9 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(4 hunks)test/ably/rest/restpresence_test.py(3 hunks)test/ably/rest/restrequest_test.py(3 hunks)test/ably/rest/resttoken_test.py(4 hunks)test/ably/utils.py(2 hunks)test/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
test/ably/rest/restchannelstatus_test.py (1)
ably/types/channeldetails.py (8)
occupancy(39-40)metrics(58-59)publishers(98-99)connections(82-83)subscribers(102-103)presence_members(90-91)presence_connections(86-87)presence_subscribers(94-95)
test/ably/rest/restinit_test.py (4)
ably/transport/defaults.py (2)
Defaults(1-63)get_port(36-46)test/ably/utils.py (1)
dont_vary_protocol(159-161)ably/rest/rest.py (4)
AblyRest(19-148)time(91-95)http(111-112)request(122-142)test/conftest.py (1)
send(35-62)
test/ably/rest/resttoken_test.py (1)
ably/rest/rest.py (1)
time(91-95)
test/ably/rest/restrequest_test.py (4)
ably/rest/rest.py (2)
request(122-142)close(147-148)ably/http/http.py (5)
version(83-84)close(134-135)headers(71-72)get(252-255)body(75-76)ably/transport/defaults.py (1)
Defaults(1-63)test/ably/testapp.py (2)
TestApp(37-115)get_ably_rest(73-77)
test/ably/rest/restchannelpublish_test.py (3)
ably/http/http.py (1)
close(134-135)test/ably/rest/resthttp_test.py (1)
side_effect(123-127)ably/rest/channel.py (2)
publish(106-132)history(32-39)
test/conftest.py (3)
ably/rest/rest.py (1)
request(122-142)ably/http/http.py (4)
Response(87-114)patch(257-260)method(63-64)url(67-68)test/ably/utils.py (1)
patch(76-86)
🪛 actionlint (1.7.7)
.github/workflows/lint.yml
16-16: property "python-version" is not defined in object type {}
(expression)
🪛 Ruff (0.11.9)
test/ably/rest/restinit_test.py
128-128: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
134-134: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9998
(SIM300)
140-140: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
209-212: Use contextlib.suppress(AblyException) instead of try-except-pass
Replace with contextlib.suppress(AblyException)
(SIM105)
test/ably/rest/restcrypto_test.py
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restauth_test.py
102-105: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
389-389: Yoda condition detected
Rewrite as count_call == 1
(SIM300)
390-390: Yoda condition detected
Rewrite as token_details.token == 'token_string'
(SIM300)
437-437: Yoda condition detected
Rewrite as token_details.token == 'another_token_string'
(SIM300)
test/conftest.py
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
111-111: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (53)
ably/util/crypto.py (1)
156-156: Good improvement to type checking.Changing from
type(key) == strtoisinstance(key, str)is a more idiomatic Python approach that properly handles inheritance and follows PEP 8 guidelines. This is a small but positive improvement to code quality..github/workflows/lint.yml (3)
13-13: Upgrade checkout action to v4
Bumpedactions/checkouttov4to take advantage of the latest performance and security improvements in the GitHub Actions runner.
17-17: Upgrade setup-python action to v5
Updatedactions/setup-pythontov5to align with the latest action enhancements and ensure compatibility with modern Python versions.
21-23: Upgrade Poetry setup action and Poetry version
Switched toabatilo/actions-poetry@v4and bumped the Poetry CLI to2.1.2for consistent dependency management and support for new features..github/workflows/check.yml (3)
23-23: Upgrade checkout action to v4
Replacedactions/checkout@v2with@v4for consistency across workflows and to leverage improved checkout performance.
27-27: Upgrade setup-python action to v5
Alignedactions/setup-pythonwith versionv5to match the lint workflow and ensure up-to-date Python environment setup.
31-33: Upgrade Poetry setup action and Poetry version
Moved toabatilo/actions-poetry@v4and updated Poetry to2.1.2, ensuring uniform dependency installation and CI reproducibility.test/ably/rest/restchannelstatus_test.py (1)
31-31: Style improvement: Added trailing spaces after line continuations.These changes are purely stylistic, adding a trailing space after commas at line continuations in the assertion statements. This improves code readability and keeps the style consistent across the codebase.
Also applies to: 34-34, 37-37, 40-40, 43-43, 46-46
ably/realtime/connectionmanager.py (2)
4-4: Library migration: Replaced httpx with niquests.This change replaces the import of
httpxwithniquestsas part of the larger migration described in the PR objectives.
55-55: Updated HTTP client usage to niquests.The HTTP client for checking connectivity has been updated from
httpx.gettoniquests.getwith corresponding exception handling updated to catchniquests.HTTPError. The functionality remains the same, and the change is consistent with the overall migration strategy.Also applies to: 58-58
ably/rest/auth.py (2)
8-8: Library migration: Replaced httpx with niquests.This change replaces the import of
httpxwithniquestsas part of the larger migration.
403-403:❓ Verification inconclusive
Updated HTTP client usage to niquests.
Replaced
httpx.AsyncClient(http2=True)withniquests.AsyncSession(). According to the PR objectives, niquests supports HTTP/2 and HTTP/3 natively, so the explicit HTTP/2 configuration is no longer needed.
🏁 Script executed:
#!/bin/bash # Let's verify that niquests supports HTTP/2 by default python -c "import niquests; print('HTTP/2 support in niquests:', 'AsyncSession' in dir(niquests))"Length of output: 234
Please verify niquests installation and HTTP/2/3 support
The change replaces
async with httpx.AsyncClient(http2=True) as client:with
async with niquests.AsyncSession() as client:but running a quick check in the sandbox raised
ModuleNotFoundError: No module named 'niquests'. Before approving:
- Ensure
niquestsis listed in your project’s dependencies and installed in the environment.- Confirm that
niquests.AsyncSessionnatively supports HTTP/2 and HTTP/3 without additional configuration.test/ably/rest/resttoken_test.py (1)
126-126: Style improvement: Added spaces after backslashes in line continuations.These changes improve readability by adding a space after the trailing backslash in the
withstatements that usepatch(). The functionality of the tests remains unchanged.Also applies to: 135-135, 188-188, 199-199
test/ably/realtime/realtimeauth_test.py (1)
328-332: HTTP client migration looks good.Successfully migrated from httpx to niquests for the JWT authentication callback. Note the parameter type change from integer
35to string"35"forexpiresIn, which aligns with niquests' parameter handling requirements.test/ably/rest/restinit_test.py (5)
3-3: Library migration: httpx → niquests.Correctly updated import to use AsyncSession from niquests package.
128-129: Fixed line continuation spacing.Fixed trailing backslash formatting in assertion statements to maintain consistent style.
Also applies to: 134-135, 140-141
🧰 Tools
🪛 Ruff (0.11.9)
128-128: Yoda condition detected
Rewrite as
Defaults.get_port(ably.options) == 9999(SIM300)
171-172: Fixed line continuation in patch statement.Corrected formatting for multi-line patch decorators.
208-208: Updated patching target for HTTP client.Correctly updated the patch target from AsyncClient.send to AsyncSession.send to reflect the new niquests library structure.
214-214: Updated URL assertion to match niquests behavior.Removed explicit port
:443from expected URL string, aligning with how niquests constructs URLs for standard ports.ably/scripts/unasync.py (2)
240-241: Updated token replacements for niquests integration.Correctly updated the token replacement dictionary to handle niquests class names and methods, replacing AsyncSession with Session for synchronous code generation.
274-274: Updated string replacements for mocking.Updated string replacement to handle the new niquests AsyncSession.send mocking pattern in generated synchronous code.
test/ably/rest/restpresence_test.py (5)
4-4: Replaced respx with responses for HTTP mocking.Migrated from respx to responses mocking library to support niquests HTTP client testing.
81-84: Improved URL port handling logic.Updated port condition to treat both 80 and 443 as default ports that don't require explicit port suffixes in URLs, matching standard HTTP/HTTPS conventions.
Also applies to: 94-97
102-102: Updated test decorators for new mocking library.Replaced @respx.mock with @responses.activate across all test methods to support the new mocking infrastructure.
Also applies to: 110-110, 118-118, 126-126, 134-134, 142-142, 150-150, 158-158, 167-167, 180-180
105-105: Updated mock response helper method calls.Changed from respx_add_empty_msg_pack to responses_add_empty_msg_pack to use the new mocking library's helpers.
Also applies to: 113-113, 121-121, 129-129, 137-137, 145-145, 153-153, 161-161, 174-174, 185-185
107-107: Updated URL parameter assertions.Modified assertions to check URL parameters as substrings in the request URL rather than accessing structured params, adapting to the responses library's API.
Also applies to: 115-115, 131-131, 139-139, 147-147, 163-164, 176-177
test/ably/rest/restpaginatedresult_test.py (8)
1-2: LGTM - New imports for URL parsing are appropriateThe addition of
parse_qsandurlparsefromurllib.parseis necessary to handle URL query string parsing manually since we're moving fromrespxtoresponses.
3-3: LGTM - Correct import for the new mocking libraryImporting
responsesis consistent with the PR objective of replacingrespx.
15-17: LGTM - Query parameter extraction is correctly updatedThe manual parsing of query parameters using
parse_qsandurlparseis correctly implemented to extract the 'page' parameter from the URL query string.
19-29: LGTM - Response format correctly updated for responses libraryThe response format is correctly changed from the previous
httpx.Responseobject to a tuple with(status, headers, body)which is what theresponseslibrary expects.
37-42: LGTM - Mock route setup properly updatedThe route mocking is correctly changed from the
respxstyle to theresponsesstyle usingresponses.get().
45-59: LGTM - Callback-based route mocking correctly implementedThe callback-based route setup is correctly transitioned to use
responses.add_callback()with the appropriate parameters.
62-62: LGTM - Mock framework initialization updatedThe mock framework initialization is correctly changed to
responses.start().
74-74: LGTM - Mock framework cleanup updatedThe mock framework cleanup is correctly changed to
responses.stop().test/ably/rest/restchannelpublish_test.py (4)
8-8: LGTM - Import updated to new HTTP client libraryImporting
niquestsinstead ofhttpxis consistent with the PR objective.
405-405: LGTM - HTTP client class correctly updatedThe async client class is correctly updated from
httpx.AsyncClienttoniquests.AsyncSession.
548-548: LGTM - Mock patch target correctly updatedThe mock patch target is correctly updated from
httpx.AsyncClient.sendtoniquests.AsyncSession.send.
554-554: LGTM - Client close method name updatedThe client close method is correctly updated from
aclose()toclose()to match the Niquests API.test/ably/rest/restrequest_test.py (4)
1-3: LGTM - Imports correctly updatedThe imports are correctly updated from
httpxandrespxtoniquestsandresponsesrespectively.
100-102: LGTM - Exception class correctly updatedThe exception class is correctly updated from
httpx.ReadTimeouttoniquests.ReadTimeout.
105-137: LGTM - Test timeout implementation updated for responses libraryThe test_timeout method has been properly rewritten to use
responsesinstead ofrespx. The implementation correctly:
- Uses
responses.start()to initialize the mock- Sets up a callback that raises a ConnectionError
- Mocks the fallback endpoint response
- Cleans up with
responses.stop()This correctly tests the fallback behavior with the new library.
134-135: LGTM - Exception class correctly updatedThe exception class reference is correctly updated from
httpx.ConnectionErrortoniquests.ConnectionError.test/ably/rest/restchannelhistory_test.py (8)
3-3: LGTM - Import correctly updatedThe import is correctly updated from
respxtoresponses.
95-98: LGTM - Port suffix logic improvedThe port suffix logic is improved to explicitly check for standard HTTP/HTTPS ports (80 and 443) that don't require explicit port specification in URLs.
102-102: LGTM - Test decorator correctly updatedThe test decorator is correctly updated from
@respx.mockto@responses.activate.
108-108: LGTM - Mock helper method correctly updatedThe helper method call is correctly updated from
self.respx_add_empty_msg_packtoself.responses_add_empty_msg_pack.
110-110: LGTM - Assertion correctly updatedThe assertion is correctly updated to check
responses.calls[0].request.urlinstead of the previousrespx.callsstructure.
112-112: LGTM - Test decorator correctly updatedThe test decorator is correctly updated from
@respx.mockto@responses.activate.
118-118: LGTM - Mock helper method correctly updatedThe helper method call is correctly updated from
self.respx_add_empty_msg_packtoself.responses_add_empty_msg_pack.
121-124: LGTM - Assertions correctly updatedThe assertions are correctly updated to check for substrings in
responses.calls[n].request.urlinstead of the previousrespx.callsstructure.pyproject.toml (2)
68-68: CI update needed forpoetry-core>=2.The build-system bump means any downstream environment still pinned to Poetry 1.x will fail with
“poetry-core >= 2 not found”. Double-checksetup-pythonand Docker images in the workflow files.
44-44:✅ Verification successful
Verify
niquestscompatibility with our Python baseline (3.7).
niquestsis declared with the caret range^3.14.1, but according to PyPI it currently advertises Python ≥ 3.8. If that is correct, users still running 3.7 will install the package only to hit runtime syntax errors.Run the following script to confirm the classifiers reported on PyPI:
If 3.7 is missing we should either (a) bump the lowest supported Python in
pyproject.tomlor (b) pin to the last version that still supports 3.7.
🏁 Script executed:
#!/usr/bin/env bash # Show niquests' supported Python versions curl -s https://pypi.org/pypi/niquests/json | jq '.info.classifiers[]' | grep -i "Programming Language :: Python ::"Length of output: 607
niquestsSupports Python 3.7—No Action NeededThe PyPI classifiers for
niquests3.14.1 include “Programming Language :: Python :: 3.7,” confirming our 3.7 baseline remains compatible. You can safely keep the dependency as:niquests = "^3.14.1"ably/http/http.py (1)
208-216:❓ Verification inconclusive
Timeout tuple may not map 1-to-1 to Niquests.
timeout=(connect, read)is accepted by httpx but Niquests’ signature follows Requests,
wheretimeoutis a single float or a(connect, read)tuple in seconds, not a
(connect_timeout, request_timeout)pair. Verify that the semantics remain identical;
otherwise some retries may trigger earlier or later than before.If the printed signature shows
timeout: float | tuple[float, float] | None, we’re fine; if not,
translate the tuple into the expectedtimeoutobject/kwargs.
🏁 Script executed:
#!/usr/bin/env bash python - <<'PY' import inspect, textwrap, niquests sig = inspect.signature(niquests.Session.request) print(textwrap.dedent(str(sig))) PYLength of output: 131
🏁 Script executed:
#!/usr/bin/env bash set -e # Locate the niquests package directory in the repo niq_dir=$(fd -t d niquests | head -n1) if [ -z "$niq_dir" ]; then niq_dir=$(find . -type d -name "niquests" | head -n1) fi echo "Niquests package directory: $niq_dir" # Display the lines around the Session class definition rg -n "class Session" -A 5 "$niq_dir" # Display the signature of the request method rg -n "def request" -A 5 "$niq_dir"Length of output: 263
🏁 Script executed:
#!/usr/bin/env bash set -e # Install niquests and inspect the signature of Session.request python - <<'PY' import subprocess, sys subprocess.check_call([sys.executable, "-m", "pip", "install", "--quiet", "niquests"]) import inspect, textwrap from niquests import Session sig = inspect.signature(Session.request) print(textwrap.dedent(str(sig))) PYLength of output: 140
Ensure Niquests timeout tuple semantics align with httpx
Thetimeout=(connect, read)tuple passed to httpx must match how Niquests’Session.requestinterprets timeouts. In Requests and httpx, the two-tuple represents connect‐ and read‐timeout in seconds. If Niquests expects a different ordering or semantics (for example, treating the second value as an overall request timeout), retries could fire too early or too late.• File: ably/http/http.py (lines 208–216)
• Verify in your Niquests version thatSession.request(timeout=…)acceptstuple[float, float]in the same order and units.
• Adjust the code to explicitly construct Niquests’ timeout parameters if needed (e.g., via keyword args or a helper).
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
test/ably/rest/restrequest_test.py (3)
142-160:⚠️ Potential issueTest still using respx mocking
This test and several others below haven't been migrated to use responses yet.
The test is still using respx.mock and httpx.Response which will fail after the migration is complete. These remaining tests need to be updated to use responses mocking.
Suggested implementation pattern:
- with respx.mock: - default_route = respx.get(default_endpoint) - fallback_route = respx.get(fallback_endpoint) - headers = { - "Content-Type": "application/json" - } - default_route.return_value = httpx.Response(503, headers=headers) - fallback_route.return_value = httpx.Response(200, headers=headers, text='[123]') + responses.start() + try: + headers = { + "Content-Type": "application/json" + } + responses.get(default_endpoint, status=503, headers=headers) + responses.get(fallback_endpoint, status=200, headers=headers, body='[123]') + result = await ably.request('GET', '/time', version=Defaults.protocol_version) - assert default_route.called + # No direct way to check if route was called, but result indicates it worked assert result.status_code == 200 assert result.items[0] == 123 + finally: + responses.stop()🧰 Tools
🪛 Ruff (0.11.9)
147-147: Undefined name
respx(F821)
148-148: Undefined name
respx(F821)
149-149: Undefined name
respx(F821)
153-153: Undefined name
httpx(F821)
154-154: Undefined name
httpx(F821)
162-181:⚠️ Potential issueTest still using respx/httpx
This test for timeout fallback also needs to be migrated to responses/niquests.
Similar to the previous test, update to use responses mocking instead of respx:
- with respx.mock: - default_route = respx.get(default_endpoint) - fallback_route = respx.get(fallback_endpoint) - headers = { - "Content-Type": "application/json" - } - default_route.side_effect = httpx.ReadTimeout - fallback_route.return_value = httpx.Response(200, headers=headers, text='[123]') + responses.start() + try: + headers = { + "Content-Type": "application/json" + } + def timeout_handler(*args, **kwargs): + raise niquests.ReadTimeout() + + responses.add_callback("GET", default_endpoint, timeout_handler) + responses.get(fallback_endpoint, status=200, headers=headers, body='[123]') + result = await ably.request('GET', '/time', version=Defaults.protocol_version) - assert default_route.called + # Result validates the fallback was used assert result.status_code == 200 assert result.items[0] == 123 + finally: + responses.stop()🧰 Tools
🪛 Ruff (0.11.9)
168-168: Undefined name
respx(F821)
169-169: Undefined name
respx(F821)
170-170: Undefined name
respx(F821)
174-174: Undefined name
httpx(F821)
175-175: Undefined name
httpx(F821)
184-212:⚠️ Potential issueRemaining tests need migration
These remaining tests still use respx/httpx and need to be migrated to use responses/niquests.
Apply similar changes as suggested for the previous tests. Each test should:
- Start with
responses.start()- Use
responses.get()orresponses.post()orresponses.add_callback()for mocking- End with
responses.stop()in afinallyblock to ensure cleanupThis will ensure consistency across the test suite and align with the PR objective of replacing httpx with niquests.
Also applies to: 214-234
🧰 Tools
🪛 Ruff (0.11.9)
196-196: Undefined name
respx(F821)
197-197: Undefined name
respx(F821)
198-198: Undefined name
respx(F821)
202-202: Undefined name
httpx(F821)
203-203: Undefined name
httpx(F821)
♻️ Duplicate comments (5)
test/ably/rest/restcrypto_test.py (3)
82-83:⚠️ Potential issueFix multi-line assert formatting
The backslashes at the end of assert lines were removed, breaking the multi-line assert statements. This causes the error messages to be disconnected from the assertions.
Apply one of these approaches consistently:
-assert "This is a string message payload" == message_contents["publish3"], \ "Expect publish3 to be expected String)" +assert ("This is a string message payload" == message_contents["publish3"], "Expect publish3 to be expected String)")Or with parentheses for implicit line continuation (preferred):
-assert "This is a string message payload" == message_contents["publish3"], \ "Expect publish3 to be expected String)" +assert ( + "This is a string message payload" == message_contents["publish3"] +), "Expect publish3 to be expected String)"Also applies to: 85-86, 88-89, 91-92
🧰 Tools
🪛 Ruff (0.11.9)
82-82: Yoda condition detected
(SIM300)
115-116:⚠️ Potential issueFix multi-line assert formatting
Same issue with broken multi-line assert statements in test_crypto_publish_256 method.
Use parentheses for implicit line continuation:
-assert "This is a string message payload" == message_contents["publish3"], \ "Expect publish3 to be expected String)" +assert ( + "This is a string message payload" == message_contents["publish3"] +), "Expect publish3 to be expected String)"Also applies to: 118-119, 121-122, 124-125
🧰 Tools
🪛 Ruff (0.11.9)
115-115: Yoda condition detected
(SIM300)
164-165:⚠️ Potential issueFix multi-line assert formatting
Same issue with broken multi-line assert statements in test_crypto_send_unencrypted method.
Use parentheses for implicit line continuation:
-assert "This is a string message payload" == message_contents["publish3"], \ "Expect publish3 to be expected String" +assert ( + "This is a string message payload" == message_contents["publish3"] +), "Expect publish3 to be expected String"Also applies to: 167-168, 170-171, 173-174
🧰 Tools
🪛 Ruff (0.11.9)
164-164: Yoda condition detected
(SIM300)
test/conftest.py (1)
56-63:⚠️ Potential issuePotential KeyError when
streamkwarg is omittedDirectly accessing
kwargs["stream"]will raise a KeyError if the 'stream' argument is not provided.-if kwargs["stream"]: +if kwargs.get("stream", False): return resptest/ably/rest/restauth_test.py (1)
108-108:⚠️ Potential issueHeader comparison uses bytes vs. str – assertion will always fail
request.headersalways yields str values, whereasbase64.b64encodereturns bytes.
The current assertion therefore compares"client_id"(string) withb"...=="(bytes) and will fail even when the header is correct.-assert client_id == base64.b64encode('client_id'.encode('ascii')) +assert client_id == base64.b64encode('client_id'.encode('ascii')).decode("utf-8")
🧹 Nitpick comments (3)
test/ably/rest/restinit_test.py (1)
128-129: Fix multi-line assert formattingThe backslash line continuations need to be properly formatted for readability.
Consider using parentheses for implicit line continuation instead:
-assert 9999 == Defaults.get_port(ably.options), \ "Unexpected port mismatch. Expected: 9999. Actual: %d" % ably.options.tls_port +assert ( + 9999 == Defaults.get_port(ably.options) +), "Unexpected port mismatch. Expected: 9999. Actual: %d" % ably.options.tls_portAlso applies to: 134-135, 140-141
🧰 Tools
🪛 Ruff (0.11.9)
128-128: Yoda condition detected
Rewrite as
Defaults.get_port(ably.options) == 9999(SIM300)
test/ably/rest/restauth_test.py (2)
101-105: Simplify with contextlib.suppress instead of try-except-passThe current try-except block that suppresses exceptions can be simplified for better readability.
-try: - await ably.http.get('/time', skip_auth=False) -except Exception: - pass +with contextlib.suppress(Exception): + await ably.http.get('/time', skip_auth=False)Don't forget to add
import contextlibat the top of the file if you make this change.🧰 Tools
🪛 Ruff (0.11.9)
102-105: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
389-389: Consider avoiding Yoda conditions for better readability.The code uses "Yoda conditions" (constant first, variable second) which can reduce readability.
-assert 1 == count_call +assert count_call == 1 -assert 'token_string' == token_details.token +assert token_details.token == 'token_string' -assert 'another_token_string' == token_details.token +assert token_details.token == 'another_token_string'Also applies to: 390-390, 437-437
🧰 Tools
🪛 Ruff (0.11.9)
389-389: Yoda condition detected
Rewrite as
count_call == 1(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)ably/http/http.py(4 hunks)ably/realtime/connectionmanager.py(2 hunks)ably/rest/auth.py(2 hunks)ably/scripts/unasync.py(2 hunks)ably/util/crypto.py(1 hunks)pyproject.toml(2 hunks)test/ably/realtime/realtimeauth_test.py(2 hunks)test/ably/rest/restauth_test.py(11 hunks)test/ably/rest/restchannelhistory_test.py(2 hunks)test/ably/rest/restchannelpublish_test.py(5 hunks)test/ably/rest/restchannelstatus_test.py(1 hunks)test/ably/rest/restcrypto_test.py(3 hunks)test/ably/rest/resthttp_test.py(9 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(4 hunks)test/ably/rest/restpresence_test.py(3 hunks)test/ably/rest/restrequest_test.py(3 hunks)test/ably/rest/resttoken_test.py(4 hunks)test/ably/utils.py(2 hunks)test/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- ably/util/crypto.py
- test/ably/rest/resttoken_test.py
🚧 Files skipped from review as they are similar to previous changes (14)
- ably/realtime/connectionmanager.py
- test/ably/rest/restchannelstatus_test.py
- ably/rest/auth.py
- ably/scripts/unasync.py
- .github/workflows/check.yml
- test/ably/rest/restpresence_test.py
- test/ably/realtime/realtimeauth_test.py
- test/ably/rest/restpaginatedresult_test.py
- pyproject.toml
- test/ably/rest/restchannelpublish_test.py
- test/ably/rest/restchannelhistory_test.py
- test/ably/utils.py
- test/ably/rest/resthttp_test.py
- ably/http/http.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/ably/rest/restinit_test.py (4)
ably/transport/defaults.py (2)
Defaults(1-63)get_port(36-46)test/ably/utils.py (2)
dont_vary_protocol(159-161)patch(76-86)ably/rest/rest.py (4)
AblyRest(19-148)time(91-95)http(111-112)request(122-142)test/conftest.py (1)
send(35-62)
🪛 actionlint (1.7.7)
.github/workflows/lint.yml
16-16: property "python-version" is not defined in object type {}
(expression)
🪛 Ruff (0.11.9)
test/ably/rest/restauth_test.py
102-105: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
389-389: Yoda condition detected
Rewrite as count_call == 1
(SIM300)
390-390: Yoda condition detected
Rewrite as token_details.token == 'token_string'
(SIM300)
437-437: Yoda condition detected
Rewrite as token_details.token == 'another_token_string'
(SIM300)
test/ably/rest/restcrypto_test.py
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restinit_test.py
128-128: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
134-134: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9998
(SIM300)
140-140: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
209-212: Use contextlib.suppress(AblyException) instead of try-except-pass
Replace with contextlib.suppress(AblyException)
(SIM105)
test/conftest.py
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
111-111: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (24)
.github/workflows/lint.yml (4)
13-13: Upgradeactions/checkoutto v4
Switching from v2 to v4 brings performance improvements and bug fixes in the checkout action. I’ve confirmed there are no breaking changes in this minor bump.
17-17: Upgradeactions/setup-pythonto v5
Moving to v5 is a good update—python-versionremains the correct input name and the behavior is backward-compatible.
21-21: Switch Poetry setup action to abatilo/actions-poetry@v4
This aligns with the latest recommended Poetry integration and should fix past issues around lockfile handling.
23-23: Bump Poetry version to 1.5.0
Upgrading Poetry should improve dependency resolution performance. Please verify in CI (or locally) thatpoetry --versionoutputs1.5.0and thatpoetry installstill succeeds without changes topoetry.lock.test/ably/rest/restinit_test.py (3)
3-3: Updated HTTP client import for the niquests migrationImport changes from httpx to niquests align with the PR objective to replace the HTTP client.
171-172: Improved spacing in patch callsAdded proper spacing after commas in the nested patch calls - this is a good formatting improvement.
208-214: Updated patching and URL assertion for niquestsThe changes correctly patch the AsyncSession.send method from niquests instead of AsyncClient.send from httpx.
The URL assertion was updated to remove the explicit port
:443which aligns with the URL format used by niquests.For improved error handling, consider using contextlib.suppress:
- try: - await ably.time() - except AblyException: - pass + with contextlib.suppress(AblyException): + await ably.time()🧰 Tools
🪛 Ruff (0.11.9)
209-212: Use
contextlib.suppress(AblyException)instead oftry-except-passReplace with
contextlib.suppress(AblyException)(SIM105)
test/ably/rest/restrequest_test.py (4)
1-4: Updated imports for niquests/responsesThe import changes from httpx/respx to niquests/responses align with the PR objective.
101-102: Updated exception type for timeoutException type correctly updated from httpx.ReadTimeout to niquests.ReadTimeout to match the new HTTP client.
134-135: Updated exception type for connection errorException type correctly updated from httpx.ConnectionError to niquests.ConnectionError to match the new HTTP client.
137-138: Added explicit stopping of responses mockThe test properly stops the responses mock after testing with fallback hosts to prevent mock leakage between tests.
test/conftest.py (4)
1-20: Good setup for making responses compatible with niquestsThis module replacement approach is a clever way to make the responses library work with niquests by making it think it's working with requests.
22-53: Well-structured async mock implementationThe NiquestsMock class provides a good extension of responses.RequestsMock with async support. The argument handling is thorough.
66-106: Robust implementation of start/stop methodsThe start/stop methods properly handle patching and unpatching of both sync and async adapters, including error cases.
108-145: Efficient setup of global mock and utility methodsThe module exports a singleton mock instance and dynamically assigns all the necessary attributes to the responses module, providing a clean interface for tests.
Minor style improvement:
-setattr(responses, "mock", mock) -setattr(responses, "_default_mock", _default_mock) +responses.mock = mock +responses._default_mock = _default_mock🧰 Tools
🪛 Ruff (0.11.9)
110-110: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
111-111: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
test/ably/rest/restauth_test.py (9)
1-1: Library imports updated correctly for the HTTP client migration.The imports have been appropriately updated to use
responsesandniquests.AsyncSessioninstead of the previousrespxandhttpxlibraries, aligning with the project's migration to Niquests as indicated in the PR objectives.Also applies to: 11-12
88-88: HTTP client mocking properly updated in test cases.All patches have been correctly updated to mock
AsyncSession.sendinstead of the previousAsyncClient.send, ensuring proper test functionality with the new HTTP client.Also applies to: 101-101, 113-113
349-433: HTTP mocking approach correctly migrated to responses framework.The test has been properly migrated from respx to responses:
- The
@responses.activatedecorator is correctly applied- The callback function properly validates request details
- Response setup uses the proper tuple structure (status, headers, body)
- Call counting logic has been appropriately reimplemented
The migration preserves all the original test logic while adapting to the new mocking framework.
🧰 Tools
🪛 Ruff (0.11.9)
389-389: Yoda condition detected
Rewrite as
count_call == 1(SIM300)
390-390: Yoda condition detected
Rewrite as
token_details.token == 'token_string'(SIM300)
466-482: Authentication URL test correctly migrated to responses framework.The test for auth URL with query string has been successfully migrated from respx to responses, maintaining the original test logic while adapting to the new HTTP mocking framework.
524-572: Token renewal mocking correctly implemented with responses.The token renewal test setup has been properly migrated to use responses:
- The URL formatting is correct
- JSON responses are properly serialized
- The responses activation/deactivation is correctly implemented
- The mock server start/stop mechanism has been updated appropriately
All logic from the original implementation has been preserved.
579-579: Assertion mechanisms correctly updated for the responses framework.The assertions for checking route call counts have been updated to match the responses API:
self.request_token_route.call_countis now used to check call countsself.request_token_route.callsis used to verify if routes were calledThese updates maintain the original test's verification logic.
Also applies to: 583-583, 604-604, 624-624
616-617: Response call count assertion properly implemented.The direct call to
responses.assert_call_count()is a good use of the responses API for verifying the expected number of requests.
637-688: Expired token test setup correctly migrated to responses.The expired token test mocking has been properly migrated:
- URL formatting and routes are correctly defined
- The JSON responses are properly serialized
- Callback functions maintain the original test logic
- The activation/deactivation of responses is correctly implemented
The migration preserves all test logic while adapting to the new framework.
690-691: Test teardown properly updated for responses.The test teardown correctly stops and resets the responses framework, ensuring proper cleanup between tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/conftest.py (1)
56-62:⚠️ Potential issuePotential KeyError when
streamkwarg omittedThe direct access to
kwargs["stream"]will raise a KeyError if the caller doesn't provide this argument, which is legitimate for many AsyncSession.send invocations.- if kwargs["stream"]: + if kwargs.get("stream", False): return resptest/ably/rest/restauth_test.py (1)
108-108:⚠️ Potential issueHeader comparison uses bytes vs. str – assertion will always fail
request.headersalways yields str values, whereasbase64.b64encodereturns bytes. The current assertion compares strings with bytes and will fail even when the header is correct.-assert client_id == base64.b64encode('client_id'.encode('ascii')) +assert client_id == base64.b64encode('client_id'.encode('ascii')).decode("utf-8")test/ably/rest/resthttp_test.py (1)
33-41: Use partial assertion for call arguments instead of exact equalityThe assertion of exact
call_argsequality is brittle as the parameter set forAsyncSession.sendmay change across niquests versions. Parameters likeallow_redirectsorproxiesmight be omitted when using defaults in future versions.Prefer checking only the parameters this test genuinely cares about:
- assert send_mock.call_args == mock.call( - mock.ANY, - timeout=(4, 10), - allow_redirects=True, - proxies=OrderedDict(), - stream=False, - verify=True, - cert=None - ) + assert send_mock.call_count == Defaults.http_max_retry_count + _, kwargs = send_mock.call_args + assert kwargs["timeout"] == (4, 10) + assert kwargs["stream"] is False + # optionally assert presence/absence of others with `.get`
🧹 Nitpick comments (6)
.github/workflows/lint.yml (1)
13-13: Upgrade to actions/checkout@v4
Usingactions/checkout@v4is correct and aligns with other workflows.Nit: consider pinning to a specific minor/patch version (e.g.,
actions/[email protected]) to avoid unintended breaks.test/ably/rest/resthttp_test.py (5)
78-90: Consider simplifying host verification logicThe current implementation creates sets and removes items as they're verified, which works but is more complex than needed.
Consider a more declarative approach:
- expected_urls_set = { - make_url(host) - for host in Options(http_max_retry_count=10).get_rest_hosts() - } - expected_hosts_set = set(Options(http_max_retry_count=10).get_rest_hosts()) - for (prep_request_tuple, _) in send_mock.call_args_list: - url = prep_request_tuple[0].url - assert url in expected_urls_set - expected_urls_set.remove(url) - current_host = urlparse(prep_request_tuple[0].url).hostname - assert current_host in expected_hosts_set - expected_hosts_set.remove(current_host) + expected_hosts = Options(http_max_retry_count=10).get_rest_hosts() + expected_urls = {make_url(host) for host in expected_hosts} + + actual_urls = [prep_request_tuple[0].url for (prep_request_tuple, _) in send_mock.call_args_list] + actual_hosts = [urlparse(url).hostname for url in actual_urls] + + # Check that we used all expected hosts exactly once + assert sorted(actual_hosts) == sorted(expected_hosts) + assert sorted(actual_urls) == sorted(expected_urls)This approach makes the test's intent clearer while maintaining the same verification.
97-104: Use responses.get with side_effect instead of add_callbackThe current implementation uses
responses.add_callbackto simulate an exception, but there's a more idiomatic way to achieve this with responses.- def _throw_error(*args, **kwargs): - raise niquests.RequestException - - responses.add_callback( - "GET", - "https://example.org/", - _throw_error - ) + responses.get( + "https://example.org/", + side_effect=niquests.RequestException + )This is more concise and follows the common pattern in the
responseslibrary.
158-159: Consider using urlparse for URL normalizationThe direct string replacement (
default_url.replace(":443/", "/")) is simple but lacks the robustness of proper URL parsing.- default_url = default_url.replace(":443/", "/") + parsed_url = urlparse(default_url) + # Normalize the URL - omit default ports + if parsed_url.port == 443 and parsed_url.scheme == 'https': + default_url = f"{parsed_url.scheme}://{parsed_url.hostname}{parsed_url.path}" + elif parsed_url.port == 80 and parsed_url.scheme == 'http': + default_url = f"{parsed_url.scheme}://{parsed_url.hostname}{parsed_url.path}"This approach is more explicit about the URL transformation and handles both HTTP and HTTPS default ports consistently.
160-165: Consider JSON serialization consistencyThe test uses
json.dumps()to serialize the error response body, but doesn't specify any formatting options likeindentorsort_keys.For consistent JSON serialization across tests, consider:
- body=json.dumps({'message': "", 'status_code': 600, 'code': 50500}), + body=json.dumps({'message': "", 'status_code': 600, 'code': 50500}, sort_keys=True),Or alternatively, use the
jsonparameter ofresponses.get()which handles serialization automatically:- content_type="application/json", - body=json.dumps({'message': "", 'status_code': 600, 'code': 50500}), + json={'message': "", 'status_code': 600, 'code': 50500},
253-254: Add comment explaining HTTP version assertionThe code correctly notes that mocked responses don't have HTTP version info, but this might be confusing for future maintainers.
- assert r.http_version == 0 # mocked dummy response don't have a http version + # When using responses for mocking, the dummy response doesn't include HTTP version information + # In a real response with HTTP/2, this would be 2 instead of 0 + assert r.http_version == 0 await ably.close()This provides clearer context about what would be expected in a real HTTP/2 request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)ably/http/http.py(4 hunks)ably/realtime/connectionmanager.py(2 hunks)ably/rest/auth.py(2 hunks)ably/scripts/unasync.py(2 hunks)ably/util/crypto.py(1 hunks)pyproject.toml(2 hunks)test/ably/realtime/realtimeauth_test.py(2 hunks)test/ably/rest/restauth_test.py(11 hunks)test/ably/rest/restchannelhistory_test.py(2 hunks)test/ably/rest/restchannelpublish_test.py(5 hunks)test/ably/rest/restchannelstatus_test.py(1 hunks)test/ably/rest/restcrypto_test.py(3 hunks)test/ably/rest/resthttp_test.py(9 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(4 hunks)test/ably/rest/restpresence_test.py(3 hunks)test/ably/rest/restrequest_test.py(7 hunks)test/ably/rest/resttoken_test.py(4 hunks)test/ably/utils.py(2 hunks)test/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/ably/rest/resttoken_test.py
🚧 Files skipped from review as they are similar to previous changes (15)
- test/ably/rest/restchannelstatus_test.py
- ably/util/crypto.py
- ably/rest/auth.py
- .github/workflows/check.yml
- test/ably/realtime/realtimeauth_test.py
- ably/realtime/connectionmanager.py
- test/ably/rest/restpresence_test.py
- test/ably/rest/restpaginatedresult_test.py
- test/ably/rest/restchannelpublish_test.py
- test/ably/rest/restrequest_test.py
- test/ably/rest/restchannelhistory_test.py
- pyproject.toml
- test/ably/utils.py
- ably/scripts/unasync.py
- ably/http/http.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/conftest.py (3)
ably/rest/rest.py (1)
request(122-142)ably/http/http.py (4)
Response(87-114)patch(257-260)method(63-64)url(67-68)test/ably/utils.py (1)
patch(76-86)
test/ably/rest/restinit_test.py (4)
ably/transport/defaults.py (2)
Defaults(1-63)get_port(36-46)test/ably/utils.py (2)
dont_vary_protocol(159-161)patch(76-86)ably/rest/rest.py (4)
AblyRest(19-148)time(91-95)http(111-112)request(122-142)test/conftest.py (1)
send(35-62)
🪛 Ruff (0.11.9)
test/conftest.py
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
111-111: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
test/ably/rest/restauth_test.py
102-105: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
389-389: Yoda condition detected
Rewrite as count_call == 1
(SIM300)
390-390: Yoda condition detected
Rewrite as token_details.token == 'token_string'
(SIM300)
437-437: Yoda condition detected
Rewrite as token_details.token == 'another_token_string'
(SIM300)
test/ably/rest/restcrypto_test.py
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restinit_test.py
128-128: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
134-134: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9998
(SIM300)
140-140: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
209-212: Use contextlib.suppress(AblyException) instead of try-except-pass
Replace with contextlib.suppress(AblyException)
(SIM105)
🪛 actionlint (1.7.7)
.github/workflows/lint.yml
16-16: property "python-version" is not defined in object type {}
(expression)
🔇 Additional comments (24)
.github/workflows/lint.yml (3)
17-17: Upgrade to actions/setup-python@v5
Switching toactions/setup-python@v5withpython-versionis valid; the actionlint warning aboutpython-versionis a known schema lag and can be safely ignored.
21-21: Upgrade to abatilo/actions-poetry@v4
Bumping toabatilo/actions-poetry@v4is consistent across CI.
23-23: Bump Poetry to 1.5.0
Updating the Poetry version to1.5.0matches the changes in thecheck.ymlworkflow. Ensure that this version continues to support all targeted Python versions (including 3.7).test/ably/rest/restcrypto_test.py (3)
82-92: Multi-line assert statements properly restoredThe spaces added before the backslashes in these assertions improve formatting while correctly maintaining the line continuation. This addresses the syntax issues identified in previous reviews.
🧰 Tools
🪛 Ruff (0.11.9)
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-125: Assert statements consistently formattedThe consistent addition of spaces before backslashes in these multi-line assert statements aligns with the changes in the previous test method, ensuring uniform formatting across the file.
🧰 Tools
🪛 Ruff (0.11.9)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-174: Line continuations fixed in test_crypto_send_unencryptedThe spaces before backslashes properly restore the multi-line assert statements that were previously causing syntax errors, keeping the custom error messages intact.
🧰 Tools
🪛 Ruff (0.11.9)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restinit_test.py (4)
3-3: Successfully migrated to niquestsThe import change from httpx to niquests.AsyncSession properly aligns with the PR's objective to replace the HTTP client.
128-129: Improved multi-line assertion formattingThe spaces added before backslashes in these assertions improve readability while maintaining proper line continuation.
Also applies to: 134-135, 140-141
🧰 Tools
🪛 Ruff (0.11.9)
128-128: Yoda condition detected
Rewrite as
Defaults.get_port(ably.options) == 9999(SIM300)
171-172: Consistent line continuation in patch statementThe space added before the backslash in the patch statement improves readability and maintains consistent formatting with other multi-line statements.
208-215: Successful adaptation to niquests HTTP clientThe patching target has been correctly updated from httpx.AsyncClient to AsyncSession, and the URL assertion has been adjusted to match niquests' URL formatting which doesn't include the explicit port
:443for HTTPS.🧰 Tools
🪛 Ruff (0.11.9)
209-212: Use
contextlib.suppress(AblyException)instead oftry-except-passReplace with
contextlib.suppress(AblyException)(SIM105)
test/conftest.py (4)
1-19: Well-structured HTTP client mocking setupThe module effectively replaces requests with niquests in sys.modules, enabling the responses mocking library to work with niquests. This approach allows reusing an existing HTTP mocking framework with the new HTTP client.
22-65: NiquestsMock class provides essential async supportThe class extends responses.RequestsMock to support mocking both synchronous and asynchronous HTTP requests, with proper handling of positional arguments and adapter handling.
66-106: Robust patching approach for both sync and async adaptersThe start/stop methods correctly manage the lifecycle of both synchronous and asynchronous HTTP adapter patchers, ensuring they're not overridden if already active and properly cleaning up resources afterward.
108-144: Effective API surface exposureThe module creates a default NiquestsMock instance and effectively exposes its API through the responses module, maintaining a familiar interface while supporting the new HTTP client functionality.
🧰 Tools
🪛 Ruff (0.11.9)
110-110: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
111-111: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
test/ably/rest/restauth_test.py (7)
1-13: Successfully migrated HTTP importsThe imports have been correctly updated to replace respx with responses and httpx with niquests, aligning with the PR's objective.
88-95: Patching target properly updatedThe patching target has been correctly updated from httpx.AsyncClient to niquests.AsyncSession, maintaining the test functionality while using the new HTTP client.
Also applies to: 101-108, 113-120
🧰 Tools
🪛 Ruff (0.11.9)
89-92: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
349-383: Successfully migrated from respx to responses for HTTP mockingThe test has been properly rewritten to use responses.add_callback instead of respx routes, with appropriate callback functions that verify request headers and params and return correctly formatted responses.
466-482: HTTP mocking updated for auth URL testThe test has been successfully migrated from respx to responses, with proper URL construction including query parameters.
524-566: Token renewal test successfully migratedThe token renewal test mocking has been properly rewritten to use responses instead of respx, with appropriate JSON response construction.
570-571: Mock teardown correctly updatedThe mock teardown has been correctly updated to use responses.stop() and responses.reset() instead of the respx equivalents.
637-687: Expired token tests successfully migratedThe HTTP mocking for expired token tests has been comprehensively migrated to the responses library, maintaining the original test functionality.
test/ably/rest/resthttp_test.py (3)
68-70: Good improvement for URL comparisonThe addition of code to strip default ports (
:443and:80) before URL comparison makes the tests more robust against slight formatting differences in URLs.
227-229: Improved URL parameter extractionThe change to use
urlparseandparse_qsfor request ID extraction is a good improvement over the previous approach.The new implementation properly handles URL parsing and query parameter extraction, making the test more robust against different URL formats.
Also applies to: 233-235
119-121:❌ Incorrect review comment
Client initialization in test_cached_fallback
The test creates a new
niquests.AsyncSession()but doesn't configure it with any custom settings that might be necessary to match the behavior of the client under test.Verify that the test client doesn't need any specific configuration to properly test the system under test:
🏁 Script executed:
#!/bin/bash # Check if the client is configured elsewhere in the test or just used for its send method rg -A 5 "AsyncSession\(\)" test/ # Check if the actual HTTP client in the implementation has any special configuration rg -A 10 "AsyncSession\(\)" ably/Length of output: 2654
No special AsyncSession configuration required
In both the implementation and its tests,niquests.AsyncSession()is instantiated with default settings and the tests simply stub out itssendmethod to control behavior. There is no custom configuration applied inably/http/http.py, and the tests intest/ably/rest/resthttp_test.pyandtest/ably/rest/restchannelpublish_test.pymirror that usage. As a result, no additional session configuration is needed fortest_cached_fallback.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/conftest.py (1)
22-64:⚠️ Potential issueCreated custom NiquestsMock class to support async HTTP mocking
The
NiquestsMockclass extendsresponses.RequestsMockto add support for asynchronous HTTP requests. The implementation properly handles both synchronous and asynchronous adapters, and correctly processes the request/response lifecycle.However, there's a potential issue in the async send method that could cause a KeyError:
- if kwargs["stream"]: + if kwargs.get("stream", False): return respThe current code will raise a KeyError if the 'stream' argument is not provided, which is legitimate for many AsyncSession.send invocations. The fix uses the get() method with a default value to safely handle missing arguments.
test/ably/rest/restauth_test.py (1)
108-108:⚠️ Potential issueHeader comparison uses bytes vs. str – assertion will always fail
The current assertion compares a string header value with a bytes object returned by base64.b64encode, which will cause it to always fail even when the header is correct.
-assert client_id == base64.b64encode('client_id'.encode('ascii')) +assert client_id == base64.b64encode('client_id'.encode('ascii')).decode('utf-8')
request.headersalways yields string values, whereasbase64.b64encodereturns bytes. The fix decodes the bytes to a string before comparison.
🧹 Nitpick comments (8)
test/ably/rest/restcrypto_test.py (6)
82-83: Fix error message typoThere's an unexpected closing parenthesis in the error message which should be removed.
- assert "This is a string message payload" == message_contents["publish3"], \ - "Expect publish3 to be expected String)" + assert "This is a string message payload" == message_contents["publish3"], \ + "Expect publish3 to be expected String"🧰 Tools
🪛 Ruff (0.11.9)
82-82: Yoda condition detected
(SIM300)
91-92: Fix mismatch in error messageThe error message states "JSONObject" but the assertion is actually checking for a JSONArray.
- assert ["This is a JSONArray message payload"] == message_contents["publish6"], \ - "Expect publish6 to be expected JSONObject" + assert ["This is a JSONArray message payload"] == message_contents["publish6"], \ + "Expect publish6 to be expected JSONArray"🧰 Tools
🪛 Ruff (0.11.9)
91-91: Yoda condition detected
(SIM300)
115-116: Fix error message typoThere's an unexpected closing parenthesis in the error message which should be removed.
- assert "This is a string message payload" == message_contents["publish3"], \ - "Expect publish3 to be expected String)" + assert "This is a string message payload" == message_contents["publish3"], \ + "Expect publish3 to be expected String"🧰 Tools
🪛 Ruff (0.11.9)
115-115: Yoda condition detected
(SIM300)
124-125: Fix mismatch in error messageThe error message states "JSONObject" but the assertion is actually checking for a JSONArray.
- assert ["This is a JSONArray message payload"] == message_contents["publish6"], \ - "Expect publish6 to be expected JSONObject" + assert ["This is a JSONArray message payload"] == message_contents["publish6"], \ + "Expect publish6 to be expected JSONArray"🧰 Tools
🪛 Ruff (0.11.9)
124-124: Yoda condition detected
(SIM300)
173-174: Fix mismatch in error messageThe error message states "JSONObject" but the assertion is actually checking for a JSONArray.
- assert ["This is a JSONArray message payload"] == message_contents["publish6"], \ - "Expect publish6 to be expected JSONObject" + assert ["This is a JSONArray message payload"] == message_contents["publish6"], \ + "Expect publish6 to be expected JSONArray"🧰 Tools
🪛 Ruff (0.11.9)
173-173: Yoda condition detected
(SIM300)
82-83: Consider alternative assert formatting stylesThe current approach of adding a space before backslashes works, but you might also consider using parentheses for multi-line asserts which is more explicit and avoids backslash continuation issues:
assert ( "This is a string message payload" == message_contents["publish3"] ), "Expect publish3 to be expected String"This style also avoids the Yoda conditions (literals on the left of comparisons) flagged by Ruff.
Also applies to: 85-86, 88-89, 91-92, 115-116, 118-119, 121-122, 124-125, 164-165, 167-168, 170-171, 173-174
🧰 Tools
🪛 Ruff (0.11.9)
82-82: Yoda condition detected
(SIM300)
.github/workflows/lint.yml (1)
21-21: Verify Poetry GitHub Action pinning
Upgrading toabatilo/actions-poetry@v2is reasonable. To improve reproducibility, consider pinning to a more specific tag (e.g.,v2.0.0) or commit SHA to prevent unintended changes if the major line receives updates.test/conftest.py (1)
108-112: Created singleton mock instance and assigned to responses namespaceThis creates an instance of
NiquestsMockwith assertion disabled by default and assigns it toresponses.mockandresponses._default_mock. This ensures the mock is accessible through the standard responses API.Minor style improvement:
-setattr(responses, "mock", mock) -setattr(responses, "_default_mock", _default_mock) +responses.mock = mock +responses._default_mock = _default_mock🧰 Tools
🪛 Ruff (0.11.9)
110-110: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
111-111: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)ably/http/http.py(4 hunks)ably/realtime/connectionmanager.py(2 hunks)ably/rest/auth.py(2 hunks)ably/scripts/unasync.py(2 hunks)ably/util/crypto.py(1 hunks)pyproject.toml(2 hunks)test/ably/realtime/realtimeauth_test.py(2 hunks)test/ably/rest/restauth_test.py(11 hunks)test/ably/rest/restchannelhistory_test.py(2 hunks)test/ably/rest/restchannelpublish_test.py(5 hunks)test/ably/rest/restchannelstatus_test.py(1 hunks)test/ably/rest/restcrypto_test.py(3 hunks)test/ably/rest/resthttp_test.py(9 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(4 hunks)test/ably/rest/restpresence_test.py(3 hunks)test/ably/rest/restrequest_test.py(7 hunks)test/ably/rest/resttoken_test.py(4 hunks)test/ably/utils.py(2 hunks)test/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/ably/rest/resttoken_test.py
- test/ably/rest/restchannelstatus_test.py
🚧 Files skipped from review as they are similar to previous changes (14)
- ably/realtime/connectionmanager.py
- test/ably/rest/restchannelhistory_test.py
- ably/util/crypto.py
- ably/rest/auth.py
- .github/workflows/check.yml
- test/ably/rest/restpresence_test.py
- test/ably/rest/resthttp_test.py
- ably/scripts/unasync.py
- test/ably/realtime/realtimeauth_test.py
- test/ably/rest/restchannelpublish_test.py
- test/ably/utils.py
- pyproject.toml
- test/ably/rest/restpaginatedresult_test.py
- ably/http/http.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/ably/rest/restrequest_test.py (6)
ably/rest/rest.py (2)
request(122-142)close(147-148)ably/http/http.py (7)
version(83-84)close(134-135)get(252-255)body(75-76)post(262-265)to_native(95-107)headers(71-72)ably/transport/defaults.py (1)
Defaults(1-63)test/ably/testapp.py (2)
TestApp(37-115)get_ably_rest(73-77)test/ably/rest/restchannelpublish_test.py (1)
get_ably_rest(526-528)ably/http/paginatedresult.py (3)
status_code(116-117)items(56-57)headers(133-134)
test/ably/rest/restauth_test.py (10)
ably/http/http.py (10)
patch(257-260)get(252-255)skip_auth(79-80)headers(71-72)url(67-68)body(75-76)auth(273-274)auth(277-278)close(134-135)post(262-265)test/ably/utils.py (2)
patch(76-86)dont_vary_protocol(159-161)ably/rest/auth.py (5)
ably(297-298)client_id(328-329)auth_params(309-310)token_details(324-325)request_token(154-230)ably/rest/rest.py (4)
http(111-112)request(122-142)AblyRest(19-148)time(91-95)ably/types/tokendetails.py (2)
token(27-28)TokenDetails(7-91)test/ably/testapp.py (2)
TestApp(37-115)get_ably_rest(73-77)test/conftest.py (2)
start(66-80)stop(82-105)test/ably/rest/restpaginatedresult_test.py (1)
callback(14-29)ably/util/exceptions.py (1)
AblyAuthException(99-100)test/ably/realtime/realtimeauth_test.py (14)
callback(91-93)callback(105-107)callback(119-121)callback(131-132)callback(181-183)callback(221-223)callback(236-238)callback(250-252)callback(270-272)callback(301-303)callback(348-350)callback(373-378)callback(486-488)callback(534-536)
🪛 Ruff (0.11.9)
test/ably/rest/restauth_test.py
102-105: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
389-389: Yoda condition detected
Rewrite as count_call == 1
(SIM300)
390-390: Yoda condition detected
Rewrite as token_details.token == 'token_string'
(SIM300)
437-437: Yoda condition detected
Rewrite as token_details.token == 'another_token_string'
(SIM300)
test/ably/rest/restcrypto_test.py
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restinit_test.py
128-128: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
134-134: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9998
(SIM300)
140-140: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
209-212: Use contextlib.suppress(AblyException) instead of try-except-pass
Replace with contextlib.suppress(AblyException)
(SIM105)
test/conftest.py
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
111-111: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🪛 actionlint (1.7.7)
.github/workflows/lint.yml
16-16: property "python-version" is not defined in object type {}
(expression)
🔇 Additional comments (26)
.github/workflows/lint.yml (3)
13-13: Approved upgrade to actions/checkout v4
Upgradingactions/checkouttov4is appropriate—it brings performance improvements and new features. Thesubmodules: 'recursive'option remains valid in this version.
17-17: Skip static analysis false positive for setup-python inputs
Theactionlinthint flagspython-versionas undefined, butactions/setup-python@v5officially supports this input. This is a known false positive from outdated schemas.
23-23: Confirm Poetry version upgrade
Bumpingpoetry-versionto1.5.0aligns with your other workflows. Ensure thepoetry.lockfile and project configuration remain compatible with this version.test/ably/rest/restinit_test.py (5)
3-3: Import change to support NiquestsThe import has been updated from httpx to niquests to support the new HTTP client library. This aligns with the PR objective of replacing httpx with Niquests.
128-129: Improved line continuation for better readabilityAdded backslashes for proper line continuation in the assertion statements.
Also applies to: 134-135, 140-141
🧰 Tools
🪛 Ruff (0.11.9)
128-128: Yoda condition detected
Rewrite as
Defaults.get_port(ably.options) == 9999(SIM300)
171-172: Added proper line continuation for mock patchThe backslash for proper line continuation has been added in the patch statement.
208-208: Updated mock patch target to use niquestsThe patch target has been updated from httpx.AsyncClient.send to niquests.AsyncSession.send, aligning with the PR objective of replacing httpx with niquests.
214-214: Updated URL assertion to match new URL handlingThe assertion has been updated to remove the explicit port
:443from the expected URL, which aligns with the updated URL construction logic in the HTTP client implementation that strips default ports from URLs.test/ably/rest/restrequest_test.py (8)
1-3: Updated imports to support Niquests and responsesThe imports have been updated to use niquests and responses libraries instead of httpx and respx, aligning with the PR objective of replacing the HTTP client and mocking libraries.
100-102: Updated exception type for timeout handlingThe exception type has been updated to use niquests.ReadTimeout instead of httpx.ReadTimeout to align with the new HTTP client library.
105-134: Replaced respx mock with responses for timeout testThe test has been refactored to use the responses library instead of respx for mocking HTTP requests. The implementation now:
- Uses
responses.start()andstop()instead of a context manager- Uses
responses.add_callbackandresponses.getto define mock endpoints- Uses niquests.ConnectionError to simulate connection failures
This change aligns with the PR objective and follows recommended practices to avoid mock leakage between tests.
131-132: Updated exception type for connection errorThe exception type has been updated to use niquests.ConnectionError instead of httpx.ConnectError to align with the new HTTP client library.
145-156: Replaced respx mock with responses for 503 fallback testThe test has been refactored to use the responses library for mocking HTTP requests, with
start()/stop()calls and proper route definitions. The assertion on the call count has been updated to match the new API.
165-187: Replaced respx mock with responses for timeout fallback testThe test has been refactored to use the responses library for mocking HTTP requests and timeout scenarios. The implementation correctly uses callback functions to simulate timeouts and properly asserts the call count using the new API.
204-211: Replaced respx mock with responses for publish fallback testThe test has been refactored to use the responses library for mocking HTTP requests during publishing operations. The implementation correctly captures the call counts and response verification.
220-247: Replaced respx mock with responses for CloudFront fallback testThe test has been refactored to use the responses library for mocking HTTP requests with CloudFront headers. The implementation properly uses the responses API to define routes, headers, and verify request execution.
test/conftest.py (3)
1-15: Implemented module monkey-patching to adapt responses to niquestsThis code replaces the
requestsmodule insys.moduleswithniquestsand its submodules, allowing theresponseslibrary (which was designed for therequestslibrary) to work withniquests. This is a clever approach to reuse the existing mocking infrastructure.
66-106: Implemented proper patching lifecycle for AsyncHTTPAdapterThe start and stop methods properly manage the patching and unpatching of both synchronous and asynchronous adapters. The stop method also contains logic to optionally assert that all registered mocked requests were called, which is useful for testing.
113-145: Created API compatibility layer for responses methodsThis code dynamically assigns common responses API methods and HTTP method shortcuts to the responses namespace, redirecting them to the
NiquestsMockinstance. This maintains a familiar interface for tests while supporting async mocking.test/ably/rest/restauth_test.py (7)
11-12: Updated imports to support Niquests and responsesThe imports have been updated to use responses and niquests.AsyncSession instead of respx and httpx.AsyncClient, aligning with the PR objective of replacing the HTTP client and mocking libraries.
88-88: Updated patch targets to use niquestsThe patch targets have been updated from httpx.AsyncClient.send to niquests.AsyncSession.send in all the authentication test methods, aligning with the PR objective.
Also applies to: 101-101, 113-113
350-390: Refactored auth URL test to use responses instead of respxThe test has been updated to use responses for mocking HTTP requests. Key changes include:
- Using the responses.activate decorator
- Adding a callback counter variable rather than relying on respx call flags
- Using responses.add_callback to register the mock endpoint
- Properly asserting the call count at the end
The implementation correctly validates the request headers, parameters, and generates appropriate responses.
🧰 Tools
🪛 Ruff (0.11.9)
389-389: Yoda condition detected
Rewrite as
count_call == 1(SIM300)
390-390: Yoda condition detected
Rewrite as
token_details.token == 'token_string'(SIM300)
395-439: Refactored HTTP GET auth test to use responses instead of respxSimilar to the POST test, this test has been updated to use responses for mocking HTTP requests. The implementation correctly builds the URL with query parameters and uses proper content type headers for the JSON response.
🧰 Tools
🪛 Ruff (0.11.9)
437-437: Yoda condition detected
Rewrite as
token_details.token == 'another_token_string'(SIM300)
467-482: Refactored query string test to use responsesThis test has been updated to use responses instead of respx for mocking HTTP requests with query strings. The implementation correctly builds the full URL with all query parameters and asserts the call count.
524-567: Refactored token renewal test to use responsesThe test for token renewal has been updated to use responses instead of respx. The implementation:
- Creates a mock POST endpoint for token requests
- Creates a callback for publishing messages that selectively fails
- Properly starts the responses mock before tests
- Uses the appropriate responses API for assertions
This aligns with the broader migration to responses throughout the test suite.
639-688: Refactored expired token test to use responsesThe test for expired token handling has been updated to use responses instead of respx. The implementation correctly mocks both the token request and time endpoints, and uses proper JSON formatting for responses.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
test/ably/rest/restcrypto_test.py (3)
82-93: Refactor multi-line assert statements for clarity and fix error message
Using backslashes for line continuation can lead to readability issues and subtle syntax errors (e.g., the stray closing parenthesis in the message at line 83). Wrap the assertion in parentheses for implicit continuation and remove the extra “)” in the error message:- assert "This is a string message payload" == message_contents["publish3"], \ - "Expect publish3 to be expected String)" + assert ( + "This is a string message payload" == message_contents["publish3"] + ), "Expect publish3 to be expected String"Apply this pattern consistently to the other three assertions in this block.
🧰 Tools
🪛 Ruff (0.11.9)
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-126: Refactor multi-line assert statements for clarity and fix error message
The same pattern occurs intest_crypto_publish_256. Using parentheses instead of backslashes improves readability and prevents formatting errors. Also remove the stray “)” in the message on line 116:- assert "This is a string message payload" == message_contents["publish3"], \ - "Expect publish3 to be expected String)" + assert ( + "This is a string message payload" == message_contents["publish3"] + ), "Expect publish3 to be expected String"Please update the remaining assertions in this segment similarly.
🧰 Tools
🪛 Ruff (0.11.9)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-175: Refactor multi-line assert statements for clarity
Intest_crypto_send_unencrypted, switch to parentheses for implicit line continuation rather than backslashes to enhance readability and maintain consistency:- assert "This is a string message payload" == message_contents["publish3"], \ - "Expect publish3 to be expected String" + assert ( + "This is a string message payload" == message_contents["publish3"] + ), "Expect publish3 to be expected String"Apply this refactor to the other three assertions in this block as well.
🧰 Tools
🪛 Ruff (0.11.9)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restrequest_test.py (1)
105-136: Great job addressing the previous review comments!All
respx.mockcontext managers have been successfully replaced withresponses.start()/stop()pairs throughout the file. The mocking patterns have been properly converted to use the responses library API.Also applies to: 146-157, 167-191, 208-215, 226-252
test/conftest.py (1)
56-63:⚠️ Potential issuePotential
KeyErrorwhen accessingkwargs["stream"]The direct access to
kwargs["stream"]on line 58 will raise aKeyErrorif the caller doesn't provide thestreamargument.Apply this fix to safely handle the missing key:
-if kwargs["stream"]: +if kwargs.get("stream", False): return resptest/ably/rest/restauth_test.py (1)
108-108:⚠️ Potential issueFix the bytes vs string comparison issue.
This is the same critical issue flagged in previous reviews that hasn't been resolved. The assertion compares a string header value with bytes returned by
base64.b64encode, causing it to always fail.Apply this fix:
-assert client_id == base64.b64encode('client_id'.encode('ascii')) +assert client_id == base64.b64encode('client_id'.encode('ascii')).decode("utf-8")
🧹 Nitpick comments (5)
test/ably/rest/restinit_test.py (1)
128-129: Consider reversing the Yoda conditions for better readability.While the current style works, Python conventionally places the variable on the left side of comparisons.
Apply this pattern to improve readability:
-assert 9999 == Defaults.get_port(ably.options), \ +assert Defaults.get_port(ably.options) == 9999, \Also applies to: 134-135, 140-141
🧰 Tools
🪛 Ruff (0.11.9)
128-128: Yoda condition detected
Rewrite as
Defaults.get_port(ably.options) == 9999(SIM300)
test/conftest.py (1)
136-137: Use direct assignment instead ofsetattrwith constants.Direct assignment is more readable and Pythonic when the attribute name is a constant.
Replace setattr calls with direct assignment:
-setattr(responses, "mock", mock) -setattr(responses, "_default_mock", _default_mock) +responses.mock = mock +responses._default_mock = _default_mock🧰 Tools
🪛 Ruff (0.11.9)
136-136: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
137-137: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
test/ably/rest/restauth_test.py (3)
102-105: Consider using contextlib.suppress for cleaner exception handling.The try-except-pass block can be simplified using
contextlib.suppress(Exception)for better readability.+from contextlib import suppress - try: - await ably.http.get('/time', skip_auth=False) - except Exception: - pass + with suppress(Exception): + await ably.http.get('/time', skip_auth=False)🧰 Tools
🪛 Ruff (0.11.9)
102-105: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
389-390: Fix Yoda conditions for better readability.The assertions use Yoda conditions which can be less readable.
-assert 1 == count_call -assert 'token_string' == token_details.token +assert count_call == 1 +assert token_details.token == 'token_string'🧰 Tools
🪛 Ruff (0.11.9)
389-389: Yoda condition detected
Rewrite as
count_call == 1(SIM300)
390-390: Yoda condition detected
Rewrite as
token_details.token == 'token_string'(SIM300)
437-437: Fix Yoda condition.-assert 'another_token_string' == token_details.token +assert token_details.token == 'another_token_string'🧰 Tools
🪛 Ruff (0.11.9)
437-437: Yoda condition detected
Rewrite as
token_details.token == 'another_token_string'(SIM300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks)ably/http/http.py(4 hunks)ably/realtime/connectionmanager.py(2 hunks)ably/rest/auth.py(2 hunks)ably/scripts/unasync.py(2 hunks)ably/util/crypto.py(1 hunks)pyproject.toml(2 hunks)test/ably/realtime/realtimeauth_test.py(2 hunks)test/ably/rest/restauth_test.py(11 hunks)test/ably/rest/restchannelhistory_test.py(2 hunks)test/ably/rest/restchannelpublish_test.py(5 hunks)test/ably/rest/restchannelstatus_test.py(1 hunks)test/ably/rest/restcrypto_test.py(3 hunks)test/ably/rest/resthttp_test.py(9 hunks)test/ably/rest/restinit_test.py(4 hunks)test/ably/rest/restpaginatedresult_test.py(4 hunks)test/ably/rest/restpresence_test.py(3 hunks)test/ably/rest/restrequest_test.py(7 hunks)test/ably/rest/resttoken_test.py(4 hunks)test/ably/utils.py(2 hunks)test/conftest.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test/ably/rest/restchannelstatus_test.py
- ably/util/crypto.py
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- ably/rest/auth.py
- .github/workflows/check.yml
- ably/realtime/connectionmanager.py
- test/ably/rest/resttoken_test.py
- test/ably/rest/restpresence_test.py
- test/ably/realtime/realtimeauth_test.py
- ably/scripts/unasync.py
- test/ably/rest/restpaginatedresult_test.py
- test/ably/rest/restchannelpublish_test.py
- test/ably/rest/restchannelhistory_test.py
- test/ably/utils.py
- ably/http/http.py
- test/ably/rest/resthttp_test.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/conftest.py (2)
ably/rest/rest.py (1)
request(122-142)ably/http/http.py (4)
Response(87-114)patch(257-260)method(63-64)url(67-68)
test/ably/rest/restinit_test.py (5)
ably/transport/defaults.py (2)
Defaults(1-63)get_port(36-46)test/ably/utils.py (2)
dont_vary_protocol(159-161)patch(76-86)ably/rest/rest.py (4)
AblyRest(19-148)time(91-95)http(111-112)request(122-142)test/conftest.py (2)
send(35-62)send(67-88)ably/util/exceptions.py (1)
AblyException(9-84)
🪛 Ruff (0.11.9)
test/conftest.py
136-136: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
137-137: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
test/ably/rest/restauth_test.py
102-105: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
389-389: Yoda condition detected
Rewrite as count_call == 1
(SIM300)
390-390: Yoda condition detected
Rewrite as token_details.token == 'token_string'
(SIM300)
437-437: Yoda condition detected
Rewrite as token_details.token == 'another_token_string'
(SIM300)
test/ably/rest/restcrypto_test.py
82-82: Yoda condition detected
(SIM300)
85-85: Yoda condition detected
(SIM300)
88-88: Yoda condition detected
(SIM300)
91-91: Yoda condition detected
(SIM300)
115-115: Yoda condition detected
(SIM300)
118-118: Yoda condition detected
(SIM300)
121-121: Yoda condition detected
(SIM300)
124-124: Yoda condition detected
(SIM300)
164-164: Yoda condition detected
(SIM300)
167-167: Yoda condition detected
(SIM300)
170-170: Yoda condition detected
(SIM300)
173-173: Yoda condition detected
(SIM300)
test/ably/rest/restinit_test.py
128-128: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
134-134: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9998
(SIM300)
140-140: Yoda condition detected
Rewrite as Defaults.get_port(ably.options) == 9999
(SIM300)
209-212: Use contextlib.suppress(AblyException) instead of try-except-pass
Replace with contextlib.suppress(AblyException)
(SIM105)
🪛 actionlint (1.7.7)
.github/workflows/lint.yml
16-16: property "python-version" is not defined in object type {}
(expression)
🔇 Additional comments (15)
.github/workflows/lint.yml (2)
13-13: Upgrade checkout action to v4
This aligns with the updates incheck.ymland ensures we get the latest features and bug fixes from the checkout action.
21-23: Update Poetry setup action and version
Switching toabatilo/actions-poetry@v2with Poetry1.5.0aligns this workflow withcheck.ymland ensures we use the latest bug fixes and performance improvements in Poetry.test/ably/rest/restinit_test.py (3)
3-3: LGTM!The import change from
httpx.AsyncClienttoniquests.AsyncSessioncorrectly aligns with the PR objective to replace httpx with niquests.
171-172: LGTM!The patch targets have been correctly updated to work with niquests:
- Line 171: Properly wraps the
AblyRest.timemethod- Line 208: Correctly patches
AsyncSession.sendinstead ofAsyncClient.sendAlso applies to: 208-208
214-214:✅ Verification successful
Verify the URL format change is intentional.
The assertion no longer expects the default HTTPS port
:443in the URL. This appears to be a behavioral difference between httpx and niquests, where niquests follows standard practice of omitting default ports.Run the following script to verify that niquests consistently omits default ports in URLs:
🏁 Script executed:
#!/bin/bash # Description: Check if there are any other URL assertions that might need similar updates # Search for URL assertions that might still include :443 or :80 rg -A 2 -B 2 'assert.*url.*:443|assert.*url.*:80' --type pyLength of output: 61
Confirm default port omission in URL assertions
All URL assertions have been checked—no other tests reference
:443or:80. Omitting the default HTTPS port aligns with niquests’ standard behavior. Tests are updated correctly.test/ably/rest/restrequest_test.py (2)
1-3: LGTM!The import changes correctly replace httpx with niquests and respx with responses, aligning with the PR objectives.
101-101: LGTM!Exception types have been correctly updated from httpx to niquests equivalents:
httpx.ReadTimeout→niquests.ReadTimeouthttpx.ConnectionError→niquests.ConnectionErrorAlso applies to: 113-113, 131-131
test/conftest.py (1)
1-172: Well-designed async support implementation!The
NiquestsMockclass and module patching approach effectively enables async HTTP mocking with niquests while maintaining compatibility with the responses API. The implementation correctly handles both sync and async adapters.🧰 Tools
🪛 Ruff (0.11.9)
136-136: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
137-137: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
test/ably/rest/restauth_test.py (7)
1-12: Migration to niquests and responses looks correct.The import changes properly replace
httpx.AsyncClientwithniquests.AsyncSessionand add the necessaryresponsesandjsonimports for the new mocking framework.
88-88: AsyncSession mocking updated correctly.The mock patches have been properly updated from
httpx.AsyncClient.sendtoniquests.AsyncSession.sendto align with the new HTTP client library.Also applies to: 101-101, 113-113
466-482: Verify responses.get usage for query string handling.The migration from respx routes to responses.get appears correct, but verify that URL parameter handling works as expected.
394-433:❓ Verification inconclusive
Verify GET request parameter handling in responses callback.
The migration changes how GET request parameters are handled. Ensure that the URL construction and parameter extraction work correctly with the responses library.
🏁 Script executed:
#!/bin/bash # Description: Verify how responses handles GET request parameters and URL construction # Check for other examples of GET requests with parameters in responses rg -A 10 "responses\.add_callback.*GET" # Look for URL parameter handling patterns rg -A 5 -B 5 "request\.url.*params"Length of output: 79
Verify GET request parameter handling in responses callback
The migration changes how GET parameters are appended and parsed. Since we couldn’t find other examples of
responses.add_callback(…, "GET")in the codebase, please manually confirm that:
- The query string is constructed exactly as expected (e.g.
?foo=token&spam=eggs) and matches the order/encoding used in your client.- The
responsescallback receives the full URL—including all params—and doesn’t drop or reorder any entries.- You’ve added (or consider adding) an explicit assertion on
request.urlor parsedrequest.paramsinsidecall_backto catch any discrepancies.
523-686:✅ Verification successful
Review the comprehensive TestRenewToken migration.
This is a major refactoring from respx to responses. The key changes include:
- Using
responses.post()andresponses.get()instead of respx routes- Changing callback return format to tuples
- Using
responses.start()/stop()instead of respx context management- Call count tracking with
route.call_countinstead of respx mechanismsThe implementation looks correct, but needs verification that all HTTP interactions work as expected.
Verify the token renewal flow works correctly with the new mocking:
🏁 Script executed:
#!/bin/bash # Description: Check for consistent patterns in responses usage across the test suite # Find all uses of responses.start/stop to ensure consistent cleanup rg -A 3 -B 3 "responses\.(start|stop|reset)" # Check for call_count usage patterns rg -A 3 -B 3 "\.call_count" # Verify JSON response handling in callbacks rg -A 5 "json\.dumps.*return"Length of output: 19720
Mocking usage consistency verified
I checked the
responses.start()/stop()/reset(),.call_countchecks, andjson.dumpsreturn patterns across the test suite and found the same mocking strategy in other REST tests (restrequest_test.py,restpaginatedresult_test.py, etc.). The refactor from respx to responses for token renewal flows is consistent and correctly mocks all HTTP interactions.No further changes needed. Ready to merge.
349-382:✅ Verification successful
Verify the responses callback implementation.
The migration from respx to responses changes the callback signature and return format. The callback now returns a tuple
(status_code, headers, body)instead of an httpx Response object.Verify that the request body parsing works correctly with the new callback format:
🏁 Script executed:
#!/bin/bash # Description: Check how responses library handles request bodies in callbacks # and verify the parse_qs usage is correct # Search for similar callback patterns in the codebase rg -A 10 -B 5 "responses\.add_callback" # Check if there are other tests using parse_qs with request.body rg -A 5 -B 5 "parse_qs.*request\.body"Length of output: 9014
Callback implementation and form‐body parsing verified
The
responses.add_callbacksignature here matches other tests (returning(status_code, headers, body)), andparse_qs(request.body)correctly parses the URL‐encoded form body ({'foo': ['token'], 'spam': ['eggs']}). No changes are needed.
616-616:✅ Verification successful
Verify responses assert_call_count usage.
The migration introduces
responses.assert_call_count()which should be verified to ensure it works correctly with the test expectations.Also applies to: 624-624
🏁 Script executed:
#!/bin/bash # Description: Check if responses.assert_call_count is the correct method # Search for documentation or examples of assert_call_count usage rg -A 5 -B 5 "assert_call_count" # Check if this should be a different assertion method rg -A 5 "responses\.assert"Length of output: 1762
🏁 Script executed:
#!/bin/bash # Find how `responses` is imported in the test file rg -n "import responses" test/ably/rest/restauth_test.pyLength of output: 78
🏁 Script executed:
#!/bin/bash rg -n -C3 "assert_call_count" test/conftest.pyLength of output: 202
🏁 Script executed:
#!/bin/bash python3 - << 'EOF' import responses # Check if assert_call_count is provided by the responses library exists = hasattr(responses, "assert_call_count") print(f"assert_call_count exists: {exists}") if exists: print(responses.assert_call_count) EOFLength of output: 133
🏁 Script executed:
#!/bin/bash # Display lines 1-200 of test/conftest.py to inspect the responses fixture setup sed -n '1,200p' test/conftest.pyLength of output: 5308
responses.assert_call_count is correctly configured and requires no change
Test setup in test/conftest.py monkey-patches responses.assert_call_count to the NiquestsMock instance, so calls like
responses.assert_call_count(f"{self.base_url}/channels/{self.channel}/messages", 1)will work as intended. No updates are needed here.
|
According to local tests, there's only one failure. from my observation it fails at the same happen in the main branch. something may have changed with the staging server. regards, |
Hello,
I had the opportunity to discuss with one maintainer of Ably-Python in early 2024 (jawah/niquests#83)
At the time, we discussed how httpx is unstable and have significant latencies issue in a concurrent environment (either sync or async). Today those issues are still there and by the look of it (upstream httpx repo) we can see how people are starting to abandon the idea of getting improvements. I am fairly certain your customers have issue regarding that topic.
That PR swap httpx with Niquests.
Niquests is a drop-in replacement for Requests that is no longer under feature freeze and have both sync & async interfaces. And of course, native support for HTTP/2 and HTTP/3.
In the linked discussion, we established that Niquests is stable and performant today.
I waited until now before working on a PR because I wanted to get enough traffic before. We reached 30k pull a day without a single incident just recently.
The PR brings the following changes:
Regards,
FWIW: Stats (httpx, requests, niquests) against ably-api /time jawah/niquests#83 (comment)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style