-
Notifications
You must be signed in to change notification settings - Fork 4
Fix ci_max after new base-client release #210
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
base: v0.x.x
Are you sure you want to change the base?
Conversation
Marenz
commented
Sep 2, 2025
- Fix call of stream_method in latest base-client
- Add Mock for initial_metadata() for base-clients GrpcStreamer
Signed-off-by: Mathias L. Baumann <[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.
Pull Request Overview
This PR fixes compatibility issues with a new base-client release by updating the streaming interface and adding necessary mock functionality.
- Updates the
StreamMicrogridDispatches
method to return a mock stream object instead of an async iterator - Removes unnecessary type casting in the client's stream method call
- Adds a
_MockStream
class to provide theinitial_metadata()
method required by the base-client's GrpcStreamer
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/frequenz/client/dispatch/test/_service.py | Adds _MockStream class and refactors StreamMicrogridDispatches to return mock stream object |
src/frequenz/client/dispatch/_client.py | Removes type casting from stream method call to fix compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
async def initial_metadata(self) -> None: | ||
"""Do nothing, just to mock the grpc call.""" | ||
_logger.debug("Called initial_metadata()") | ||
|
Copilot
AI
Sep 2, 2025
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.
The initial_metadata()
method should return metadata instead of None to properly mock the gRPC interface. Consider returning an empty metadata object or the expected metadata type.
async def initial_metadata(self) -> None: | |
"""Do nothing, just to mock the grpc call.""" | |
_logger.debug("Called initial_metadata()") | |
async def initial_metadata(self) -> list: | |
"""Return empty metadata to mock the grpc call.""" | |
_logger.debug("Called initial_metadata()") | |
return [] |
Copilot uses AI. Check for mistakes.
async def stream() -> AsyncIterator[StreamMicrogridDispatchesResponse]: | ||
"""Stream microgrid dispatches changes.""" | ||
_logger.debug("Starting stream for microgrid %s", request.microgrid_id) |
Copilot
AI
Sep 2, 2025
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.
[nitpick] The nested stream()
function should be extracted to a separate method or moved outside the method to improve readability and maintainability.
Copilot uses AI. Check for mistakes.
stream_name="StreamMicrogridDispatches", | ||
stream_method=lambda: cast( | ||
AsyncIterator[StreamMicrogridDispatchesResponse], | ||
self.stub.StreamMicrogridDispatches( | ||
request, | ||
timeout=self._stream_timeout_seconds, | ||
), | ||
stream_method=lambda: self.stub.StreamMicrogridDispatches( | ||
request, | ||
timeout=self._stream_timeout_seconds, | ||
), |
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.
Ah, interesting. Maybe other projects could be affected about this, if they didn't remove the casts after moving to use the proper async stub. 🤔
Once more life shows that there are no small or improbable breaking changes 😢
Signed-off-by: Mathias L. Baumann <[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.
LGTM, but now that v0.11.1 is yanked, I would wait until we figure out why is this version failing before we merge this.