26de6bb7de
CI / lint (push) Successful in 15s
CI / build (push) Successful in 17s
CI / quality (push) Successful in 18s
CI / typecheck (push) Successful in 38s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 50s
CI / unit_tests (push) Successful in 2m13s
CI / docker (push) Successful in 41s
CI / integration_tests (push) Successful in 3m11s
CI / coverage (push) Successful in 4m30s
CI / benchmark-publish (push) Successful in 16m39s
## Summary
Fix `agents project create` not persisting projects to the database. `NamespacedProjectRepository.create()` called `session.flush()` but never `session.commit()`, so projects were invisible to subsequent `agents project list` invocations that open a separate session.
## Changes
**Production fix** (`src/cleveragents/infrastructure/database/repositories.py`):
- Replace `session.flush()` with `session.commit()` in `NamespacedProjectRepository.create()`
- Add `finally: session.close()` guard for proper session lifecycle
**Tests & benchmarks**:
- 4 Behave BDD regression scenarios (`features/project_create_persist.feature`)
- Robot Framework integration smoke tests (`robot/project_create_persist.robot`)
- ASV benchmarks for create-then-list round-trip (`benchmarks/project_create_persist_bench.py`)
## Review feedback addressed
- **H3**: Added `finally: session.close()` to `create()` method
- **M1**: Removed redundant `session.flush()` before `session.commit()`
- **M2**: Updated stale TDD "expected to fail" comments — this PR includes the fix
- **M4**: Rewrote CHANGELOG entry to describe the fix, not just tests
- **F1**: Updated Robot documentation (Suite Setup/Teardown kept — needed for `${PYTHON}`)
- **F2**: Fixed bare assertion `"my-app"` to `"local/my-app"` in namespace scenario
- **F3**: Removed redundant `Base.metadata.create_all()` from `_make_fresh_repo()`
## Process
- Single squashed commit, rebased onto `master` (no merge commits)
- Prescribed commit message from issue #589 metadata
ISSUES CLOSED: #589
Reviewed-on: #591
Reviewed-by: Rui Hu <rui.hu@cleverthis.com>
Co-authored-by: Brent E. Edwards <brent.edwards@cleverthis.com>
Co-committed-by: Brent E. Edwards <brent.edwards@cleverthis.com>
185 lines
7.1 KiB
Python
185 lines
7.1 KiB
Python
"""Step definitions for project_create_persist.feature (bug #589).
|
|
|
|
Regression tests verifying that ``agents project create`` commits data
|
|
to the database so that ``agents project list`` shows previously created
|
|
projects.
|
|
|
|
Design rationale
|
|
~~~~~~~~~~~~~~~~
|
|
The bug was that ``NamespacedProjectRepository.create()`` called
|
|
``session.flush()`` but never ``session.commit()``. Each CLI invocation
|
|
obtains a **new** session from the factory, so flushed-but-uncommitted
|
|
rows were invisible to subsequent invocations.
|
|
|
|
To exercise the fix we use a **file-based** SQLite database and patch
|
|
``_get_namespaced_project_repo`` so that every CLI invocation receives a
|
|
*fresh* ``NamespacedProjectRepository`` backed by a **new session
|
|
factory** pointing at the same file. This mirrors real CLI behaviour
|
|
where each ``agents project …`` invocation gets its own session.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import shutil
|
|
import tempfile
|
|
from typing import Any
|
|
from unittest.mock import patch
|
|
|
|
from behave import given, then, when
|
|
from sqlalchemy import create_engine
|
|
from sqlalchemy.orm import sessionmaker
|
|
from sqlalchemy.pool import NullPool
|
|
from typer.testing import CliRunner
|
|
|
|
from cleveragents.cli.commands.project import app as project_app
|
|
from cleveragents.infrastructure.database.models import Base
|
|
from cleveragents.infrastructure.database.repositories import (
|
|
NamespacedProjectRepository,
|
|
)
|
|
|
|
# Patch target - the module-level DI helper that returns the project repo.
|
|
_PATCH_PROJECT_REPO = "cleveragents.cli.commands.project._get_namespaced_project_repo"
|
|
# The --invariant flag is never passed in these scenarios, so
|
|
# _store_project_extras is effectively a no-op. We patch it defensively
|
|
# to avoid side-effects from DI look-ups in the real implementation.
|
|
_PATCH_STORE_EXTRAS = "cleveragents.cli.commands.project._store_project_extras"
|
|
|
|
runner = CliRunner()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Helpers
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _make_fresh_repo(db_path: str) -> NamespacedProjectRepository:
|
|
"""Build a *new* repo with its own engine/session for *db_path*.
|
|
|
|
Each call simulates a separate CLI invocation: a new engine and session
|
|
factory are created, so uncommitted data from a prior invocation is not
|
|
visible. ``NullPool`` is used so that SQLite file handles are released
|
|
immediately when the engine is no longer referenced.
|
|
|
|
Schema is pre-created once during the Background step, so we skip
|
|
``Base.metadata.create_all()`` here to avoid redundant DDL.
|
|
"""
|
|
engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool)
|
|
factory = sessionmaker(bind=engine, expire_on_commit=False)
|
|
return NamespacedProjectRepository(session_factory=factory)
|
|
|
|
|
|
def _cleanup_persist_db(context: Any) -> None:
|
|
"""Remove the temporary directory created for the test."""
|
|
shutil.rmtree(context.persist_tmpdir, ignore_errors=True)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Background
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@given("a fresh project-persist database is initialised")
|
|
def step_init_persist_db(context: Any) -> None:
|
|
"""Create a temp dir with a fresh SQLite file for persistence tests."""
|
|
context.persist_tmpdir = tempfile.mkdtemp(prefix="persist_589_")
|
|
context.persist_db_path = os.path.join(context.persist_tmpdir, "test.db")
|
|
|
|
# Pre-create schema so every fresh repo sees the tables.
|
|
engine = create_engine(
|
|
f"sqlite:///{context.persist_db_path}",
|
|
echo=False,
|
|
poolclass=NullPool,
|
|
)
|
|
Base.metadata.create_all(engine)
|
|
engine.dispose()
|
|
|
|
# Initialise result holders.
|
|
context.persist_create_results = [] # type: ignore[attr-defined]
|
|
context.persist_list_result = None
|
|
context.persist_dup_result = None
|
|
|
|
context.add_cleanup(_cleanup_persist_db, context)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# When steps - create
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I create a project named "{name}" via the persist CLI')
|
|
def step_create_project_persist(context: Any, name: str) -> None:
|
|
"""Invoke ``project create <name>`` with a fresh repo per call."""
|
|
repo = _make_fresh_repo(context.persist_db_path)
|
|
with (
|
|
patch(_PATCH_PROJECT_REPO, return_value=repo),
|
|
patch(_PATCH_STORE_EXTRAS),
|
|
):
|
|
result = runner.invoke(project_app, ["create", name])
|
|
assert result.exit_code == 0, (
|
|
f"project create '{name}' should exit 0 but got "
|
|
f"{result.exit_code}. output:\n{result.output}"
|
|
)
|
|
context.persist_create_results.append(result)
|
|
|
|
|
|
@when('I attempt to create a duplicate project named "{name}" via the persist CLI')
|
|
def step_create_duplicate_persist(context: Any, name: str) -> None:
|
|
"""Invoke ``project create`` again for the same name (expect error)."""
|
|
repo = _make_fresh_repo(context.persist_db_path)
|
|
with (
|
|
patch(_PATCH_PROJECT_REPO, return_value=repo),
|
|
patch(_PATCH_STORE_EXTRAS),
|
|
):
|
|
result = runner.invoke(project_app, ["create", name])
|
|
context.persist_dup_result = result
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# When steps - list
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when("I list projects via the persist CLI")
|
|
def step_list_projects_persist(context: Any) -> None:
|
|
"""Invoke ``project list`` with a fresh repo pointing at the same DB."""
|
|
repo = _make_fresh_repo(context.persist_db_path)
|
|
with patch(_PATCH_PROJECT_REPO, return_value=repo):
|
|
result = runner.invoke(project_app, ["list"])
|
|
assert result.exit_code == 0, (
|
|
f"project list should exit 0 but got {result.exit_code}. "
|
|
f"output:\n{result.output}"
|
|
)
|
|
context.persist_list_result = result
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Then steps
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@then('the persist project list should contain "{name}"')
|
|
def step_persist_list_contains(context: Any, name: str) -> None:
|
|
"""Assert that the list output includes the given project name."""
|
|
result = context.persist_list_result
|
|
assert result is not None, "project list was not invoked"
|
|
output = result.output
|
|
assert name in output, (
|
|
f"Expected '{name}' in project list output but got:\n{output}"
|
|
)
|
|
|
|
|
|
@then('the persist duplicate creation should fail with "{text}"')
|
|
def step_persist_dup_fails(context: Any, text: str) -> None:
|
|
"""Assert that the duplicate creation produced an error containing *text*."""
|
|
result = context.persist_dup_result
|
|
assert result is not None, "duplicate create was not invoked"
|
|
assert result.exit_code != 0, (
|
|
f"Duplicate create should fail (exit_code != 0) but got "
|
|
f"{result.exit_code}. output:\n{result.output}"
|
|
)
|
|
output = result.output
|
|
assert text.lower() in output.lower(), (
|
|
f"Expected '{text}' in duplicate-create output but got:\n{output}"
|
|
)
|