Address PR comments

Signed-off-by: lugi0 <[email protected]>

tests/conftest.py

@@ -63,7 +63,7 @@ def ci_s3_bucket_region(pytestconfig: pytest.Config) -> str:
     ci_bucket_region = pytestconfig.option.ci_s3_bucket_region
     if not ci_bucket_region:
         raise ValueError(
-            "Bucket name for the models bucket is not defined."
+            "Region for the ci s3 bucket is not defined."
             "Either pass with `--ci-s3-bucket-region` or set `CI_S3_BUCKET_REGION` environment variable"
         )
     return ci_bucket_region
@@ -74,7 +74,7 @@ def ci_s3_bucket_endpoint(pytestconfig: pytest.Config) -> str:
     ci_bucket_endpoint = pytestconfig.option.ci_s3_bucket_endpoint
     if not ci_bucket_endpoint:
         raise ValueError(
-            "Bucket name for the models bucket is not defined."
+            "Endpoint for the ci s3 bucket is not defined."
             "Either pass with `--ci-s3-bucket-endpoint` or set `CI_S3_BUCKET_ENDPOINT` environment variable"
         )
     return ci_bucket_endpoint
@@ -96,7 +96,7 @@ def models_s3_bucket_region(pytestconfig: pytest.Config) -> str:
     models_bucket_region = pytestconfig.option.models_s3_bucket_region
     if not models_bucket_region:
         raise ValueError(
-            "Bucket name for the models bucket is not defined."
+            "region for the models bucket is not defined."
             "Either pass with `--models-s3-bucket-region` or set `MODELS_S3_BUCKET_REGION` environment variable"
         )
     return models_bucket_region
@@ -107,7 +107,7 @@ def models_s3_bucket_endpoint(pytestconfig: pytest.Config) -> str:
     models_bucket_endpoint = pytestconfig.option.models_s3_bucket_endpoint
     if not models_bucket_endpoint:
         raise ValueError(
-            "Bucket name for the models bucket is not defined."
+            "endpoint for the models bucket is not defined."
             "Either pass with `--models-s3-bucket-endpoint` or set `MODELS_S3_BUCKET_ENDPOINT` environment variable"
         )
     return models_bucket_endpoint

tests/constants.py

@@ -0,0 +1,22 @@
+from enum import Enum
+
+
+class KServeDeploymentType(Enum):
+    SERVERLESS: str = "Serverless"
+    RAW_DEPLOYMENT: str = "RawDeployment"
+
+
+class ModelStoragePath(Enum):
+    FLAN_T5_SMALL: str = "flan-t5-small/flan-t5-small-caikit"
+
+
+class ModelFormat(Enum):
+    CAIKIT: str = "caikit"
+
+
+class CurlOutput(Enum):
+    HEALTH_OK: str = "OK"
+
+
+class ModelEndpoint(Enum):
+    HEALTH: str = "health"

tests/model_serving/model_server/private_endpoint/conftest.py

@@ -13,11 +13,11 @@
 from tests.model_serving.model_server.utils import create_isvc
 from tests.model_serving.model_server.private_endpoint.utils import (
     create_sidecar_pod,
-    get_flan_deployment,
+    get_kserve_predictor_deployment,
     b64_encoded_string,
 )
 from utilities.infra import create_ns
-from tests.model_serving.constants import KSERVE_SERVERLESS
+from tests.constants import KServeDeploymentType, ModelStoragePath, ModelFormat
 
 
 LOGGER = get_logger(name=__name__)
@@ -88,10 +88,10 @@ def endpoint_isvc(
         client=admin_client,
         name="test",
         namespace=endpoint_namespace.name,
-        deployment_mode=KSERVE_SERVERLESS,
+        deployment_mode=KServeDeploymentType.SERVERLESS.value,
         storage_key="endpoint-s3-secret",
-        storage_path="flan-t5-small/flan-t5-small-caikit",
-        model_format="caikit",
+        storage_path=ModelStoragePath.FLAN_T5_SMALL.value,
+        model_format=ModelFormat.CAIKIT.value,
         runtime=endpoint_sr.name,
     ) as isvc:
         yield isvc
@@ -135,7 +135,7 @@ def endpoint_pod_with_istio_sidecar(
     with create_sidecar_pod(
         admin_client=admin_client,
         namespace=endpoint_namespace.name,
-        istio=True,
+        use_istio=True,
         pod_name="test-with-istio",
     ) as pod:
         yield pod
@@ -148,7 +148,7 @@ def endpoint_pod_without_istio_sidecar(
     with create_sidecar_pod(
         admin_client=admin_client,
         namespace=endpoint_namespace.name,
-        istio=False,
+        use_istio=False,
         pod_name="test",
     ) as pod:
         yield pod
@@ -162,7 +162,7 @@ def diff_pod_with_istio_sidecar(
     with create_sidecar_pod(
         admin_client=admin_client,
         namespace=diff_namespace.name,
-        istio=True,
+        use_istio=True,
         pod_name="test-with-istio",
     ) as pod:
         yield pod
@@ -176,15 +176,15 @@ def diff_pod_without_istio_sidecar(
     with create_sidecar_pod(
         admin_client=admin_client,
         namespace=diff_namespace.name,
-        istio=False,
+        use_istio=False,
         pod_name="test",
     ) as pod:
         yield pod
 
 
 @pytest.fixture()
 def ready_predictor(admin_client: DynamicClient, endpoint_isvc: InferenceService) -> None:
-    get_flan_deployment(
+    get_kserve_predictor_deployment(
         namespace=endpoint_isvc.namespace,
         client=admin_client,
         name_prefix=endpoint_isvc.name,

tests/model_serving/model_server/private_endpoint/test_kserve_private_endpoint.py

@@ -6,20 +6,25 @@
 from ocp_resources.pod import Pod
 from ocp_resources.deployment import Deployment
 from tests.model_serving.model_server.private_endpoint.utils import curl_from_pod
+from tests.constants import CurlOutput, ModelEndpoint
 
 
 LOGGER = get_logger(name=__name__)
 
 
 class TestKserveInternalEndpoint:
+    "Tests the internal endpoint of a kserve predictor"
+
     def test_deploy_model_state_loaded(
         self: Self, endpoint_namespace: Namespace, endpoint_isvc: InferenceService, ready_predictor: Deployment
     ) -> None:
+        "Verifies that the predictor gets to state Loaded"
         assert endpoint_isvc.instance.status.modelStatus.states.activeModelState == "Loaded"
 
     def test_deploy_model_url(
         self: Self, endpoint_namespace: Namespace, endpoint_isvc: InferenceService, ready_predictor: Deployment
     ) -> None:
+        "Verifies that the internal endpoint has the expected formatting"
         assert (
             endpoint_isvc.instance.status.address.url
             == f"https://{endpoint_isvc.name}.{endpoint_namespace.name}.svc.cluster.local"
@@ -30,56 +35,56 @@ def test_curl_with_istio_same_ns(
         endpoint_isvc: InferenceService,
         endpoint_pod_with_istio_sidecar: Pod,
     ) -> None:
-        LOGGER.info("Testing curl from the same namespace with a pod part of the service mesh")
+        "Verifies the response from the health endpoint, sending a request from a pod in the same ns and part of the Istio Service Mesh"
 
         curl_stdout = curl_from_pod(
             isvc=endpoint_isvc,
             pod=endpoint_pod_with_istio_sidecar,
-            endpoint="health",
+            endpoint=ModelEndpoint.HEALTH.value,
         )
-        assert curl_stdout == "OK"
+        assert curl_stdout == CurlOutput.HEALTH_OK.value
 
     def test_curl_with_istio_diff_ns(
         self: Self,
         endpoint_isvc: InferenceService,
         diff_pod_with_istio_sidecar: Pod,
     ) -> None:
-        LOGGER.info("Testing curl from a different namespace with a pod part of the service mesh")
+        "Verifies the response from the health endpoint, sending a request from a pod in a different ns and part of the Istio Service Mesh"
 
         curl_stdout = curl_from_pod(
             isvc=endpoint_isvc,
             pod=diff_pod_with_istio_sidecar,
-            endpoint="health",
+            endpoint=ModelEndpoint.HEALTH.value,
             protocol="https",
         )
-        assert curl_stdout == "OK"
+        assert curl_stdout == CurlOutput.HEALTH_OK.value
 
     def test_curl_outside_istio_same_ns(
         self: Self,
         endpoint_isvc: InferenceService,
         endpoint_pod_without_istio_sidecar: Pod,
     ) -> None:
-        LOGGER.info("Testing curl from the same namespace with a pod not part of the service mesh")
+        "Verifies the response from the health endpoint, sending a request from a pod in the same ns and not part of the Istio Service Mesh"
 
         curl_stdout = curl_from_pod(
             isvc=endpoint_isvc,
             pod=endpoint_pod_without_istio_sidecar,
-            endpoint="health",
+            endpoint=ModelEndpoint.HEALTH.value,
             protocol="https",
         )
-        assert curl_stdout == "OK"
+        assert curl_stdout == CurlOutput.HEALTH_OK.value
 
     def test_curl_outside_istio_diff_ns(
         self: Self,
         endpoint_isvc: InferenceService,
         diff_pod_without_istio_sidecar: Pod,
     ) -> None:
-        LOGGER.info("Testing curl from a different namespace with a pod not part of the service mesh")
+        "Verifies the response from the health endpoint, sending a request from a pod in a different ns and not part of the Istio Service Mesh"
 
         curl_stdout = curl_from_pod(
             isvc=endpoint_isvc,
             pod=diff_pod_without_istio_sidecar,
-            endpoint="health",
+            endpoint=ModelEndpoint.HEALTH.value,
             protocol="https",
         )
-        assert curl_stdout == "OK"
+        assert curl_stdout == CurlOutput.HEALTH_OK.value

tests/model_serving/model_server/private_endpoint/utils.py

@@ -47,7 +47,7 @@ def __str__(self) -> str:
         return msg
 
 
-def get_flan_deployment(client: DynamicClient, namespace: str, name_prefix: str) -> Deployment:
+def get_kserve_predictor_deployment(client: DynamicClient, namespace: str, name_prefix: str) -> Deployment:
     deployments = list(
         Deployment.get(
             label_selector=f"serving.kserve.io/inferenceservice={name_prefix}",
@@ -62,9 +62,9 @@ def get_flan_deployment(client: DynamicClient, namespace: str, name_prefix: str)
             deployment.wait_for_replicas()
             return deployment
     elif len(deployments) > 1:
-        raise ResourceNotUniqueError(f"Multiple flan predictor deployments found in namespace {namespace}")
+        raise ResourceNotUniqueError(f"Multiple predictor deployments found in namespace {namespace}")
     else:
-        raise ResourceNotFoundError(f"Flan predictor deployment not found in namespace {namespace}")
+        raise ResourceNotFoundError(f"Predictor deployment not found in namespace {namespace}")
 
 
 def curl_from_pod(
@@ -86,11 +86,11 @@ def curl_from_pod(
 def create_sidecar_pod(
     admin_client: DynamicClient,
     namespace: str,
-    istio: bool,
+    use_istio: bool,
     pod_name: str,
 ) -> Generator[Pod, Any, Any]:
     cmd = f"oc run {pod_name} -n {namespace} --image=registry.access.redhat.com/rhel7/rhel-tools"
-    if istio:
+    if use_istio:
         cmd = f'{cmd} --annotations=sidecar.istio.io/inject="true"'
 
     cmd += " -- sleep infinity"

tests/model_serving/model_server/storage/pvc/conftest.py

@@ -17,7 +17,7 @@
 
 from tests.model_serving.model_server.storage.constants import NFS_STR
 from tests.model_serving.model_server.utils import create_isvc
-from tests.model_serving.constants import KSERVE_SERVERLESS
+from tests.constants import KServeDeploymentType
 from utilities.serving_runtime import ServingRuntimeFromTemplate
 
 
@@ -125,7 +125,7 @@ def inference_service(
         "runtime": serving_runtime.name,
         "storage_uri": f"pvc://{model_pvc.name}/{downloaded_model_data}",
         "model_format": serving_runtime.instance.spec.supportedModelFormats[0].name,
-        "deployment_mode": request.param.get("deployment-mode", KSERVE_SERVERLESS),
+        "deployment_mode": request.param.get("deployment-mode", KServeDeploymentType.SERVERLESS.value),
     }
 
     if min_replicas := request.param.get("min-replicas"):

tests/model_serving/model_server/storage/pvc/test_kserve_pvc_rwx.py

@@ -1,6 +1,6 @@
 import shlex
 from typing import List
-from tests.model_serving.constants import KSERVE_SERVERLESS
+from tests.constants import KServeDeploymentType
 
 import pytest
 
@@ -25,7 +25,7 @@
             {"model-dir": "test-dir"},
             {"access-modes": "ReadWriteMany", "storage-class-name": NFS_STR},
             KSERVE_OVMS_SERVING_RUNTIME_PARAMS,
-            INFERENCE_SERVICE_PARAMS | {"deployment-mode": KSERVE_SERVERLESS, "min-replicas": 2},
+            INFERENCE_SERVICE_PARAMS | {"deployment-mode": KServeDeploymentType.SERVERLESS.value, "min-replicas": 2},
         )
     ],
     indirect=True,