Layers -14 Additional List and create layers tests#1831
Open
bhanvimenghani wants to merge 10 commits into
Open
Layers -14 Additional List and create layers tests#1831bhanvimenghani wants to merge 10 commits into
bhanvimenghani wants to merge 10 commits into
Conversation
Contributor
Reviewer's GuideExtends createLayer and listLayers REST API test coverage, adds a cleanup helper for layers, and refactors validation/error-message expectations to align with more detailed server-side validation behavior. 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 3 issues, and left some high level feedback:
- In
test_create_layer_mandatory_fields_validation, thequery_invalid_promql_syntaxcase uses alayer_presencestring with malformed JSON ("up{job="), which will causejson.loads()to fail before the API is exercised; consider making the JSON syntactically valid while keeping the PromQL semantically invalid so the server-side validation is actually tested. - The performance-oriented tests (e.g.
test_list_layers_performance_with_many_layersandtest_layer_detection_performance_multiple_layers) currently only measure and print timings while asserting on counts; if these are meant to guard performance, consider asserting against a loose upper bound or clearly documenting that they are informational to avoid future confusion about their purpose. - There is quite a bit of duplicated code for creating temporary JSON files and layers across the new tests; consider extracting a small helper function (e.g.
create_temp_layer_file(json_obj)orcreate_test_layer(layer_name, config_overrides)) to reduce repetition and make the tests easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_create_layer_mandatory_fields_validation`, the `query_invalid_promql_syntax` case uses a `layer_presence` string with malformed JSON (`"up{job="`), which will cause `json.loads()` to fail before the API is exercised; consider making the JSON syntactically valid while keeping the PromQL semantically invalid so the server-side validation is actually tested.
- The performance-oriented tests (e.g. `test_list_layers_performance_with_many_layers` and `test_layer_detection_performance_multiple_layers`) currently only measure and print timings while asserting on counts; if these are meant to guard performance, consider asserting against a loose upper bound or clearly documenting that they are informational to avoid future confusion about their purpose.
- There is quite a bit of duplicated code for creating temporary JSON files and layers across the new tests; consider extracting a small helper function (e.g. `create_temp_layer_file(json_obj)` or `create_test_layer(layer_name, config_overrides)`) to reduce repetition and make the tests easier to maintain.
## Individual Comments
### Comment 1
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_list_layers.py" line_range="511" />
<code_context>
+
+@pytest.mark.layers
+@pytest.mark.sanity
+def test_list_layers_sorting_order(cluster_type):
+ """
+ Test Description: This test validates the sorting order of layers returned by listLayers API.
</code_context>
<issue_to_address>
**issue (testing):** The `test_list_layers_sorting_order` test does not assert any specific ordering, so it doesn't really validate sorting behavior.
The test only prints the order and computes `is_alphabetically_sorted` without asserting on it, so it will pass regardless of the API’s behavior. Please either add an assertion on the expected order (alphabetical vs. creation time, per the spec), or, if ordering is intentionally undefined, rename the test and remove the sorting logic to avoid a misleading signal.
</issue_to_address>
### Comment 2
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_layer_detection.py" line_range="54" />
<code_context>
+ pytest.param("hotspot-micrometer-config.json", id="hotspot_multiple_queries"),
+ pytest.param("quarkus-micrometer-config.json", id="quarkus_single_query"),
+])
+def test_layer_detection_with_valid_query(cluster_type, layer_file):
+ """
+ Test Description: Validates layer creation with query-based detection
</code_context>
<issue_to_address>
**suggestion (testing):** The layer detection tests only assert successful layer creation, not actual detection behavior.
These tests (e.g. `test_layer_detection_with_valid_query`, `test_layer_detection_always_present`, and the integration tests) only assert that `create_layer` succeeds, but never validate that detection rules are actually applied (e.g. layers being associated with the right containers/experiments). If this suite is intended to cover detection behavior, please add at least one end-to-end scenario that: (1) defines a layer with specific detection rules, (2) runs experiments that should and should not match, and (3) asserts that the correct layer is detected/applied. If that’s out of scope, consider renaming the tests to reflect that they only validate layer creation, not detection.
Suggested implementation:
```python
def test_layer_creation_with_valid_query(cluster_type, layer_file):
"""
Test Description: Validates layer *creation* using query-based configurations
Uses existing layer configs with real Prometheus queries
Expected: Layer should be created successfully; detection behavior is not validated here
"""
```
To fully align the test suite with the clarified intent (“only validate layer creation, not detection”), similar renaming and docstring updates should be applied to:
1. `test_layer_detection_always_present` in this file: rename to something like `test_layer_creation_always_present` and update its docstring to clearly state it only verifies successful layer creation.
2. Any integration tests whose names/descriptions imply they validate detection behavior but in practice only assert successful `create_layer` responses; those should be renamed and their docstrings adjusted accordingly (e.g. “layer creation via REST API”) unless you add true end-to-end detection assertions as described in your review comment.
</issue_to_address>
### Comment 3
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_list_layers.py" line_range="384" />
<code_context>
+
+@pytest.mark.layers
+@pytest.mark.sanity
+def test_list_layers_performance_with_many_layers(cluster_type):
+ """
+ Test Description: This test validates listLayers API performance when listing 100+ layers.
</code_context>
<issue_to_address>
**suggestion (testing):** The performance test measures response time but does not assert any threshold, which may make it misleading or noisy.
This test only asserts functional behavior while labeling itself as a performance test. If you want to catch regressions, add a reasonable upper bound on `response_time` and/or mark it as `@pytest.mark.slow`. Otherwise, consider renaming/rewording the test to emphasize handling many layers rather than performance.
Suggested implementation:
```python
@pytest.mark.layers
@pytest.mark.sanity
@pytest.mark.slow
def test_list_layers_performance_with_many_layers(cluster_type):
"""
Test Description: This test validates listLayers API performance when listing 100+ layers.
It creates multiple layers, measures response time, and asserts it stays under a reasonable threshold
to catch performance regressions.
"""
```
```python
num_layers = 10
created_layers = []
max_response_time_seconds = 5.0
print(f"Creating {num_layers} layers for performance testing...")
```
```python
response_time = response.elapsed.total_seconds()
assert response_time <= max_response_time_seconds, (
f"listLayers response time {response_time:.3f}s exceeded "
f"threshold of {max_response_time_seconds:.3f}s when listing {num_layers} layers"
)
```
If the variable holding the HTTP response is not named `response`, update the SEARCH/REPLACE block accordingly to match the actual variable name.
You may also want to tune `max_response_time_seconds` (e.g., 2.0s, 10.0s) based on realistic expectations for the environment where this test runs (local vs CI vs production-like).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| @pytest.mark.layers | ||
| @pytest.mark.sanity | ||
| def test_list_layers_sorting_order(cluster_type): |
Contributor
There was a problem hiding this comment.
issue (testing): The test_list_layers_sorting_order test does not assert any specific ordering, so it doesn't really validate sorting behavior.
The test only prints the order and computes is_alphabetically_sorted without asserting on it, so it will pass regardless of the API’s behavior. Please either add an assertion on the expected order (alphabetical vs. creation time, per the spec), or, if ordering is intentionally undefined, rename the test and remove the sorting logic to avoid a misleading signal.
bhanvimenghani
changed the title
bhanvimenghani
force-pushed
the
extra_layers_tests
branch
from
2c6a350 to
c36c290
Compare
bhanvimenghani
changed the title
Contributor
|
@bhanvimenghani Can you please trigger the jenkins test and post the results ? |
12 tasks
chandrams
reviewed
May 11, 2026
| @pytest.mark.sanity | ||
| def test_list_layers_performance_with_many_layers(cluster_type): | ||
| """ | ||
| Test Description: This test validates listLayers API performance when listing 100+ layers. |
Contributor
There was a problem hiding this comment.
This test should not be part of functional tests. You can add it to load testing and capture the relevant metrics
Contributor
Author
There was a problem hiding this comment.
removed the tests from other functional tests.
Contributor
|
@bhanvimenghani Can you please address sourcery comments and share the test results |
1 similar comment
Contributor
|
@bhanvimenghani Can you please address sourcery comments and share the test results |
bhanvimenghani
changed the title
bhanvimenghani
force-pushed
the
extra_layers_tests
branch
from
9227c8e to
b09dec7
Compare
Contributor
Author
|
test_create_layer_output.txt |
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using string sentinels like
"SKIP_APIVERSION","SKIP_KIND", and"SKIP_METADATA"to control JSON shape adds indirection and makes the cases harder to follow; consider usingNone/missing arguments or separate parametrized cases instead of overloading string values. - The new constant
LAYER_QUERY_INVALID_PROMQL_MSGinutils.pyis defined but not referenced anywhere; either wire it into the validation/error checks or remove it to avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using string sentinels like `"SKIP_APIVERSION"`, `"SKIP_KIND"`, and `"SKIP_METADATA"` to control JSON shape adds indirection and makes the cases harder to follow; consider using `None`/missing arguments or separate parametrized cases instead of overloading string values.
- The new constant `LAYER_QUERY_INVALID_PROMQL_MSG` in `utils.py` is defined but not referenced anywhere; either wire it into the validation/error checks or remove it to avoid dead code.
## Individual Comments
### Comment 1
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_create_layer.py" line_range="512" />
<code_context>
@pytest.mark.parametrize("test_name, expected_error_msg, apiVersion, kind, metadata_name, layer_name, details, layer_presence, tunables", [
("tunable_null_upper_bound", TUNABLE_NULL_BOUNDS_MSG % 't1', "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"name": "t1", "value_type": "double", "upper_bound": null, "lower_bound": "10", "step": 1}]'),
("tunable_null_lower_bound", TUNABLE_NULL_BOUNDS_MSG % 't1', "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"name": "t1", "value_type": "double", "upper_bound": "100", "lower_bound": null, "step": 1}]'),
</code_context>
<issue_to_address>
**suggestion (testing):** Relying on full backend error string for invalid PromQL may make this test brittle
The `"query_invalid_promql_syntax"` case asserts on the full backend error string, including `"Unexpected response status: 400"`, which makes the test fragile to harmless wording changes. Since `LAYER_QUERY_INVALID_PROMQL_MSG` is now defined in `utils.py`, can this test instead assert on that message (or a stable substring) so it checks the validation contract rather than backend phrasing details?
Suggested implementation:
```python
from .utils import (
TUNABLE_NULL_BOUNDS_MSG,
TUNABLE_NULL_STEP_MSG,
LAYER_QUERY_INVALID_PROMQL_MSG,
)
```
```python
("query_invalid_promql_syntax", LAYER_QUERY_INVALID_PROMQL_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"name": "q1", "query": "sum(invalid{label=\\"value\\"})"}]'),
```
1. If the exact error string in the `"query_invalid_promql_syntax"` tuple differs from the one used in the SEARCH block, adjust the SEARCH side to match the current line in your file exactly.
2. If the import block for `.utils` is structured differently (e.g., a single-line import or additional constants), integrate `LAYER_QUERY_INVALID_PROMQL_MSG` into that existing import style instead of the shown multi-line form.
</issue_to_address>
### Comment 2
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_list_layers.py" line_range="397-406" />
<code_context>
+
+@pytest.mark.layers
+@pytest.mark.sanity
+def test_list_layers_case_sensitivity(cluster_type):
+ """
+ Test Description: This test validates if layer name search is case-sensitive.
+ Creates a layer and tries to list it with different case variations.
+ """
+ form_kruize_url(cluster_type)
+
+ # Create a layer with mixed case name
+ layer_name = "TestCaseLayer"
+ # Cleanup before creating to prevent 409 conflicts
+ delete_layer(layer_name)
+
+ tmp_json_file = "/tmp/create_layer_case_test.json"
</code_context>
<issue_to_address>
**suggestion (testing):** Use pytest’s `tmp_path` (or similar) instead of hardcoded `/tmp` paths in new tests
In `test_list_layers_case_sensitivity` (and similar tests), the request body is written directly to `/tmp/create_layer_case_test.json`. Hardcoding `/tmp` risks file collisions in parallel runs and can behave differently across environments. Please switch to pytest’s `tmp_path` fixture (e.g. `tmp_path / "create_layer_case_test.json"`) so each test uses an isolated, per-run temp directory and avoids cross-test interference.
Suggested implementation:
```python
@pytest.mark.layers
@pytest.mark.sanity
def test_list_layers_case_sensitivity(cluster_type, tmp_path):
```
```python
tmp_json_file = tmp_path / "create_layer_case_test.json"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -406,15 +512,15 @@ def test_create_layer_presence_combinations(test_name, expected_error_msg, apiVe | |||
| @pytest.mark.parametrize("test_name, expected_error_msg, apiVersion, kind, metadata_name, layer_name, details, layer_presence, tunables", [ | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Relying on full backend error string for invalid PromQL may make this test brittle
The "query_invalid_promql_syntax" case asserts on the full backend error string, including "Unexpected response status: 400", which makes the test fragile to harmless wording changes. Since LAYER_QUERY_INVALID_PROMQL_MSG is now defined in utils.py, can this test instead assert on that message (or a stable substring) so it checks the validation contract rather than backend phrasing details?
Suggested implementation:
from .utils import (
TUNABLE_NULL_BOUNDS_MSG,
TUNABLE_NULL_STEP_MSG,
LAYER_QUERY_INVALID_PROMQL_MSG,
) ("query_invalid_promql_syntax", LAYER_QUERY_INVALID_PROMQL_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"name": "q1", "query": "sum(invalid{label=\\"value\\"})"}]'),- If the exact error string in the
"query_invalid_promql_syntax"tuple differs from the one used in the SEARCH block, adjust the SEARCH side to match the current line in your file exactly. - If the import block for
.utilsis structured differently (e.g., a single-line import or additional constants), integrateLAYER_QUERY_INVALID_PROMQL_MSGinto that existing import style instead of the shown multi-line form.
Comment on lines
+397
to
+406
| def test_list_layers_case_sensitivity(cluster_type): | ||
| """ | ||
| Test Description: This test validates if layer name search is case-sensitive. | ||
| Creates a layer and tries to list it with different case variations. | ||
| """ | ||
| form_kruize_url(cluster_type) | ||
|
|
||
| # Create a layer with mixed case name | ||
| layer_name = "TestCaseLayer" | ||
| # Cleanup before creating to prevent 409 conflicts |
Contributor
There was a problem hiding this comment.
suggestion (testing): Use pytest’s tmp_path (or similar) instead of hardcoded /tmp paths in new tests
In test_list_layers_case_sensitivity (and similar tests), the request body is written directly to /tmp/create_layer_case_test.json. Hardcoding /tmp risks file collisions in parallel runs and can behave differently across environments. Please switch to pytest’s tmp_path fixture (e.g. tmp_path / "create_layer_case_test.json") so each test uses an isolated, per-run temp directory and avoids cross-test interference.
Suggested implementation:
@pytest.mark.layers
@pytest.mark.sanity
def test_list_layers_case_sensitivity(cluster_type, tmp_path): tmp_json_file = tmp_path / "create_layer_case_test.json"…tching, and tmp_path fixture
…nd null elements in arrays)
Contributor
Author
|
Sourcery comments accommodated , comment from previous pr accommodated , test doc updated |
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In tests like
test_create_layer_optional_fields,test_create_layer_mandatory_fields_validation, and others, the JSON construction/writing pattern is repeated; consider extracting a small helper to build and persist the request payloads to reduce duplication and make future adjustments to the schema easier. - The
cleanup_all_layers()helper inkruize.pyhardcodes200and assumes the response body is a list; using the existingSUCCESS_200_STATUS_CODEconstant and explicitly handling non-list or error responses would make this utility more robust and consistent with the rest of the helpers. - The
test_list_layers_sorting_ordertest assumes the global list of layers only differs by the four layers created in the test; to avoid flakiness when other layers exist, consider invokingcleanup_all_layers()at the beginning (and optionally end) or scoping the check to a filtered subset keyed by a unique prefix.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In tests like `test_create_layer_optional_fields`, `test_create_layer_mandatory_fields_validation`, and others, the JSON construction/writing pattern is repeated; consider extracting a small helper to build and persist the request payloads to reduce duplication and make future adjustments to the schema easier.
- The `cleanup_all_layers()` helper in `kruize.py` hardcodes `200` and assumes the response body is a list; using the existing `SUCCESS_200_STATUS_CODE` constant and explicitly handling non-list or error responses would make this utility more robust and consistent with the rest of the helpers.
- The `test_list_layers_sorting_order` test assumes the global list of layers only differs by the four layers created in the test; to avoid flakiness when other layers exist, consider invoking `cleanup_all_layers()` at the beginning (and optionally end) or scoping the check to a filtered subset keyed by a unique prefix.
## Individual Comments
### Comment 1
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_create_layer.py" line_range="164-173" />
<code_context>
ensure_layer_deleted(layer_name)
+@pytest.mark.layers
+@pytest.mark.sanity
+@pytest.mark.parametrize("test_name, apiVersion, kind, metadata_name, layer_name, details, layer_presence, tunables", [
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a malformed-JSON body test in addition to empty/whitespace cases
To better cover the createLayer JSON parsing path, please add a test that sends non-empty but syntactically invalid JSON (e.g. `{` or `[}`). This validates the distinct failure mode where the body exists but cannot be parsed, beyond the empty/whitespace cases already covered.
Suggested implementation:
```python
def test_create_layer_optional_fields(test_name, apiVersion, kind, metadata_name, layer_name, details, layer_presence, tunables, cluster_type, tmp_path):
"""
Test Description: Validates createLayer API accepts requests with optional fields missing or null
"""
form_kruize_url(cluster_type)
@pytest.mark.layers
@pytest.mark.sanity
@pytest.mark.parametrize("malformed_body", [
"{",
"[",
])
def test_create_layer_malformed_json_body(cluster_type, malformed_body):
"""
Test Description: Validates createLayer API returns an error when the request body contains malformed JSON.
This specifically covers the case where the body is non-empty but syntactically invalid, distinct from empty/whitespace bodies.
"""
form_kruize_url(cluster_type)
# Assuming form_kruize_url sets KRUIZE_URL or a similar base URL environment variable
kruize_url = os.environ.get("KRUIZE_URL")
assert kruize_url, "KRUIZE_URL must be set by form_kruize_url or test configuration"
url = f"{kruize_url}/createLayer"
headers = {"Content-Type": "application/json"}
response = requests.post(url, data=malformed_body, headers=headers)
# Malformed JSON should result in a client error; 400 Bad Request is the typical status code
assert response.status_code == 400, (
f"Expected HTTP 400 for malformed JSON body, got {response.status_code} with body: {response.text}"
)
```
If the createLayer endpoint path or base URL differs (e.g. a helper like `get_kruize_url()` or a different environment variable is used elsewhere in this file), update the construction of `url` to match the existing convention.
If the API uses a different status code for JSON parse errors (for example, 422 Unprocessable Entity), adjust the `assert response.status_code == 400` accordingly or broaden it to the specific expected code.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
Jenkins Run link https://ci.app-svc-perf.corp.redhat.com/job/ExternalTeams/job/Autotune/job/kruize_functional_tests/551/ |
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
3 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.
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
Expand and harden test coverage for createLayer and listLayers REST APIs, including additional validation scenarios and test utilities.
New Features:
Enhancements:
Tests: