Repository: kubeflow/training-operator (language: python)

Issue:
[SDK] Add more unit tests for TrainingClient APIs
We need to add more unit tests for `TrainingClient` APIs by [updating the `training_client_test.py`.](https://github.com/kubeflow/training-operator/blob/8909f2bfd4680ef7a90672b690b0f4df1d6fdfb9/sdk/python/kubeflow/training/api/training_client_test.py)
That should help us to detect bugs like this one: https://github.com/kubeflow/training-operator/pull/2160.
Let's use this issue to track unit tests for various public APIs.


Feel free to submit PR to add unit tests for one of the following APIs, just comment on this issue on which unit tests you are working on:

- [x] `create_job` https://github.com/kubeflow/training-operator/pull/1938
- [x] `wait_for_job_conditions` https://github.com/kubeflow/training-operator/pull/2196
- [x] `get_job` https://github.com/kubeflow/training-operator/pull/2205
- [x] `list_jobs` https://github.com/kubeflow/training-operator/pull/2267
- [x] `get_job_conditions` https://github.com/kubeflow/training-operator/pull/2253
- [x] `is_job_created` https://github.com/kubeflow/training-operator/pull/2253
- [x] `is_job_running` https://github.com/kubeflow/training-operator/pull/2253
- [x] `is_job_restarting` https://github.com/kubeflow/training-operator/pull/2253
- [x] `is_job_succeeded` https://github.com/kubeflow/training-operator/pull/2253
- [x] `is_job_failed` https://github.com/kubeflow/training-operator/pull/2253
- [x] `get_job_pods` https://github.com/kubeflow/training-operator/pull/2175
- [x] `get_job_pod_names` https://github.com/kubeflow/training-operator/pull/2192
- [x] `get_job_logs` https://github.com/kubeflow/training-operator/pull/2275
- [x] `update_job` https://github.com/kubeflow/training-operator/pull/2192
- [x] `delete_job` https://github.com/kubeflow/training-operator/pull/2232

cc @kubeflow/wg-training-leads @deepanker13 @Electronic-Waste @tariq-hasan 

/good-first-issue
/area sdk
/area testing


Discussion / hints:
@andreyvelich: 
	This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed [here](https://git.k8s.io/community/contributors/guide/help-wanted.md#good-first-issue).

If this request no longer meets these requirements, the label can be removed
by commenting with the `/remove-good-first-issue` command.


<details>

In response to [this](https://github.com/kubeflow/training-operator/issues/2161):

>We need to add more unit tests for `TrainingClient` APIs by [updating the `training_client_test.py`.](https://github.com/kubeflow/training-operator/blob/8909f2bfd4680ef7a90672b690b0f4df1d6fdfb9/sdk/python/kubeflow/training/api/training_client_test.py)
>That should help us to detect bugs like this one: https://github.com/kubeflow/training-operator/pull/2160.
>Let's use this issue to track unit tests for various public APIs.
>
>
>Feel free to submit PR to add unit tests for one of the following APIs, just comment on this issue on which unit tests you are working on:
>
>- [x] `create_job`
>- [ ] `wait_for_job_conditions`
>- [ ] `get_job`
>- [ ] `list_jobs`
>- [ ] `get_job_conditions` 
>- [ ] `is_job_created`
>- [ ] `is_job_running`
>- [ ] `is_job_restarting`
>- [ ] `is_job_succeeded`
>- [ ] `is_job_failed`
>- [ ] `get_job_pods`
>- [ ] `get_job_pod_names`
>- [ ] `get_job_logs`
>- [ ] `update_job`
>- [ ] `delete_job`
>
>cc @kubeflow/wg-training-leads @deepanker13 @Electronic-Waste @tariq-hasan 
>
>/good-first-issue
>/area sdk
>/area testing


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
Hello @andreyvelich I am working on writing unit test for get_job, so should I seperate the test_data both for create_job and get_job individually or combine them into single test_data by adding one more identifier to each sample which tells for which function ( create_job or get_job) the test is for? 

Example test data -

   `(
        "create_job_valid_flow_to_create_job_using_image",
        "create_job",
        {
            "name": "test-job",
            "namespace": "test",
            "base_image": "docker.io/test-training",
            "num_workers": 2,
        },
        "success",
    )`

  `(
        "get_job_valid",
        "get_job",
        {"namespace": "test", "job_name": "valid_job"},
        {"mock": "job_response"}
    ),`
Thank you for your contribution @ayushrakesh!

I think it is easier to separate tests data for `create_job` and `get_job` API, so it will be easier for us to maintain those lists.
 
WDYT @droctothorpe @deepanker13 @kubeflow/wg-training-leads ?
> Thank you for your contribution @ayushrakesh!
> 
> I think it is easier to separate tests data for `create_job` and `get_job` API, so it will be easier for us to maintain those lists.
> 
> WDYT @droctothorpe @deepanker13 @kubeflow/wg-training-leads ?

As my understanding, this is unit tests, not integration tests. So, dedicated test data every for functions would be better.
In general, if you can recycle fixtures to reduce redundancy, that's usually advisable; whether or not that's possible depends on the expected IO of the two functions; `create_jobs` doesn't return anything (though maybe it should), so I'm not sure if it makes sense to consolidate the fixtures in this context. 
@andreyvelich Thank you for guiding me in contributing to Training-Operator and Katib.

I'll write some test cases for `training_client_test.py` as soon as I understand the whole logics and the source code of Training-Operator.
Thank you @Electronic-Waste.
So, maybe @ayushrakesh can create unit test for `get_job` API and @Electronic-Waste can work on 
`wait_for_job_conditions` API.
I'm interested in working on the unit test for **get_job_pods**, any objection?
Sure, thank you for your time @YosiElias!
/assign @ayushrakesh @Electronic-Waste @YosiElias 
Having a different list will make code more readable, Also should we create a folder and have a  test file for each function to make it more organised? @andreyvelich 
> Having a different list will make code more readable, Also should we create a folder and have a test file for each function to make it more organised? @andreyvelich

@deepanker13 Usually, test file have the same name as the actual file with `_test` suffix, 
E.g. you can check the KFP client example: https://github.com/kubeflow/pipelines/tree/master/sdk/python/kfp/client.

Will the number of test cases be too many to keep in a single `training_client_test` module?

I was thinking of moving test data, fixtures and utility functions from the [katib_client_test](https://github.com/kubeflow/katib/blob/master/sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py) module, for example, and putting these in newly created `test_data`, `conftest` and `utils` modules.

That way, I was thinking that the code may be easier to maintain in the `katib_client_test` module.

And `katib_client_test` and its associated `test_data`, `conftest` and `utils` modules would be moved to a newly created `sdk/python/v1beta1/kubeflow/katib/tests` folder.

Please let me know your thoughts.

Reference: https://github.com/kubeflow/katib/compare/master...tariq-hasan:katib:add-unit-test-for-katib-client-tune
@tariq-hasan I think, we discussed before with @droctothorpe that we want to keep `_test` file alongside with actual files: https://github.com/kubeflow/katib/issues/2184#issuecomment-1660475384.
I understand that this cause file to grow, but we are already doing the same for go unit tests. For example: https://github.com/kubeflow/katib/blob/master/pkg/webhook/v1beta1/experiment/validator/validator_test.go.

I feel that the test data should be associated with function that we test: `get_job`, `create_job`, `train`, etc.
KFP follows the same way: https://github.com/kubeflow/pipelines/tree/master/sdk/python/kfp/client.

Any concerns you see with that @tariq-hasan ?

That sounds good. Thanks for the clarification.

Fix patch:
diff --git a/sdk/python/kubeflow/training/constants/constants.py b/sdk/python/kubeflow/training/constants/constants.py
index 0513c3e31e..76cd1967d0 100644
--- a/sdk/python/kubeflow/training/constants/constants.py
+++ b/sdk/python/kubeflow/training/constants/constants.py
@@ -22,6 +22,9 @@
 # The default PIP index URL to download Python packages.
 DEFAULT_PIP_INDEX_URL = "https://pypi.org/simple"
 
+# The default namespace in case namespace not define explicitly
+DEFAULT_NAMESPACE = "default"
+
 # Annotation to disable Istio sidecar.
 ISTIO_SIDECAR_INJECTION = "sidecar.istio.io/inject"
 
diff --git a/sdk/python/kubeflow/training/utils/utils.py b/sdk/python/kubeflow/training/utils/utils.py
index 250252677f..04665951de 100644
--- a/sdk/python/kubeflow/training/utils/utils.py
+++ b/sdk/python/kubeflow/training/utils/utils.py
@@ -69,7 +69,7 @@ def is_running_in_k8s():
 
 def get_default_target_namespace():
     if not is_running_in_k8s():
-        return "default"
+        return constants.DEFAULT_NAMESPACE
     with open("/var/run/secrets/kubernetes.io/serviceaccount/namespace", "r") as f:
         return f.readline()
 


Test patch:
diff --git a/sdk/python/kubeflow/training/api/training_client_test.py b/sdk/python/kubeflow/training/api/training_client_test.py
index e4d76b2766..04187ac20c 100644
--- a/sdk/python/kubeflow/training/api/training_client_test.py
+++ b/sdk/python/kubeflow/training/api/training_client_test.py
@@ -17,6 +17,9 @@
 from kubernetes.client import V1Container
 from kubernetes.client import V1ResourceRequirements
 
+LIST_RESPONSE = [{"metadata": {"name": "Dummy V1PodList"}}]
+TEST_NAME = "test"
+
 
 def create_namespaced_custom_object_response(*args, **kwargs):
     if args[2] == "timeout":
@@ -25,6 +28,20 @@ def create_namespaced_custom_object_response(*args, **kwargs):
         raise RuntimeError()
 
 
+def list_namespaced_pod_response(*args, **kwargs):
+    class MockResponse:
+        def get(self, timeout):
+            # Simulate a response from the Kubernetes API, and pass timeout for verification
+            LIST_RESPONSE[0]["timeout"] = timeout
+            if args[0] == "timeout":
+                raise multiprocessing.TimeoutError()
+            if args[0] == "runtime":
+                raise Exception()
+            return Mock(items=LIST_RESPONSE)
+
+    return MockResponse()
+
+
 def generate_container() -> V1Container:
     return V1Container(
         name="pytorch",
@@ -55,7 +72,7 @@ def generate_pytorchjob(
 
 
 def create_job():
-    job_namespace = "test"
+    job_namespace = TEST_NAME
     container = generate_container()
     master = KubeflowOrgV1ReplicaSpec(
         replicas=1,
@@ -87,10 +104,10 @@ def __init__(self, kind) -> None:
         self.kind = kind
 
 
-test_data = [
+test_data_create_job = [
     (
         "invalid extra parameter",
-        {"job": create_job(), "namespace": "test", "base_image": "test_image"},
+        {"job": create_job(), "namespace": TEST_NAME, "base_image": "test_image"},
         ValueError,
     ),
     ("invalid job kind", {"job_kind": "invalid_job_kind"}, ValueError),
@@ -149,14 +166,14 @@ def __init__(self, kind) -> None:
     ),
     (
         "valid flow",
-        {"job": create_job(), "namespace": "test"},
+        {"job": create_job(), "namespace": TEST_NAME},
         "success",
     ),
     (
         "valid flow to create job from func",
         {
             "name": "test-job",
-            "namespace": "test",
+            "namespace": TEST_NAME,
             "train_func": lambda: print("Test Training Function"),
             "base_image": "docker.io/test-training",
             "num_workers": 3,
@@ -169,7 +186,7 @@ def __init__(self, kind) -> None:
         "valid flow to create job using image",
         {
             "name": "test-job",
-            "namespace": "test",
+            "namespace": TEST_NAME,
             "base_image": "docker.io/test-training",
             "num_workers": 2,
         },
@@ -177,6 +194,61 @@ def __init__(self, kind) -> None:
     ),
 ]
 
+test_data_get_job_pods = [
+    (
+        "valid flow with default namespace and default timeout",
+        {
+            "name": TEST_NAME,
+        },
+        f"{constants.JOB_NAME_LABEL}={TEST_NAME}",
+        LIST_RESPONSE,
+    ),
+    (
+        "invalid replica_type",
+        {"name": TEST_NAME, "replica_type": "invalid_replica_type"},
+        "Label not relevant",
+        ValueError,
+    ),
+    (
+        "invalid replica_type (uppercase)",
+        {"name": TEST_NAME, "replica_type": constants.REPLICA_TYPE_WORKER},
+        "Label not relevant",
+        ValueError,
+    ),
+    (
+        "valid flow with specific timeout, replica_index, replica_type and master role",
+        {
+            "name": TEST_NAME,
+            "namespace": "test_namespace",
+            "timeout": 60,
+            "is_master": True,
+            "replica_type": constants.REPLICA_TYPE_MASTER.lower(),
+            "replica_index": 0,
+        },
+        f"{constants.JOB_NAME_LABEL}={TEST_NAME},{constants.JOB_ROLE_LABEL}={constants.JOB_ROLE_MASTER}"
+        f",{constants.REPLICA_TYPE_LABEL}={constants.REPLICA_TYPE_MASTER.lower()},{constants.REPLICA_INDEX_LABEL}=0",
+        LIST_RESPONSE,
+    ),
+    (
+        "invalid flow with TimeoutError",
+        {
+            "name": TEST_NAME,
+            "namespace": "timeout",
+        },
+        "Label not relevant",
+        TimeoutError,
+    ),
+    (
+        "invalid flow with RuntimeError",
+        {
+            "name": TEST_NAME,
+            "namespace": "runtime",
+        },
+        "Label not relevant",
+        RuntimeError,
+    ),
+]
+
 
 @pytest.fixture
 def training_client():
@@ -187,14 +259,19 @@ def training_client():
                 side_effect=create_namespaced_custom_object_response
             )
         ),
-    ), patch("kubernetes.client.CoreV1Api", return_value=Mock()), patch(
+    ), patch(
+        "kubernetes.client.CoreV1Api",
+        return_value=Mock(
+            list_namespaced_pod=Mock(side_effect=list_namespaced_pod_response)
+        ),
+    ), patch(
         "kubernetes.config.load_kube_config", return_value=Mock()
     ):
         client = TrainingClient(job_kind=constants.PYTORCHJOB_KIND)
         yield client
 
 
-@pytest.mark.parametrize("test_name,kwargs,expected_output", test_data)
+@pytest.mark.parametrize("test_name,kwargs,expected_output", test_data_create_job)
 def test_create_job(training_client, test_name, kwargs, expected_output):
     """
     test create_job function of training client
@@ -206,3 +283,29 @@ def test_create_job(training_client, test_name, kwargs, expected_output):
     except Exception as e:
         assert type(e) is expected_output
     print("test execution complete")
+
+
+@pytest.mark.parametrize(
+    "test_name,kwargs,expected_label_selector,expected_output",
+    test_data_get_job_pods,
+)
+def test_get_job_pods(
+    training_client, test_name, kwargs, expected_label_selector, expected_output
+):
+    """
+    test get_job_pods function of training client
+    """
+    print("Executing test:", test_name)
+    try:
+        out = training_client.get_job_pods(**kwargs)
+        # Verify that list_namespaced_pod called with specified arguments
+        training_client.core_api.list_namespaced_pod.assert_called_with(
+            kwargs.get("namespace", constants.DEFAULT_NAMESPACE),
+            label_selector=expected_label_selector,
+            async_req=True,
+        )
+        assert out[0].pop("timeout") == kwargs.get("timeout", constants.DEFAULT_TIMEOUT)
+        assert out == expected_output
+    except Exception as e:
+        assert type(e) is expected_output
+    print("test execution complete")