fix: use blocking sleep instead of asyncio in tests#616
Conversation
WalkthroughAdds waiter utilities for async and sync polling in tests, updates REST channel publish tests to use waiter-based checks instead of sleeps, and extends unasync token replacement to map assert_waiter to assert_waiter_sync. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test (pytest)
participant C as REST Channel
participant W as Waiter (assert_waiter)
participant A as Ably REST API
T->>C: publish(data or messages)
C-->>T: HTTP response (201)
T->>W: await assert_waiter(check_data)
loop poll until condition true or timeout
W->>A: GET latest message / history
A-->>W: Message payload
W-->>W: verify encoding/data/type
end
W-->>T: success or raise TimeoutError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
So, tests were failing because of this issue? Since sleep statements often make tests flaky, I asked cursor for kotlin assertWaiter equivalent here
import asyncio
from typing import Callable, Awaitable
async def assert_waiter(block: Callable[[], Awaitable[bool]], timeout_ms: int = 10_000) -> None:
timeout_seconds = timeout_ms / 1000.0
try:
await asyncio.wait_for(_poll_until_success(block), timeout=timeout_seconds)
except asyncio.TimeoutError:
raise asyncio.TimeoutError(f"Condition not met within {timeout_ms}ms")
async def _poll_until_success(block: Callable[[], Awaitable[bool]]) -> None:
while True:
success = await block()
if success:
break
await asyncio.sleep(0.1)
Usage for the same =>
async def test_something():
async def condition_checker():
return some_condition_is_met()
await assert_waiter(condition_checker)
await assert_waiter(condition_checker, timeout_ms=5000)
I guess this will be similar to waiting library -> https://pypi.org/project/waiting/
ttypic
force-pushed
the
fix/try-to-fix-broken-tests
branch
from
a05d4f6 to
f8850ae
Compare
github-actions
bot
temporarily deployed
to
staging/pull/616/features
ttypic
force-pushed
the
fix/try-to-fix-broken-tests
branch
from
f8850ae to
ac541c0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/616/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/ably/utils.py (1)
209-229: Use monotonic clock for timeouts in sync waitertime.time() is wall-clock and can go backwards/forwards. Use time.monotonic() for elapsed timing.
- start_time = time.time() + start_time = time.monotonic() @@ - if time.time() - start_time >= timeout: + if time.monotonic() - start_time >= timeout: raise TimeoutError(f"Condition not met within {timeout}s")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ably/scripts/unasync.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/scripts/unasync.py
🧰 Additional context used
🧬 Code graph analysis (1)
test/ably/rest/restchannelpublish_test.py (3)
ably/rest/channel.py (3)
ably(143-144)publish(106-132)history(32-39)test/ably/utils.py (1)
assert_waiter(186-198)ably/http/paginatedresult.py (2)
status_code(116-117)items(56-57)
🪛 Ruff (0.12.2)
test/ably/utils.py
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
226-226: Avoid specifying long messages outside the exception class
(TRY003)
test/ably/rest/restchannelpublish_test.py
405-405: Use of assert detected
(S101)
411-411: Function definition does not bind loop variable encoding
(B023)
412-412: Function definition does not bind loop variable encoding
(B023)
413-413: Function definition does not bind loop variable msg_data
(B023)
415-415: Function definition does not bind loop variable msg_data
(B023)
421-421: Use of assert detected
(S101)
426-426: Function definition does not bind loop variable expected_value
(B023)
426-426: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
426-426: Function definition does not bind loop variable expected_type
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.9)
- GitHub Check: check (3.7)
- GitHub Check: check (3.8)
- GitHub Check: check (3.10)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
🔇 Additional comments (1)
test/ably/rest/restchannelpublish_test.py (1)
22-22: assert_waiter mapping confirmed
The unasync token map in ably/scripts/unasync.py includesassert_waiter → assert_waiter_sync, so sync-converted tests will useassert_waiter_syncinstead of the async variant.
ttypic
force-pushed
the
fix/try-to-fix-broken-tests
branch
from
ac541c0 to
ab9306d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/616/features
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/ably/utils.py (1)
213-236: Fix CI: avoid bare except in sync waiterSame E722 issue in the sync path; keep retries while not masking system-exiting exceptions.
while True: try: success = block() if success: - break - except: - pass + break + except Exception: + pass if time.time() - start_time >= timeout: raise TimeoutError(f"Condition not met within {timeout}s") time.sleep(0.1)
♻️ Duplicate comments (3)
test/ably/utils.py (1)
186-199: Match docstring: raise built-in TimeoutError and suppress chainingRaise the built-in TimeoutError (not asyncio.TimeoutError) and use
from Noneto satisfy B904 and align with the docstring.try: - await asyncio.wait_for(_poll_until_success(block), timeout=timeout) - except asyncio.TimeoutError: - raise asyncio.TimeoutError(f"Condition not met within {timeout}s") + await asyncio.wait_for(_poll_until_success(block), timeout=timeout) + except asyncio.TimeoutError: + raise TimeoutError(f"Condition not met within {timeout}s") from Nonetest/ably/rest/restchannelpublish_test.py (2)
404-417: Harden polling: guard empty list, bind loop vars, add client timeoutPrevents IndexError during eventual consistency, avoids late-binding issues (B023), and keeps polls snappy.
- async def check_data(): - async with httpx.AsyncClient(http2=True) as client: - r = await client.get(url, auth=auth) - item = r.json()[0] - encoding_is_correct = item.get('encoding') == encoding - if encoding == 'json': - return encoding_is_correct and json.loads(item['data']) == json.loads(msg_data) - else: - return encoding_is_correct and item['data'] == msg_data + async def check_data(encoding=encoding, msg_data=msg_data): + async with httpx.AsyncClient(http2=True, timeout=2.0) as client: + r = await client.get(url, auth=auth) + items = r.json() + if not items: + return False + item = items[0] + encoding_is_correct = item.get('encoding') == encoding + if encoding == 'json': + return encoding_is_correct and json.loads(item['data']) == json.loads(msg_data) + else: + return encoding_is_correct and item['data'] == msg_data
420-428: Harden history check: guard empty, bind vars, and use isinstanceAvoids empty history access, captures expected values, and replaces direct type equality per E721.
- async def check_history(): - history = await channel.history() - message = history.items[0] - return message.data == expected_value and type(message.data) == type_mapping[expected_type] + async def check_history(expected_value=expected_value, expected_type=expected_type): + history = await channel.history() + items = history.items + if not items: + return False + message = items[0] + return message.data == expected_value and isinstance(message.data, type_mapping[expected_type])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ably/scripts/unasync.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/scripts/unasync.py
🧰 Additional context used
🧬 Code graph analysis (1)
test/ably/rest/restchannelpublish_test.py (2)
ably/rest/channel.py (3)
ably(143-144)publish(106-132)history(32-39)test/ably/utils.py (1)
assert_waiter(186-198)
🪛 Ruff (0.12.2)
test/ably/rest/restchannelpublish_test.py
405-405: Use of assert detected
(S101)
411-411: Function definition does not bind loop variable encoding
(B023)
412-412: Function definition does not bind loop variable encoding
(B023)
413-413: Function definition does not bind loop variable msg_data
(B023)
415-415: Function definition does not bind loop variable msg_data
(B023)
421-421: Use of assert detected
(S101)
426-426: Function definition does not bind loop variable expected_value
(B023)
426-426: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
426-426: Function definition does not bind loop variable expected_type
(B023)
test/ably/utils.py
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
207-207: Do not use bare except
(E722)
207-208: try-except-pass detected, consider logging the exception
(S110)
229-229: Do not use bare except
(E722)
229-230: try-except-pass detected, consider logging the exception
(S110)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Linting check
test/ably/utils.py
[error] 207-207: flake8: E722 do not use bare 'except' during 'poetry run flake8' step.
[error] 229-229: flake8: E722 do not use bare 'except' during 'poetry run flake8' step.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
- GitHub Check: check (3.8)
- GitHub Check: check (3.7)
- GitHub Check: check (3.9)
🔇 Additional comments (1)
test/ably/rest/restchannelpublish_test.py (1)
22-22: Verified mapping forassert_waiter
assert_waiter→assert_waiter_syncis defined in_TOKEN_REPLACEwithinscripts/unasync.py.
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Seems we have other flaky tests, current test_interoperability working just fine.
…remove temporary delays
ttypic
force-pushed
the
fix/try-to-fix-broken-tests
branch
from
ab9306d to
4f08dc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/ably/utils.py (2)
186-199: Raise built‑in TimeoutError and suppress chaining to match docstringUnify with sync variant; avoid surfacing asyncio-specific type.
- try: - await asyncio.wait_for(_poll_until_success(block), timeout=timeout) - except asyncio.TimeoutError: - raise asyncio.TimeoutError(f"Condition not met within {timeout}s") + try: + await asyncio.wait_for(_poll_until_success(block), timeout=timeout) + except asyncio.TimeoutError as err: + raise TimeoutError(f"Condition not met within {timeout}s") from None
201-211: Don’t swallow cancellations; re‑raise CancelledErrorSwallowing CancelledError breaks task cancellation semantics and can hang shutdowns.
async def _poll_until_success(block: Callable[[], Awaitable[bool]]) -> None: while True: try: success = await block() if success: break - except Exception: - pass + except asyncio.CancelledError: + raise + except Exception: + pass await asyncio.sleep(0.1)test/ably/rest/restchannelpublish_test.py (2)
404-417: Avoid flakiness: guard empty list, bind loop vars, add per‑request timeoutPrevents IndexError before persistence; avoids late-binding issues; keeps polls snappy.
- async def check_data(): - async with httpx.AsyncClient(http2=True) as client: - r = await client.get(url, auth=auth) - item = r.json()[0] - encoding_is_correct = item.get('encoding') == encoding - if encoding == 'json': - return encoding_is_correct and json.loads(item['data']) == json.loads(msg_data) - else: - return encoding_is_correct and item['data'] == msg_data + async def check_data(encoding=encoding, msg_data=msg_data): + async with httpx.AsyncClient(http2=True, timeout=2.0) as client: + r = await client.get(url, auth=auth) + items = r.json() + if not items: + return False + item = items[0] + encoding_is_correct = item.get('encoding') == encoding + if encoding == 'json': + return encoding_is_correct and json.loads(item['data']) == json.loads(msg_data) + else: + return encoding_is_correct and item['data'] == msg_data
420-428: History polling: guard empties, bind vars, and prefer isinstancePrevents IndexError and E721; robust to subclasses.
- async def check_history(): - history = await channel.history() - message = history.items[0] - return message.data == expected_value and type(message.data) == type_mapping[expected_type] + async def check_history(expected_value=expected_value, expected_type=expected_type): + history = await channel.history() + items = history.items + if not items: + return False + message = items[0] + return message.data == expected_value and isinstance(message.data, type_mapping[expected_type])
🧹 Nitpick comments (2)
test/ably/utils.py (1)
213-236: Use monotonic clock for timeout accountingAvoids skew from system clock changes.
- start_time = time.time() + start_time = time.monotonic() @@ - if time.time() - start_time >= timeout: + if time.monotonic() - start_time >= timeout: raise TimeoutError(f"Condition not met within {timeout}s")test/ably/rest/restchannelpublish_test.py (1)
404-417: Optional: reuse one AsyncClient across polls to cut connection churnCreate one client and close it in finally; bind into closures.
Example:
client = httpx.AsyncClient(http2=True, timeout=2.0) try: async def check_data(client=client, encoding=encoding, msg_data=msg_data): r = await client.get(url, auth=auth) ... async def check_history(client=client, expected_value=expected_value, expected_type=expected_type): history = await channel.history() ... await assert_waiter(check_data) await assert_waiter(check_history) finally: await client.aclose()Also applies to: 420-428
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ably/scripts/unasync.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ably/scripts/unasync.py
🧰 Additional context used
🧬 Code graph analysis (1)
test/ably/rest/restchannelpublish_test.py (3)
ably/rest/channel.py (3)
ably(143-144)publish(106-132)history(32-39)test/ably/utils.py (1)
assert_waiter(186-198)ably/types/message.py (1)
Message(24-226)
🪛 Ruff (0.12.2)
test/ably/rest/restchannelpublish_test.py
405-405: Use of assert detected
(S101)
411-411: Function definition does not bind loop variable encoding
(B023)
412-412: Function definition does not bind loop variable encoding
(B023)
413-413: Function definition does not bind loop variable msg_data
(B023)
415-415: Function definition does not bind loop variable msg_data
(B023)
421-421: Use of assert detected
(S101)
426-426: Function definition does not bind loop variable expected_value
(B023)
426-426: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
426-426: Function definition does not bind loop variable expected_type
(B023)
test/ably/utils.py
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
207-208: try-except-pass detected, consider logging the exception
(S110)
207-207: Do not catch blind exception: Exception
(BLE001)
229-230: try-except-pass detected, consider logging the exception
(S110)
229-229: Do not catch blind exception: Exception
(BLE001)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.12)
- GitHub Check: check (3.9)
- GitHub Check: check (3.13)
- GitHub Check: check (3.10)
- GitHub Check: check (3.8)
- GitHub Check: check (3.7)
🔇 Additional comments (1)
test/ably/utils.py (1)
1-1: LGTM: imports added for waiter utilitiesNo issues spotted.
Also applies to: 7-9
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
You can double check if some other tests are flaky and create a new issue to fix them
Otherwise LGTM
2 participants
Summary by CodeRabbit
Tests
Chores