Adding E2E and Integration Tests - #25
Open
shekhar316 wants to merge 1 commit into
Open
Conversation
Signed-off-by: SHEKHAR SAXENA <shekhar.saxena@ibm.com>
Reviewer's GuideIntroduce a Python-based pytest E2E framework for kruize-optimizer that orchestrates cluster setup/teardown, installs Kruize, Prometheus, benchmarks, and the optimizer, then validates pod health and core Kruize APIs via Kubernetes and HTTP clients. File-Level Changes
Possibly linked issues
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}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
cluster-typeoption acceptsminikube, but several code paths (e.g.,kruize_local_patch, optimizer install/overlays, port forwarding, kruize_url) either treat onlykind/openshiftor assumekind, so it would be good to either fully implement minikube handling or explicitly reject it. - The
kruize_clientfixture is defined both inconftest.pyandtest_kruize_api.py, which is redundant and could lead to confusion; consider keeping a single shared fixture inconftest.py. - Port-forward subprocesses are only cleaned up in normal teardown, so it would help to add more defensive cleanup (e.g., finalizers or signal/KeyboardInterrupt handling) to avoid leaving orphaned port-forward processes if pytest exits early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `cluster-type` option accepts `minikube`, but several code paths (e.g., `kruize_local_patch`, optimizer install/overlays, port forwarding, kruize_url) either treat only `kind`/`openshift` or assume `kind`, so it would be good to either fully implement minikube handling or explicitly reject it.
- The `kruize_client` fixture is defined both in `conftest.py` and `test_kruize_api.py`, which is redundant and could lead to confusion; consider keeping a single shared fixture in `conftest.py`.
- Port-forward subprocesses are only cleaned up in normal teardown, so it would help to add more defensive cleanup (e.g., finalizers or signal/KeyboardInterrupt handling) to avoid leaving orphaned port-forward processes if pytest exits early.
## Individual Comments
### Comment 1
<location path="tests/e2e/tests/test_kruize_api.py" line_range="16" />
<code_context>
+
+@pytest.mark.e2e
+@pytest.mark.api
+class TestKruizeAPI:
+ """Test Kruize API endpoints."""
+
</code_context>
<issue_to_address>
**issue (testing):** Add E2E coverage for optimizer webhooks and error handling as described in the PR
This file only exercises the list* endpoints and checks for sysbench experiments, but the PR description calls for end-to-end coverage of optimizer webhooks and their error handling. Please extend the E2E suite to cover:
- Happy-path webhook calls with valid payloads, asserting expected status codes and side effects.
- Negative cases (malformed JSON, missing fields, invalid values, unsupported operations) with precise status codes and error messages.
- Optionally, idempotency / duplicate-submission behavior if that’s part of the contract.
That will better validate webhook-driven workflows rather than just API reachability.
</issue_to_address>
### Comment 2
<location path="tests/e2e/tests/test_kruize_api.py" line_range="79-88" />
<code_context>
+ def test_sysbench_experiment_present(self, kruize_client, config):
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen sysbench experiment assertions to avoid brittle assumptions and make failures more informative
This test usefully checks that a sysbench experiment is created, but it assumes `kubernetes_objects` is non-empty and only inspects the first element. To make it more robust and avoid IndexError or misleading passes, you could:
- Assert that `sysbench_exp['kubernetes_objects']` is a non-empty list before indexing.
- Select the sysbench workload explicitly (e.g., `next(obj for obj in sysbench_exp['kubernetes_objects'] if obj['name'] == 'sysbench')`) and assert namespace/container on that object.
- Optionally assert other key fields (e.g., `type == 'deployment'`, container name, cluster/datasource consistency) so the test catches regressions in experiment construction, not just presence.
</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.e2e | ||
| @pytest.mark.api | ||
| class TestKruizeAPI: |
There was a problem hiding this comment.
issue (testing): Add E2E coverage for optimizer webhooks and error handling as described in the PR
This file only exercises the list* endpoints and checks for sysbench experiments, but the PR description calls for end-to-end coverage of optimizer webhooks and their error handling. Please extend the E2E suite to cover:
- Happy-path webhook calls with valid payloads, asserting expected status codes and side effects.
- Negative cases (malformed JSON, missing fields, invalid values, unsupported operations) with precise status codes and error messages.
- Optionally, idempotency / duplicate-submission behavior if that’s part of the contract.
That will better validate webhook-driven workflows rather than just API reachability.
Comment on lines
+79
to
+88
| def test_sysbench_experiment_present(self, kruize_client, config): | ||
| """Test that sysbench experiment was auto-created by optimizer. | ||
|
|
||
| After the optimizer starts and waits for OPTIMIZER_WAIT_DURATION, | ||
| it should have auto-created experiments for labeled workloads. | ||
| """ | ||
| experiments = kruize_client.list_experiments() | ||
|
|
||
| assert isinstance(experiments, list), "listExperiments should return a list" | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Strengthen sysbench experiment assertions to avoid brittle assumptions and make failures more informative
This test usefully checks that a sysbench experiment is created, but it assumes kubernetes_objects is non-empty and only inspects the first element. To make it more robust and avoid IndexError or misleading passes, you could:
- Assert that
sysbench_exp['kubernetes_objects']is a non-empty list before indexing. - Select the sysbench workload explicitly (e.g.,
next(obj for obj in sysbench_exp['kubernetes_objects'] if obj['name'] == 'sysbench')) and assert namespace/container on that object. - Optionally assert other key fields (e.g.,
type == 'deployment', container name, cluster/datasource consistency) so the test catches regressions in experiment construction, not just presence.
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.
Add a self-contained end-to-end test framework for Kruize Optimizer, including deployment orchestration and comprehensive workflow and webhook tests.
New Features:
Summary by Sourcery
Introduce a Python-based end-to-end testing framework that provisions the Kruize environment, deploys benchmarks and optimizer components, and validates their health and API behavior against a running cluster.
New Features:
Enhancements:
Tests: