diff --git a/CHANGELOG.md b/CHANGELOG.md index 95193207c..8235ae7b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ Changed `wf10_batch.robot` to be less likely to create files, and `plan_generation_graph.robot` to give more test answers. ## [Unreleased] +- **TDD regression tests for automation_profile DI bypass** (#1031): Added BDD scenarios + and Robot Framework integration tests verifying that ``_get_service()`` in + ``automation_profile.py`` resolves ``AutomationProfileService`` through the DI container + rather than directly calling ``create_engine`` or ``sessionmaker``. Bug #990 was fixed by + PR #1181 before this TDD test PR merged; these tests serve as permanent regression guards + confirming the fix remains in place. - Added team collaboration features: multi-user connection handling with user identity tracking (TeamMember with owner/admin/member/viewer roles), role-based access control (TeamPermission with read/write/admin/manage_members), diff --git a/features/steps/tdd_automation_profile_di_bypass_steps.py b/features/steps/tdd_automation_profile_di_bypass_steps.py new file mode 100644 index 000000000..9cfbf000e --- /dev/null +++ b/features/steps/tdd_automation_profile_di_bypass_steps.py @@ -0,0 +1,176 @@ +"""Step definitions for tdd_automation_profile_di_bypass.feature. + +These steps verify that ``_get_service()`` in +``cleveragents.cli.commands.automation_profile`` delegates to +``container.automation_profile_service()`` rather than manually constructing +the service with ``create_engine`` / ``sessionmaker`` / repositories. + +Bug #990 was fixed by PR #1181. The ``@tdd_expected_fail`` tag was omitted +because the fix landed on master before this TDD test PR merged; these +scenarios now run as permanent regression tests confirming the DI bypass +remains fixed. + +Step namespacing: all steps use the ``ap990-`` prefix to avoid collisions +with other test suites. The defensive patching approach provides both a DI +container mock *and* infrastructure mocks so that a regression causes the +tests to fail for the right reason (the DI bypass itself) rather than an +unrelated import error or attribute lookup. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context + +# --------------------------------------------------------------------------- +# Constants — patch targets +# --------------------------------------------------------------------------- + +_PATCH_GET_CONTAINER = "cleveragents.application.container.get_container" +_PATCH_CREATE_ENGINE = "sqlalchemy.create_engine" +_PATCH_SESSIONMAKER = "sqlalchemy.orm.sessionmaker" + + +# --------------------------------------------------------------------------- +# Scenario 1: _get_service delegates to container.automation_profile_service +# --------------------------------------------------------------------------- + + +@given("ap990- the DI container has an automation_profile_service provider") +def step_ap990_container_has_provider(context: Context) -> None: + """Set up a mock container with an automation_profile_service() callable. + + Defensive patching: both a DI container mock and silent sqlalchemy stubs + are set up so that if _get_service() regresses to the bug #990 pattern it + reaches the Then assertion (and fails there with a meaningful message) + rather than crashing on a missing DB URL. + """ + mock_service: MagicMock = MagicMock(name="MockAutomationProfileService") + mock_container: MagicMock = MagicMock(name="MockContainer") + mock_container.automation_profile_service.return_value = mock_service + + context.ap990_mock_container: MagicMock = mock_container + context.ap990_mock_service: MagicMock = mock_service + # Shared slot used by the When step that is also shared with Scenarios 2/3. + context.ap990_di_mock_container: MagicMock = mock_container + context.ap990_returned_service: Any = None + + +@when("ap990- _get_service is called with the mocked container") +def step_ap990_call_get_service_with_mock(context: Context) -> None: + """Call _get_service() with get_container() returning the mock container.""" + from cleveragents.cli.commands.automation_profile import _get_service + + with patch(_PATCH_GET_CONTAINER, return_value=context.ap990_mock_container): + context.ap990_returned_service = _get_service() + + +@then( + "ap990- the returned service should be the one from container.automation_profile_service" +) +def step_ap990_verify_service_identity(context: Context) -> None: + """The returned service must be exactly what container.automation_profile_service() returns. + + An identity check (``is``) catches regressions where _get_service() + bypasses the container and constructs a fresh AutomationProfileService + independently — the resulting object would differ from the mock sentinel. + """ + assert context.ap990_returned_service is context.ap990_mock_service, ( + f"Expected the service returned by container.automation_profile_service(), " + f"but got {type(context.ap990_returned_service)!r}. " + "_get_service() is NOT delegating to the DI container." + ) + + +# --------------------------------------------------------------------------- +# Scenario 2: _get_service does not call create_engine directly +# --------------------------------------------------------------------------- + + +@given("ap990- create_engine is patched to detect direct calls") +def step_ap990_patch_create_engine(context: Context) -> None: + """Patch create_engine and set up a DI mock so _get_service() can succeed. + + Defensive patching: the DI mock ensures the fixed code path (container + delegation) completes successfully. The create_engine mock catches any + regression where _get_service() rebuilds the session factory manually. + """ + create_engine_mock: MagicMock = MagicMock(name="MockCreateEngine") + patcher = patch(_PATCH_CREATE_ENGINE, create_engine_mock) + patcher.start() + context.add_cleanup(patcher.stop) + context.ap990_create_engine_mock: MagicMock = create_engine_mock + + mock_service: MagicMock = MagicMock(name="MockAutomationProfileServiceForEngine") + mock_container: MagicMock = MagicMock(name="MockContainerForEngine") + mock_container.automation_profile_service.return_value = mock_service + context.ap990_di_mock_container: MagicMock = mock_container + + +@when("ap990- _get_service is called with a valid DI container") +def step_ap990_call_get_service_di_path(context: Context) -> None: + """Call _get_service() via the DI path; the mock container is set in the Given step.""" + from cleveragents.cli.commands.automation_profile import _get_service + + with patch(_PATCH_GET_CONTAINER, return_value=context.ap990_di_mock_container): + context.ap990_result: Any = _get_service() + + +@then("ap990- create_engine should not have been called") +def step_ap990_verify_create_engine_not_called(context: Context) -> None: + """create_engine must never be called; the DI container owns engine construction. + + If this assertion fails, _get_service() has regressed to the bug #990 + pattern of manually building the persistence layer with a raw SQLAlchemy + engine instead of delegating to container.automation_profile_service(). + """ + mock: MagicMock = context.ap990_create_engine_mock + assert not mock.called, ( + f"Bug #990 regression: _get_service() called create_engine() directly " + f"({mock.call_count} time(s)) instead of delegating to " + "container.automation_profile_service()." + ) + + +# --------------------------------------------------------------------------- +# Scenario 3: _get_service does not call sessionmaker directly +# --------------------------------------------------------------------------- + + +@given("ap990- sessionmaker is patched to detect direct calls") +def step_ap990_patch_sessionmaker(context: Context) -> None: + """Patch sessionmaker and set up a DI mock so _get_service() can succeed. + + Defensive patching mirrors Scenario 2: the DI mock ensures the fixed + code path completes without error, and the sessionmaker mock detects any + regression where _get_service() builds the ORM session factory directly. + """ + sessionmaker_mock: MagicMock = MagicMock(name="MockSessionmaker") + patcher = patch(_PATCH_SESSIONMAKER, sessionmaker_mock) + patcher.start() + context.add_cleanup(patcher.stop) + context.ap990_sessionmaker_mock: MagicMock = sessionmaker_mock + + mock_service: MagicMock = MagicMock(name="MockAutomationProfileServiceForSM") + mock_container: MagicMock = MagicMock(name="MockContainerForSM") + mock_container.automation_profile_service.return_value = mock_service + context.ap990_di_mock_container: MagicMock = mock_container + + +@then("ap990- sessionmaker should not have been called") +def step_ap990_verify_sessionmaker_not_called(context: Context) -> None: + """sessionmaker must never be called; the DI container owns session factory setup. + + If this assertion fails, _get_service() has regressed to the bug #990 + pattern of manually constructing the ORM session factory with a raw + sessionmaker call instead of delegating to container.automation_profile_service(). + """ + mock: MagicMock = context.ap990_sessionmaker_mock + assert not mock.called, ( + f"Bug #990 regression: _get_service() called sessionmaker() directly " + f"({mock.call_count} time(s)) instead of delegating to " + "container.automation_profile_service()." + ) diff --git a/features/tdd_automation_profile_di_bypass.feature b/features/tdd_automation_profile_di_bypass.feature new file mode 100644 index 000000000..8167e667b --- /dev/null +++ b/features/tdd_automation_profile_di_bypass.feature @@ -0,0 +1,32 @@ +@tdd_bug @tdd_bug_990 @tdd_issue @tdd_issue_990 +Feature: TDD Bug #990 — automation_profile._get_service() bypasses DI container + As a developer relying on the DI container for service wiring + I want _get_service() in automation_profile.py to resolve AutomationProfileService + through the DI container + So that service construction is consistent and dependencies are correctly injected + + This feature captures bug #990. Previously, _get_service() bypassed the DI + container by directly constructing the persistence layer with create_engine() + and sessionmaker() rather than delegating to container.automation_profile_service(). + This caused service wiring to diverge from the canonical DI path and broke + dependency injection for AutomationProfileService. + + Bug #990 was fixed by PR #1181, which updated _get_service() to resolve the + service through the DI container. The @tdd_expected_fail tag has been omitted + because the fix landed on master before this TDD test PR merged; these scenarios + serve as permanent regression tests confirming the DI bypass remains fixed. + + Scenario: Bug #990 — _get_service resolves AutomationProfileService through the DI container + Given ap990- the DI container has an automation_profile_service provider + When ap990- _get_service is called with the mocked container + Then ap990- the returned service should be the one from container.automation_profile_service + + Scenario: Bug #990 — _get_service does not call create_engine directly + Given ap990- create_engine is patched to detect direct calls + When ap990- _get_service is called with a valid DI container + Then ap990- create_engine should not have been called + + Scenario: Bug #990 — _get_service does not call sessionmaker directly + Given ap990- sessionmaker is patched to detect direct calls + When ap990- _get_service is called with a valid DI container + Then ap990- sessionmaker should not have been called diff --git a/robot/helper_tdd_automation_profile_di_bypass.py b/robot/helper_tdd_automation_profile_di_bypass.py new file mode 100644 index 000000000..1e95d9cc2 --- /dev/null +++ b/robot/helper_tdd_automation_profile_di_bypass.py @@ -0,0 +1,121 @@ +"""Helper script for tdd_automation_profile_di_bypass.robot smoke tests. + +Each subcommand exercises ``_get_service()`` from +``cleveragents.cli.commands.automation_profile`` to verify that it +delegates to the DI container rather than manually constructing the +persistence layer with ``create_engine`` / ``sessionmaker``. + +Bug #990 was fixed by PR #1181. These helpers report pass (exit 0 + sentinel) +when the DI delegation is correct, and fail (exit 1) when the bug regresses. +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +# Ensure local source tree is importable. +_ROOT = Path(__file__).resolve().parents[1] +_SRC = str(_ROOT / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +_PATCH_GET_CONTAINER = "cleveragents.application.container.get_container" +_PATCH_CREATE_ENGINE = "sqlalchemy.create_engine" +_PATCH_SESSIONMAKER = "sqlalchemy.orm.sessionmaker" + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def container_resolution() -> None: + """Verify _get_service() returns the container's automation_profile_service. + + Exits 0 with sentinel when the DI container is correctly used (bug fixed). + Exits 1 when _get_service() does not delegate to the container (bug regressed). + """ + from cleveragents.cli.commands.automation_profile import _get_service + + mock_service: MagicMock = MagicMock(name="MockAutomationProfileService") + mock_container: MagicMock = MagicMock(name="MockContainer") + mock_container.automation_profile_service.return_value = mock_service + + with patch(_PATCH_GET_CONTAINER, return_value=mock_container): + result: Any = _get_service() + + if result is not mock_service: + print( + f"_get_service() returned {type(result)!r} instead of the " + "container service. Bug #990 regression: not delegating to DI container.", + file=sys.stderr, + ) + sys.exit(1) + + print("tdd-ap990-container-resolution-ok") + + +def di_bypass() -> None: + """Verify _get_service() does not call create_engine or sessionmaker directly. + + Exits 0 with sentinel when neither is called (bug fixed). + Exits 1 with an explanation when either is called (bug regressed). + """ + from cleveragents.cli.commands.automation_profile import _get_service + + create_engine_mock: MagicMock = MagicMock(name="MockCreateEngine") + sessionmaker_mock: MagicMock = MagicMock(name="MockSessionmaker") + + mock_service: MagicMock = MagicMock(name="MockAutomationProfileService") + mock_container: MagicMock = MagicMock(name="MockContainer") + mock_container.automation_profile_service.return_value = mock_service + + with ( + patch(_PATCH_CREATE_ENGINE, create_engine_mock), + patch(_PATCH_SESSIONMAKER, sessionmaker_mock), + patch(_PATCH_GET_CONTAINER, return_value=mock_container), + ): + _get_service() + + if create_engine_mock.called: + print( + f"Bug #990 regression: _get_service() called create_engine() directly " + f"({create_engine_mock.call_count} time(s)) instead of delegating to " + "container.automation_profile_service().", + file=sys.stderr, + ) + sys.exit(1) + + if sessionmaker_mock.called: + print( + f"Bug #990 regression: _get_service() called sessionmaker() directly " + f"({sessionmaker_mock.call_count} time(s)) instead of delegating to " + "container.automation_profile_service().", + file=sys.stderr, + ) + sys.exit(1) + + print("tdd-ap990-di-bypass-ok") + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "container-resolution": container_resolution, + "di-bypass": di_bypass, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) + sys.exit(1) + _COMMANDS[sys.argv[1]]() diff --git a/robot/tdd_automation_profile_di_bypass.robot b/robot/tdd_automation_profile_di_bypass.robot new file mode 100644 index 000000000..cba9f8020 --- /dev/null +++ b/robot/tdd_automation_profile_di_bypass.robot @@ -0,0 +1,31 @@ +*** Settings *** +Documentation TDD Bug #990 — automation_profile._get_service() bypasses DI container +... Integration smoke tests verifying that _get_service() in +... automation_profile.py resolves AutomationProfileService through the +... DI container rather than calling create_engine or sessionmaker directly. +... Bug #990 was fixed by PR #1181; these tests serve as regression guards. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_automation_profile_di_bypass.py + +*** Test Cases *** +TDD Automation Profile DI Container Resolution + [Documentation] Verify _get_service() returns the service from the DI container + [Tags] tdd_bug tdd_bug_990 tdd_issue tdd_issue_990 + ${result}= Run Process ${PYTHON} ${HELPER} container-resolution cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-ap990-container-resolution-ok + +TDD Automation Profile DI Bypass Check + [Documentation] Verify _get_service() does not call create_engine or sessionmaker directly + [Tags] tdd_bug tdd_bug_990 tdd_issue tdd_issue_990 + ${result}= Run Process ${PYTHON} ${HELPER} di-bypass cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-ap990-di-bypass-ok