From 4c5589da4edcd7b8f021e94609c547cb393c394f Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Mon, 9 Mar 2026 23:29:14 +0000 Subject: [PATCH] test(cli): add failing TDD tests for session list DI container error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 10 Behave BDD scenarios (@tdd_bug @tdd_bug_554 @tdd_expected_fail) for the session list DI wiring bug where _get_session_service() calls container.db() but the Container has no db provider (AttributeError). Scenarios cover empty list, format validation (JSON/YAML/plain/rich), init-then-list lifecycle, and post-create list paths. Implement @tdd_expected_fail infrastructure: Behave after_scenario hook inverts FAIL→PASS (expected) and PASS→FAIL (unexpected fix), plus Robot Framework listener (Listener API v3) with identical semantics registered via --listener in both integration_tests and slow_integration_tests nox sessions. Migrate 18 existing TDD scenarios across 5 feature files from legacy @tdd @bugNNN convention to @tdd_bug @tdd_bug_NNN per CONTRIBUTING.md § TDD Bug Test Tags. Includes Robot Framework integration smoke tests and ASV service-layer benchmarks. Refs: #554 --- CHANGELOG.md | 12 + benchmarks/session_list_bench.py | 157 ++++++++++++++ features/cli_init_yes_flag.feature | 10 +- features/environment.py | 31 +++ features/project_create_persist.feature | 8 +- features/project_show_after_create.feature | 6 +- features/resource_type_bootstrap_fs.feature | 6 +- features/resource_type_bootstrap_git.feature | 6 +- features/session_list_error.feature | 85 ++++++++ features/steps/session_list_error_steps.py | 217 +++++++++++++++++++ noxfile.py | 30 ++- robot/session_list_error.robot | 58 +++++ robot/tdd_expected_fail_listener.py | 48 ++++ 13 files changed, 651 insertions(+), 23 deletions(-) create mode 100644 benchmarks/session_list_bench.py create mode 100644 features/session_list_error.feature create mode 100644 features/steps/session_list_error_steps.py create mode 100644 robot/session_list_error.robot create mode 100644 robot/tdd_expected_fail_listener.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d32928830..44bbfaee5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ 3 Robot Framework integration tests, ASV benchmarks (5 time + 2 track), and reference documentation. (#195) + ### Added - Resource type single-inheritance via `inherits` field (ADR-042) (#513) - Inheritance chain resolution, field merging, and polymorphic type matching @@ -20,6 +21,17 @@ - Polymorphic handler resolution with ancestor-type fallback - CLI: `agents resource type list` shows Inherits column; `type show` displays inheritance chain - Alembic migration `m6_004_resource_type_inherits` adds `inherits` column to `resource_types` +- Added TDD regression tests for `agents session list` DI container wiring + error (bug #554). `_get_session_service()` calls `container.db()` but the + `Container` class has no `db` provider, raising `AttributeError`. Includes + 10 Behave BDD scenarios (`@tdd_bug @tdd_bug_554 @tdd_expected_fail`) + covering empty list, empty-list format validation (JSON/YAML/plain), + init-then-list lifecycle, post-create list, rich/JSON/plain/YAML output + formats, and stderr error-path assertions. Robot Framework integration + smoke tests and ASV service-layer benchmarks. Implements + `@tdd_expected_fail` infrastructure (Behave `after_scenario` hook and Robot + listener) and migrates 18 existing TDD scenarios from `@tdd @bugNNN` to + `@tdd_bug @tdd_bug_NNN` convention. (#554) - Fixed `agents project show` not finding a project immediately after creation. Extended the `session.commit()` fix from #589 to also cover `update()` and `delete()` in `NamespacedProjectRepository`, and updated the class docstring diff --git a/benchmarks/session_list_bench.py b/benchmarks/session_list_bench.py new file mode 100644 index 000000000..d17abfd11 --- /dev/null +++ b/benchmarks/session_list_bench.py @@ -0,0 +1,157 @@ +"""ASV benchmarks for session list service-layer performance baseline (bug #554). + +Measures the cost of listing sessions through ``PersistentSessionService`` +using a file-based SQLite database. These benchmarks construct the service +directly (bypassing DI wiring) and therefore do **not** exercise the +``_get_session_service()`` code path that triggers bug #554. Their purpose +is to establish a performance baseline for the service layer itself. +""" + +from __future__ import annotations + +import os +import shutil +import sys +import tempfile +from pathlib import Path + +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +from sqlalchemy import create_engine # noqa: E402 +from sqlalchemy.orm import sessionmaker # noqa: E402 + +from cleveragents.application.services.session_service import ( # noqa: E402 + PersistentSessionService, +) +from cleveragents.infrastructure.database.models import Base # noqa: E402 +from cleveragents.infrastructure.database.repositories import ( # noqa: E402 + SessionMessageRepository, + SessionRepository, +) + + +class SessionListDISuite: + """Benchmark session list through the service layer (direct construction). + + Engine and sessionmaker are built once in ``setup()`` and reused across + ``time_list_empty`` iterations. Methods that mutate state + (``time_list_after_create``, ``track_list_after_create_count``) use + per-method setup/teardown to get a fresh database each time, preventing + row accumulation across ASV iterations. + + Does **not** exercise ``_get_session_service()`` or the DI container + wiring. + """ + + timeout = 30.0 + + # -- suite-level setup (shared engine for read-only benchmarks) ---------- + + def setup(self) -> None: + self._tmpdir = tempfile.mkdtemp(prefix="bench_sle_554_") + self._db_path = os.path.join(self._tmpdir, "bench.db") + self._engine = create_engine( + f"sqlite:///{self._db_path}", + echo=False, + ) + Base.metadata.create_all(self._engine) + self._session_factory = sessionmaker( + bind=self._engine, + expire_on_commit=False, + ) + + def teardown(self) -> None: + self._engine.dispose() + shutil.rmtree(self._tmpdir, ignore_errors=True) + + def _make_service(self) -> PersistentSessionService: + """Build a PersistentSessionService using the shared session factory.""" + return PersistentSessionService( + session_repo=SessionRepository( + session_factory=self._session_factory, + ), + message_repo=SessionMessageRepository( + session_factory=self._session_factory, + ), + ) + + # -- per-method setup for mutating benchmarks ---------------------------- + + def _fresh_engine(self) -> None: + """Create an isolated engine+factory so create() doesn't accumulate.""" + self._mut_tmpdir = tempfile.mkdtemp(prefix="bench_sle_554_mut_") + db_path = os.path.join(self._mut_tmpdir, "bench.db") + self._mut_engine = create_engine( + f"sqlite:///{db_path}", + echo=False, + ) + Base.metadata.create_all(self._mut_engine) + self._mut_factory = sessionmaker( + bind=self._mut_engine, + expire_on_commit=False, + ) + + def _dispose_fresh(self) -> None: + if hasattr(self, "_mut_engine"): + self._mut_engine.dispose() + if hasattr(self, "_mut_tmpdir"): + shutil.rmtree(self._mut_tmpdir, ignore_errors=True) + + def _make_mut_service(self) -> PersistentSessionService: + return PersistentSessionService( + session_repo=SessionRepository(session_factory=self._mut_factory), + message_repo=SessionMessageRepository( + session_factory=self._mut_factory, + ), + ) + + # -- ASV per-method hooks ------------------------------------------------ + + def setup_time_list_after_create(self) -> None: + self._fresh_engine() + + def teardown_time_list_after_create(self) -> None: + self._dispose_fresh() + + def setup_track_list_after_create_count(self) -> None: + self._fresh_engine() + + def teardown_track_list_after_create_count(self) -> None: + self._dispose_fresh() + + def setup_time_list_empty(self) -> None: + self._empty_svc = self._make_service() + + # -- benchmarks ---------------------------------------------------------- + + def time_list_empty(self) -> None: + """List sessions when DB is empty (service-layer only, no DI).""" + self._empty_svc.list() + + def time_list_after_create(self) -> None: + """Create then list (service-layer only, fresh DB per iteration). + + Bypasses ``_get_session_service()`` / DI container — measures the + service-layer round-trip cost, not the DI wiring path. + """ + svc = self._make_mut_service() + svc.create() + svc.list() + + def track_list_after_create_count(self) -> int: + """Track session list persistence at the service layer. + + Returns the count of sessions visible after a create. Uses a + fresh DB per iteration so the count is always exactly 1. + This benchmark constructs ``PersistentSessionService`` directly, + bypassing the DI container — it does **not** reproduce bug #554. + """ + svc = self._make_mut_service() + svc.create(actor_name="bench/test") + sessions = svc.list() + return len(sessions) + + +SessionListDISuite.track_list_after_create_count.unit = "sessions" diff --git a/features/cli_init_yes_flag.feature b/features/cli_init_yes_flag.feature index 128e206de..f2a3609f8 100644 --- a/features/cli_init_yes_flag.feature +++ b/features/cli_init_yes_flag.feature @@ -13,14 +13,14 @@ Feature: CLI init --yes flag for non-interactive initialization I want to run "agents init --yes" for non-interactive initialization So that I can skip interactive prompts and use sensible defaults - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: agents init --yes completes without error Given I have a temporary project directory for init When I run agents init with the --yes flag Then the init command should exit with code 0 And the project service initialize_project should have been called - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: --yes suppresses interactive prompts Given I have a temporary project directory for init When I run agents init with the --yes flag @@ -28,7 +28,7 @@ Feature: CLI init --yes flag for non-interactive initialization And the init output should contain "Initialized (non-interactive)" And no interactive prompt should have been presented - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: -y short-form alias completes without error Given I have a temporary project directory for init When I run agents init with the -y flag @@ -36,7 +36,7 @@ Feature: CLI init --yes flag for non-interactive initialization And the init output should contain "Initialized (non-interactive)" And the project service initialize_project should have been called - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: Output includes expected initialization summary Given I have a temporary project directory for init When I run agents init with the --yes flag @@ -48,7 +48,7 @@ Feature: CLI init --yes flag for non-interactive initialization And the init output should contain "logs, cache, sessions, contexts" And the init output should contain "Initialized" - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: Interactive mode without --yes presents a prompt Given I have a temporary project directory for init When I run agents init without the --yes flag diff --git a/features/environment.py b/features/environment.py index e71e026f8..97fc13fb4 100644 --- a/features/environment.py +++ b/features/environment.py @@ -307,6 +307,37 @@ def before_scenario(context, scenario): def after_scenario(context, scenario): """Clean up after each scenario.""" + # ── @tdd_expected_fail inversion ────────────────────────────────── + # When a scenario is tagged @tdd_expected_fail the test captures a + # bug that has NOT yet been fixed. The assertions describe the + # *correct* (post-fix) behaviour, so the scenario is expected to + # FAIL while the bug exists. We invert the result so CI stays + # green: + # • scenario FAILED → mark PASSED (expected — bug still exists) + # • scenario PASSED → mark FAILED (unexpected — fix landed but + # the @tdd_expected_fail tag was not removed) + # See CONTRIBUTING.md § TDD Bug Test Tags for the full convention. + if "tdd_expected_fail" in scenario.tags: + from behave.model import Status + + if scenario.status == Status.failed: + # Expected failure — reset all steps and the scenario so + # Behave counts this as a pass. + for step in scenario.steps: + step.status = Status.passed + step.error_message = None + scenario.clear_status() + scenario.set_status(Status.passed) + elif scenario.status == Status.passed: + # Unexpected pass — the bug appears fixed but the tag was + # not removed. Force a failure so the developer notices. + scenario.set_status(Status.failed) + scenario.error_message = ( + "[tdd_expected_fail] Test passed but still has the " + "tdd_expected_fail tag. The bug appears to be fixed " + "— remove the tag." + ) + # Return to original directory first if hasattr(context, "original_cwd"): os.chdir(context.original_cwd) diff --git a/features/project_create_persist.feature b/features/project_create_persist.feature index 23b168b4d..09902d647 100644 --- a/features/project_create_persist.feature +++ b/features/project_create_persist.feature @@ -7,13 +7,13 @@ Feature: Project create persists to database Background: Given a fresh project-persist database is initialised - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Created project appears in project list When I create a project named "local/my-app" via the persist CLI And I list projects via the persist CLI Then the persist project list should contain "local/my-app" - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Multiple created projects all appear in list When I create a project named "local/alpha" via the persist CLI And I create a project named "local/beta" via the persist CLI @@ -21,13 +21,13 @@ Feature: Project create persists to database Then the persist project list should contain "local/alpha" And the persist project list should contain "local/beta" - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Bare project name uses default namespace and persists When I create a project named "my-app" via the persist CLI And I list projects via the persist CLI Then the persist project list should contain "local/my-app" - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Creating a duplicate project produces an error When I create a project named "local/dup-proj" via the persist CLI And I attempt to create a duplicate project named "local/dup-proj" via the persist CLI diff --git a/features/project_show_after_create.feature b/features/project_show_after_create.feature index fa8d30de4..01ebbb4e4 100644 --- a/features/project_show_after_create.feature +++ b/features/project_show_after_create.feature @@ -7,14 +7,14 @@ Feature: Project show displays a created project Background: Given a fresh project-show database is initialised - @tdd @bug590 + @tdd_bug @tdd_bug_590 Scenario: Show displays a project that was just created When I create a project named "local/my-app" via the project-show CLI And I show the project "local/my-app" via the project-show CLI Then the project-show output should contain "local/my-app" And the project-show exit code should be 0 - @tdd @bug590 + @tdd_bug @tdd_bug_590 Scenario: Show displays correct details for a created project with description When I create a described project named "local/webapp" with description "My web app" via the project-show CLI And I show the project "local/webapp" via the project-show CLI @@ -22,7 +22,7 @@ Feature: Project show displays a created project And the project-show output should contain "My web app" And the project-show exit code should be 0 - @tdd @bug590 + @tdd_bug @tdd_bug_590 Scenario: Show returns error for a project that does not exist When I show the project "local/nonexistent" via the project-show CLI Then the project-show output should contain "not found" diff --git a/features/resource_type_bootstrap_fs.feature b/features/resource_type_bootstrap_fs.feature index e786b3e91..9ae2fc742 100644 --- a/features/resource_type_bootstrap_fs.feature +++ b/features/resource_type_bootstrap_fs.feature @@ -17,7 +17,7 @@ Feature: Built-in fs-directory Resource Type Bootstrap # ResourceRegistryService.__init__(), you will need to update the Given # step to exercise the init path instead of constructing a bare service. - @tdd @bug523 + @tdd_bug @tdd_bug_523 Scenario: fs-directory type exists after init without explicit bootstrap call Given a fresh in-memory resource registry without bootstrap When I query the fs bootstrap resource type registry for "fs-directory" @@ -25,7 +25,7 @@ Feature: Built-in fs-directory Resource Type Bootstrap # ── Regression: bootstrap function itself works correctly ── - @tdd @bug523 + @tdd_bug @tdd_bug_523 Scenario: After initialization fs-directory type exists in the registry Given a fresh in-memory resource registry with bootstrap When I query the fs bootstrap resource type registry for "fs-directory" @@ -35,7 +35,7 @@ Feature: Built-in fs-directory Resource Type Bootstrap # ── CLI add command ──────────────────────────────────────── - @tdd @bug523 + @tdd_bug @tdd_bug_523 Scenario: resource add fs-directory succeeds after bootstrap Given a fresh in-memory resource registry with bootstrap When I run resource add for type "fs-directory" named "local/test" with path "/tmp/test" diff --git a/features/resource_type_bootstrap_git.feature b/features/resource_type_bootstrap_git.feature index 34e7e62b8..d8718d8f3 100644 --- a/features/resource_type_bootstrap_git.feature +++ b/features/resource_type_bootstrap_git.feature @@ -16,7 +16,7 @@ Feature: Built-in git-checkout type bootstrap on initialization # ResourceRegistryService.__init__(), you will need to update the Given # step to exercise the init path instead of constructing a bare service. - @tdd @bug524 + @tdd_bug @tdd_bug_524 Scenario: git-checkout type is missing when bootstrap is not called during init Given a bootstrap-git fresh in-memory resource registry without bootstrap When I query the bootstrap-git resource type registry for "git-checkout" @@ -24,7 +24,7 @@ Feature: Built-in git-checkout type bootstrap on initialization # ── Regression: bootstrap function itself works correctly ── - @tdd @bug524 + @tdd_bug @tdd_bug_524 Scenario: After initialization the git-checkout type exists in the resource type registry Given a bootstrap-git fresh in-memory resource registry with bootstrap When I query the bootstrap-git resource type registry for "git-checkout" @@ -36,7 +36,7 @@ Feature: Built-in git-checkout type bootstrap on initialization # ── CLI resource add succeeds ────────────────────────────── - @tdd @bug524 + @tdd_bug @tdd_bug_524 Scenario: agents resource add git-checkout succeeds without Resource type not found error Given a bootstrap-git fresh in-memory resource registry with bootstrap When I run bootstrap-git resource add for type "git-checkout" named "local/test" with path "/tmp/repo" and branch "main" diff --git a/features/session_list_error.feature b/features/session_list_error.feature new file mode 100644 index 000000000..3be95a257 --- /dev/null +++ b/features/session_list_error.feature @@ -0,0 +1,85 @@ +# TDD tests for bug #554 — expected to fail until the DI container fix lands. +# Once the fix is applied, remove the @tdd_expected_fail tags and verify all +# scenarios pass. +Feature: Session list command handles missing database gracefully + As a developer using the agents CLI + I want "agents session list" to work after a fresh init + So that I can view my sessions without a DI container error + + Background: + Given a session-list-error CLI runner using the real DI path + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list returns empty list when no sessions exist + When I invoke session-list-error list with default format + Then the session-list-error command should exit successfully + And the session-list-error output should contain "No sessions found" + And the session-list-error output should not contain "AttributeError" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list after init does not raise DI error + When I invoke session-list-error list with default format + Then the session-list-error command should exit successfully + And the session-list-error output should not contain "AttributeError" + And the session-list-error output should not contain "INTERNAL" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list returns sessions after creation via service + Given a session-list-error service with a pre-populated session + When I invoke session-list-error list with default format + Then the session-list-error command should exit successfully + And the session-list-error output should contain "Sessions (" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list works with rich output format + Given a session-list-error service with a pre-populated session + When I invoke session-list-error list with format "rich" + Then the session-list-error command should exit successfully + And the session-list-error output should contain "Sessions (" + And the session-list-error output should not contain "AttributeError" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list works with JSON output format + Given a session-list-error service with a pre-populated session + When I invoke session-list-error list with format "json" + Then the session-list-error command should exit successfully + And the session-list-error output should be valid JSON containing "sessions" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list works with plain output format + Given a session-list-error service with a pre-populated session + When I invoke session-list-error list with format "plain" + Then the session-list-error command should exit successfully + And the session-list-error output should contain "Sessions (" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Session list works with YAML output format + Given a session-list-error service with a pre-populated session + When I invoke session-list-error list with format "yaml" + Then the session-list-error command should exit successfully + And the session-list-error output should be valid YAML containing "sessions" + + # Empty-list format scenarios (F2/F3) — exercises the empty-list code path + # with explicit output formats. The production code currently bypasses + # --format for empty lists, so these document the expected behaviour. + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Empty session list with JSON format produces valid JSON + When I invoke session-list-error list with format "json" + Then the session-list-error command should exit successfully + And the session-list-error output should be valid JSON containing "sessions" + And the session-list-error output should not contain "AttributeError" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Empty session list with YAML format produces valid YAML + When I invoke session-list-error list with format "yaml" + Then the session-list-error command should exit successfully + And the session-list-error output should be valid YAML containing "sessions" + And the session-list-error output should not contain "AttributeError" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + Scenario: Empty session list with plain format does not error + When I invoke session-list-error list with format "plain" + Then the session-list-error command should exit successfully + And the session-list-error output should contain "No sessions found" + And the session-list-error output should not contain "AttributeError" diff --git a/features/steps/session_list_error_steps.py b/features/steps/session_list_error_steps.py new file mode 100644 index 000000000..3001d99de --- /dev/null +++ b/features/steps/session_list_error_steps.py @@ -0,0 +1,217 @@ +"""Step definitions for session_list_error.feature (bug #554). + +TDD regression tests for ``agents session list`` after ``agents init``. +These scenarios assert the correct expected behaviour and will fail until +the DI container fix is applied. + +Design rationale +~~~~~~~~~~~~~~~~ +``_get_session_service()`` calls ``container.db()`` but the DI ``Container`` +class has no ``db`` provider, raising ``AttributeError``. + +We reset ``_service`` to ``None`` so the real ``_get_session_service()`` is +exercised. A file-based SQLite database and ``CLEVERAGENTS_DATABASE_URL`` +override ensure the commands can reach the database once the fix lands. + +Private API access +~~~~~~~~~~~~~~~~~~ +This module accesses ``session_mod._service`` (module-level singleton cache) +to force the real ``_get_session_service()`` code path during tests. This is +intentional: the public API (``CliRunner.invoke``) does not expose the DI +wiring that triggers the bug, so we must bypass the cache to exercise it. +""" + +from __future__ import annotations + +import json +import os +import shutil +import tempfile + +import yaml +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine +from sqlalchemy.orm import scoped_session, sessionmaker +from typer.testing import CliRunner + +from cleveragents.application.container import reset_container +from cleveragents.application.services.session_service import ( + PersistentSessionService, +) +from cleveragents.cli.commands import session as session_mod +from cleveragents.cli.commands.session import app as session_app +from cleveragents.infrastructure.database.models import Base +from cleveragents.infrastructure.database.repositories import ( + SessionMessageRepository, + SessionRepository, +) + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _setup_real_di_path(context: Context) -> None: + """Prepare a temp dir with a fresh SQLite DB and override container.""" + # Store original _service so cleanup can restore it. + context.sle_original_service = session_mod._service + + # Reset any stale DI container singleton before configuring the env var + # so that cached providers don't carry over from a prior test suite (F17). + reset_container() + + context.sle_tmpdir = tempfile.mkdtemp(prefix="session_list_err_554_") + + # Register cleanup immediately after mkdtemp so the temp directory is + # always removed even if the rest of setup fails (F21). + context.add_cleanup(_cleanup_sle, context) + + context.sle_db_path = os.path.join(context.sle_tmpdir, "test.db") + db_url = f"sqlite:///{context.sle_db_path}" + + # Create schema so the DB file exists with all tables. + engine = create_engine(db_url, echo=False) + try: + Base.metadata.create_all(engine) + finally: + engine.dispose() + + # Override the container's database_url so real DI can find the DB. + os.environ["CLEVERAGENTS_DATABASE_URL"] = db_url + + # Reset the module-level _service so _get_session_service() is used. + # This direct attribute mutation is fragile — if the module's internal + # caching mechanism changes (e.g. lazy singleton via descriptor), this + # line will need to be updated. See module docstring for rationale. + session_mod._service = None + + context.sle_result = None + + +def _cleanup_sle(context: Context) -> None: + """Remove temp dir, restore env, original _service, and container.""" + session_mod._service = context.sle_original_service + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + + # Reset the DI container singleton to avoid polluting later scenarios. + reset_container() + + shutil.rmtree(context.sle_tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a session-list-error CLI runner using the real DI path") +def step_session_list_error_runner(context: Context) -> None: + _setup_real_di_path(context) + + +# --------------------------------------------------------------------------- +# Given - pre-populated session +# --------------------------------------------------------------------------- + + +@given("a session-list-error service with a pre-populated session") +def step_pre_populate_session(context: Context) -> None: + """Insert a session directly via the repository so list has data.""" + db_url = f"sqlite:///{context.sle_db_path}" + engine = create_engine(db_url, echo=False) + try: + factory = scoped_session( + sessionmaker(bind=engine, expire_on_commit=False), + ) + repo = SessionRepository(session_factory=factory) + msg_repo = SessionMessageRepository(session_factory=factory) + svc = PersistentSessionService(repo, msg_repo) + svc.create(actor_name="openai/gpt-4") + # Commit via the scoped session so the data is visible to later queries. + factory().commit() + factory.remove() + finally: + engine.dispose() + + +# --------------------------------------------------------------------------- +# When - list +# --------------------------------------------------------------------------- + + +@when("I invoke session-list-error list with default format") +def step_invoke_list_default(context: Context) -> None: + context.sle_result = runner.invoke(session_app, ["list"]) + + +@when('I invoke session-list-error list with format "{fmt}"') +def step_invoke_list_format(context: Context, fmt: str) -> None: + context.sle_result = runner.invoke(session_app, ["list", "--format", fmt]) + + +# --------------------------------------------------------------------------- +# Then - assertions +# --------------------------------------------------------------------------- + + +@then("the session-list-error command should exit successfully") +def step_exit_success(context: Context) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + assert result.exit_code == 0, ( + f"Expected exit code 0, got {result.exit_code}.\n" + f"Output: {result.output}\n" + f"Exception: {result.exception!r}" + ) + + +@then('the session-list-error output should contain "{text}"') +def step_output_contains(context: Context, text: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + assert text in result.output, ( + f"Expected '{text}' in output but got:\n{result.output}" + ) + + +@then('the session-list-error output should not contain "{text}"') +def step_output_not_contains(context: Context, text: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + assert text not in result.output, ( + f"Did not expect '{text}' in output but found it:\n{result.output}" + ) + + +@then('the session-list-error output should be valid JSON containing "{key}"') +def step_output_json_key(context: Context, key: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + try: + data = json.loads(result.output) + except json.JSONDecodeError as exc: + raise AssertionError(f"Output is not valid JSON:\n{result.output}") from exc + assert isinstance(data, dict), f"Expected JSON object, got {type(data)}: {data}" + assert key in data, f"Key '{key}' not in JSON: {data}" + assert isinstance(data[key], list), ( + f"Expected '{key}' to be a list, got {type(data[key])}: {data[key]}" + ) + + +@then('the session-list-error output should be valid YAML containing "{key}"') +def step_output_yaml_key(context: Context, key: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + try: + data = yaml.safe_load(result.output) + except yaml.YAMLError as exc: + raise AssertionError(f"Output is not valid YAML:\n{result.output}") from exc + assert isinstance(data, dict), f"Expected YAML dict, got {type(data)}: {data}" + assert key in data, f"Key '{key}' not in YAML: {data}" + assert isinstance(data[key], list), ( + f"Expected '{key}' to be a list, got {type(data[key])}: {data[key]}" + ) diff --git a/noxfile.py b/noxfile.py index 8489678f6..89bf9b713 100644 --- a/noxfile.py +++ b/noxfile.py @@ -371,7 +371,7 @@ def main(argv=None): if processes <= 1 or coverage_mode or len(feature_paths) == 1: # ---- sequential in-process mode ---- - failed, total = _run_features_inprocess(feature_paths, other_args) + _, total = _run_features_inprocess(feature_paths, other_args) else: # ---- parallel in-process mode (multiprocessing fork) ---- # Pre-import heavy modules so forked children get them for free. @@ -398,21 +398,37 @@ def main(argv=None): [(chunk, other_args) for chunk in chunks], ) - failed = False summaries = [] - for worker_failed, stdout, stderr, summary in results: + for _worker_failed, stdout, stderr, summary in results: if stdout: print(stdout, end="") if stderr: print(stderr, end="", file=sys.stderr) - failed = failed or worker_failed summaries.append(summary) total = _merge_summaries(summaries) wall = time.monotonic() - start _print_overall_summary(total, wall_seconds=wall) - if failed or _has_failures(total): + # Use the summary-based check rather than the raw runner ``failed`` + # boolean. The ``@tdd_expected_fail`` handler in environment.py + # inverts scenario statuses for TDD bug-capture tests, but behave's + # ``runner.run()`` tracks step failures in a local variable that + # cannot be updated by after_scenario hooks. Relying solely on the + # summary (which reflects the corrected scenario statuses) ensures + # that TDD-inverted scenarios do not cause a spurious exit-code 1. + if _has_failures(total): + sys.exit(1) + + # Safety net: if features were requested but zero scenarios ran, the + # runner crashed before executing any scenario (e.g. ``before_all`` + # failure). Treat this as a failure so CI does not silently pass. + if feature_paths and total["scenarios"]["passed"] == 0 and total["scenarios"]["failed"] == 0: + print( + "ERROR: features were requested but no scenarios ran -- " + "possible runner-level crash.", + file=sys.stderr, + ) sys.exit(1) @@ -576,6 +592,8 @@ def integration_tests(session: nox.Session): "code_blocks", "--exclude", "wip", + "--listener", + "robot/tdd_expected_fail_listener.py", *robot_args, "robot/", ) @@ -598,6 +616,8 @@ def slow_integration_tests(session: nox.Session): "log.html", "--xunit", "xunit.xml", + "--listener", + "robot/tdd_expected_fail_listener.py", "robot/", *session.posargs, ) diff --git a/robot/session_list_error.robot b/robot/session_list_error.robot new file mode 100644 index 000000000..e7d0ecc62 --- /dev/null +++ b/robot/session_list_error.robot @@ -0,0 +1,58 @@ +*** Settings *** +Documentation Integration smoke test for session list DI error (bug #554). +... TDD-style tests — expected to FAIL until the DI container fix +... lands. The bug is that ``_get_session_service()`` calls +... ``container.db()`` but the DI container has no ``db`` provider, +... causing an ``AttributeError``. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Comments *** +# NOTE: ``Should Contain`` and ``Should Not Contain`` are case-sensitive +# by default in Robot Framework. The assertions below rely on the exact +# output format produced by the CLI: +# - "AttributeError" (Python exception class name, title-case) +# If the production code changes its error message casing, update these +# assertions accordingly. + +*** Test Cases *** +Session List After Init Should Not Error + [Documentation] After agents init, session list should exit 0 and show + ... "No sessions found" rather than a DI AttributeError. + [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sle_554_') + ${init}= Run Process ${PYTHON} -m cleveragents init sle-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init should exit 0 but got ${init.rc}. stderr: ${init.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents session list + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${list.rc} 0 + ... msg=session list should exit 0 but got ${list.rc}. stderr: ${list.stderr} + Should Not Contain ${list.stderr} AttributeError + ... msg=session list should not raise AttributeError in stderr: ${list.stderr} + Should Not Contain ${list.stdout} AttributeError + ... msg=session list should not raise AttributeError in stdout: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True + +Session List JSON Format Does Not Error + [Documentation] session list --format json should exit 0 without raising + ... a DI AttributeError. + [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sle_554_json_') + ${init}= Run Process ${PYTHON} -m cleveragents init sle-json + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init should exit 0 but got ${init.rc}. stderr: ${init.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents session list --format json + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${list.rc} 0 + ... msg=session list --format json should exit 0 but got ${list.rc}. stderr: ${list.stderr} + Should Not Contain ${list.stderr} AttributeError + ... msg=session list --format json should not raise AttributeError in stderr: ${list.stderr} + Should Not Contain ${list.stdout} AttributeError + ... msg=session list --format json should not raise AttributeError in stdout: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True diff --git a/robot/tdd_expected_fail_listener.py b/robot/tdd_expected_fail_listener.py new file mode 100644 index 000000000..d89f9ced7 --- /dev/null +++ b/robot/tdd_expected_fail_listener.py @@ -0,0 +1,48 @@ +"""Robot Framework listener that inverts results for ``tdd_expected_fail`` tests. + +When a test is tagged ``tdd_expected_fail``, the listener treats a FAIL as a +PASS (the bug still exists, which is expected) and a PASS as a FAIL (the bug +was fixed but the tag was not removed). + +See CONTRIBUTING.md § TDD Bug Test Tags for the full convention. + +Usage:: + + pabot ... --listener robot/tdd_expected_fail_listener.py ... + +This listener uses the Robot Framework Listener API v3. +""" + +from __future__ import annotations + +from typing import Any + +ROBOT_LISTENER_API_VERSION = 3 + +_TAG = "tdd_expected_fail" + + +def end_test(data: Any, result: Any) -> None: + """Invert the result of tests tagged ``tdd_expected_fail``.""" + tags = getattr(result, "tags", []) + if _TAG not in tags: + return + + status: str = getattr(result, "status", "") + original_message: str = getattr(result, "message", "") + + if status == "FAIL": + # Expected failure — bug still exists. Mark as PASS. + result.status = "PASS" + result.message = ( + f"[tdd_expected_fail] Expected failure (bug still present). " + f"Original: {original_message}" + ) + elif status == "PASS": + # Unexpected pass — bug appears fixed, tag should be removed. + result.status = "FAIL" + result.message = ( + "[tdd_expected_fail] Test passed but still has the " + "tdd_expected_fail tag. The bug appears to be fixed — " + "remove the tag." + ) -- 2.52.0