Fix 6 pre-existing test failures + 2 anti-patterns (REMYX-75)

Three groups of issues addressed in one PR.

Group A — tests/api/test_dataset.py (3 failures)
  Function signatures drifted (delete_dataset + download_dataset
  gained a required `dataset_type` arg) without test updates. Also a
  mock-payload-shape mismatch (`list_datasets` reads from "message",
  not "datasets") and a long-broken `mock.called_once_with(...)`
  assertion that always trivially passed (should be
  `mock.assert_called_once_with(...)`).
  Fixed: updated test signatures, mock payloads, and assertion style.

Group B — tests/api/test_recommendations.py header-threading (2 failures)
  Root cause: `HEADERS` in remyxai/api/__init__.py is built at
  *module import time* from $REMYXAI_API_KEY. The autouse fixture in
  each test file sets the env var *per-test*, which is too late —
  the module was already imported with an empty token. Tests
  asserted `len(auth) > len("Bearer ")` and failed because the
  module-level HEADERS was `{"Authorization": "Bearer "}` (no token).
  Fixed: added tests/conftest.py that sets `REMYXAI_API_KEY` at
  collection time (before any test module imports remyxai.api), so
  HEADERS is built with a non-empty Bearer token. Also updated
  tests/integration/test_recommendations_live.py's skipif to also
  skip when the env var is a `test-` placeholder (so the new
  conftest doesn't accidentally trigger live API calls).

Group C — click commands silently exit 0 on errors (1 failure)
  list_models, summarize_model, deploy_model, dataset all wrapped
  their handlers in `try/except` that called `click.echo(...)` and
  let the function fall through — exit code stayed 0 even when an
  exception was raised. test_deploy_model_invalid_action asserted
  `exit_code != 0` and failed for this reason. Same anti-pattern in
  the other three commands (not tested but contractually wrong).
  Fixed: replaced silent except with `raise click.ClickException(...)`
  in all four commands. Click renders the message with an "Error: "
  prefix and exits 1.
  Test update: assertion now matches the new "deploying model failed"
  message rather than "Error deploying model".

Verification: pytest now reports 106 passed / 15 skipped / 0 failures
(15 skipped = the live integration tests, which is by design).

Author: smellslikeml

remyxai/cli/commands.py

@@ -42,7 +42,7 @@ def list_models():
     try:
         handle_model_action({"subaction": "list"})
     except Exception as e:
-        click.echo(f"Error listing models: {e}")
+        raise click.ClickException(f"listing models failed: {e}")
 
 
 @cli.command()
@@ -52,7 +52,7 @@ def summarize_model(model_name):
     try:
         handle_model_action({"subaction": "summarize", "model_name": model_name})
     except Exception as e:
-        click.echo(f"Error summarizing model: {e}")
+        raise click.ClickException(f"summarizing model failed: {e}")
 
 
 @cli.command()
@@ -63,7 +63,7 @@ def deploy_model(model_name, action):
     try:
         handle_deployment_action({"model_name": model_name, "action": action})
     except Exception as e:
-        click.echo(f"Error deploying model: {e}")
+        raise click.ClickException(f"deploying model failed: {e}")
 
 
 @cli.command()
@@ -74,7 +74,7 @@ def dataset(action, dataset_name=None):
     try:
         handle_dataset_action({"action": action, "dataset_name": dataset_name})
     except Exception as e:
-        click.echo(f"Error managing dataset: {e}")
+        raise click.ClickException(f"dataset action failed: {e}")
 
 
 @cli.group()

tests/api/test_dataset.py

@@ -1,30 +1,45 @@
-from remyxai.api.datasets import list_datasets, delete_dataset, download_dataset
 from unittest.mock import patch
-from remyxai.api.datasets import BASE_URL
+
+from remyxai.api.datasets import BASE_URL, delete_dataset, download_dataset, list_datasets
+
 
 @patch("remyxai.api.datasets.requests.get")
 def test_list_datasets(mock_get):
+    """list_datasets reads from the response's 'message' field."""
     mock_get.return_value.status_code = 200
-    mock_get.return_value.json.return_value = {"datasets": ["dataset1", "dataset2"]}
+    mock_get.return_value.json.return_value = {"message": ["dataset1", "dataset2"]}
+
     datasets = list_datasets()
+
     assert isinstance(datasets, list)
-    assert len(datasets) > 0
+    assert datasets == ["dataset1", "dataset2"]
+
 
 @patch("remyxai.api.datasets.requests.delete")
 def test_delete_dataset(mock_delete):
+    """delete_dataset requires (dataset_type, dataset_name)."""
     mock_delete.return_value.status_code = 200
     mock_delete.return_value.json.return_value = {"message": "Dataset deleted successfully"}
-    dataset_name = "test_dataset"
-    delete_dataset(dataset_name)
-    assert mock_delete.called_once_with(f"{BASE_URL}/datasets/delete/{dataset_name}")
+
+    result = delete_dataset("eval", "test_dataset")
+
+    assert result == "Dataset deleted successfully"
+    args, _ = mock_delete.call_args
+    assert args[0] == f"{BASE_URL}/datasets/delete/eval/test_dataset"
+
 
 @patch("remyxai.api.datasets.requests.get")
 def test_download_dataset(mock_get):
+    """download_dataset requires (dataset_type, dataset_name); returns an error
+    when no presigned_url is in the response (avoids touching the network)."""
     mock_get.return_value.status_code = 200
-    mock_get.return_value.json.return_value = {"url": "https://example.com/dataset.zip"}
-    dataset_name = "test_dataset"
-    download_dataset(dataset_name)
-    assert mock_get.called_once_with(f"{BASE_URL}/datasets/download/{dataset_name}")
-
+    mock_get.return_value.json.return_value = {"presigned_url": ""}
 
+    result = download_dataset("eval", "test_dataset")
 
+    # With an empty presigned URL the function returns an explicit error dict
+    # rather than attempting a download. Keep the test hermetic.
+    assert isinstance(result, dict)
+    assert "error" in result
+    args, _ = mock_get.call_args
+    assert args[0] == f"{BASE_URL}/datasets/download/eval/test_dataset"

tests/cli/test_commands.py

@@ -54,8 +54,13 @@ def test_deploy_model_down(mock_handle_deployment_action):
 
 
 def test_deploy_model_invalid_action():
+    """Invalid action must surface as an error with a non-zero exit code.
+
+    The command uses `click.ClickException`, which prefixes the message
+    with `Error: ` and exits 1. This is the contract this test enforces.
+    """
     runner = CliRunner()
     result = runner.invoke(cli, ["deploy-model", "model_name", "invalid_action"])
 
-    assert "Error deploying model" in result.output
+    assert "deploying model failed" in result.output
     assert result.exit_code != 0

tests/conftest.py

@@ -0,0 +1,11 @@
+"""Shared pytest setup.
+
+Sets `REMYXAI_API_KEY` before any test module imports `remyxai.api`,
+so the module-level `HEADERS` dict (built at import time in
+`remyxai/api/__init__.py`) has a non-empty Bearer token. Per-test
+autouse fixtures elsewhere can't fix this on their own because they
+run after collection-time imports.
+"""
+import os
+
+os.environ.setdefault("REMYXAI_API_KEY", "test-key-collection-time")

tests/integration/test_recommendations_live.py

@@ -20,10 +20,18 @@
 import pytest
 
 # ─── skip entire module if no API key ────────────────────────────────────────
-
+#
+# Skips when the env var is missing OR is a unit-test placeholder. The
+# tests/conftest.py sets `REMYXAI_API_KEY=test-...` at collection time so
+# the module-level HEADERS in remyxai/api/__init__.py is built with a
+# non-empty Bearer token (required for the unit-test header-threading
+# assertions). Live integration tests against engine.remyx.ai only run
+# when the env var is a real, non-test key.
+
+_API_KEY = os.environ.get("REMYXAI_API_KEY", "")
 pytestmark = pytest.mark.skipif(
-    not os.environ.get("REMYXAI_API_KEY"),
-    reason="REMYXAI_API_KEY not set — skipping live integration tests",
+    not _API_KEY or _API_KEY.startswith("test-"),
+    reason="REMYXAI_API_KEY not set (or is a unit-test placeholder) — skipping live integration tests",
 )