Improve docs and tests and fix minor issues #1178
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
llucax
requested review from
ela-kotulska-frequenz
and removed request for
a team
github-actions
bot
added
part:tests
llucax
added
the
cmd:skip-release-notes
|
Skipping release notes as the docs improvements are too minor to be worth mentioning. |
llucax
added
the
type:tech-debt
There was a problem hiding this comment.
Pull Request Overview
This pull request improves documentation, enhances test cases, and fixes some minor issues in the codebase. Key changes include:
- Converting the ComponentMetricsData class to a dataclass for better clarity and immutability.
- Refactoring error handling in the battery manager to use pattern matching and improving error messages.
- Updating tests to use asyncio timeouts and refining documentation and comments in the component graph module.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/frequenz/sdk/timeseries/battery_pool/_component_metrics.py | Converted initializer and properties to dataclass fields. |
| src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py | Updated error handling using match expressions and modified error message formatting. |
| tests/timeseries/test_producer.py | Adjusted test case tuple structure and added more descriptive test numbers. |
| src/frequenz/sdk/timeseries/formula_engine/_formula_formatter.py | Changed metric fetcher string extraction to use str(metric_fetcher). |
| tests/microgrid/power_distributing/test_battery_distribution_algorithm.py | Enhanced test descriptions for battery distribution with varying capacities. |
| tests/microgrid/power_distributing/test_power_distributing.py | Replaced asyncio.wait with asyncio.timeout for cleaner test execution. |
| src/frequenz/sdk/microgrid/component_graph.py | Revised docstrings and corrected typos for improved clarity. |
Comments suppressed due to low confidence (1)
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py:237
- The error message construction uses a redundant '+' on the second line, which may cause a type error since unary plus is not valid for strings. Remove the extra '+' operator and merge the string concatenation appropriately.
"No data for at least one of the given batteries: "
+ self._str_ids(request.component_ids)
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR accumulates assorted cleanups and minor improvements aimed at enhancing code clarity, consistency, and test robustness. Key changes include:
- Replacing asyncio.wait-based timeout handling with async timeout contexts in multiple test cases.
- Improving error message formatting and consistency in the battery manager.
- Refining documentation and code comments (including spelling corrections) across the codebase.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/microgrid/power_distributing/test_power_distributing.py | Updated timeout handling for receiving results in tests. |
| src/frequenz/sdk/timeseries/battery_pool/_component_metrics.py | Converted class initializers to dataclass fields for simplicity. |
| src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py | Switched exception handling to a match statement and updated error message formatting; refactored _get_components_data to return a string error message consistently. |
| tests/timeseries/test_producer.py | Revised test case tuple definitions and added debug prints to indicate test step progression. |
| src/frequenz/sdk/timeseries/formula_engine/_formula_formatter.py | Modified value extraction from MetricFetcher for improved string representation. |
| tests/microgrid/power_distributing/test_battery_distribution_algorithm.py | Clarified test case docstrings in battery distribution tests. |
| src/frequenz/sdk/microgrid/component_graph.py | Revised documentation wording and corrected minor typos for consistency. |
Comments suppressed due to low confidence (2)
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py:238
- [nitpick] Consider using an f-string to format the full error message on a single line (e.g., f"No data for at least one of the given batteries: {self._str_ids(request.component_ids)}") to improve readability and consistency.
+ self._str_ids(request.component_ids)
tests/timeseries/test_producer.py:173
- [nitpick] Consider removing or gating debug print statements in test cases to reduce noise during automated test runs if they are no longer required for debugging.
print(f"----------------------------------------------------")
shsms
previously approved these changes
Mar 17, 2025
github-project-automation
bot
moved this from Review in progress
to Review approved
in Python SDK Roadmap
This is mainly to get the nice `__str__` and `__repr__` methods for better debugging. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Change the name from "test case" to "test step" in the fallback test, as they are not really isolated test cases, but steps in one long test (as next steps depend on the previous step to have happened). This makes it more clear that steps are connected and need to run in sequence. Also the index was made part of the test steps definition, so we can show more clearly which (sub-)stop failed, and we also add a print line indicating which step is being ran. Signed-off-by: Leandro Lucarella <[email protected]>
A `KeyError` was raised to indicate errors while getting inverters for batteries, but this have a couple of issues. First, the `KeyError` was converted to a string to get an error message, but `KeyError` is a very special error, and the argument is interpreted as a *key*, not as an error message, so when converting the error to a `str`, the result is the `repr()` of the *key*, so the strings are quoted and escaped, which is not what we want to report errors downstream. Second, one of the reported errors isn't really a `KeyError`, it is just a mismatch between what is requested and the current microgrid topology, so using a `KeyError` was completely wrong. Since these errors were only used to report errors from a direct call, we change `_get_components_data()` to return `list[InvBatPair] | str`, where if there is an error, a `str` with the error description is returned directly. As an extra, we sort the IDs when used in error messages, so they are easier to read for users, and more deterministic for tests. Tests are also simplified to remove the use of regex, which were probably only used because string were quoted. Signed-off-by: Leandro Lucarella <[email protected]>
Use `async with asyncio.timeout()` instead of using `asyncio.wait()` with extra tasks. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
|
Rebased, needs a new approval @shsms |
shsms
approved these changes
Mar 17, 2025
github-project-automation
bot
moved this from Review approved
to Done
in Python SDK Roadmap
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.
This PR accumulate a set of assorted cleanups and minor improvements.