build: upgrade websockets dependency to support v14-v16#398
Open
omidroshani wants to merge 3 commits intoautoscrape-labs:mainfrom
Open
build: upgrade websockets dependency to support v14-v16#398omidroshani wants to merge 3 commits intoautoscrape-labs:mainfrom
omidroshani wants to merge 3 commits intoautoscrape-labs:mainfrom
Conversation
Broadens the websockets version constraint from ^14 (>=14, <15) to >=14,<17, resolving dependency conflicts with packages requiring websockets >=15 (e.g. fastmcp). Closes autoscrape-labs#388
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
|
hey @omidroshani , thanks for this! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_connection_handler.py (1)
384-392: Test name says “concurrent”, but execution is sequential.This loop runs
create_command_futuresequentially, so it validates uniqueness across repeated calls, not true concurrency. Please either rename the test or introduce real concurrent invocation.Minimal clarity fix (rename)
-async def test_command_ids_unique_under_concurrent_calls(connection_handler): +async def test_command_ids_unique_across_multiple_calls(connection_handler):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_connection_handler.py` around lines 384 - 392, The test test_command_ids_unique_under_concurrent_calls currently calls connection_handler._command_manager.create_command_future sequentially; change it to actually invoke create_command_future concurrently (or rename the test). To fix by making it concurrent, wrap the calls in async tasks and await them together (e.g., create a list of asyncio.create_task(...) or use asyncio.gather to call connection_handler._command_manager.create_command_future(cmd) 100 times) so IDs are assigned under true concurrency, then collect cmd['id'] values and assert len(set(assigned_ids)) == 100; alternatively, if you prefer the minimal change, rename the test to test_command_ids_unique_under_sequential_calls and leave the loop as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_connection_handler.py`:
- Around line 363-380: The test test_single_receiver_task_created cancels the
receiver task (first_task.cancel()) but never awaits it, which can leave a
pending task; update the test to await the canceled task (await first_task) and
handle the expected cancellation (e.g., catching asyncio.CancelledError) after
calling first_task.cancel() so _receive_task is properly cleaned up and no
pending-task warnings occur; locate usage of
connection_handler_closed._receive_task and the final first_task.cancel() in the
test and replace the bare cancel with cancel then await/handle the cancellation.
---
Nitpick comments:
In `@tests/test_connection_handler.py`:
- Around line 384-392: The test test_command_ids_unique_under_concurrent_calls
currently calls connection_handler._command_manager.create_command_future
sequentially; change it to actually invoke create_command_future concurrently
(or rename the test). To fix by making it concurrent, wrap the calls in async
tasks and await them together (e.g., create a list of asyncio.create_task(...)
or use asyncio.gather to call
connection_handler._command_manager.create_command_future(cmd) 100 times) so IDs
are assigned under true concurrency, then collect cmd['id'] values and assert
len(set(assigned_ids)) == 100; alternatively, if you prefer the minimal change,
rename the test to test_command_ids_unique_under_sequential_calls and leave the
loop as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 506ab5d0-495f-4cb4-ac5b-cf36e4ebee54
📒 Files selected for processing (1)
tests/test_connection_handler.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_connection_handler.py (2)
387-395: Test name and behavior are mismatched (concurrentvs sequential loop).At Line 387, the test title says “concurrent calls”, but Line 389–Line 392 executes sequentially. Rename it (or make it truly concurrent) to keep intent accurate.
Minimal rename for accuracy
-async def test_command_ids_unique_under_concurrent_calls(connection_handler): +async def test_command_ids_unique_across_multiple_calls(connection_handler):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_connection_handler.py` around lines 387 - 395, The test name claims concurrency but the loop calls connection_handler._command_manager.create_command_future(cmd) sequentially; either rename test_command_ids_unique_under_concurrent_calls to something like test_command_ids_unique_under_sequential_calls or modify the test to perform concurrent invocations (e.g., schedule many create_command_future calls via asyncio.gather or similar) so create_command_future is exercised concurrently; update references to the test function name and keep assertions on cmd['id'] uniqueness and count unchanged.
339-360: Strengthen concurrent-send assertions to validate response correctness, not just count.Right now the test only verifies total results and send count. Adding response-shape checks would better guard against mismatched futures/results under concurrency.
Suggested assertion hardening
assert len(results) == 5 + assert {result['id'] for result in results} == {1, 2, 3, 4, 5} + assert {result['result'] for result in results} == { + 'ok1', 'ok2', 'ok3', 'ok4', 'ok5' + } assert connection_handler._ws_connection.send.await_count == 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_connection_handler.py` around lines 339 - 360, The test test_concurrent_send_does_not_raise currently only checks length and send await_count; enhance it to verify each returned result matches the expected payload and that each send call carried the correct command by asserting the results list equals ['ok1','ok2',... ] (or the full parsed JSON per command) and/or asserting _ws_connection.send was called with the expected serialized commands in the same order; locate the test body and update assertions around connection_handler.execute_command, connection_handler._ws_connection.send, and connection_handler._command_manager.create_command_future to compare actual responses (parsed JSON or result values) against the expected per-command responses rather than only checking counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_connection_handler.py`:
- Around line 387-395: The test name claims concurrency but the loop calls
connection_handler._command_manager.create_command_future(cmd) sequentially;
either rename test_command_ids_unique_under_concurrent_calls to something like
test_command_ids_unique_under_sequential_calls or modify the test to perform
concurrent invocations (e.g., schedule many create_command_future calls via
asyncio.gather or similar) so create_command_future is exercised concurrently;
update references to the test function name and keep assertions on cmd['id']
uniqueness and count unchanged.
- Around line 339-360: The test test_concurrent_send_does_not_raise currently
only checks length and send await_count; enhance it to verify each returned
result matches the expected payload and that each send call carried the correct
command by asserting the results list equals ['ok1','ok2',... ] (or the full
parsed JSON per command) and/or asserting _ws_connection.send was called with
the expected serialized commands in the same order; locate the test body and
update assertions around connection_handler.execute_command,
connection_handler._ws_connection.send, and
connection_handler._command_manager.create_command_future to compare actual
responses (parsed JSON or result values) against the expected per-command
responses rather than only checking counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20d369a3-cac5-4b74-a3a3-e089cec41572
📒 Files selected for processing (1)
tests/test_connection_handler.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
websocketsversion constraint from^14(>=14, <15) to>=14, <17websockets >=15(e.g.fastmcp 3.1.0)Closes #388
Test plan
ConnectionClosed,ClientConnection,State,connect(),send(),recv(),ping(),close()APIs are unchanged in v16Summary by CodeRabbit
Chores
Tests