Merge pull request lightspeed-core#970 from tisnik/lcore-1141-better-comments-in-e2e-tests

LCORE-1141: better comments in e2e tests

Author: tisnik

tests/e2e/features/environment.py

@@ -50,7 +50,25 @@ def _fetch_models_from_service() -> dict:
 
 
 def before_all(context: Context) -> None:
-    """Run before and after the whole shooting match."""
+    """Run before and after the whole shooting match.
+
+    Initialize global test environment before the test suite runs.
+
+    Sets context.deployment_mode from the E2E_DEPLOYMENT_MODE environment
+    variable (default "server") and context.is_library_mode accordingly.
+
+    Attempts to detect a default LLM model and provider via
+    _fetch_models_from_service() and stores results in context.default_model
+    and context.default_provider; if detection fails, falls back to
+    "gpt-4-turbo" and "openai".
+
+    Parameters:
+        context (Context): Behave context into which this function writes:
+            - deployment_mode (str): "server" or "library".
+            - is_library_mode (bool): True when deployment_mode is "library".
+            - default_model (str): Detected model id or fallback model.
+            - default_provider (str): Detected provider id or fallback provider.
+    """
     # Detect deployment mode from environment variable
     context.deployment_mode = os.getenv("E2E_DEPLOYMENT_MODE", "server").lower()
     context.is_library_mode = context.deployment_mode == "library"
@@ -76,7 +94,18 @@ def before_all(context: Context) -> None:
 
 
 def before_scenario(context: Context, scenario: Scenario) -> None:
-    """Run before each scenario is run."""
+    """Run before each scenario is run.
+
+    Prepare scenario execution by skipping scenarios based on tags and
+    selecting a scenario-specific configuration.
+
+    Skips the scenario if it has the `skip` tag, if it has the `local` tag
+    while the test run is not in local mode, or if it has
+    `skip-in-library-mode` when running in library mode. When the scenario is
+    tagged with `InvalidFeedbackStorageConfig` or `NoCacheConfig`, sets
+    `context.scenario_config` to the appropriate configuration file path for
+    the current deployment mode (library-mode or server-mode).
+    """
     if "skip" in scenario.effective_tags:
         scenario.skip("Marked with @skip")
         return
@@ -100,7 +129,31 @@ def before_scenario(context: Context, scenario: Scenario) -> None:
 
 
 def after_scenario(context: Context, scenario: Scenario) -> None:
-    """Run after each scenario is run."""
+    """Run after each scenario is run.
+
+    Perform per-scenario teardown: restore scenario-specific configuration and,
+    in server mode, attempt to restart and verify the Llama Stack container if
+    it was previously running.
+
+    If the scenario used an alternate feedback storage or no-cache
+    configuration, the original feature configuration is restored and the
+    lightspeed-stack container is restarted. When not running in library mode
+    and the context indicates the Llama Stack was running before the scenario,
+    this function attempts to start the llama-stack container and polls its
+    health endpoint until it becomes healthy or a timeout is reached.
+
+    Parameters:
+        context (Context): Behave test context. Expected attributes used here include:
+            - feature_config: path to the feature-level configuration to restore.
+            - is_library_mode (bool): whether tests run in library mode.
+            - llama_stack_was_running (bool, optional): whether llama-stack was
+              running before the scenario.
+            - hostname_llama, port_llama (str/int, optional): host and port
+              used for the llama-stack health check.
+        scenario (Scenario): Behave scenario whose tags determine which
+        scenario-specific teardown actions to run (e.g.,
+        "InvalidFeedbackStorageConfig", "NoCacheConfig").
+    """
     if "InvalidFeedbackStorageConfig" in scenario.effective_tags:
         switch_config(context.feature_config)
         restart_container("lightspeed-stack")
@@ -161,7 +214,10 @@ def after_scenario(context: Context, scenario: Scenario) -> None:
 
 
 def before_feature(context: Context, feature: Feature) -> None:
