-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Access] Add unit test for websocket controller #6762
[Access] Add unit test for websocket controller #6762
Conversation
* Add unit test for websocket controller * Add mock for websocket connection * Add mock for data provider * Add mock for data provider factory * Add mock for websocket connection The WebSocket Controller interacts with: 1. Data Provider: Supplies data to the controller. 2. WebSocket Connection: Handles communication with the client. To properly test the controller's logic, we mock these interactions. Since the controller runs two parallel routines (reader and writer), the tests also ensure both can shut down cleanly. A done channel is used in the tests to coordinate this process.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6762 +/- ##
==========================================
- Coverage 42.10% 40.99% -1.12%
==========================================
Files 1524 2100 +576
Lines 139324 184862 +45538
==========================================
+ Hits 58665 75785 +17120
- Misses 75746 102708 +26962
- Partials 4913 6369 +1456
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There are no tests for error cases. I think we should add them as part of #6642. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After first round of review - looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few small comments. otherwise looks good
return | ||
} | ||
c.logger.Debug().Err(err).Msg("error reading message from client") | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what type of errors are we allowing here? does it make sense to just close the connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be done later in PR #6798
It will notify the client, that something went wrong with the incoming message
Co-authored-by: Peter Argue <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one looks good to me!
@Guitarheroua can you fix these conflicts. after that it's ready to merge |
84f9c4c
to
a4e07a3
Compare
…lachyn/6635-tests-for-websocket-controller
Closes: #6635
The WebSocket Controller interacts with:
To properly test the controller's logic, we mock these interactions. Since the controller runs two parallel routines (reader and writer), the tests also ensure both can shut down cleanly.
A done channel is used in the tests to coordinate this process.