Fix stream() not working with base-client 0.11#180
Fix stream() not working with base-client 0.11#180Marenz merged 1 commit intofrequenz-floss:v0.x.xfrom
stream() not working with base-client 0.11#180Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures the stream() helper recreates a stream if the prior one isn’t running, and adds a test to exercise calling stream() twice.
- Switches from checking
broadcaster._channel.is_closedto publicbroadcaster.is_runningin_get_stream - Adds a second call to
client.stream()in the test to catch regressions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_client.py | Added a second client.stream(microgrid_id) call in the test |
| src/frequenz/client/dispatch/_client.py | Replaced protected _channel.is_closed check with is_running |
Comments suppressed due to low confidence (2)
tests/test_client.py:313
- The test calls
stream()a second time but doesn't assert any behavior. Consider adding an assertion that the returned stream matches the original (e.g.,assert second_stream is stream) or behaves as expected to ensure idempotent behavior is verified.
_ = client.stream(microgrid_id)
src/frequenz/client/dispatch/_client.py:245
- [nitpick] The docstring for
_get_streamdoes not mention that it will discard and recreate streams when they're not running. Consider updating it to reflect this behavior for better maintainability.
if broadcaster is not None and not broadcaster.is_running:
|
I took the liberty to change your PR description with the one Copilot created, you really need to work on your PR/commit messages, they are really not helping the reviewers at all. Not you can even ask the AI overlords to do it for you, so it shouldn't be a very complicated task 😒 |
| stream = client.stream(microgrid_id) | ||
|
|
||
| # Call function again to test behavior if stream already exists | ||
| _ = client.stream(microgrid_id) | ||
|
|
There was a problem hiding this comment.
Wouldn't it make sense to create a separate test for the double call? What if there is a bug that affects the stream() function in a way that if doesn't work when called only once but works when called twice?
There was a problem hiding this comment.
I split the test now
And fix test to catch it next time. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
This PR ensures the
stream()helper recreates a stream if the prior one isn’t running, and adds a test to exercise callingstream()twice.