diff --git a/CHANGELOG.md b/CHANGELOG.md index aa05d7a39..36aab2df1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,10 @@ help content that varies for main-screen, slash-command, reference, and shell prompt modes. Updated Behave and Robot coverage for help-panel rendering and mode switching. (#1013) - +- Added direct BDD coverage for `_fast_init_or_upgrade` early-return behavior, + template-copy/fallback delegation paths, and the existing-empty-DB branch in + `features/fast_init_upgrade.feature`. Uses race-safe temp-path allocation + (`mkstemp`/`mkdtemp`) throughout new fast-init test steps. (#733) - Expanded the TUI slash command overlay catalog to include 67 commands across 14 groups, aligned with the specification command reference for session, persona, scope, plan, project, registry/config, context, and utility flows. diff --git a/features/fast_init_upgrade.feature b/features/fast_init_upgrade.feature new file mode 100644 index 000000000..de78aa16f --- /dev/null +++ b/features/fast_init_upgrade.feature @@ -0,0 +1,59 @@ +Feature: Fast init_or_upgrade early-return behavior + As the test harness + I need _fast_init_or_upgrade to skip Alembic migrations for existing databases + So that parallel test execution does not hang on redundant migrations + + Background: + Given the fast-init template-DB patch is installed + + @mock_only + Scenario: Non-empty database with matching prefix skips original migration + Given a non-empty SQLite database file with a matching scenario prefix + When I call init_or_upgrade on the fast-init database + Then the original init_or_upgrade should not have been invoked + And the database file content should be unchanged + + @mock_only + Scenario: Template is copied for a new database with matching prefix + Given a non-existent SQLite database path with a matching scenario prefix + When I call init_or_upgrade on the fast-init database + Then the database file should be a copy of the template + And the original init_or_upgrade should not have been invoked + + @mock_only + Scenario: Existing empty database with matching prefix copies template + Given an empty SQLite database file with a matching scenario prefix + When I call init_or_upgrade on the fast-init database + Then the database file should be a copy of the template + And the original init_or_upgrade should not have been invoked + + @mock_only + Scenario: Non-matching prefix delegates to original init_or_upgrade + Given a SQLite database URL whose filename has a non-matching prefix + When I call init_or_upgrade on the fast-init database + Then the original init_or_upgrade should have been invoked exactly once + + @mock_only + Scenario: In-memory SQLite delegates to original init_or_upgrade + Given an in-memory SQLite database URL for fast-init testing + When I call init_or_upgrade on the fast-init database + Then the original init_or_upgrade should have been invoked exactly once + + @mock_only + Scenario: Non-SQLite URL delegates to original init_or_upgrade + Given a non-SQLite database URL for fast-init testing + When I call init_or_upgrade on the fast-init database + Then the original init_or_upgrade should have been invoked exactly once + + @mock_only + Scenario: Bare sqlite:// URL delegates to original init_or_upgrade + Given a bare sqlite:// database URL for fast-init testing + When I call init_or_upgrade on the fast-init database + Then the original init_or_upgrade should have been invoked exactly once + + @mock_only + Scenario: Delegation forwards runner instance and keyword arguments + Given a non-SQLite database URL for fast-init testing + When I call init_or_upgrade with keyword arguments on the fast-init database + Then the original init_or_upgrade should have been invoked exactly once + And the original should have received the runner instance and keyword arguments diff --git a/features/mocks/fast_init_test_helpers.py b/features/mocks/fast_init_test_helpers.py new file mode 100644 index 000000000..3ce3b19c8 --- /dev/null +++ b/features/mocks/fast_init_test_helpers.py @@ -0,0 +1,95 @@ +"""Test helpers for exercising ``_fast_init_or_upgrade`` closure logic. + +This module provides utilities that safely swap the +``_original_init_or_upgrade`` reference captured inside the +``_fast_init_or_upgrade`` closure (installed by +``features.environment._install_template_db_patch``) with a +``unittest.mock.MagicMock`` so that step definitions can assert +whether the original migration runner was invoked. + +All helpers live in ``features/mocks/`` per the project's mock-placement +rule. +""" + +from __future__ import annotations + +import contextlib +import types +from collections.abc import Generator +from typing import Any +from unittest.mock import MagicMock + + +def _find_closure_cell( + func: types.FunctionType, + var_name: str, +) -> tuple[int, types.CellType] | None: + """Locate a closure cell by free-variable name. + + Args: + func: The function whose closure to inspect. + var_name: The free-variable name to find. + + Returns: + A ``(index, cell)`` tuple if found, otherwise ``None``. + + Raises: + TypeError: If *func* is not a function with a closure. + """ + if not isinstance(func, types.FunctionType): + raise TypeError(f"Expected a function, got {type(func).__name__}") + freevars: tuple[str, ...] = func.__code__.co_freevars + closure: tuple[types.CellType, ...] | None = func.__closure__ + if closure is None: + return None + for idx, name in enumerate(freevars): + if name == var_name: + return idx, closure[idx] + return None + + +@contextlib.contextmanager +def patch_original_init_or_upgrade() -> Generator[MagicMock]: + """Context manager that replaces ``_original_init_or_upgrade`` inside + the ``_fast_init_or_upgrade`` closure with a :class:`~unittest.mock.MagicMock`. + + Yields the mock so callers can assert on call counts and arguments. + Restores the original cell contents on exit. + + Raises: + RuntimeError: If the fast-init patch is not installed on + ``MigrationRunner.init_or_upgrade`` (e.g. template DB missing). + + Example usage in a step definition:: + + with patch_original_init_or_upgrade() as mock_original: + runner.init_or_upgrade() + assert mock_original.called + """ + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + + fast_fn: Any = MigrationRunner.init_or_upgrade + if not isinstance(fast_fn, types.FunctionType): + raise RuntimeError( + "MigrationRunner.init_or_upgrade is not a plain function — " + "the _fast_init_or_upgrade patch does not appear to be installed." + ) + + result = _find_closure_cell(fast_fn, "_original_init_or_upgrade") + if result is None: + raise RuntimeError( + "Cannot find '_original_init_or_upgrade' in the closure of " + f"{fast_fn.__qualname__}. Is the template-DB patch installed?" + ) + + _idx, cell = result + cell_ref: Any = cell + saved: Any = cell_ref.cell_contents + mock = MagicMock(name="_original_init_or_upgrade") + cell_ref.cell_contents = mock + try: + yield mock + finally: + cell_ref.cell_contents = saved diff --git a/features/steps/fast_init_upgrade_steps.py b/features/steps/fast_init_upgrade_steps.py new file mode 100644 index 000000000..55334de84 --- /dev/null +++ b/features/steps/fast_init_upgrade_steps.py @@ -0,0 +1,284 @@ +"""Step definitions for fast_init_upgrade.feature. + +Exercises the ``_fast_init_or_upgrade`` closure installed by +``features.environment._install_template_db_patch`` — specifically the +early-return path for non-empty databases, the template-copy path for +new databases, and the fallback delegation to the original +``MigrationRunner.init_or_upgrade``. +""" + +from __future__ import annotations + +import contextlib +import filecmp +import os +import shutil +import tempfile +import types +from pathlib import Path +from typing import Any + +from behave import given, then, when +from behave.runner import Context + +# --------------------------------------------------------------------------- +# Internal helpers +# --------------------------------------------------------------------------- + + +def _register_path_cleanup(context: Context, path: str) -> None: + """Schedule *path* (and its SQLite sidecar files) for removal.""" + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + + def _remove() -> None: + for suffix in ("", "-journal", "-wal", "-shm"): + with contextlib.suppress(OSError): + os.unlink(path + suffix) + + context._cleanup_handlers.append(_remove) + + +def _register_directory_cleanup(context: Context, path: str) -> None: + """Schedule directory *path* for recursive removal.""" + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + + def _remove() -> None: + with contextlib.suppress(OSError): + shutil.rmtree(path) + + context._cleanup_handlers.append(_remove) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("the fast-init template-DB patch is installed") +def step_verify_patch_installed(context: Context) -> None: + """Verify that ``_install_template_db_patch`` has been applied.""" + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + + fast_fn: Any = MigrationRunner.init_or_upgrade + if not isinstance(fast_fn, types.FunctionType): + raise AssertionError( + "MigrationRunner.init_or_upgrade is not a plain function — " + "the _fast_init_or_upgrade patch does not appear to be installed. " + "Ensure CLEVERAGENTS_TEMPLATE_DB is set and the template exists." + ) + + freevars: tuple[str, ...] = fast_fn.__code__.co_freevars + if "_original_init_or_upgrade" not in freevars: + raise AssertionError( + "MigrationRunner.init_or_upgrade does not capture " + "'_original_init_or_upgrade' — the template-DB patch is not " + "installed as expected." + ) + + # Store template path for later steps. + template_db: str | None = os.environ.get("CLEVERAGENTS_TEMPLATE_DB") + if not template_db or not Path(template_db).is_file(): + raise AssertionError( + "CLEVERAGENTS_TEMPLATE_DB is not set or the file does not exist." + ) + context.fast_init_template_path = template_db + + +# --------------------------------------------------------------------------- +# Given — non-empty DB with matching prefix +# --------------------------------------------------------------------------- + + +@given("a non-empty SQLite database file with a matching scenario prefix") +def step_create_nonempty_db(context: Context) -> None: + """Create a temporary non-empty file whose name starts with a matching prefix.""" + fd, path = tempfile.mkstemp(suffix=".db", prefix="test_fastinit_") + os.write(fd, b"non-empty-content-to-simulate-migrated-db") + os.close(fd) + context.fast_init_db_path = path + context.fast_init_db_url = f"sqlite:///{path}" + context.fast_init_original_content = Path(path).read_bytes() + _register_path_cleanup(context, path) + + +# --------------------------------------------------------------------------- +# Given — template-copy path (new DB) +# --------------------------------------------------------------------------- + + +@given("a non-existent SQLite database path with a matching scenario prefix") +def step_create_nonexistent_db_path(context: Context) -> None: + """Generate a path for a non-existent file with a matching prefix.""" + temp_dir = tempfile.mkdtemp(prefix="test_fastinit_new_") + path = str(Path(temp_dir) / "test_fastinit.db") + context.fast_init_db_path = path + context.fast_init_db_url = f"sqlite:///{path}" + _register_path_cleanup(context, path) + _register_directory_cleanup(context, temp_dir) + + +@given("an empty SQLite database file with a matching scenario prefix") +def step_create_empty_db(context: Context) -> None: + """Create an existing zero-byte DB file with a matching prefix.""" + fd, path = tempfile.mkstemp(suffix=".db", prefix="test_fastinit_empty_") + os.close(fd) + context.fast_init_db_path = path + context.fast_init_db_url = f"sqlite:///{path}" + _register_path_cleanup(context, path) + + +# --------------------------------------------------------------------------- +# Given — fallback paths (non-matching / in-memory / non-SQLite) +# --------------------------------------------------------------------------- + + +@given("a SQLite database URL whose filename has a non-matching prefix") +def step_create_nonmatching_prefix_url(context: Context) -> None: + """Create a URL whose filename prefix is not in _SCENARIO_DB_PREFIXES.""" + fd, path = tempfile.mkstemp(suffix=".db", prefix="nomatch_fastinit_") + os.close(fd) + context.fast_init_db_path = path + context.fast_init_db_url = f"sqlite:///{path}" + _register_path_cleanup(context, path) + + +@given("an in-memory SQLite database URL for fast-init testing") +def step_create_inmemory_url(context: Context) -> None: + """Set up an in-memory SQLite URL.""" + context.fast_init_db_path = None + context.fast_init_db_url = "sqlite:///:memory:" + + +@given("a non-SQLite database URL for fast-init testing") +def step_create_nonsqlite_url(context: Context) -> None: + """Set up a non-SQLite URL (PostgreSQL placeholder).""" + context.fast_init_db_path = None + context.fast_init_db_url = "postgresql://fake_user:fake_pass@localhost/testdb" + + +@given("a bare sqlite:// database URL for fast-init testing") +def step_create_bare_sqlite_url(context: Context) -> None: + """Set up a bare ``sqlite://`` URL (no path, no ``:memory:``).""" + context.fast_init_db_path = None + context.fast_init_db_url = "sqlite://" + + +# --------------------------------------------------------------------------- +# When +# --------------------------------------------------------------------------- + + +@when("I call init_or_upgrade on the fast-init database") +def step_call_init_or_upgrade(context: Context) -> None: + """Invoke ``MigrationRunner.init_or_upgrade`` via the fast-init patch.""" + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + from features.mocks.fast_init_test_helpers import ( + patch_original_init_or_upgrade, + ) + + runner = MigrationRunner(database_url=context.fast_init_db_url) + + with patch_original_init_or_upgrade() as mock_original: + runner.init_or_upgrade() + context.fast_init_mock_called = mock_original.called + context.fast_init_mock_call_count = mock_original.call_count + context.fast_init_mock_call_args = mock_original.call_args + context.fast_init_runner = runner + + +@when("I call init_or_upgrade with keyword arguments on the fast-init database") +def step_call_init_or_upgrade_with_kwargs(context: Context) -> None: + """Invoke ``init_or_upgrade`` with keyword arguments to verify forwarding.""" + from cleveragents.infrastructure.database.migration_runner import ( + MigrationRunner, + ) + from features.mocks.fast_init_test_helpers import ( + patch_original_init_or_upgrade, + ) + + runner = MigrationRunner(database_url=context.fast_init_db_url) + + with patch_original_init_or_upgrade() as mock_original: + runner.init_or_upgrade(require_confirmation=True) + context.fast_init_mock_called = mock_original.called + context.fast_init_mock_call_count = mock_original.call_count + context.fast_init_mock_call_args = mock_original.call_args + context.fast_init_runner = runner + + +# --------------------------------------------------------------------------- +# Then — original invocation assertions +# --------------------------------------------------------------------------- + + +@then("the original init_or_upgrade should not have been invoked") +def step_assert_original_not_called(context: Context) -> None: + """Assert the mocked original was never called.""" + assert not context.fast_init_mock_called, ( + "Expected _original_init_or_upgrade NOT to be called, " + f"but it was called {context.fast_init_mock_call_count} time(s)." + ) + + +@then("the original init_or_upgrade should have been invoked exactly once") +def step_assert_original_called_once(context: Context) -> None: + """Assert the mocked original was called exactly once.""" + assert context.fast_init_mock_call_count == 1, ( + "Expected _original_init_or_upgrade to be called exactly once, " + f"but it was called {context.fast_init_mock_call_count} time(s)." + ) + + +@then("the original should have received the runner instance and keyword arguments") +def step_assert_original_received_runner_and_kwargs(context: Context) -> None: + """Assert the mock was called with the correct runner and kwargs.""" + call_args = context.fast_init_mock_call_args + assert call_args is not None, ( + "Expected _original_init_or_upgrade to have been called, but call_args is None." + ) + positional = call_args.args + assert len(positional) >= 1, ( + "Expected _original_init_or_upgrade to receive the runner as " + f"first positional argument, but got {len(positional)} positional args." + ) + assert positional[0] is context.fast_init_runner, ( + "Expected the first positional argument to be the MigrationRunner " + "instance, but a different object was passed." + ) + assert call_args.kwargs.get("require_confirmation") is True, ( + "Expected keyword argument 'require_confirmation=True' to be " + f"forwarded, but got kwargs={call_args.kwargs}." + ) + + +# --------------------------------------------------------------------------- +# Then — file-content assertions +# --------------------------------------------------------------------------- + + +@then("the database file content should be unchanged") +def step_assert_file_unchanged(context: Context) -> None: + """Assert the DB file still has its original content.""" + current_content = Path(context.fast_init_db_path).read_bytes() + assert current_content == context.fast_init_original_content, ( + "Database file content changed unexpectedly." + ) + + +@then("the database file should be a copy of the template") +def step_assert_file_is_template_copy(context: Context) -> None: + """Assert the DB file was created as a copy of the template.""" + db_path = Path(context.fast_init_db_path) + assert db_path.exists(), f"Database file was not created at {db_path}" + assert db_path.stat().st_size > 0, "Database file is empty" + assert filecmp.cmp( + str(db_path), + context.fast_init_template_path, + shallow=False, + ), "Database file does not match the template"