POC PR - Unit testing with mocking <Not for merge>#1754
Draft
pinkygupta-hub wants to merge 3 commits intokruize:mvp_demofrom
Draft
POC PR - Unit testing with mocking <Not for merge>#1754pinkygupta-hub wants to merge 3 commits intokruize:mvp_demofrom
pinkygupta-hub wants to merge 3 commits intokruize:mvp_demofrom
Conversation
Contributor
Reviewer's GuideAdds mocked unit test coverage for the bulk REST API, including fully and partially mocked scenarios to validate job_id handling, filtering behavior, and connection failures without requiring a live cluster. 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 2 issues, and left some high level feedback:
- The test module manipulates
sys.path(sys.path.append("../../")) to import helpers; consider using a proper Python package layout andconftest.py/pytestpythonpathconfiguration instead so tests don't rely on relative path hacks. - In
test_bulk_api_partial_mocked, theexpectedparameter from the parametrization is never used; if the intent is to validate filter shaping, add explicit assertions against the payload passed to the mocked calls or removeexpectedto avoid confusion. - Calling
logging.basicConfigat import time in the test module can interfere with other tests' logging configuration; prefer usingcaplogor per-test logger setup rather than globally configuring logging in the test file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test module manipulates `sys.path` (`sys.path.append("../../")`) to import helpers; consider using a proper Python package layout and `conftest.py`/pytest `pythonpath` configuration instead so tests don't rely on relative path hacks.
- In `test_bulk_api_partial_mocked`, the `expected` parameter from the parametrization is never used; if the intent is to validate filter shaping, add explicit assertions against the payload passed to the mocked calls or remove `expected` to avoid confusion.
- Calling `logging.basicConfig` at import time in the test module can interfere with other tests' logging configuration; prefer using `caplog` or per-test logger setup rather than globally configuring logging in the test file.
## Individual Comments
### Comment 1
<location> `tests/scripts/local_monitoring_tests/rest_apis/test_bulk_mocked.py:86-95` </location>
<code_context>
+@patch.object(helpers.kruize, "URL", "http://mock-cluster:8080", create=True)
</code_context>
<issue_to_address>
**issue (testing):** Parametrized `expected` filter is never asserted, so the partial-mocked test does not actually validate filter behavior
This test parametrizes `filter_setup` and `expected`, but only checks `response.status_code == 200`; `expected` is never used. As written, it doesn’t actually verify how include/exclude filters or label modes are handled.
Please add assertions that:
- The POST is called with the correct payload derived from `filter_setup` (e.g. via `mock_post_bulk_api.assert_called_with` or `call_args`).
- The GET response or any logic using `get_bulk_job_status` is interpreted as intended.
If the intent is to validate filter composition, use `expected` to assert on the downstream structure/query. Otherwise, consider removing `expected` from the parameters to avoid implying behavior the test doesn’t cover.
</issue_to_address>
### Comment 2
<location> `tests/scripts/local_monitoring_tests/rest_apis/test_bulk_mocked.py:7-12` </location>
<code_context>
+import sys
+from unittest.mock import patch, MagicMock
+
+sys.path.append("../../")
+
+# Kruize helpers
+import helpers.kruize
+from helpers.kruize import post_bulk_api
+from helpers.utils import form_kruize_url, get_kruize_url
+
+
</code_context>
<issue_to_address>
**nitpick:** Consider avoiding `sys.path.append` in tests in favor of a package-aware import strategy
Using `sys.path.append("../../")` makes the tests sensitive to how they’re invoked (repo root vs subdirectory, IDE vs CI), which can cause hard‑to‑debug import issues. Prefer a package-aware approach instead, for example:
- Structure the project as a package (with `__init__.py`) and use normal absolute imports, or
- Configure `pytest` (e.g., via `conftest.py` / `pythonpath`) to expose shared helpers.
This isn’t functionally incorrect, but updating it would improve maintainability and environment-independence of the tests.
</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
+86
to
+95
| @patch.object(helpers.kruize, "URL", "http://mock-cluster:8080", create=True) | ||
| @patch("helpers.kruize.requests.post") | ||
| @patch("helpers.kruize.get_bulk_job_status") | ||
| @pytest.mark.parametrize( | ||
| "filter_setup, expected", | ||
| [ | ||
| ({"include": {"namespace": ["default"], "workload": ["wl1"], "containers": ["ctr1"]}}, | ||
| {"namespace": ["default"], "workload": ["wl1"], "containers": ["ctr1"]}), | ||
| ({"include": {"namespace": ["default"]}}, {"namespace": ["default"]}), | ||
| ({"include": {"labels": {"cost": "true"}}}, {"labels": {"cost": "true"}, "mode": "include"}), |
Contributor
There was a problem hiding this comment.
issue (testing): Parametrized expected filter is never asserted, so the partial-mocked test does not actually validate filter behavior
This test parametrizes filter_setup and expected, but only checks response.status_code == 200; expected is never used. As written, it doesn’t actually verify how include/exclude filters or label modes are handled.
Please add assertions that:
- The POST is called with the correct payload derived from
filter_setup(e.g. viamock_post_bulk_api.assert_called_withorcall_args). - The GET response or any logic using
get_bulk_job_statusis interpreted as intended.
If the intent is to validate filter composition, use expected to assert on the downstream structure/query. Otherwise, consider removing expected from the parameters to avoid implying behavior the test doesn’t cover.
Comment on lines
+7
to
+12
| sys.path.append("../../") | ||
|
|
||
| # Kruize helpers | ||
| import helpers.kruize | ||
| from helpers.kruize import post_bulk_api | ||
| from helpers.utils import form_kruize_url, get_kruize_url |
Contributor
There was a problem hiding this comment.
nitpick: Consider avoiding sys.path.append in tests in favor of a package-aware import strategy
Using sys.path.append("../../") makes the tests sensitive to how they’re invoked (repo root vs subdirectory, IDE vs CI), which can cause hard‑to‑debug import issues. Prefer a package-aware approach instead, for example:
- Structure the project as a package (with
__init__.py) and use normal absolute imports, or - Configure
pytest(e.g., viaconftest.py/pythonpath) to expose shared helpers.
This isn’t functionally incorrect, but updating it would improve maintainability and environment-independence of the tests.
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
pinkygupta-hub
changed the title
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
POC: Add Unit Tests with Mocked Dependencies (Reduce Live Cluster Dependency)
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
Introduce mocked unit tests for the bulk REST API to validate behavior without requiring a live cluster.
Tests: