Applying review comments

Author: petyaslavova

tests/test_scenario/fault_injector_client.py

@@ -1,9 +1,21 @@
 import json
+import logging
+import time
 import urllib.request
-from typing import Dict, Any, Optional, Union
+from typing import Dict, Any, Optional, Tuple, Union
 from enum import Enum
 
+import pytest
 
+class TaskStatuses:
+    """Class to hold completed statuses constants."""
+
+    FAILED = "failed"
+    FINISHED = "finished"
+    SUCCESS = "success"
+    RUNNING = "running"
+
+    COMPLETED_STATUSES = [FAILED, FINISHED, SUCCESS]
 class ActionType(str, Enum):
     DMC_RESTART = "dmc_restart"
     FAILOVER = "failover"
@@ -103,3 +115,34 @@ def execute_rladmin_command(
                 error_body = json.loads(e.read().decode("utf-8"))
                 raise ValueError(f"Validation Error: {error_body}")
             raise
+
+    def get_operation_result(
+        self,
+        action_id: str,
+        timeout: int = 60,
+    ) -> Dict[str, Any]:
+        """Get the result of a specific action"""
+        start_time = time.time()
+        check_interval = 3
+        while time.time() - start_time < timeout:
+            try:
+                status_result = self.get_action_status(action_id)
+                operation_status = status_result.get("status", "unknown")
+
+                if operation_status in TaskStatuses.COMPLETED_STATUSES:
+                    logging.debug(
+                        f"Operation {action_id} completed with status: "
+                        f"{operation_status}"
+                    )
+                    if operation_status != TaskStatuses.SUCCESS:
+                        pytest.fail(
+                            f"Operation {action_id} failed: {status_result}"
+                        )
+                    return status_result
+
+                time.sleep(check_interval)
+            except Exception as e:
+                logging.warning(f"Error checking operation status: {e}")
+                time.sleep(check_interval)
+        else:
+            raise TimeoutError(f"Timeout waiting for operation {action_id}")

tests/test_scenario/hitless_upgrade_helpers.py

@@ -9,20 +9,10 @@
     ActionRequest,
     ActionType,
     FaultInjectorClient,
+    TaskStatuses,
 )
 
 
-class TaskStatuses:
-    """Class to hold completed statuses constants."""
-
-    FAILED = "failed"
-    FINISHED = "finished"
-    SUCCESS = "success"
-    RUNNING = "running"
-
-    COMPLETED_STATUSES = [FAILED, FINISHED, SUCCESS]
-
-
 class ClientValidations:
     @staticmethod
     def wait_push_notification(
@@ -32,15 +22,15 @@ def wait_push_notification(
     ):
         """Wait for a push notification to be received."""
         start_time = time.time()
-        check_interval = 1  # Check more frequently during operations
+        check_interval = 0.2  # Check more frequently during operations
         test_conn = (
             connection if connection else redis_client.connection_pool.get_connection()
         )
 
         try:
             while time.time() - start_time < timeout:
                 try:
-                    if test_conn.can_read(timeout=0.5):
+                    if test_conn.can_read(timeout=0.2):
                         # reading is important, it triggers the push notification
                         push_response = test_conn.read_response(push_request=True)
                         logging.debug(
@@ -61,33 +51,6 @@ def wait_push_notification(
 
 
 class ClusterOperations:
-    @staticmethod
-    def get_operation_result(
-        fault_injector: FaultInjectorClient,
-        action_id: str,
-        timeout: int = 60,
-    ) -> Tuple[str, dict]:
-        """Get the result of a specific action"""
-        start_time = time.time()
-        check_interval = 3
-        while time.time() - start_time < timeout:
-            try:
-                status_result = fault_injector.get_action_status(action_id)
-                operation_status = status_result.get("status", "unknown")
-
-                if operation_status in TaskStatuses.COMPLETED_STATUSES:
-                    logging.debug(
-                        f"Operation {action_id} completed with status: "
-                        f"{operation_status}"
-                    )
-                    return operation_status, status_result
-
-                time.sleep(check_interval)
-            except Exception as e:
-                logging.warning(f"Error checking operation status: {e}")
-                time.sleep(check_interval)
-        else:
-            raise TimeoutError(f"Timeout waiting for operation {action_id}")
 
     @staticmethod
     def get_cluster_nodes_info(
@@ -113,16 +76,11 @@ def get_cluster_nodes_info(
                     f"Failed to trigger get cluster status action for bdb_id {bdb_id}: {trigger_action_result}"
                 )
 
-            status, action_status_check_response = (
-                ClusterOperations.get_operation_result(
-                    fault_injector, action_id, timeout=timeout
+            action_status_check_response = (
+                fault_injector.get_operation_result(
+                    action_id, timeout=timeout
                 )
             )
-
-            if status != TaskStatuses.SUCCESS:
-                pytest.fail(
-                    f"Failed to get cluster nodes info: {action_status_check_response}"
-                )
             logging.info(
                 f"Completed cluster nodes info reading: {action_status_check_response}"
             )

tests/test_scenario/test_hitless_upgrade.py

@@ -135,11 +135,10 @@ def _execute_migration(
 
         self._migration_executed = True
 
-        migrate_status, migrate_result = ClusterOperations.get_operation_result(
-            fault_injector_client, migrate_action_id, timeout=MIGRATE_TIMEOUT
+        migrate_result = fault_injector_client.get_operation_result(
+            migrate_action_id, timeout=MIGRATE_TIMEOUT
         )
-        if migrate_status != TaskStatuses.SUCCESS:
-            pytest.fail(f"Failed to execute rladmin migrate: {migrate_result}")
+        logging.debug(f"Migration result: {migrate_result}")
 
     def _execute_bind(
         self,
@@ -153,11 +152,10 @@ def _execute_bind(
 
         self._bind_executed = True
 
-        bind_status, bind_result = ClusterOperations.get_operation_result(
-            fault_injector_client, bind_action_id, timeout=BIND_TIMEOUT
+        bind_result = fault_injector_client.get_operation_result(
+            bind_action_id, timeout=BIND_TIMEOUT
         )
-        if bind_status != TaskStatuses.SUCCESS:
-            pytest.fail(f"Failed to execute rladmin bind endpoint: {bind_result}")
+        logging.debug(f"Bind result: {bind_result}")
 
     def _execute_migrate_bind_flow(
         self,