Add tests for notifications - #2008
Open
khansaad wants to merge 3 commits into
Open
Conversation
Signed-off-by: Saad Khan <saakhan@ibm.com>
Contributor
Reviewer's GuideAdds helper utilities and extensive unit/integration tests to verify provisioning-state (optimised/under/over-provisioned) notifications emitted by the recommendation engine for CPU and memory, across local monitoring REST APIs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
utils.py, the newPROVISIONING_CODESandOPTIMISED_CODESsets are currently unused; consider either wiring them into existing validation helpers or removing them to avoid dead code. - Several places load JSON via
json.load(open(...))(e.g., in_setup_metric_and_metadata_profileand the integration test) — it would be more robust to usewith open(...) as f:to ensure file handles are properly closed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `utils.py`, the new `PROVISIONING_CODES` and `OPTIMISED_CODES` sets are currently unused; consider either wiring them into existing validation helpers or removing them to avoid dead code.
- Several places load JSON via `json.load(open(...))` (e.g., in `_setup_metric_and_metadata_profile` and the integration test) — it would be more robust to use `with open(...) as f:` to ensure file handles are properly closed.
## Individual Comments
### Comment 1
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_notifications.py" line_range="433-437" />
<code_context>
+ assert errorMsg == "", f"Schema validation error: {errorMsg}"
+
+ # Provisioning notification validation across every engine block
+ engine_notif_blocks = _collect_engine_notifications(list_reco_json)
+ assert engine_notif_blocks, "No engine notification blocks found — recommendations may not have been generated"
+
+ for engine_notifications in engine_notif_blocks:
+ validate_provisioning_notifications(engine_notifications)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the integration test by asserting that each engine notification block actually contains at least one provisioning-related code.
Currently the test would still pass if an engine block contains only non‑provisioning notifications, because `validate_provisioning_notifications` skips such dimensions. Given the docstring guarantees a provisioning state per resource dimension, add an assertion that each `engine_notifications` block includes at least one provisioning code before calling `validate_provisioning_notifications`, e.g. using `any(code in engine_notifications for code in ALL_PROVISIONING_CODES)`. This ensures the test fails if the backend stops emitting provisioning notifications but still returns other notification types.
```suggestion
# Schema validation
errorMsg = validate_list_reco_json(list_reco_json, list_reco_json_local_monitoring_schema)
assert errorMsg == "", f"Schema validation error: {errorMsg}"
# Provisioning notification validation across every engine block
engine_notif_blocks = _collect_engine_notifications(list_reco_json)
assert engine_notif_blocks, "No engine notification blocks found — recommendations may not have been generated"
for engine_notifications in engine_notif_blocks:
# Ensure each engine block actually contains at least one provisioning notification
assert any(
code in engine_notifications
for code in ALL_PROVISIONING_CODES
), "Engine notification block missing provisioning notifications"
validate_provisioning_notifications(engine_notifications)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Saad Khan <saakhan@ibm.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
_DIMENSIONSstructure invalidate_provisioning_notifications()and the_CODE_TO_MSGmapping intest_notifications.pyboth encode code/message relationships; consider centralizing this mapping (e.g., inhelpers.utils) to avoid divergence between the helpers and tests if codes or messages change. _create_experiment_from_templatewrites to a fixed/tmp/create_exp_{test_name}.jsonpath; using unique, per-test temp files (e.g., viatempfileor including a UUID) would reduce the chance of filename collisions when tests run in parallel.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_DIMENSIONS` structure in `validate_provisioning_notifications()` and the `_CODE_TO_MSG` mapping in `test_notifications.py` both encode code/message relationships; consider centralizing this mapping (e.g., in `helpers.utils`) to avoid divergence between the helpers and tests if codes or messages change.
- `_create_experiment_from_template` writes to a fixed `/tmp/create_exp_{test_name}.json` path; using unique, per-test temp files (e.g., via `tempfile` or including a UUID) would reduce the chance of filename collisions when tests run in parallel.
## Individual Comments
### Comment 1
<location path="tests/scripts/helpers/utils.py" line_range="1685-1694" />
<code_context>
+def validate_provisioning_notifications(engine_notifications: dict):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to cover dimensions being skipped and non-provisioning codes co-existing with provisioning codes
The helper supports two untested behaviours: skipping dimensions with no provisioning codes, and allowing non-provisioning codes to co-exist with provisioning codes. Please add unit tests in `TestProvisioningNotificationHelpers` that:
- use a notifications dict where one dimension has no provisioning codes but another does, asserting the first is skipped and the second is validated, and
- include irrelevant notification codes alongside a valid provisioning code, asserting `validate_provisioning_notifications` still passes and ignores the extra codes.
This will codify the intended behaviour and reduce regression risk as new notification types are added.
Suggested implementation:
```python
def validate_provisioning_notifications(engine_notifications: dict):
"""Validate that each resource dimension in engine-level notifications carries exactly one
provisioning-state code (optimised OR under-provisioned OR over-provisioned) with the
correct message text.
This covers all four dimensions:
- CPU request (323004 / 323006 / 323007)
- CPU limit (323005 / 323008 / 323009)
- Memory request (324003 / 324005 / 324006)
- Memory limit (324004 / 324007 / 324008)
Notes:
- Dimensions that contain no provisioning-state codes (only unrelated notification codes)
are intentionally skipped and do not cause the validation to fail.
- Non-provisioning notification codes may co-exist alongside a provisioning-state code
for a dimension; they are ignored by this helper.
```
To fully implement your review comment, the following tests should be added inside the existing `TestProvisioningNotificationHelpers` test class in `tests/scripts/helpers/utils.py` (or wherever that class currently lives):
```python
class TestProvisioningNotificationHelpers:
# ... existing tests ...
def test_dimension_without_provisioning_codes_is_skipped(self):
"""
A dimension that carries only non-provisioning notifications should be ignored,
while another dimension with a valid provisioning code must still be validated.
"""
engine_notifications = {
# Dimension with only non-provisioning codes – should be skipped by
# validate_provisioning_notifications
"cpu_request": {
# Use one of the known non-provisioning codes already used in the tests
# or another clearly non-provisioning code
"999999": {"message": "Some unrelated notification"},
},
# Dimension with a valid provisioning-state code – must still be validated
"cpu_limit": {
# Choose a provisioning code and message that is already used in the existing
# tests/fixtures for this helper so the expected message text matches whatever
# validate_provisioning_notifications asserts internally.
#
# Example (adjust code/message to match your real fixtures):
"323005": {"message": "CPU limits are optimally provisioned."},
},
# You can include other dimensions if your existing tests/fixtures expect them,
# reusing the same provisioning codes/messages.
}
# This should not raise; the dimension without provisioning codes is skipped,
# and the dimension with provisioning codes is validated.
validate_provisioning_notifications(engine_notifications)
def test_non_provisioning_codes_are_ignored_alongside_provisioning_codes(self):
"""
Non-provisioning notifications can co-exist with provisioning-state notifications for
the same dimension; validate_provisioning_notifications should ignore the extras.
"""
engine_notifications = {
"cpu_request": {
# Valid provisioning code + correct message (reuse values from existing tests)
"323004": {"message": "CPU requests are optimally provisioned."},
# Additional irrelevant notification code that should be ignored
"999999": {"message": "Some unrelated CPU request notification"},
},
# Include any other dimensions your existing tests expect, again reusing
# known-good provisioning codes/messages from your fixtures.
}
# This should not raise; the extra non-provisioning code must be ignored.
validate_provisioning_notifications(engine_notifications)
```
Key points to align with the rest of your codebase:
1. **Reuse existing codes and messages**: In the examples above, replace the hardcoded codes (`323004`, `323005`, etc.) and message strings with the same values your existing provisioning tests/fixtures use so that `validate_provisioning_notifications`'s internal expectations are satisfied.
2. **Integrate with existing fixtures/helpers**: If you already have a factory or fixture that produces a valid `engine_notifications` structure, prefer to:
- start from that fixture,
- then remove provisioning codes from one dimension (for the “skipped dimension” test),
- and add an extra non-provisioning code to another dimension (for the “ignored extras” test),
so the new tests remain in sync with any future changes to messages.
3. **Place tests in `TestProvisioningNotificationHelpers`**: Insert these methods into the existing test class for provisioning helpers, maintaining your current naming/style conventions and any shared setup (e.g. using `self.engine_notifications` or pytest fixtures).
</issue_to_address>
### Comment 2
<location path="tests/scripts/helpers/utils.py" line_range="236-245" />
<code_context>
+MEMORY_LIMIT_UNDER_PROVISIONED_MSG = "Workload is under-provisioned for Memory LIMITS. Kruize recommends increasing Memory limit allocation."
+MEMORY_LIMIT_OVER_PROVISIONED_MSG = "Workload is over-provisioned for Memory LIMITS. Kruize recommends reducing Memory limit allocation to optimize costs."
+
+# All provisioning-state notification codes (optimised + under + over) grouped for use in
+# validation helpers and tests.
+ALL_PROVISIONING_CODES = {
+ CPU_REQUEST_OPTIMISED_CODE,
+ CPU_REQUEST_UNDER_PROVISIONED_CODE,
+ CPU_REQUEST_OVER_PROVISIONED_CODE,
+ CPU_LIMIT_OPTIMISED_CODE,
+ CPU_LIMIT_UNDER_PROVISIONED_CODE,
+ CPU_LIMIT_OVER_PROVISIONED_CODE,
+ MEMORY_REQUEST_OPTIMISED_CODE,
+ MEMORY_REQUEST_UNDER_PROVISIONED_CODE,
+ MEMORY_REQUEST_OVER_PROVISIONED_CODE,
+ MEMORY_LIMIT_OPTIMISED_CODE,
+ MEMORY_LIMIT_UNDER_PROVISIONED_CODE,
+ MEMORY_LIMIT_OVER_PROVISIONED_CODE,
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a unit test to assert the exact contents of ALL_PROVISIONING_CODES
Since this constant underpins the engine notification integration test, please add a focused unit test (either here or in `TestProvisioningNotificationHelpers`) that asserts `ALL_PROVISIONING_CODES` matches a hard-coded set of expected IDs. This will detect any future omissions or unintended additions when new codes are introduced.
Suggested implementation:
```python
MEMORY_REQUEST_UNDER_PROVISIONED_CODE = "324005"
MEMORY_REQUEST_OVER_PROVISIONED_CODE = "324006"
def test_all_provisioning_codes_exact_contents():
"""
Ensure ALL_PROVISIONING_CODES contains exactly the expected notification codes.
This guards against accidental omissions or additions when new notification
codes are introduced.
"""
expected_codes = {
CPU_REQUEST_OPTIMISED_CODE,
CPU_REQUEST_UNDER_PROVISIONED_CODE,
CPU_REQUEST_OVER_PROVISIONED_CODE,
CPU_LIMIT_OPTIMISED_CODE,
CPU_LIMIT_UNDER_PROVISIONED_CODE,
CPU_LIMIT_OVER_PROVISIONED_CODE,
MEMORY_REQUEST_OPTIMISED_CODE,
MEMORY_REQUEST_UNDER_PROVISIONED_CODE,
MEMORY_REQUEST_OVER_PROVISIONED_CODE,
MEMORY_LIMIT_OPTIMISED_CODE,
MEMORY_LIMIT_UNDER_PROVISIONED_CODE,
MEMORY_LIMIT_OVER_PROVISIONED_CODE,
}
assert ALL_PROVISIONING_CODES == expected_codes
```
This test assumes `ALL_PROVISIONING_CODES` is defined in this module or imported into it. If `ALL_PROVISIONING_CODES` lives in a different module (e.g. a production module under `kruize`), add an appropriate import at the top of `tests/scripts/helpers/utils.py`, for example:
- `from kruize.notifications import ALL_PROVISIONING_CODES`
Adjust the import path to match your actual project structure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1685
to
+1694
| def validate_provisioning_notifications(engine_notifications: dict): | ||
| """Validate that each resource dimension in engine-level notifications carries exactly one | ||
| provisioning-state code (optimised OR under-provisioned OR over-provisioned) with the | ||
| correct message text. | ||
|
|
||
| This covers all four dimensions: | ||
| - CPU request (323004 / 323006 / 323007) | ||
| - CPU limit (323005 / 323008 / 323009) | ||
| - Memory request (324003 / 324005 / 324006) | ||
| - Memory limit (324004 / 324007 / 324008) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests to cover dimensions being skipped and non-provisioning codes co-existing with provisioning codes
The helper supports two untested behaviours: skipping dimensions with no provisioning codes, and allowing non-provisioning codes to co-exist with provisioning codes. Please add unit tests in TestProvisioningNotificationHelpers that:
- use a notifications dict where one dimension has no provisioning codes but another does, asserting the first is skipped and the second is validated, and
- include irrelevant notification codes alongside a valid provisioning code, asserting
validate_provisioning_notificationsstill passes and ignores the extra codes.
This will codify the intended behaviour and reduce regression risk as new notification types are added.
Suggested implementation:
def validate_provisioning_notifications(engine_notifications: dict):
"""Validate that each resource dimension in engine-level notifications carries exactly one
provisioning-state code (optimised OR under-provisioned OR over-provisioned) with the
correct message text.
This covers all four dimensions:
- CPU request (323004 / 323006 / 323007)
- CPU limit (323005 / 323008 / 323009)
- Memory request (324003 / 324005 / 324006)
- Memory limit (324004 / 324007 / 324008)
Notes:
- Dimensions that contain no provisioning-state codes (only unrelated notification codes)
are intentionally skipped and do not cause the validation to fail.
- Non-provisioning notification codes may co-exist alongside a provisioning-state code
for a dimension; they are ignored by this helper.To fully implement your review comment, the following tests should be added inside the existing TestProvisioningNotificationHelpers test class in tests/scripts/helpers/utils.py (or wherever that class currently lives):
class TestProvisioningNotificationHelpers:
# ... existing tests ...
def test_dimension_without_provisioning_codes_is_skipped(self):
"""
A dimension that carries only non-provisioning notifications should be ignored,
while another dimension with a valid provisioning code must still be validated.
"""
engine_notifications = {
# Dimension with only non-provisioning codes – should be skipped by
# validate_provisioning_notifications
"cpu_request": {
# Use one of the known non-provisioning codes already used in the tests
# or another clearly non-provisioning code
"999999": {"message": "Some unrelated notification"},
},
# Dimension with a valid provisioning-state code – must still be validated
"cpu_limit": {
# Choose a provisioning code and message that is already used in the existing
# tests/fixtures for this helper so the expected message text matches whatever
# validate_provisioning_notifications asserts internally.
#
# Example (adjust code/message to match your real fixtures):
"323005": {"message": "CPU limits are optimally provisioned."},
},
# You can include other dimensions if your existing tests/fixtures expect them,
# reusing the same provisioning codes/messages.
}
# This should not raise; the dimension without provisioning codes is skipped,
# and the dimension with provisioning codes is validated.
validate_provisioning_notifications(engine_notifications)
def test_non_provisioning_codes_are_ignored_alongside_provisioning_codes(self):
"""
Non-provisioning notifications can co-exist with provisioning-state notifications for
the same dimension; validate_provisioning_notifications should ignore the extras.
"""
engine_notifications = {
"cpu_request": {
# Valid provisioning code + correct message (reuse values from existing tests)
"323004": {"message": "CPU requests are optimally provisioned."},
# Additional irrelevant notification code that should be ignored
"999999": {"message": "Some unrelated CPU request notification"},
},
# Include any other dimensions your existing tests expect, again reusing
# known-good provisioning codes/messages from your fixtures.
}
# This should not raise; the extra non-provisioning code must be ignored.
validate_provisioning_notifications(engine_notifications)Key points to align with the rest of your codebase:
- Reuse existing codes and messages: In the examples above, replace the hardcoded codes (
323004,323005, etc.) and message strings with the same values your existing provisioning tests/fixtures use so thatvalidate_provisioning_notifications's internal expectations are satisfied. - Integrate with existing fixtures/helpers: If you already have a factory or fixture that produces a valid
engine_notificationsstructure, prefer to:- start from that fixture,
- then remove provisioning codes from one dimension (for the “skipped dimension” test),
- and add an extra non-provisioning code to another dimension (for the “ignored extras” test),
so the new tests remain in sync with any future changes to messages.
- Place tests in
TestProvisioningNotificationHelpers: Insert these methods into the existing test class for provisioning helpers, maintaining your current naming/style conventions and any shared setup (e.g. usingself.engine_notificationsor pytest fixtures).
Comment on lines
+236
to
+245
| # All provisioning-state notification codes (optimised + under + over) grouped for use in | ||
| # validation helpers and tests. | ||
| ALL_PROVISIONING_CODES = { | ||
| CPU_REQUEST_OPTIMISED_CODE, | ||
| CPU_REQUEST_UNDER_PROVISIONED_CODE, | ||
| CPU_REQUEST_OVER_PROVISIONED_CODE, | ||
| CPU_LIMIT_OPTIMISED_CODE, | ||
| CPU_LIMIT_UNDER_PROVISIONED_CODE, | ||
| CPU_LIMIT_OVER_PROVISIONED_CODE, | ||
| MEMORY_REQUEST_OPTIMISED_CODE, |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a unit test to assert the exact contents of ALL_PROVISIONING_CODES
Since this constant underpins the engine notification integration test, please add a focused unit test (either here or in TestProvisioningNotificationHelpers) that asserts ALL_PROVISIONING_CODES matches a hard-coded set of expected IDs. This will detect any future omissions or unintended additions when new codes are introduced.
Suggested implementation:
MEMORY_REQUEST_UNDER_PROVISIONED_CODE = "324005"
MEMORY_REQUEST_OVER_PROVISIONED_CODE = "324006"
def test_all_provisioning_codes_exact_contents():
"""
Ensure ALL_PROVISIONING_CODES contains exactly the expected notification codes.
This guards against accidental omissions or additions when new notification
codes are introduced.
"""
expected_codes = {
CPU_REQUEST_OPTIMISED_CODE,
CPU_REQUEST_UNDER_PROVISIONED_CODE,
CPU_REQUEST_OVER_PROVISIONED_CODE,
CPU_LIMIT_OPTIMISED_CODE,
CPU_LIMIT_UNDER_PROVISIONED_CODE,
CPU_LIMIT_OVER_PROVISIONED_CODE,
MEMORY_REQUEST_OPTIMISED_CODE,
MEMORY_REQUEST_UNDER_PROVISIONED_CODE,
MEMORY_REQUEST_OVER_PROVISIONED_CODE,
MEMORY_LIMIT_OPTIMISED_CODE,
MEMORY_LIMIT_UNDER_PROVISIONED_CODE,
MEMORY_LIMIT_OVER_PROVISIONED_CODE,
}
assert ALL_PROVISIONING_CODES == expected_codesThis test assumes ALL_PROVISIONING_CODES is defined in this module or imported into it. If ALL_PROVISIONING_CODES lives in a different module (e.g. a production module under kruize), add an appropriate import at the top of tests/scripts/helpers/utils.py, for example:
from kruize.notifications import ALL_PROVISIONING_CODES
Adjust the import path to match your actual project structure.
12 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Please describe the issue or feature and the summary of changes made to fix this.
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Add validation utilities and comprehensive tests to ensure recommendation engine provisioning notifications are present, mutually exclusive, and carry correct messages for CPU and memory resources.
Enhancements:
Tests: