Address Copilot PR feedback and add seriallocal marking

Author: shaypal5

scripts/test-local.sh

@@ -25,7 +25,7 @@ COVERAGE_REPORT="term"
 KEEP_RUNNING=false
 SELECTED_CORES=""
 INCLUDE_LOCAL_CORES=false
-TEST_FILES=""
+TEST_FILES=()
 PARALLEL=false
 PARALLEL_WORKERS="auto"
 
@@ -102,7 +102,7 @@ while [[ $# -gt 0 ]]; do
                 usage
                 exit 1
             fi
-            TEST_FILES="$TEST_FILES $1"
+            TEST_FILES+=("$1")
             shift
             ;;
         --help)
@@ -542,37 +542,37 @@ main() {
 
     # Run pytest
     # Build pytest command
-    PYTEST_CMD="pytest"
+    PYTEST_ARGS=(pytest)
     # and the specific pytest command for running serial pickle tests
-    SERIAL_PYTEST_CMD="pytest -m seriallocal"
+    SERIAL_PYTEST_ARGS=(pytest -m seriallocal)
     # Only add -n0 if pytest-xdist is available; otherwise, plain pytest is already serial
     if python - << 'EOF' >/dev/null 2>&1
 import xdist  # noqa: F401
 EOF
     then
-        SERIAL_PYTEST_CMD="$SERIAL_PYTEST_CMD -n0"
+        SERIAL_PYTEST_ARGS+=(-n0)
     fi
 
     # Add test files if specified
-    if [ -n "$TEST_FILES" ]; then
-        PYTEST_CMD="$PYTEST_CMD $TEST_FILES"
-        print_message $BLUE "Test files specified: $TEST_FILES"
+    if [ ${#TEST_FILES[@]} -gt 0 ]; then
+        PYTEST_ARGS+=("${TEST_FILES[@]}")
+        print_message $BLUE "Test files specified: ${TEST_FILES[*]}"
         # and turn off serial local tests, so we run only selected files
         run_serial_local_tests=false
     fi
 
     # Add markers if needed (only if no specific test files were given)
-    if [ -z "$TEST_FILES" ]; then
+    if [ ${#TEST_FILES[@]} -eq 0 ]; then
         # Check if we selected all cores - if so, run all tests without marker filtering
         all_cores="memory mongo pickle redis s3 sql"
         selected_sorted=$(echo "$SELECTED_CORES" | tr ' ' '\n' | sort | tr '\n' ' ' | xargs)
         all_sorted=$(echo "$all_cores" | tr ' ' '\n' | sort | tr '\n' ' ' | xargs)
 
         if [ "$selected_sorted" != "$all_sorted" ]; then
-            PYTEST_CMD="$PYTEST_CMD -m \"$pytest_markers\""
+            PYTEST_ARGS+=(-m "$pytest_markers")
         else
             print_message $BLUE "Running all tests without markers since all cores are selected"
-            PYTEST_CMD="$PYTEST_CMD -m \"not seriallocal\""
+            PYTEST_ARGS+=(-m "not seriallocal")
             run_serial_local_tests=true
         fi
     else
@@ -582,19 +582,19 @@ EOF
         all_sorted=$(echo "$all_cores" | tr ' ' '\n' | sort | tr '\n' ' ' | xargs)
 
         if [ "$selected_sorted" != "$all_sorted" ]; then
-            PYTEST_CMD="$PYTEST_CMD -m \"$pytest_markers\""
+            PYTEST_ARGS+=(-m "$pytest_markers")
         fi
     fi
 
     # Add verbose flag if needed
     if [ "$VERBOSE" = true ]; then
-        PYTEST_CMD="$PYTEST_CMD -v"
-        SERIAL_PYTEST_CMD="$SERIAL_PYTEST_CMD -v"
+        PYTEST_ARGS+=(-v)
+        SERIAL_PYTEST_ARGS+=(-v)
     fi
 
     # Add parallel testing options if requested
     if [ "$PARALLEL" = true ]; then
-        PYTEST_CMD="$PYTEST_CMD -n $PARALLEL_WORKERS"
+        PYTEST_ARGS+=(-n "$PARALLEL_WORKERS")
 
         # Show parallel testing info
         if [ "$PARALLEL_WORKERS" = "auto" ]; then
@@ -610,16 +610,16 @@ EOF
     fi
 
     # Add coverage options
-    PYTEST_CMD="$PYTEST_CMD --cov=cachier --cov-report=$COVERAGE_REPORT"
-    SERIAL_PYTEST_CMD="$SERIAL_PYTEST_CMD --cov=cachier --cov-report=$COVERAGE_REPORT --cov-append"
+    PYTEST_ARGS+=(--cov=cachier --cov-report="$COVERAGE_REPORT")
+    SERIAL_PYTEST_ARGS+=(--cov=cachier --cov-report="$COVERAGE_REPORT" --cov-append)
 
     # Print and run the command
-    print_message $BLUE "Running: $PYTEST_CMD"
-    eval $PYTEST_CMD
+    print_message $BLUE "Running: $(printf '%q ' "${PYTEST_ARGS[@]}")"
+    "${PYTEST_ARGS[@]}"
 
     if [ "$run_serial_local_tests" = true ]; then
-        print_message $BLUE "Running serial local tests (pickle, memory) with: $SERIAL_PYTEST_CMD"
-        eval $SERIAL_PYTEST_CMD
+        print_message $BLUE "Running serial local tests (pickle, memory) with: $(printf '%q ' "${SERIAL_PYTEST_ARGS[@]}")"
+        "${SERIAL_PYTEST_ARGS[@]}"
     else
         print_message $BLUE "Skipping serial local tests (pickle, memory) since not requested"
     fi

tests/README.md

@@ -371,7 +371,7 @@ the corresponding Docker service started beforehand.
 1. **Import Errors**: Install backend-specific requirements
 
    ```bash
-   pip install -r tests/redis_requirements.txt
+   pip install -r tests/requirements_redis.txt
    ```
 
 2. **Docker Not Running**: Start Docker Desktop or daemon

tests/conftest.py

@@ -2,18 +2,27 @@
 
 import logging
 import os
+import re
 from urllib.parse import parse_qs, unquote, urlencode, urlparse, urlunparse
 
 import pytest
 
 logger = logging.getLogger(__name__)
 
 
+def _worker_schema_name(worker_id: str) -> str | None:
+    """Return a safe SQL schema name for an xdist worker ID."""
+    match = re.fullmatch(r"gw(\d+)", worker_id)
+    if match is None:
+        return None
+    return f"test_worker_{match.group(1)}"
+
+
 @pytest.fixture(autouse=True)
 def inject_worker_schema_for_sql_tests(monkeypatch, request):
     """Automatically inject worker-specific schema into SQL connection string.
 
-    This fixture enables parallel SQL test execution by giving each pytest- xdist worker its own PostgreSQL schema,
+    This fixture enables parallel SQL test execution by giving each pytest-xdist worker its own PostgreSQL schema,
     preventing table creation conflicts.
 
     """
@@ -34,7 +43,11 @@ def inject_worker_schema_for_sql_tests(monkeypatch, request):
 
     if "postgresql" in original_url:
         # Create worker-specific schema name
-        schema_name = f"test_worker_{worker_id.replace('gw', '')}"
+        schema_name = _worker_schema_name(worker_id)
+        if schema_name is None:
+            logger.warning("Unexpected worker ID for SQL schema isolation: %s", worker_id)
+            yield
+            return
 
         # Parse the URL
         parsed = urlparse(original_url)
@@ -118,8 +131,12 @@ def isolated_cache_directory(tmp_path, monkeypatch, request, worker_id):
 
         monkeypatch.setattr(cachier.config._global_params, "cache_dir", str(cache_dir))
 
-        # Also set environment variable as a backup
-        monkeypatch.setenv("CACHIER_TEST_CACHE_DIR", str(cache_dir))
+
+def pytest_collection_modifyitems(items):
+    """Mark local backends as serial-local for the split test runner flow."""
+    for item in items:
+        if "memory" in item.keywords or "pickle" in item.keywords:
+            item.add_marker(pytest.mark.seriallocal)
 
 
 @pytest.fixture(scope="session", autouse=True)
@@ -140,7 +157,10 @@ def cleanup_test_schemas(request):
         original_url = os.environ.get("SQLALCHEMY_DATABASE_URL", "")
 
         if "postgresql" in original_url:
-            schema_name = f"test_worker_{worker_id.replace('gw', '')}"
+            schema_name = _worker_schema_name(worker_id)
+            if schema_name is None:
+                logger.warning("Unexpected worker ID for SQL schema cleanup: %s", worker_id)
+                return
 
             try:
                 from sqlalchemy import create_engine, text
@@ -174,19 +194,3 @@ def cleanup_test_schemas(request):
             except Exception as e:
                 # If cleanup fails, it's not critical
                 logger.debug(f"Failed to cleanup schema {schema_name}: {e}")
-
-
-def pytest_addoption(parser):
-    """Add custom command line options for parallel testing."""
-    parser.addoption(
-        "--parallel",
-        action="store_true",
-        default=False,
-        help="Run tests in parallel using pytest-xdist",
-    )
-    parser.addoption(
-        "--parallel-workers",
-        action="store",
-        default="auto",
-        help="Number of parallel workers (default: auto)",
-    )