diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fb005cd3..2e632e073 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,20 @@ - 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 create` DI container wiring + error (bug #570). `_get_session_service()` calls `container.db()` but the + `Container` class has no `db` provider, raising `AttributeError`. Same root + cause as #554. Includes 4 Behave BDD scenarios + (`@tdd_bug @tdd_bug_570 @tdd_expected_fail`), Robot Framework integration + smoke tests, and ASV service-layer benchmarks. Tests exercise the real DI + path with `_service = None` and a file-based SQLite database. + Also implements the `@tdd_expected_fail` inversion infrastructure: + a Behave `after_scenario` hook in `features/environment.py` that flips + pass/fail for `@tdd_expected_fail` scenarios, and a Robot Framework + Listener API v3 plugin (`robot/tdd_expected_fail_listener.py`) with + identical semantics. Migrates 18 existing TDD scenarios from the old + `@tdd @bugNNN` convention to standardised `@tdd_bug @tdd_bug_NNN` tags. + (#570) - Fixed intermittent race condition in M4 validation integration tests when running under pabot. Root cause was three-pronged: shared SQLite DB URL, shared CLEVERAGENTS_HOME directory, and singleton leaks in chained CLI diff --git a/benchmarks/session_create_error_bench.py b/benchmarks/session_create_error_bench.py new file mode 100644 index 000000000..c84f1bf2c --- /dev/null +++ b/benchmarks/session_create_error_bench.py @@ -0,0 +1,100 @@ +"""ASV benchmarks for session create service-layer performance (bug #570). + +Measures the cost of creating a session through ``PersistentSessionService`` +using a file-based SQLite database so that each operation exercises the full +service layer (repository -> SQLAlchemy -> SQLite round-trip). + +Note: these benchmarks construct ``PersistentSessionService`` directly and do +**not** exercise the DI container wiring path (``_get_session_service`` / +``container.db()``). Their purpose is to establish a service-layer create +performance baseline so regressions can be detected after the bug-fix lands. +Same root cause as bug #554. +""" + +from __future__ import annotations + +import importlib +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) + +import cleveragents # noqa: E402 + +importlib.reload(cleveragents) + +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 SessionCreateDISuite: + """Benchmark session create through the service layer. + + Engine and sessionmaker are built once in ``setup()`` and reused across + benchmark iterations to measure service-layer cost without engine + construction overhead. + """ + + timeout = 60 + + def setup(self) -> None: + self._tmpdir = tempfile.mkdtemp(prefix="bench_sce_570_") + 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 + ), + ) + + def time_create_session(self) -> None: + """Create a session via the service layer.""" + svc = self._make_service() + svc.create() + + def time_create_with_actor(self) -> None: + """Create a session with a custom actor.""" + svc = self._make_service() + svc.create(actor_name="openai/gpt-4") + + def track_create_persists(self) -> int: + """Track session create persistence at the service layer. + + Returns the count of sessions created via the service. This + benchmark constructs ``PersistentSessionService`` directly, + bypassing the DI container (``_get_session_service`` / + ``container.db()``). It therefore does **not** reproduce bug + #570 — its purpose is to establish a service-layer persistence + baseline so regressions can be detected after the fix lands. + """ + svc = self._make_service() + svc.create(actor_name="bench/create-test") + sessions = svc.list() + return len(sessions) + + +SessionCreateDISuite.track_create_persists.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_create_error.feature b/features/session_create_error.feature new file mode 100644 index 000000000..667a33e00 --- /dev/null +++ b/features/session_create_error.feature @@ -0,0 +1,38 @@ +# TDD tests for bug #570 — expected to fail until the DI container fix lands. +# The @tdd_expected_fail tag causes the test framework to invert pass/fail so +# these scenarios pass CI while the bug is unfixed. The bug-fix developer +# removes @tdd_expected_fail (keeping @tdd_bug and @tdd_bug_570) once the fix +# is applied. +Feature: Session create command resolves DI container wiring + As a developer using the agents CLI + I want "agents session create" to work after a fresh init + So that I can create interactive sessions without a DI container error + + Background: + Given a session-create-error CLI runner using the real DI path + + @tdd_bug @tdd_bug_570 @tdd_expected_fail + Scenario: Session create produces a new session + When I invoke session-create-error create with no arguments + Then the session-create-error command should exit successfully + And the session-create-error output should contain "session_id:" + + @tdd_bug @tdd_bug_570 @tdd_expected_fail + Scenario: Created session persists and can be retrieved + When I invoke session-create-error create with no arguments + Then the session-create-error command should exit successfully + When I invoke session-create-error list to verify persistence + Then the session-create-error list should show at least one session + + @tdd_bug @tdd_bug_570 @tdd_expected_fail + Scenario: Session create with custom actor succeeds + When I invoke session-create-error create with actor "openai/gpt-4" + Then the session-create-error command should exit successfully + And the session-create-error output should contain "openai/gpt-4" + And the session-create-error output should contain "session_id:" + + @tdd_bug @tdd_bug_570 @tdd_expected_fail + Scenario: Session create with arbitrary actor name succeeds + When I invoke session-create-error create with actor "nonexistent/bogus-actor-999" + Then the session-create-error command should exit successfully + And the session-create-error output should contain "nonexistent/bogus-actor-999" diff --git a/features/steps/repositories_coverage_r2_steps.py b/features/steps/repositories_coverage_r2_steps.py index 149f8f014..296bd933f 100644 --- a/features/steps/repositories_coverage_r2_steps.py +++ b/features/steps/repositories_coverage_r2_steps.py @@ -18,7 +18,7 @@ from behave import given, then, when from behave.runner import Context from sqlalchemy import create_engine from sqlalchemy.exc import OperationalError -from sqlalchemy.orm import Session, sessionmaker +from sqlalchemy.orm import Session, scoped_session, sessionmaker from cleveragents.core.exceptions import DatabaseError from cleveragents.domain.models.core.checkpoint import Checkpoint, CheckpointMetadata @@ -234,7 +234,20 @@ def step_fresh_db(context: Context) -> None: engine = create_engine("sqlite:///:memory:", echo=False) Base.metadata.create_all(engine) context.r2_engine = engine - context.r2_session_factory = sessionmaker(bind=engine) + # Use scoped_session so that every ``factory()`` call within the + # same thread returns the *same* Session instance. With plain + # ``sessionmaker``, each ``factory()`` call creates a new Session. + # SQLite in-memory uses ``SingletonThreadPool`` (one connection per + # thread), so all sessions share the same connection. When a + # session created inside a repository method goes out of scope, + # Python's garbage collector may close it, issuing an implicit + # ROLLBACK on the shared connection — wiping flushed-but-uncommitted + # rows written by *other* sessions. Under high memory pressure + # (e.g. 32 parallel worker processes) GC fires often enough to + # cause intermittent data loss between ``flush()`` and ``commit()``. + # ``scoped_session`` avoids the problem entirely: one Session lives + # for the whole scenario, so there is no premature close/rollback. + context.r2_session_factory = scoped_session(sessionmaker(bind=engine)) # Pre-create repos used by multiple scenarios context.r2_skill_repo = SkillRepository( diff --git a/features/steps/session_create_error_steps.py b/features/steps/session_create_error_steps.py new file mode 100644 index 000000000..7913b6bde --- /dev/null +++ b/features/steps/session_create_error_steps.py @@ -0,0 +1,186 @@ +"""Step definitions for session_create_error.feature (bug #570). + +TDD regression tests for ``agents session create`` 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``. Same root cause +as bug #554. + +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. + +All CLI invocations use ``--format plain`` so output goes through +``typer.echo`` (captured by ``CliRunner``) rather than ``rich.console``. + +Private API access (``session_mod._service``, ``session_mod._reset_session_service``) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +These step definitions access private attributes of the ``session`` CLI +module because the DI integration tests *must* force the module to re-run +its service resolution logic. The module caches a singleton +``_service`` instance; resetting it to ``None`` is the only way to make +the CLI re-exercise ``_get_session_service()`` (the buggy code path). +If the module's internal caching mechanism changes, these tests will +need to be updated accordingly. +""" + +from __future__ import annotations + +import os +import shutil +import tempfile +from typing import Any + +from behave import given, then, when +from sqlalchemy import create_engine +from typer.testing import CliRunner + +from cleveragents.application.container import reset_container +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 + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _setup_real_di_path(context: Any) -> None: + """Prepare a temp dir with a fresh SQLite DB and override container. + + Registers cleanup immediately after capturing original state so that + env vars and module state are restored even if later setup lines raise. + """ + # Store original _service so cleanup can restore it. + context.sce_original_service = session_mod._service + context.sce_tmpdir = tempfile.mkdtemp(prefix="session_create_err_570_") + + # Register cleanup early so env/state is always restored (SEC-3). + context.add_cleanup(_cleanup_sce, context) + + context.sce_db_path = os.path.join(context.sce_tmpdir, "test.db") + db_url = f"sqlite:///{context.sce_db_path}" + + # Create schema so the DB file exists with all tables. + engine = create_engine(db_url, echo=False) + Base.metadata.create_all(engine) + engine.dispose() + + # Override the container's database_url so real DI can find the DB. + os.environ["CLEVERAGENTS_DATABASE_URL"] = db_url + + # Reset global DI container so post-fix DI reads the fresh env var. + reset_container() + + # 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.sce_result = None + context.sce_list_result = None + + +def _cleanup_sce(context: Any) -> None: + """Remove temp dir, restore env and original _service. + + Also resets the module-level ``_service`` cache via the public API so + that any DI-created engine is released before the temp dir is removed. + """ + # Ensure module-level cache is cleared; GC will release any engine when + # the container is reset (done separately via reset_container()). + session_mod._reset_session_service() + # Then restore the original _service value. + session_mod._service = context.sce_original_service + # Release any DI-created connections before removing temp dir. + reset_container() + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + shutil.rmtree(context.sce_tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a session-create-error CLI runner using the real DI path") +def step_session_create_error_runner(context: Any) -> None: + _setup_real_di_path(context) + + +# --------------------------------------------------------------------------- +# When - create +# --------------------------------------------------------------------------- + + +@when("I invoke session-create-error create with no arguments") +def step_invoke_create_no_args(context: Any) -> None: + context.sce_result = runner.invoke(session_app, ["create", "--format", "plain"]) + + +@when('I invoke session-create-error create with actor "{actor}"') +def step_invoke_create_with_actor(context: Any, actor: str) -> None: + context.sce_result = runner.invoke( + session_app, ["create", "--actor", actor, "--format", "plain"] + ) + + +# --------------------------------------------------------------------------- +# When - list (for persistence verification) +# --------------------------------------------------------------------------- + + +@when("I invoke session-create-error list to verify persistence") +def step_invoke_list_after_create(context: Any) -> None: + context.sce_list_result = runner.invoke(session_app, ["list", "--format", "plain"]) + + +# --------------------------------------------------------------------------- +# Then - assertions +# --------------------------------------------------------------------------- + + +@then("the session-create-error command should exit successfully") +def step_exit_success(context: Any) -> None: + result = context.sce_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-create-error output should contain "{text}"') +def step_output_contains(context: Any, text: str) -> None: + result = context.sce_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-create-error list should show at least one session") +def step_list_shows_sessions(context: Any) -> None: + result = context.sce_list_result + assert result is not None, "List was not invoked" + assert result.exit_code == 0, ( + f"Expected list exit code 0, got {result.exit_code}.\n" + f"Output: {result.output}\n" + f"Exception: {result.exception!r}" + ) + # In plain format the output should contain "total:" with a count > 0. + assert "total:" in result.output, ( + f"Expected 'total:' in plain list output but got:\n{result.output}" + ) + assert "total: 0" not in result.output, ( + f"Expected at least one session but got:\n{result.output}" + ) 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_create_error.robot b/robot/session_create_error.robot new file mode 100644 index 000000000..a57e239cb --- /dev/null +++ b/robot/session_create_error.robot @@ -0,0 +1,54 @@ +*** Settings *** +Documentation Integration smoke test for session create DI error (bug #570). +... 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``. Same root cause as bug #554. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Test Cases *** +Session Create After Init Should Not Error + [Documentation] After agents init, session create --format plain should + ... exit 0 and produce a new session rather than a DI + ... AttributeError. + [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sce_570_') + ${init}= Run Process ${PYTHON} -m cleveragents init sce-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} + ${create}= Run Process ${PYTHON} -m cleveragents session create --format plain + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=session create should exit 0 but got ${create.rc}. stderr: ${create.stderr} + Should Not Contain ${create.stderr} AttributeError + ... msg=session create should not raise AttributeError: ${create.stderr} + [Teardown] Remove Directory ${tmpdir} recursive=True + +Session Create Then List Shows Created Session + [Documentation] After creating a session, listing should show it. + [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sce_570_list_') + ${init}= Run Process ${PYTHON} -m cleveragents init sce-list + ... 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} + ${create}= Run Process ${PYTHON} -m cleveragents session create --format plain + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=session create should exit 0 but got ${create.rc}. stderr: ${create.stderr} + Should Not Contain ${create.stderr} AttributeError + ... msg=session create should not raise AttributeError: ${create.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents session list --format plain + ... 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 Contain ${list.stdout} total: + ... msg=Expected 'total:' in plain output: ${list.stdout} + Should Not Contain ${list.stdout} total: 0 + ... msg=Expected at least one session in list output: ${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." + )