test: subscribe param compatibility tests and docs (#17)#355
test: subscribe param compatibility tests and docs (#17)#355julienrbrt merged 3 commits intomainfrom
Conversation
Automatically adds newly opened issues to the EVStack Evolve project board (orgs/evstack/projects/7). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create pkg/rpc/core/events_test.go with table-driven tests covering: Subscribe (valid query, empty query, oversized query, malformed query, max clients exceeded, max subscriptions per client exceeded), Unsubscribe (valid after subscribe, malformed query), and UnsubscribeAll (after subscribe). - Add mockWSConn implementing rpctypes.WSRPCConnection to handle the background goroutine started by Subscribe. - Add doc comment to Subscribe explaining that CometBFT's jsonParamsToArgs normalises map/array param formats to a plain string before this function is called, making it compatible with legacy Tendermint WS clients (#17). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add goroutine behavior coverage that was completely absent: - TestSubscribe_EventDelivery: publish a NewBlock event to the real EventBus and assert WriteRPCResponse is called on the WSConn mock with the correct subscription ID and ResultEvent.Query payload. Uses a buffered channel + select-with-timeout for synchronisation. - TestSubscribe_SlowClient: set CloseOnSlowClient=true, have WriteRPCResponse return an error, publish event, assert TryWriteRPCResponse is called with "slow client" in Error.Data. - TestSubscribe_Cancellation: stop the EventBus to trigger removeAll(nil), assert TryWriteRPCResponse carries "CometBFT exited" reason. Edge case additions: - Boundary query (exactly 512 chars) succeeds — confirms the > not >= condition in the length guard. - Unsubscribe without prior Subscribe returns ErrSubscriptionNotFound. - UnsubscribeAll with no subscriptions returns ErrSubscriptionNotFound. - State verification: NumClientSubscriptions == 0 after Unsubscribe and UnsubscribeAll success cases. Mock fixes: - Replace blanket .Maybe() with .Once() + Run(signal-channel) for goroutine-path assertions; AssertExpectations called in each new test. - .Maybe() retained only on cleanup-path mocks (TryWriteRPCResponse triggered by EventBus Stop in test cleanup for non-cancellation tests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and clarity of the WebSocket event handling in the RPC layer. It introduces a comprehensive suite of tests for subscription and unsubscription functionalities, covering various scenarios from input validation to client behavior. Additionally, it updates the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces comprehensive tests for Subscribe, Unsubscribe, and UnsubscribeAll WebSocket event handlers, significantly improving test coverage. It also adds a documentation comment to the Subscribe function to clarify parameter format compatibility. The new tests cover various scenarios including input validation, client limits, event delivery, slow client handling, and subscription cancellation, which is a great addition to the codebase.
julienrbrt
changed the title
2 participants
Summary
Partially addresses #17 with test coverage and documentation. No code logic changes.
Issue #17 asks: WebSocket subscribe should use plain string query instead of
SubscribeArgsstruct for compatibility with Tendermint WS clients.Finding: CometBFT v0.38's
jsonParamsToArgsalready normalizes both map ({"query": "..."}) and array (["..."]) param formats into a plain string before reaching ev-abci'sSubscribehandler. The function signature already takesquery string. So the param format issue is handled at the CometBFT layer — no code change needed in ev-abci.What this PR adds:
Subscribe,Unsubscribe,UnsubscribeAllcovering input validation, event delivery, slow client handling, subscription cancellation, and boundary conditionsSubscribeexplaining the param format compatibility situation for future maintainersWhat remains for full #17 closure:
/websocketwas never registered, so WS connections were non-functional regardless — WebSockets: string should be used instead ofSubscribeArgs#17 was not observable until that fix landsTest coverage
WriteRPCResponsecalled with correct payloadCloseOnSlowClient=true+ write error triggers cancellation responseRelated: #18, #20
Test plan
make testpasses with race detectionmake lintpasses with zero warnings🤖 Generated with Claude Code