-    """Run before each feature file is exercised."""
+    """Run before each feature file is exercised.
+
+    Prepare per-feature test environment and apply feature-specific configuration.
+    """
     if "Authorized" in feature.tags:
         mode_dir = "library-mode" if context.is_library_mode else "server-mode"
         context.feature_config = (
@@ -178,7 +234,11 @@ def before_feature(context: Context, feature: Feature) -> None:
 
 
 def after_feature(context: Context, feature: Feature) -> None:
-    """Run after each feature file is exercised."""
+    """Run after each feature file is exercised.
+
+    Perform feature-level teardown: restore any modified configuration and
+    clean up feedback conversations.
+    """
     if "Authorized" in feature.tags:
         switch_config(context.default_config_backup)
         restart_container("lightspeed-stack")

tests/e2e/features/steps/auth.py

@@ -8,7 +8,11 @@
 
 @given("I set the Authorization header to {header_value}")
 def set_authorization_header_custom(context: Context, header_value: str) -> None:
-    """Set a custom Authorization header value."""
+    """Set a custom Authorization header value.
+
+    Parameters:
+        header_value (str): The value to set for the `Authorization` header.
+    """
     if not hasattr(context, "auth_headers"):
         context.auth_headers = {}
     context.auth_headers["Authorization"] = header_value
@@ -29,6 +33,10 @@ def access_rest_api_endpoint_post(
     """Send POST HTTP request with payload in the endpoint as parameter to tested service.
 
     The response is stored in `context.response` attribute.
+
+    Parameters:
+        endpoint (str): Endpoint path to call; will be normalized.
+        user_id (str): Value used for the `user_id` query parameter (surrounding quotes are removed).
     """
     endpoint = normalize_endpoint(endpoint)
     user_id = user_id.replace('"', "")

tests/e2e/features/steps/common.py

@@ -7,7 +7,14 @@
 
 @given("The service is started locally")
 def service_is_started_locally(context: Context) -> None:
-    """Check the service status."""
+    """Check the service status.
+
+    Populate the Behave context with local service endpoint values read from
+    environment variables.
+
+    Parameters:
+        context (Context): Behave context object to receive the endpoint attributes.
+    """
     assert context is not None
     context.hostname = os.getenv("E2E_LSC_HOSTNAME", "localhost")
     context.port = os.getenv("E2E_LSC_PORT", "8080")
@@ -17,5 +24,15 @@ def service_is_started_locally(context: Context) -> None:
 
 @given("The system is in default state")
 def system_in_default_state(context: Context) -> None:
-    """Check the default system state."""
+    """Check the default system state.
+
+    Ensure the Behave test context is present for steps that assume the system
+    is in its default state.
+
+    Parameters:
+        context (Context): Behave Context instance used to store and share test state.
+
+    Raises:
+        AssertionError: If `context` is None.
+    """
     assert context is not None

tests/e2e/features/steps/health.py

@@ -8,7 +8,22 @@
 
 @given("The llama-stack connection is disrupted")
 def llama_stack_connection_broken(context: Context) -> None:
-    """Break llama_stack connection by stopping the container."""
+    """Break llama_stack connection by stopping the container.
+
+    Disrupts the Llama Stack service by stopping its Docker container and
+    records whether it was running.
+
+    Checks whether the Docker container named "llama-stack" is running; if it
+    is, stops the container, waits briefly for the disruption to take effect,
+    and sets `context.llama_stack_was_running` to True so callers can restore
+    state later. If the container is not running, the flag remains False. On
+    failure to run Docker commands, prints a warning message describing the
+    error.
+
+    Parameters:
+        context (behave.runner.Context): Behave context used to store
+        `llama_stack_was_running` and share state between steps.
+    """
     # Store original state for restoration
     context.llama_stack_was_running = False
 
@@ -39,6 +54,12 @@ def llama_stack_connection_broken(context: Context) -> None:
 
 @given("the service is stopped")
 def stop_service(context: Context) -> None:
-    """Stop service."""
+    """Stop service.
+
+    Stop a service used by the current test scenario.
+
+    Parameters:
+        context (Context): Behave step context carrying scenario state and configuration.
+    """
     # TODO: add step implementation
     assert context is not None

tests/e2e/utils/utils.py

@@ -10,15 +10,42 @@
 
 
 def normalize_endpoint(endpoint: str) -> str:
-    """Normalize endpoint to be added into the URL."""
+    """Normalize endpoint to be added into the URL.
+
+    Ensure an endpoint string is suitable for inclusion in a URL.
+
+    Removes any double-quote characters and prepends a leading slash if one is
+    not already present.
+
+    Parameters:
+        endpoint (str): The endpoint string to normalize.
+
+    Returns:
+        str: The normalized endpoint starting with '/' and containing no double-quote characters.
+    """
     endpoint = endpoint.replace('"', "")
     if not endpoint.startswith("/"):
         endpoint = "/" + endpoint
     return endpoint
 
 
 def validate_json(message: Any, schema: Any) -> None:
-    """Check the JSON message with the given schema."""
+    """Check the JSON message with the given schema.
+
+    Validate a JSON-like object against a jsonschema-compatible schema.
+
+    Parameters:
+        message (Any): The JSON-like instance to validate (typically a dict or list).
+        schema (Any): A jsonschema-compatible schema describing the expected structure.
+
+    Returns:
+        None
+
+    Raises:
+        AssertionError: If the instance does not conform to the schema or if
+        the schema itself is invalid; the assertion message contains the
+        underlying jsonschema error.
+    """
     try:
         jsonschema.validate(
             instance=message,
@@ -33,7 +60,23 @@ def validate_json(message: Any, schema: Any) -> None:
 
 
 def wait_for_container_health(container_name: str, max_attempts: int = 3) -> None:
-    """Wait for container to be healthy."""
+    """Wait for container to be healthy.
+
+    Polls a Docker container until its health status becomes `healthy` or the
+    attempt limit is reached.
+
+    Checks the container's `Health.Status` using `docker inspect` up to
+    `max_attempts`, printing progress and final status messages. Transient
+    inspect errors or timeouts are ignored and retried; the function returns
+    after the container is observed healthy or after all attempts complete.
+
+    Returns:
+        None
+
+    Parameters:
+        container_name (str): Docker container name or ID to check.
+        max_attempts (int): Maximum number of health check attempts (default 3).
+    """
     for attempt in range(max_attempts):
         try:
             result = subprocess.run(
@@ -71,6 +114,13 @@ def validate_json_partially(actual: Any, expected: Any) -> None:
     """Recursively validate that `actual` JSON contains all keys and values specified in `expected`.
 
     Extra elements/keys are ignored. Raises AssertionError if validation fails.
+
+    Returns:
+        None
+
+    Raises:
+        AssertionError: If a required key is missing, no list element matches
+        an expected item, or a value does not equal the expected value.
     """
     if isinstance(expected, dict):
         for key, expected_value in expected.items():
@@ -98,7 +148,24 @@ def validate_json_partially(actual: Any, expected: Any) -> None:
 def switch_config(
     source_path: str, destination_path: str = "lightspeed-stack.yaml"
 ) -> None:
-    """Overwrite the config in `destination_path` by `source_path`."""
+    """Overwrite the config in `destination_path` by `source_path`.
+
+    Replace the destination configuration file with the file at source_path.
+
+    Parameters:
+        source_path (str): Path to the replacement configuration file.
+        destination_path (str): Path to the configuration file to be
+        overwritten (defaults to "lightspeed-stack.yaml").
+
+    Returns:
+        None
+
+    Raises:
+        FileNotFoundError: If source_path does not exist.
+        PermissionError: If the file cannot be read or destination cannot be
+                         written due to permissions.
+        OSError: For other OS-related failures during the copy operation.
+    """
     try:
         shutil.copy(source_path, destination_path)
     except (FileNotFoundError, PermissionError, OSError) as e:
@@ -107,7 +174,19 @@ def switch_config(
 
 
 def create_config_backup(config_path: str) -> str:
-    """Create a backup of `config_path` if it does not already exist."""
+    """Create a backup of `config_path` if it does not already exist.
+
+    Ensure a backup of the given configuration file exists by creating a
+    `.backup` copy if it is missing.
+
+    Returns:
+        str: Path to the backup file (original path with `.backup` appended).
+
+    Raises:
+        FileNotFoundError: If the source config file does not exist.
+        PermissionError: If the process lacks permission to read or write the files.
+        OSError: For other OS-level errors encountered while copying.
+    """
     backup_file = f"{config_path}.backup"
     if not os.path.exists(backup_file):
         try:
@@ -119,7 +198,18 @@ def create_config_backup(config_path: str) -> str:
 
 
 def remove_config_backup(backup_path: str) -> None:
-    """Delete the backup file at `backup_path` if it exists."""
+    """Delete the backup file at `backup_path` if it exists.
+
+    Remove the backup file at the given path if it exists.
+
+    If the file is present, attempts to delete it; on failure prints a warning with the error.
+
+    Returns:
+        None
+
+    Parameters:
+        backup_path (str): Filesystem path to the backup file to remove.
+    """
     if os.path.exists(backup_path):
         try:
             os.remove(backup_path)
@@ -128,7 +218,15 @@ def remove_config_backup(backup_path: str) -> None:
 
 
 def restart_container(container_name: str) -> None:
-    """Restart a Docker container by name and wait until it is healthy."""
+    """Restart a Docker container by name and wait until it is healthy.
+
+    Returns:
+        None
+
+    Raises:
+        subprocess.CalledProcessError: if the `docker restart` command fails.
+        subprocess.TimeoutExpired: if the `docker restart` command times out.
+    """
     try:
         subprocess.run(
             ["docker", "restart", container_name],
@@ -147,13 +245,12 @@ def restart_container(container_name: str) -> None:
 def replace_placeholders(context: Context, text: str) -> str:
     """Replace {MODEL} and {PROVIDER} placeholders with actual values from context.
 
-    Args:
-        context: Behave context containing default_model and default_provider
-        text: String that may contain {MODEL} and {PROVIDER} placeholders
+    Parameters:
+        context (Context): Behave context containing default_model and default_provider
+        text (str): String that may contain {MODEL} and {PROVIDER} placeholders
 
     Returns:
         String with placeholders replaced by actual values
-
     """
     result = text.replace("{MODEL}", context.default_model)
     result = result.replace("{PROVIDER}", context.default_provider)