Fix sensor data streaming without metrics filter #148
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test was requesting only one metric, but also sensing only one metric, so the filtering wasn't being tested. We also rename the test to make it more specific. Signed-off-by: Leandro Lucarella <[email protected]>
When no metric filter was passed to `stream_sensor_data()` the transform callback failed because the `metrics` argument wasn't passed when calling `sensor_data_samples_from_proto()` internally, making the gRPC streaming task stop with an unhandled exception. This commit makes the `metrics` argument optional for `sensor_data_samples_from_proto()` to fix that. A test case with no metric filters is added too. Signed-off-by: Leandro Lucarella <[email protected]>
github-actions
bot
added
part:tests
llucax
added
the
cmd:skip-release-notes
|
Skipping release notes as this fixes unreleased code. |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that when no metric filters are provided, stream_sensor_data() still returns all sensor metrics by making the metrics argument optional and updating the filtering logic. It also adds and refines tests to cover streaming with and without metric filters.
- Allow
sensor_data_samples_from_proto()to default to retrieving all metrics if none are specified. - Adjust the filter logic to handle
Nonefor metrics and add a test for streaming without filters, while improving the existing filter test.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_client.py | Renamed and extended tests to cover streaming with one metric filter and without filters |
| src/frequenz/client/microgrid/_sensor_proto.py | Made metrics parameter optional and updated filtering logic |
Comments suppressed due to low confidence (2)
tests/test_client.py:915
- [nitpick] The test name
test_stream_sensor_data_one_metricsuggests a single-metric scenario, but the test data yields two metrics when no filter is passed. Consider renaming the test to clarify that it's streaming without filters or adjust the data and assertions to match the intended filter behavior.
async def test_stream_sensor_data_one_metric(
tests/test_client.py:915
- This test never passes a metrics filter to
client.stream_sensor_data, so it's not validating filtered behavior. Add the appropriatemetricsargument to the call and assert that only the specified metric samples are returned.
async def test_stream_sensor_data_one_metric(
Merged
via the queue into
frequenz-floss:v0.x.x
with commit May 22, 2025
a24d1c3
6 of 7 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When no metric filter was passed to
stream_sensor_data()the transform callback failed because themetricsargument wasn't passed when callingsensor_data_samples_from_proto()internally, making the gRPC streaming task stop with an unhandled exception.This PR makes the
metricsargument optional forsensor_data_samples_from_proto()to fix that.A test case with no metric filters is added too, and the test case testing metric filters is improved to actually test the filtering.