Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 12, 2025

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.

Copilot AI review requested due to automatic review settings June 12, 2025 14:58
@Marenz Marenz requested review from a team as code owners June 12, 2025 14:58
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:dispatcher labels Jun 12, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_closed to public broadcaster.is_running in _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_stream does 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:

@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jun 12, 2025
@Marenz Marenz enabled auto-merge June 12, 2025 16:28
@llucax
Copy link
Contributor

llucax commented Jun 13, 2025

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 😒

Comment on lines 310 to 319
stream = client.stream(microgrid_id)

# Call function again to test behavior if stream already exists
_ = client.stream(microgrid_id)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the test now

And fix test to catch it next time.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz requested a review from llucax June 16, 2025 09:10
@Marenz Marenz added this pull request to the merge queue Jun 16, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 10e3986 Jun 16, 2025
5 checks passed
@Marenz Marenz deleted the baaaaased branch June 16, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:dispatcher part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants