chore(optimizer): Add unit tests for Optimizer KubernetesBackend#334
chore(optimizer): Add unit tests for Optimizer KubernetesBackend#334ruskaruma wants to merge 1 commit intokubeflow:mainfrom
Conversation
…flow#126) Signed-off-by: ruskaruma <ishaan.sinha10@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ruskaruma
changed the title
There was a problem hiding this comment.
Pull request overview
Adds comprehensive unit tests for Optimizer’s KubernetesBackend to cover all public methods and expands mocked Kubernetes/Katib interactions to support those scenarios.
Changes:
- Added fixtures + mock handlers to simulate Katib Experiment/Trial CRUD, events, and logs.
- Added new parametrized tests covering
optimize, job lifecycle methods, logs, results, polling, deletion, and events. - Renamed prior
test_optimizetotest_optimize_no_input_mutationto avoid a name collision.
| ) | ||
| def test_optimize(kubernetes_backend, test_case): | ||
| """Test KubernetesBackend.optimize and Experiment CR creation.""" | ||
| print("Executing test:", test_case.name) |
There was a problem hiding this comment.
pytest captures stdout; these print() calls add noise and can make failures harder to scan. Prefer removing them, or use pytest’s reporting mechanisms (e.g., -vv already shows parametrized case IDs) / caplog if you need diagnostic output.
|
|
||
| except Exception as e: | ||
| assert type(e) is test_case.expected_error | ||
| print("test execution complete") |
There was a problem hiding this comment.
pytest captures stdout; these print() calls add noise and can make failures harder to scan. Prefer removing them, or use pytest’s reporting mechanisms (e.g., -vv already shows parametrized case IDs) / caplog if you need diagnostic output.
| ) | ||
| def test_list_jobs(kubernetes_backend, test_case): | ||
| """Test KubernetesBackend.list_jobs returns expected OptimizationJob list.""" | ||
| print("Executing test:", test_case.name) |
There was a problem hiding this comment.
pytest captures stdout; these print() calls add noise and can make failures harder to scan. Prefer removing them, or use pytest’s reporting mechanisms (e.g., -vv already shows parametrized case IDs) / caplog if you need diagnostic output.
|
|
||
| except Exception as e: | ||
| assert type(e) is test_case.expected_error | ||
| print("test execution complete") |
There was a problem hiding this comment.
pytest captures stdout; these print() calls add noise and can make failures harder to scan. Prefer removing them, or use pytest’s reporting mechanisms (e.g., -vv already shows parametrized case IDs) / caplog if you need diagnostic output.
| ) | ||
| def test_get_job(kubernetes_backend, test_case): | ||
| """Test KubernetesBackend.get_job with success and error paths.""" | ||
| print("Executing test:", test_case.name) |
There was a problem hiding this comment.
pytest captures stdout; these print() calls add noise and can make failures harder to scan. Prefer removing them, or use pytest’s reporting mechanisms (e.g., -vv already shows parametrized case IDs) / caplog if you need diagnostic output.
| except Exception as e: | ||
| assert type(e) is test_case.expected_error |
There was a problem hiding this comment.
Using type(e) is ... is brittle and fails for subclasses; it also makes it harder to accept wrapped exceptions (common around K8s client calls). Prefer isinstance(e, test_case.expected_error) and, where helpful, assert on message/context separately.
| def conditional_error_handler(*args, **kwargs): | ||
| """Raise simulated errors based on resource name.""" | ||
| if args[2] == TIMEOUT: | ||
| raise multiprocessing.TimeoutError() |
There was a problem hiding this comment.
The mocks raise multiprocessing.TimeoutError, but the parametrized cases expect TimeoutError. That makes the tests rely on an implicit conversion inside the backend (or they’ll break if that behavior changes). Consider raising built-in TimeoutError directly in these mock handlers (or update expected exception types consistently) so the tests assert the backend behavior rather than a specific internal translation path.
| def get_namespaced_custom_object_response(*args, **kwargs): | ||
| """Return a mocked Experiment CR via mock async thread.""" | ||
| mock_thread = Mock() | ||
| if args[2] == TIMEOUT or args[4] == TIMEOUT: | ||
| raise multiprocessing.TimeoutError() |
There was a problem hiding this comment.
The mocks raise multiprocessing.TimeoutError, but the parametrized cases expect TimeoutError. That makes the tests rely on an implicit conversion inside the backend (or they’ll break if that behavior changes). Consider raising built-in TimeoutError directly in these mock handlers (or update expected exception types consistently) so the tests assert the backend behavior rather than a specific internal translation path.
| def list_namespaced_custom_object_response(*args, **kwargs): | ||
| """Return ExperimentList or TrialList via mock async thread.""" | ||
| mock_thread = Mock() | ||
| if args[2] == TIMEOUT: | ||
| raise multiprocessing.TimeoutError() |
There was a problem hiding this comment.
The mocks raise multiprocessing.TimeoutError, but the parametrized cases expect TimeoutError. That makes the tests rely on an implicit conversion inside the backend (or they’ll break if that behavior changes). Consider raising built-in TimeoutError directly in these mock handlers (or update expected exception types consistently) so the tests assert the backend behavior rather than a specific internal translation path.
|
|
||
| assert test_case.expected_status == SUCCESS | ||
| assert isinstance(name, str) | ||
| assert len(name) == 12 |
There was a problem hiding this comment.
Asserting an exact generated name length is a fragile contract for a unit test (small changes to name generation can cause unrelated failures). Prefer asserting properties that matter to behavior (e.g., non-empty, DNS-1123-compliant if required, and that the generated name is used in the Experiment metadata passed to create_namespaced_custom_object).
| assert len(name) == 12 | |
| assert len(name) > 0 |
What this PR does / why we need it:
Adds unit tests for the Optimizer KubernetesBackend, covering all 8 public methods:
optimize,list_jobs,get_job,get_job_logs,get_best_results,wait_for_job_status,delete_job, andget_job_events. Addressesreview feedback from #224 which went stale.
Also renames the existing
test_optimize(input mutation test) totest_optimize_no_input_mutationto avoid name collision with the newtest_optimizethat verifies Experiment CR creation.Which issue(s) this PR fixes:
Fixes #126
Checklist: