chore: remove async/await from websocket-client send operations and convert the test to use synchronous library#1918
Conversation
Harsh16gupta
requested review from
Adi-204,
asyncapi-bot-eve,
derberg and
magicmatatjahu
as code owners
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces async function declarations and await expressions with synchronous definitions and direct calls in generated Python WebSocket client code and tests; also simplifies discriminator computation by replacing an early-return guard with a conditional assignment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/components/src/components/SendOperations.js (1)
30-37:⚠️ Potential issue | 🟡 MinorFix Python docstrings to reference correct WebSocket type.
The docstring incorrectly references
websockets.WebSocketCommonProtocol(async library), but the codebase uses the synchronouswebsocket-clientlibrary (import websocketin DependencyProvider.js). The socket parameter should be typed aswebsocket.WebSocketApp. This same error also appears inpackages/templates/clients/websocket/python/components/Send.jsline 19.Proposed changes
SendOperations.js (line 36):
- socket (websockets.WebSocketCommonProtocol): The WebSocket to send through. + socket (websocket.WebSocketApp): The WebSocket to send through.Send.js (line 19):
- socket (websockets.WebSocketCommonProtocol): The WebSocket to send through. + socket (websocket.WebSocketApp): The WebSocket to send through.packages/templates/clients/websocket/python/components/Send.js (1)
12-27:⚠️ Potential issue | 🟡 MinorFix incorrect WebSocket type in the
_sendmethod docstring.The docstring references
websockets.WebSocketCommonProtocol(async library), but this template uses the synchronouswebsocket-clientlibrary and passesself.ws_app, which is aWebSocketAppinstance. Update the type annotation to match:Suggested change
- socket (websockets.WebSocketCommonProtocol): The WebSocket to send through. + socket (websocket.WebSocketApp): The WebSocket to send through.
🧹 Nitpick comments (2)
packages/templates/clients/websocket/python/components/Send.js (1)
3-33: Add JSDoc for the Send component (params/returns/errors).The exported
Sendfunction lacks JSDoc, which is required for JS files. Please include param types, return type, and error conditions (even if “none”).🧾 Suggested JSDoc
export function Send({ sendOperations }) { +/** + * Renders the Python _send helper when send operations exist. + * + * `@param` {{ sendOperations: Array<Object> }} props - Component props. + * `@returns` {JSX.Element|null} Rendered helper code or null when no operations exist. + * `@throws` {Error} Does not throw. + */ +export function Send({ sendOperations }) {packages/components/src/components/SendOperations.js (1)
105-113: Complete JSDoc for SendOperations with return and error info.The existing JSDoc omits
@returnsand error conditions, which are required for JS files.🧾 Suggested JSDoc completion
/** * Component for rendering WebSocket send operation methods. * Generates both static and instance methods for sending messages through WebSocket connections. * * `@param` {Object} props - Component props. * `@param` {SupportedLanguage} props.language - The target programming language. * `@param` {Array<Object>} props.sendOperations - Array of send operations from AsyncAPI document. * `@param` {string} props.clientName - The name of the client class. + * `@returns` {Array<JSX.Element>|null} Rendered operation methods or null when there are no send operations. + * `@throws` {Error} Does not throw. */ export function SendOperations({ language, sendOperations, clientName }) {
|
@Adi-204 I tried to look at the failed test but can't figure out why it is failing, please guide. All executed checks (SonarCloud, CodeRabbit) are passing. Please let me know if you’d like me to rerun anything after approval. |
Adi-204
left a comment
Adi-204
left a comment
There was a problem hiding this comment.
@Harsh16gupta acceptance test for python are failing please look into it and in the PR desc I was expecting screenshots of running clients with python example.py command and not the test results.
@Adi-204 thanks for the review, and sorry for the misunderstanding. |
|
@Adi-204 I've investigated the failing Python acceptance test and found. The IssueThe acceptance test file (
In await HoppscotchEchoWebSocketClient.send_echo_message_static(expected_outgoing_message, websocket)But:
Proposed SolutionI see two approaches: Option 1: Send directly in the test (simpler) async def handler(websocket):
message = expected_outgoing_message
if isinstance(message, dict):
message = json.dumps(message)
await websocket.send(message) # Use websockets library directlyThis replicates the client's message formatting logic without mixing libraries. Option 2: Refactor test to use websocket-client QuestionWhich approach would you prefer? |
|
hey @Harsh16gupta first let's wait for https://asyncapi.slack.com/archives/C072JMTJ85Q/p1770225233478619 |
|
@Adi-204 Sorry for not updating the screenshots until now. I was using WSL, and the command During the current debugging (after my PR fix), I found that the Python generated client.py (for postman) does not include: Because of this, the runtime crashes when on_message tries to access these attributes. To verify this, I manually added these lines to the generated client.py: 
After adding them, the runtime error was resolved. This confirmed that the issue was caused by missing initialization in the generated client. The main question then was why this is generated for Hoppscotch but not for Postman. While tracing the generator logic, in the file after With this change, the constructor always initializes the required fields and uses an empty list when there are no receive operations. 
Waiting for your approval before committing all these changes. |
So the main problem for the failing of the test was that we are using But send_echo_message_static() expects a socket from websocket-client library (sync) So instead of calling the client’s send method from the async test server, the test sends the message directly using the websockets library, while replicating the same message-formatting logic used by the client: so I changed the test server with the option 1 (to use the same logic ) and then ran the test locally What I concluded from this:
so what do you think, is the approach good? |
Dart template
Js template
While testing the JavaScript template, I found a small issue in the README.
The README currently mentions the following path: Java template
Python template
All templates (Dart, JavaScript, Java, and Python) were verified by running them locally after applying the discussed fixes, and all are working correctly. Please let me know if you’re okay with me committing all the remaining changes based on this. |
Adi-204
left a comment
Adi-204
left a comment
There was a problem hiding this comment.
@Harsh16gupta I checked your proposed solution option 1 in detail. I don't think we can go with that solution, because basically you are replacing await HoppscotchEchoWebSocketClient.send_echo_message_static with same logic but let's say someone makes changes in send_echo_message_static it will not get tested with acceptance test as you just hardcode logic in acceptance test and it will pass always.
We also have comment Most improtant part of test where we test clients send message which also suggest you have to use generated client to test for send functionality and you are suggesting to remove this?
|
@Adi-204 Thank you for the detailed feedback. I understand your concern now. Option 1 doesn't actually test the client's send method. If someone modifies the client's What's the recommended approach to solve this? Should I:
Please let me know your preference and I'll update the PR accordingly. |
|
@Harsh16gupta I tried to debug locally and find a solution such that we don't need to do a lot of code changes, but that doesn't seem to be easy task. Two options
I checked with option 2 we will have to make a lot of changes and I'm not 100% sure about how much changes needs to done in option 1 as acceptance test was setup by @derberg so he is the best person to know about that. Maybe you can try analyze acceptance test infra, @derberg wdyt 🤔 ? approach should we go for? |
|
@Adi-204 That makes sense, thanks for digging into it. 1st one
2nd one
Just a suggestion, happy to keep it in this PR |
|
@Harsh16gupta yeah the 1st one you mention is missing but just adding that won't enable clients to work and scope of issue is to solve not working clients. |
|
Hey maintainers I tried Option 1 (it worked and was much simpler than Option 2), as discussed. Please let me know if this direction makes sense or if you’d prefer a different approach. |
|
@Harsh16gupta no need to prepare docs and stuff, always just directly drop your research in github comments. Because here we don't have much options I would recommend you can proceed with your proposed solution. But remember if you are doing changes in acceptance test we need review/approval from @derberg as he introduce this testing and have much more idea/vision than me. He won't review docs, so just push your changes in this PR! |
|
Thanks for the guidance, Adi! |
Harsh16gupta
force-pushed
the
bug/async-await-1857-clean
branch
from
e97ead8 to
e3745d8
Compare
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)
packages/templates/clients/websocket/python/components/Send.js (1)
13-27:⚠️ Potential issue | 🟡 MinorCorrect removal of
async/await— but generated docstring references the wrong library type.The sync conversion is correct. However, Line 19 still references
websockets.WebSocketCommonProtocol, which is the type from the asyncwebsocketslibrary. Since this PR switches to the synchronouswebsocket-clientlibrary, the type hint in the generated docstring should reflect that (e.g.,websocket.WebSocketor a more generic description).This same stale type reference also appears in
SendOperations.js(Line 36).📝 Suggested fix
- socket (websockets.WebSocketCommonProtocol): The WebSocket to send through. + socket: The WebSocket connection to send through.
🤖 Fix all issues with AI agents
In `@packages/templates/clients/websocket/test/python/test_acceptance.py`:
- Line 6: The file test_acceptance.py has an unused import "import json"; remove
that import line from the top of the file (delete the "import json" statement)
so the test file no longer contains an unused symbol.
🧹 Nitpick comments (3)
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js (1)
4-9: JSDoc@returnsis now stale — the component never returnsnull.Since the early return was removed, this component always renders a
<Text>element. The|nullin the return type is no longer accurate.📝 Suggested fix
- * `@returns` {React.Element|null} Rendered initialization code or null. + * `@returns` {React.Element} Rendered initialization code for receive operation discriminators.packages/templates/clients/websocket/python/components/Send.js (1)
3-6: Missing JSDoc on exportedSendcomponent.As per coding guidelines, JS functions should have JSDoc comments including parameter types and return values.
📝 Suggested JSDoc
+/** + * Component that generates the internal `_send` helper for the Python WebSocket client. + * + * `@param` {Object} props - Component properties. + * `@param` {Array<Object>} props.sendOperations - Send operations from the AsyncAPI document. + * `@returns` {React.Element|null} Rendered Python `_send` method or null if no send operations. + */ export function Send({ sendOperations }) {packages/templates/clients/websocket/test/python/test_acceptance.py (1)
60-83: Consider checking the return value ofserver_ready.wait().
server_ready.wait(timeout=5)returnsFalseif the timeout expires before the event is set, but the return value is not checked. If the server fails to start, the test will proceed and produce a confusing failure instead of a clear startup error.Also, regarding the static analysis hint (S104: binding to all interfaces on Line 73): this is expected in a containerized test environment where Microcks needs to reach this server.
📝 Suggested fix
- server_ready.wait(timeout=5) + assert server_ready.wait(timeout=5), "WebSocket server failed to start within 5 seconds"
packages/templates/clients/websocket/test/python/test_acceptance.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/templates/clients/websocket/test/python/test_acceptance.py`:
- Around line 71-82: The server thread currently calls serve(handler, "0.0.0.0",
port) but never starts the accept loop, so update run_server to call
server.serve_forever() after creating the server (and before waiting for
test_complete) so the WebSocket server actually accepts connections; ensure you
still set server_ready.set() after server creation and keep server.shutdown()
for cleanup so server_thread (created using run_server) will block servicing
connections until test_complete.is_set().
|
Addressing CodeRabbit feedback. |
Harsh16gupta
changed the title
|
Hi maintainers, all requested changes are done. Templates are verified (screenshots attached in the PR description), and acceptance tests are also passing locally. |
Adi-204
left a comment
Adi-204
left a comment
There was a problem hiding this comment.
@Harsh16gupta left few comment. In PR desc remove Dart and JS client running as we have not change anything in those, rather add screenshots of all different clients generated in python like https://github.com/asyncapi/generator/tree/master/packages/templates/clients/websocket/python#client-for-slack-with-auto-routing
It looks good, but I would recommend to drop a comment after addressing changes about how your approach in acceptance test infra works same as before.
packages/templates/clients/websocket/python/test/components/__snapshots__/Requires.test.js.snap
Show resolved
Hide resolved
packages/templates/clients/websocket/test/python/test_acceptance.py
Outdated
Show resolved
Hide resolved
…857-clean Merge remote-tracking branch 'upstream/master' into bug/async-await-1857-clean
|
How the Synchronous Test Approach Works the Same as BeforeSame Validation Flow:
What Changed:
The Critical Improvement:Old Version: async def handler(websocket):
await HoppscotchEchoWebSocketClient.send_echo_message_static(
expected_outgoing_message,
websocket
)Incompatible with sync client New Version: def handler(websocket):
HoppscotchEchoWebSocketClient.send_echo_message_static(
expected_outgoing_message,
websocket
)Now working correctly:
ThreadingOld approach: Used async/await for concurrency server = await websockets.serve(handler, port=port)
await asyncio.sleep(1)
# ... poll Microcks ...
await asyncio.sleep(2)New approach: Uses threading for concurrency def run_server():
with serve(handler, "0.0.0.0", port) as server:
server_ready.set()
server.serve_forever() # Blocks this thread
server_thread = threading.Thread(target=run_server, daemon=True)
server_thread.start()
# Main thread continues to poll MicrocksBoth provide concurrency, just different mechanisms:
Result: Same validation logic, correct sync/sync compatibility. Both test client functionality, message formatting, and AsyncAPI compliance identically. |
3 participants
Description
Fixes async/sync mismatch in the Python WebSocket client template. The template used
async/awaitwith the synchronouswebsocket-clientlibrary, causing runtime errors.This PR:
async/awaitfrom send operationswebsockets.synclibraryFixes #1857
Screenshots
Templates
Python WebSocket Client
Client for Slack
Client for Slack with Auto-Routing
Acceptance test
Summary by CodeRabbit
Refactor
Tests