test(cli): add failing tests for agents init --yes missing option (#536) #566
@@ -2,6 +2,11 @@
|
||||
|
||||
## Unreleased
|
||||
|
||||
- Added TDD-style failing Behave BDD tests for the missing `agents init --yes` flag.
|
||||
Five scenarios: four TDD-failing tests (exit code, prompt suppression, `-y` alias,
|
||||
output summary) and one regression guard for interactive mode. Includes Robot
|
||||
Framework smoke tests and ASV benchmarks. Tests are intentionally failing until
|
||||
the bug fix for #522 is applied. (#536)
|
||||
- Implemented UKO Layer 1 Domain Ontologies (`uko-doc:`, `uko-data:`, `uko-infra:`)
|
||||
in the OWL/Turtle ontology file (`docs/ontology/uko.ttl`). Added 17 `uko-doc:` classes
|
||||
(Document, Section, Paragraph, Citation, etc.), 13 `uko-data:` classes (Table, Column,
|
||||
|
||||
@@ -1,4 +1,10 @@
|
||||
[behave]
|
||||
paths = features
|
||||
# Exclude @wip scenarios globally so TDD failing tests do not break CI.
|
||||
# Any contributor tagging a scenario @wip will have it skipped by default.
|
||||
# NOTE: --tags=@wip on the CLI will NOT work; Behave ANDs ini and CLI tags.
|
||||
# To run a @wip scenario locally, target it by file/line number:
|
||||
# behave features/<file>.feature:<line>
|
||||
tags = ~@wip
|
||||
stdout_capture = no
|
||||
stderr_capture = no
|
||||
|
||||
@@ -0,0 +1,118 @@
|
||||
"""ASV benchmarks for ``agents init --yes`` invocation time.
|
||||
|
||||
Measures in-process execution time for the init command with the --yes flag,
|
||||
which should perform non-interactive initialization using defaults.
|
||||
|
||||
These benchmarks target bug #522 and are expected to error until the fix
|
||||
is applied (the --yes flag does not yet exist).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import shutil
|
||||
import sys
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from unittest.mock import create_autospec, patch
|
||||
|
||||
try:
|
||||
from cleveragents.cli.main import app
|
||||
except ModuleNotFoundError:
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src"))
|
||||
from cleveragents.cli.main import app
|
||||
|
||||
from typer.testing import CliRunner
|
||||
|
||||
from cleveragents.application.services.project_service import ProjectService
|
||||
|
||||
|
||||
class _MockProject:
|
||||
"""Lightweight stand-in for the Project model with spec fields.
|
||||
|
||||
|
|
||||
We cannot use ``create_autospec(Project)`` because the spec-required
|
||||
output fields (``data_dir``, ``config_path``, ``database_status``,
|
||||
|
brent.edwards marked this conversation as resolved
Outdated
brent.edwards
commented
Nit: No Nit: No `teardown()` method to clean up `self._tmpdir`. ASV calls `setup()` before each iteration, so repeated runs will accumulate orphan temp directories. Consider adding:
```python
def teardown(self) -> None:
import shutil
shutil.rmtree(self._tmpdir, ignore_errors=True)
```
|
||||
``directories``) do not yet exist on the legacy ``Project`` model.
|
||||
"""
|
||||
|
brent.edwards marked this conversation as resolved
Outdated
brent.edwards
commented
Minor: The mock setup block (lines 37-45 / 51-59) is duplicated between the two benchmark methods. Could extract a helper like Minor: The mock setup block (lines 37-45 / 51-59) is duplicated between the two benchmark methods. Could extract a helper like `_invoke_with_mock(self, args: list[str])` to reduce duplication. Not blocking — the two methods are short enough that readability isn't harmed.
|
||||
|
||||
name: str
|
||||
path: Path
|
||||
data_dir: Path
|
||||
config_path: Path
|
||||
database_status: str
|
||||
directories: list[str]
|
||||
|
||||
|
||||
class InitYesFlagSuite:
|
||||
"""Benchmark ``agents init --yes`` invocation."""
|
||||
|
||||
timeout = 30.0
|
||||
|
||||
|
hurui200320
commented
M2: This benchmark tests **M2**: This benchmark tests `--path` which is not in the spec (`specification.md:1217` defines only `agents init [--yes|-y]`). TDD benchmarks should drive toward spec behavior. Recommend removing this method.
|
||||
def setup(self) -> None:
|
||||
self._runner = CliRunner()
|
||||
self._tmpdir = tempfile.mkdtemp()
|
||||
self._last_exit_code = -1
|
||||
|
||||
def teardown(self) -> None:
|
||||
|
CoreRasurae
commented
F5 — MEDIUM: Missing The suite benchmarks Suggested fix — add a **F5 — MEDIUM: Missing `-y` short-form benchmark variant**
The suite benchmarks `--yes` (long form) and `--yes --path` but not `-y` (short form). If alias resolution introduces different overhead, this would go undetected.
**Suggested fix — add a `-y` benchmark:**
```python
def time_init_short_y_flag(self) -> None:
"""Measure end-to-end latency of ``agents init -y``."""
self._invoke_with_mock(["init", "-y"], "bench-project")
```
|
||||
shutil.rmtree(self._tmpdir, ignore_errors=True)
|
||||
|
||||
def _invoke_with_mock(self, args: list[str], project_name: str) -> None:
|
||||
|
CoreRasurae
commented
F4 — MEDIUM: Benchmark stores exit code but never validates it The docstring correctly notes that benchmark data is only meaningful on exit code 0, but Suggested fix — consider one of:
**F4 — MEDIUM: Benchmark stores exit code but never validates it**
The docstring correctly notes that benchmark data is only meaningful on exit code 0, but `_last_exit_code` is stored and never checked. After the `--yes` flag is implemented, a regression that reintroduces `NoSuchOption` would silently cause the benchmark to report error-path latency with no signal to the developer.
**Suggested fix — consider one of:**
1. Add a `track_exit_code` method so ASV tracks the value as a metric:
```python
def track_exit_code(self) -> int:
self._invoke_with_mock(["init", "--yes"], "bench-project")
return self._last_exit_code
```
2. Or add an assertion inside `_invoke_with_mock` gated by a flag (disabled while TDD-failing, enabled after fix).
|
||||
"""Invoke the CLI with a mocked container and the given *args*.
|
||||
|
CoreRasurae
commented
F3 (MEDIUM): The spec defines Suggestion: Add a comment noting **F3 (MEDIUM):** The spec defines `agents init [--yes|-y]` with **no** `--path` option (`specification.md:1217`). The spec's `init` is a global environment reset, not a project init. When #522 aligns the implementation with the spec, `--path` may be removed, making this benchmark invalid.
**Suggestion:** Add a comment noting `--path` is an implementation detail subject to change with #522, or remove this benchmark.
|
||||
|
||||
Stores the result exit code so benchmark data is only meaningful
|
||||
when the command succeeds (exit code 0). When ``--yes`` is not yet
|
||||
implemented the benchmark measures error-path latency; after the
|
||||
fix it measures real init latency.
|
||||
"""
|
||||
with patch(
|
||||
"cleveragents.application.container.get_container"
|
||||
) as mock_container:
|
||||
mock_service = create_autospec(ProjectService, instance=True)
|
||||
mock_project = _MockProject()
|
||||
mock_project.name = project_name
|
||||
mock_project.path = Path(self._tmpdir)
|
||||
# Pre-populate spec-required fields (specification.md:1381-1386)
|
||||
# so benchmarks measure real output paths once #522 is fixed.
|
||||
|
CoreRasurae
commented
F1 -- Bug (Medium): Missing ASV Every
This new **F1 -- Bug (Medium): Missing ASV `.unit` metadata on `track_exit_code()`**
Every `track_*` method across all benchmark files in this codebase declares a `.unit` class attribute for ASV metric labeling. Examples:
- `bench_unit_tests.py:90` -- `StepModuleDiscoverySuite.track_step_file_count.unit = "files"`
- `bench_unit_tests.py:200` -- `TrackTestSuiteMetrics.track_scenario_line_count.unit = "scenarios"`
- `bench_coverage_report.py:205` -- `CoverageFileSuite.track_source_file_count.unit = "files"`
- `bench_subprocess_overhead.py:55` -- `SubprocessCountSuite.track_subprocess_count.unit = "subprocesses"`
This new `track_exit_code()` is missing the attribute. Without it, ASV will display the metric with no unit label in its HTML reports and JSON results. Add after the class body:
```python
InitYesFlagSuite.track_exit_code.unit = "exit_code"
```
|
||||
mock_project.data_dir = Path(self._tmpdir)
|
||||
mock_project.config_path = Path(self._tmpdir) / "config.toml"
|
||||
mock_project.database_status = "initialized (schema v3)"
|
||||
mock_project.directories = ["logs", "cache", "sessions", "contexts"]
|
||||
mock_service.initialize_project.return_value = mock_project
|
||||
mock_container.return_value.project_service.return_value = mock_service
|
||||
|
||||
result = self._runner.invoke(app, args)
|
||||
self._last_exit_code = result.exit_code
|
||||
|
||||
def time_init_yes_flag(self) -> None:
|
||||
"""Measure end-to-end latency of ``agents init --yes``."""
|
||||
self._invoke_with_mock(["init", "--yes"], "bench-project")
|
||||
|
hurui200320
commented
H1: **H1**: `# type: ignore[attr-defined]` — `pyrightconfig.json` sets `"include": ["src"]`, so `benchmarks/` is not type-checked by Pyright. This comment is both unnecessary and a `CONTRIBUTING.md` violation ("Never use `# type: ignore`"). Remove it.
|
||||
|
||||
def time_init_short_y_flag(self) -> None:
|
||||
"""Measure end-to-end latency of ``agents init -y``."""
|
||||
self._invoke_with_mock(["init", "-y"], "bench-project")
|
||||
|
||||
def time_init_yes_flag_with_path(self) -> None:
|
||||
"""Measure latency of ``agents init --yes --path <dir>``.
|
||||
|
||||
NOTE: ``--path`` is an implementation detail of the current ``init``
|
||||
command. The spec (``specification.md:1217``) defines only
|
||||
``agents init [--yes|-y]`` with no ``--path`` option. When #522
|
||||
aligns the command with the spec this benchmark may need to be
|
||||
removed or updated.
|
||||
"""
|
||||
self._invoke_with_mock(
|
||||
["init", "--yes", "--path", self._tmpdir], "bench-path-project"
|
||||
)
|
||||
|
||||
def track_exit_code(self) -> int:
|
||||
"""Track the CLI exit code as an ASV metric.
|
||||
|
||||
Returns 0 when ``--yes`` is correctly implemented; non-zero while
|
||||
the flag is missing. This lets ASV detect regressions that
|
||||
reintroduce ``NoSuchOption`` without silently reporting error-path
|
||||
latency.
|
||||
"""
|
||||
self._invoke_with_mock(["init", "--yes"], "bench-project")
|
||||
return self._last_exit_code
|
||||
|
||||
|
||||
InitYesFlagSuite.track_exit_code.unit = "exit_code"
|
||||
@@ -0,0 +1,56 @@
|
|||||||||||||||||||
# These tests target bug #522 and are expected to fail until the fix is applied.
|
|||||||||||||||||||
#
|
|||||||||||||||||||
# NOTE FOR FIX AUTHOR (#522):
|
|||||||||||||||||||
# Scenarios 1-4 will fail for TWO independent reasons:
|
|||||||||||||||||||
# (a) The --yes / -y flag is not yet implemented (NoSuchOption).
|
|||||||||||||||||||
# (b) The current init_command output format (project.py) does not match
|
|||||||||||||||||||
# the spec-defined output at docs/specification.md:1381-1402.
|
|||||||||||||||||||
|
CoreRasurae
commented
F8 — LOW: No All scenarios use Consider adding **F8 — LOW: No `@wip` or expected-failure tag for CI filtering**
All scenarios use `@tdd @bug522` but there is no tag that the CI runner / nox session can use to skip or expect-fail these tests. Without a filter, CI will report 4 failures on every run until #522 is fixed.
Consider adding `@wip` and configuring `nox -s unit_tests` to pass `--tags='not @wip'` (or equivalent), or to treat `@wip` failures as expected.
|
|||||||||||||||||||
# The fix must address both: add the --yes flag AND remodel the output to
|
|||||||||||||||||||
# match the spec (Data Dir, Config, Database, Directories fields with the
|
|||||||||||||||||||
# "Initialized (non-interactive)" status message).
|
|||||||||||||||||||
Feature: CLI init --yes flag for non-interactive initialization
|
|||||||||||||||||||
As a developer using CleverAgents in CI or scripts
|
|||||||||||||||||||
I want to run "agents init --yes" for non-interactive initialization
|
|||||||||||||||||||
So that I can skip interactive prompts and use sensible defaults
|
|||||||||||||||||||
|
|||||||||||||||||||
@tdd @bug522 @wip
|
|||||||||||||||||||
Scenario: agents init --yes completes without error
|
|||||||||||||||||||
Given I have a temporary project directory for init
|
|||||||||||||||||||
|
brent.edwards
commented
The expected output strings ( Resolved — Corrected in commit The expected output strings (`"initialized successfully"`, `"Location:"`, `"Database:"`, `"Project Initialized"`) are reasonable guesses based on the specification, but since the `--yes` code path doesn't exist yet, the bug-fix author for #522 will need to ensure their output matches these expectations — or coordinate to update these assertions. Just flagging so this is on the radar for whoever picks up #522.
---
**Resolved** — Corrected in commit `429ba8db` per F1 feedback from @CoreRasurae. These were not guesses — the spec provides exact expected output at `docs/specification.md:1381-1402` and Workflow Example 1 at `34317-34326`. Assertions now match the specification: `"Initialized (non-interactive)"`, `"Data Dir:"`, `"Database:"`, `"Initialized"` (box title).
|
|||||||||||||||||||
When I run agents init with the --yes flag
|
|||||||||||||||||||
|
CoreRasurae
commented
F9 — LOW: Prompt-absence check is unreachable until fix is applied The This is an inherent limitation of TDD failing tests — not something that needs fixing now, but worth being aware of when the fix PR is submitted. At that point, ensure the prompt check is manually verified to be exercising real logic. **F9 — LOW: Prompt-absence check is unreachable until fix is applied**
The `no interactive prompt should have been presented` step (line 20) will never execute in the current TDD state because the preceding exit-code assertion at line 18 fails first (since `--yes` doesn't exist yet). This means the prompt-suppression logic has zero validation that the step itself works correctly until the fix arrives.
This is an inherent limitation of TDD failing tests — not something that needs fixing now, but worth being aware of when the fix PR is submitted. At that point, ensure the prompt check is manually verified to be exercising real logic.
|
|||||||||||||||||||
Then the init command should exit with code 0
|
|||||||||||||||||||
And the project service initialize_project should have been called
|
|||||||||||||||||||
|
|||||||||||||||||||
@tdd @bug522 @wip
|
|||||||||||||||||||
Scenario: --yes suppresses interactive prompts
|
|||||||||||||||||||
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
|
|||||||||||||||||||
|
CoreRasurae
commented
F2 — HIGH: No mock behavioral verification for The Without it, the Suggested fix — add mock verification: **F2 — HIGH: No mock behavioral verification for `-y` short-form path**
The `--yes` scenario (line 8) includes `And the project service initialize_project should have been called`, verifying the mock was actually exercised. This scenario lacks that step.
Without it, the `-y` code path could produce correct output text through a fallback/error path without ever calling `initialize_project`, and the test would still pass.
**Suggested fix — add mock verification:**
```gherkin
And the project service initialize_project should have been called
```
|
|||||||||||||||||||
And the init output should contain "Initialized (non-interactive)"
|
|||||||||||||||||||
And no interactive prompt should have been presented
|
|||||||||||||||||||
|
|||||||||||||||||||
@tdd @bug522 @wip
|
|||||||||||||||||||
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
|
|||||||||||||||||||
|
CoreRasurae
commented
F4 -- Test Coverage (Low): Spec-required output value formats not verified The spec at
All four assertions check only that the label exists, not the value structure. The tests would pass even if the output printed **F4 -- Test Coverage (Low): Spec-required output value formats not verified**
The spec at `docs/specification.md:1381-1388` and the Workflow Example at line 34319-34326 define specific value content for each field:
| Field | Spec value | Tested? |
|:------|:-----------|:--------|
| `Data Dir:` | `.../.cleveragents (created)` | Key only |
| `Config:` | `.../.cleveragents/config.toml` | Key only |
| `Database:` | `initialized (schema v3)` | Key only |
| `Directories:` | `logs, cache, sessions, contexts` | Key only |
All four assertions check only that the label exists, not the value structure. The tests would pass even if the output printed `Database: ERROR` or `Directories:` with no value. Consider adding at least one assertion that spot-checks a value format, e.g.:
```gherkin
And the init output should contain "initialized (schema v3)"
```
|
|||||||||||||||||||
Then the init command should exit with code 0
|
|||||||||||||||||||
And the init output should contain "Initialized (non-interactive)"
|
|||||||||||||||||||
|
CoreRasurae
commented
F1 — HIGH: Missing The spec (
The issue's acceptance criterion #3 explicitly requires: "produces the expected output (data dir, config, database, directories)". Two of the four are missing. Suggested fix — add two more assertion steps: **F1 — HIGH: Missing `Config:` and `Directories:` output assertions against specification**
The spec (`docs/specification.md:1381-1400`) defines four output fields for `agents init --yes`:
| Spec field | Tested here? |
|:-----------|:-------------|
| `Data Dir:` | Yes (line 34) |
| `Config:` | **No** |
| `Database:` | Yes (line 35) |
| `Directories:` | **No** |
The issue's acceptance criterion #3 explicitly requires: *"produces the expected output (data dir, config, database, directories)"*. Two of the four are missing.
**Suggested fix — add two more assertion steps:**
```gherkin
And the init output should contain "Config:"
And the init output should contain "Directories:"
```
|
|||||||||||||||||||
And the project service initialize_project should have been called
|
|||||||||||||||||||
|
|||||||||||||||||||
@tdd @bug522 @wip
|
|||||||||||||||||||
Scenario: Output includes expected initialization summary
|
|||||||||||||||||||
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 init output should contain "Data Dir:"
|
|||||||||||||||||||
And the init output should contain "Config:"
|
|||||||||||||||||||
And the init output should contain "Database:"
|
|||||||||||||||||||
|
hurui200320
commented
L1: Gherkin convention uses **L1**: Gherkin convention uses `And` for consecutive assertion steps. This should be `And the init output should indicate interactive mode`.
|
|||||||||||||||||||
And the init output should contain "Directories:"
|
|||||||||||||||||||
And the init output should contain "logs, cache, sessions, contexts"
|
|||||||||||||||||||
And the init output should contain "Initialized"
|
|||||||||||||||||||
|
|||||||||||||||||||
@tdd @bug522
|
|||||||||||||||||||
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
|
|||||||||||||||||||
Then the init command should exit with code 0
|
|||||||||||||||||||
And the init output should indicate interactive mode
|
|||||||||||||||||||
@@ -0,0 +1,210 @@
|
||||
"""Step definitions for CLI init --yes flag tests (bug #522).
|
||||
|
||||
These tests are TDD-style: they assert the CORRECT expected behaviour of
|
||||
``agents init --yes`` and are expected to FAIL until the bug fix is applied.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from unittest.mock import create_autospec, patch
|
||||
|
||||
from behave import given, then, when
|
||||
from typer.testing import CliRunner
|
||||
|
||||
from cleveragents.application.services.project_service import ProjectService
|
||||
from cleveragents.cli.main import app
|
||||
|
||||
|
||||
|
brent.edwards
commented
Minor: Worth confirming Resolved — Addressed in commit Minor: `context.original_cwd` is saved here but never restored (no `os.chdir(context.original_cwd)` in a cleanup/after step). The existing `project_commands_coverage_steps.py` has the same pattern, so this is consistent — but if `environment.py`'s `after_scenario` doesn't handle CWD restoration and the temp dir gets cleaned up first, subsequent scenarios could see a stale CWD.
Worth confirming `after_scenario` handles this, or adding a restore in a `@then` cleanup step or `after_scenario` hook. Low priority since it matches existing convention.
---
**Resolved** — Addressed in commit `435093c2`. Added `context.add_cleanup(_restore_cwd, context)` which restores the original CWD and cleans up the temp directory via a Behave cleanup callback, ensuring proper teardown regardless of scenario outcome.
|
||||
def _restore_cwd(context):
|
||||
|
CoreRasurae
commented
F4 (MEDIUM): This overwrites Suggestion: Use a step-private name and match the framework's type: **F4 (MEDIUM):** This overwrites `context.original_cwd` set by `environment.py:222` (`os.getcwd()` → `str`) with a `Path` object, creating a type inconsistency. It also silently collides with the framework attribute.
**Suggestion:** Use a step-private name and match the framework's type:
```python
context._init_original_cwd = os.getcwd()
```
|
||||
"""Restore the original working directory, env var, and clean up."""
|
||||
os.chdir(context._init_original_cwd)
|
||||
if context._init_original_home is None:
|
||||
|
CoreRasurae
commented
F5 -- Test Flaw (Low): Missing The Robot tests properly isolate via Currently mitigated because Consider adding: and restoring it during cleanup, matching the Robot test isolation pattern. **F5 -- Test Flaw (Low): Missing `CLEVERAGENTS_HOME` environment isolation**
The Robot tests properly isolate via `CLEVERAGENTS_HOME` set in `common.resource:23`. This Behave step relies on `os.chdir(context.temp_dir)` but never sets `CLEVERAGENTS_HOME`.
Currently mitigated because `get_container()` is mocked at line 40, so the real init path is never exercised. However, when the #522 bug fix is applied and mocks are potentially adjusted, if any code path resolves paths via `CLEVERAGENTS_HOME` instead of CWD, the test could leak into the developer's real home directory. The `before_all()` hook in `features/environment.py` does not set `CLEVERAGENTS_HOME` either.
Consider adding:
```python
os.environ["CLEVERAGENTS_HOME"] = context.temp_dir
```
and restoring it during cleanup, matching the Robot test isolation pattern.
|
||||
os.environ.pop("CLEVERAGENTS_HOME", None)
|
||||
else:
|
||||
os.environ["CLEVERAGENTS_HOME"] = context._init_original_home
|
||||
shutil.rmtree(context.temp_dir, ignore_errors=True)
|
||||
|
||||
|
||||
def _create_init_mocks(context):
|
||||
"""Create and configure mocked container, service, and project.
|
||||
|
CoreRasurae
commented
F6 -- Informational: Spec vs. implementation alignment risk in mock target The spec at These steps mock If the #522 fix remodels **F6 -- Informational: Spec vs. implementation alignment risk in mock target**
The spec at `docs/specification.md:1219-1224` describes `agents init` as a **global environment reset**: *"Initialize or reset the global CleverAgents environment. This wipes any existing data and re-creates the global config and database."*
These steps mock `project_service.initialize_project()`, which aligns with the current implementation at `src/cleveragents/cli/main.py:351-415` where `init` delegates to `project_init_command()`. However, the spec semantics (global environment reset) differ from the implementation semantics (project initialization).
If the #522 fix remodels `init` to match the spec's global-reset semantics (wipe data, re-create config/database, create directories), the mock target would need to change. This is inherent to the TDD approach but worth noting so the fix implementer is aware the mock strategy may need adaptation.
|
||||
|
||||
Returns ``(patcher, mock_service)`` where *patcher* is the started
|
||||
``patch`` context manager.
|
||||
|
CoreRasurae
commented
F2 (MEDIUM): The mock only sets Suggestion: Pre-populate spec-required fields: **F2 (MEDIUM):** The mock only sets `.name` and `.path`, but `specification.md:1381-1386` requires the init output to include `Data Dir:`, `Config:`, `Database:`, and `Directories:` fields. When the #522 fix remodels `init_command()`, unset MagicMock attributes will stringify as `<MagicMock id='...'>`, causing output assertions to fail for the wrong reason.
**Suggestion:** Pre-populate spec-required fields:
```python
mock_project.data_dir = Path(context.temp_dir)
mock_project.config_path = Path(context.temp_dir) / "config.toml"
mock_project.database_status = "initialized (schema v3)"
mock_project.directories = ["logs", "cache", "sessions", "contexts"]
```
|
||||
|
||||
The service mock uses ``create_autospec(ProjectService)`` so that
|
||||
attribute access and method calls are validated against the real
|
||||
``ProjectService`` interface. The project mock uses a typed
|
||||
``_MockProject`` class instead of ``MagicMock`` because the
|
||||
spec-required output fields (``data_dir``, ``config_path``,
|
||||
``database_status``, ``directories``) do not yet exist on the legacy
|
||||
``Project`` model — they will be added when the #522 fix aligns the
|
||||
model with ``docs/specification.md:1381-1386``. Using
|
||||
``create_autospec(Project)`` would reject those attribute assignments.
|
||||
"""
|
||||
mock_service = create_autospec(ProjectService, instance=True)
|
||||
|
||||
class _MockProject:
|
||||
"""Lightweight stand-in for the Project model with spec fields."""
|
||||
|
||||
name: str
|
||||
path: Path
|
||||
data_dir: Path
|
||||
config_path: Path
|
||||
database_status: str
|
||||
directories: list[str]
|
||||
|
||||
mock_project = _MockProject()
|
||||
mock_project.name = Path(context.temp_dir).name
|
||||
mock_project.path = Path(context.temp_dir)
|
||||
# Pre-populate spec-required fields (specification.md:1381-1386) so
|
||||
# that when the #522 fix lands, output assertions fail for the right
|
||||
# reason (real bugs) rather than MagicMock stringification artefacts.
|
||||
mock_project.data_dir = Path(context.temp_dir)
|
||||
mock_project.config_path = Path(context.temp_dir) / "config.toml"
|
||||
|
brent.edwards
commented
Nit: When the bug-fix author implements That way the step actually exercises the "no prompts" invariant rather than duplicating the exit-code check. Not blocking since the fix PR will likely revisit this step anyway. Resolved — Addressed in commit Nit: `step_no_interactive_prompt` currently only re-asserts `exit_code == 0`, which the preceding step in the scenario already checks. This makes the step a tautology — it doesn't actually verify that no prompts were presented.
When the bug-fix author implements `--yes`, consider strengthening this to something like:
```python
# Verify no prompt-like tokens appeared in output
output = context.result["output"]
for prompt_token in ("[Y/n]", "[y/N]", "? ", "Enter "):
assert prompt_token not in output, (
f"Unexpected prompt token '{prompt_token}' found in output:\n{output}"
)
```
That way the step actually exercises the "no prompts" invariant rather than duplicating the exit-code check. Not blocking since the fix PR will likely revisit this step anyway.
---
**Resolved** — Addressed in commit `435093c2`. The step now checks for prompt-like tokens (`[Y/n]`, `[y/N]`, `Continue? `, `Proceed? `, `Enter `, `Confirm `, `(yes/no)`) instead of duplicating the exit-code assertion. The overly broad `"? "` token was subsequently refined in commit `429ba8db` per F5 feedback from @CoreRasurae.
|
||||
mock_project.database_status = "initialized (schema v3)"
|
||||
mock_project.directories = ["logs", "cache", "sessions", "contexts"]
|
||||
mock_service.initialize_project.return_value = mock_project
|
||||
|
||||
patcher = patch("cleveragents.application.container.get_container")
|
||||
|
hurui200320
commented
M3: The mock patches **M3**: The mock patches `get_container` and accesses `.project_service().initialize_project()`, which is the current (pre-spec) implementation. The spec defines `agents init` as a global environment reset (`specification.md:1219-1224`), not a project-level operation. Add a comment noting this coupling will change when #522 refactors init to match the spec.
|
||||
mock_container = patcher.start()
|
||||
mock_container.return_value.project_service.return_value = mock_service
|
||||
|
||||
return patcher, mock_service
|
||||
|
||||
|
||||
@given("I have a temporary project directory for init")
|
||||
def step_temp_project_directory(context):
|
||||
"""Create a temporary directory and store it on *context*."""
|
||||
context.temp_dir = tempfile.mkdtemp()
|
||||
# Use step-private attribute names (prefixed with _init_) to avoid
|
||||
# colliding with environment.py's context.original_cwd (str).
|
||||
# Match the framework's type (str via os.getcwd()) for consistency.
|
||||
context._init_original_cwd = os.getcwd()
|
||||
context._init_original_home = os.environ.get("CLEVERAGENTS_HOME")
|
||||
os.environ["CLEVERAGENTS_HOME"] = context.temp_dir
|
||||
os.chdir(context.temp_dir)
|
||||
context.add_cleanup(_restore_cwd, context)
|
||||
|
||||
|
||||
def _run_init_with_flag(context, flag: str) -> None:
|
||||
"""Invoke ``agents init`` with the given flag via the Typer test runner."""
|
||||
runner = CliRunner()
|
||||
|
||||
patcher, mock_service = _create_init_mocks(context)
|
||||
try:
|
||||
result = runner.invoke(app, ["init", flag])
|
||||
finally:
|
||||
patcher.stop()
|
||||
|
||||
context.init_yes_result = {
|
||||
"exit_code": result.exit_code,
|
||||
"output": result.output,
|
||||
}
|
||||
|
CoreRasurae
commented
F6 — MEDIUM: Prompt token The substring Suggested fix — narrow the token or switch to a pattern: Alternatively, a regex like **F6 — MEDIUM: Prompt token `"Enter "` risks false positives**
The substring `"Enter "` could match legitimate non-prompt output such as `"Entered configuration..."` or `"Enter key configured"`. The spec's interactive prompt pattern is `Continue? [y/N]:` — more precise patterns would reduce false-positive risk.
**Suggested fix — narrow the token or switch to a pattern:**
```python
prompt_tokens = (
"[Y/n]",
"[y/N]",
"Continue? ",
"Proceed? ",
"Enter a ", # more specific than bare "Enter "
"Confirm ",
"(yes/no)",
)
```
Alternatively, a regex like `r"(?:Enter|Confirm|Proceed|Continue)\s.*[:?]"` would match actual prompt structures more precisely.
CoreRasurae
commented
F3 -- Test Flaw (Low): Prompt token narrowing reduces negative-assertion coverage Changing from
A more targeted approach -- such as a regex like **F3 -- Test Flaw (Low): Prompt token narrowing reduces negative-assertion coverage**
Changing from `"Enter "` to `"Enter a "` addresses false-positive risk on legitimate output (e.g., "Enterprise"), but it now misses realistic interactive prompt patterns like:
- `"Enter project name: "`
- `"Enter path: "`
- `"Enter value: "`
A more targeted approach -- such as a regex like `r"Enter\s+\S+.*[:\?]"` or checking for `"Enter "` only at line boundaries -- would preserve detection coverage without the false-positive risk. As-is, the negative assertion has a blind spot for a common prompt pattern.
|
||||
context.init_yes_raw_result = result
|
||||
context.init_yes_mock_service = mock_service
|
||||
|
||||
|
||||
@when("I run agents init with the --yes flag")
|
||||
def step_run_init_yes(context):
|
||||
"""Invoke ``agents init --yes`` via the Typer test runner."""
|
||||
_run_init_with_flag(context, "--yes")
|
||||
|
||||
|
||||
@when("I run agents init with the -y flag")
|
||||
def step_run_init_short_yes(context):
|
||||
"""Invoke ``agents init -y`` via the Typer test runner."""
|
||||
_run_init_with_flag(context, "-y")
|
||||
|
||||
|
||||
|
CoreRasurae
commented
F1 (MEDIUM) + F5 (LOW): The greedy Suggestion: Non-greedy **F1 (MEDIUM) + F5 (LOW):** The greedy `.*` will consume the entire remainder of the line up to the **last** `:` or `?`, which can over-match if the init output contains colons in non-prompt contexts (e.g., `Config: /path`). Also, `\?` is redundant inside `[]`.
**Suggestion:**
```python
enter_prompt = re.search(r"Enter\s+\S+.*?[:?]", output)
```
Non-greedy `.*?` stops at the first `:` or `?`, matching only the actual prompt.
|
||||
@then("the init command should exit with code {code:d}")
|
||||
def step_init_exit_code(context, code):
|
||||
"""Assert the init command exited with the expected code."""
|
||||
actual = context.init_yes_result["exit_code"]
|
||||
assert actual == code, (
|
||||
f"Expected exit code {code}, got {actual}. "
|
||||
f"Output: {context.init_yes_result['output']}"
|
||||
)
|
||||
|
||||
|
||||
@then('the init output should contain "{text}"')
|
||||
def step_init_output_contains(context, text):
|
||||
"""Assert that the init command output contains *text*."""
|
||||
output = context.init_yes_result["output"]
|
||||
assert text in output, f"Expected '{text}' in output:\n{output}"
|
||||
|
||||
|
||||
@then("the project service initialize_project should have been called")
|
||||
def step_initialize_project_called(context):
|
||||
"""Assert the mock project service's initialize_project was invoked."""
|
||||
context.init_yes_mock_service.initialize_project.assert_called_once()
|
||||
|
||||
|
||||
@when("I run agents init without the --yes flag")
|
||||
def step_run_init_no_yes(context):
|
||||
"""Invoke ``agents init`` without --yes (interactive mode)."""
|
||||
runner = CliRunner()
|
||||
|
||||
patcher, _mock_service = _create_init_mocks(context)
|
||||
try:
|
||||
result = runner.invoke(app, ["init"])
|
||||
finally:
|
||||
patcher.stop()
|
||||
|
||||
context.init_yes_result = {
|
||||
"exit_code": result.exit_code,
|
||||
"output": result.output,
|
||||
}
|
||||
context.init_yes_raw_result = result
|
||||
|
||||
|
||||
@then("the init output should indicate interactive mode")
|
||||
def step_output_indicates_interactive(context):
|
||||
"""Assert that without --yes the output does NOT contain the
|
||||
non-interactive marker, indicating the command ran in interactive mode.
|
||||
|
||||
This is the negative complement to the --yes scenarios: when the fix
|
||||
lands, ``agents init`` (without --yes) should either present a prompt
|
||||
or omit the ``Initialized (non-interactive)`` marker.
|
||||
"""
|
||||
output = context.init_yes_result["output"]
|
||||
assert "Initialized (non-interactive)" not in output, (
|
||||
"Expected interactive mode but output contains "
|
||||
f"'Initialized (non-interactive)':\n{output}"
|
||||
)
|
||||
|
||||
|
||||
@then("no interactive prompt should have been presented")
|
||||
def step_no_interactive_prompt(context):
|
||||
"""Assert that no interactive prompt was presented.
|
||||
|
||||
Verifies that the command output does not contain common prompt-like
|
||||
tokens, which would indicate the ``--yes`` flag failed to suppress
|
||||
interactive prompts.
|
||||
"""
|
||||
output = context.init_yes_result["output"]
|
||||
prompt_tokens = (
|
||||
"[Y/n]",
|
||||
"[y/N]",
|
||||
"Continue? ",
|
||||
"Proceed? ",
|
||||
"Confirm ",
|
||||
"(yes/no)",
|
||||
)
|
||||
for token in prompt_tokens:
|
||||
assert token not in output, (
|
||||
f"Unexpected prompt token '{token}' found in output:\n{output}"
|
||||
)
|
||||
# Regex catches "Enter project name:", "Enter path:", "Enter value?"
|
||||
# without false-positiving on "Entered configuration" or "Enterprise".
|
||||
# Non-greedy .*? stops at the first : or ? to avoid over-matching
|
||||
# when the line contains colons in non-prompt contexts (e.g. "Config: /path").
|
||||
enter_prompt = re.search(r"Enter\s+\S+.*?[:?]", output)
|
||||
assert enter_prompt is None, (
|
||||
f"Unexpected Enter-style prompt found in output: "
|
||||
f"'{enter_prompt.group()}'\n{output}"
|
||||
)
|
||||
@@ -574,6 +574,8 @@ def integration_tests(session: nox.Session):
|
||||
"discovery",
|
||||
"--exclude",
|
||||
"code_blocks",
|
||||
"--exclude",
|
||||
"wip",
|
||||
*robot_args,
|
||||
"robot/",
|
||||
)
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
*** Settings ***
|
||||
Documentation Integration smoke test for agents init --yes (bug #522).
|
||||
... These tests are TDD-style and expected to FAIL until the
|
||||
... bug fix is applied.
|
||||
Resource ${CURDIR}/common.resource
|
||||
Library Process
|
||||
Library OperatingSystem
|
||||
Library String
|
||||
Suite Setup Setup Test Environment
|
||||
Suite Teardown Cleanup Test Environment
|
||||
|
||||
*** Test Cases ***
|
||||
Init Yes Flag Exits Without Error
|
||||
[Documentation] agents init --yes should complete with exit code 0
|
||||
[Tags] wip
|
||||
${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='init_yes_')
|
||||
${result}= Run Process ${PYTHON} -m cleveragents init --yes
|
||||
... timeout=60s cwd=${tmpdir}
|
||||
|
CoreRasurae
commented
F7 — MEDIUM: In Robot Framework, Every Robot Framework style guide places Suggested fix: Move **F7 — MEDIUM: `[Teardown]` placed between action and assertion steps**
In Robot Framework, `[Teardown]` is a test setting that always runs after the full test body, regardless of where it appears in the source. Placing it between `Run Process` and `Should Be Equal` is functionally correct but visually misleading — it looks like teardown happens before the assertions.
Every Robot Framework style guide places `[Teardown]` as the last line of the test case. This same pattern applies to all three test cases in this file (lines 18, 27, 38).
**Suggested fix:** Move `[Teardown]` to the final line of each test case.
|
||||
Should Be Equal As Integers ${result.rc} 0
|
||||
... msg=Expected exit code 0 but got ${result.rc}. stderr: ${result.stderr}
|
||||
[Teardown] Remove Directory ${tmpdir} recursive=True
|
||||
|
||||
Init Yes Flag Produces Summary Output
|
||||
[Documentation] agents init --yes should produce the initialization summary
|
||||
... with all spec-required output fields.
|
||||
[Tags] wip
|
||||
${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='init_yes_out_')
|
||||
${result}= Run Process ${PYTHON} -m cleveragents init --yes
|
||||
... timeout=60s cwd=${tmpdir}
|
||||
Should Be Equal As Integers ${result.rc} 0
|
||||
|
CoreRasurae
commented
F3 — HIGH: Robot "Produces Summary Output" only checks status message, not spec fields This test verifies Suggested fix — add output field assertions: **F3 — HIGH: Robot "Produces Summary Output" only checks status message, not spec fields**
This test verifies `Initialized (non-interactive)` but none of the four spec-required output fields (`Data Dir:`, `Config:`, `Database:`, `Directories:`). This makes it nearly identical to the exit-code-only test above.
**Suggested fix — add output field assertions:**
```robot
Should Contain ${result.stdout} Data Dir:
... msg=Output should contain Data Dir field per spec
Should Contain ${result.stdout} Config:
... msg=Output should contain Config field per spec
Should Contain ${result.stdout} Database:
... msg=Output should contain Database field per spec
Should Contain ${result.stdout} Directories:
... msg=Output should contain Directories field per spec
```
|
||||
... msg=Expected exit code 0 but got ${result.rc}. stderr: ${result.stderr}
|
||||
Should Contain ${result.stdout} Initialized (non-interactive)
|
||||
... msg=Output should contain non-interactive initialization message per spec
|
||||
Should Contain ${result.stdout} Data Dir:
|
||||
... msg=Output should contain Data Dir field per spec
|
||||
Should Contain ${result.stdout} Config:
|
||||
... msg=Output should contain Config field per spec
|
||||
Should Contain ${result.stdout} Database:
|
||||
... msg=Output should contain Database field per spec
|
||||
Should Contain ${result.stdout} Directories:
|
||||
... msg=Output should contain Directories field per spec
|
||||
[Teardown] Remove Directory ${tmpdir} recursive=True
|
||||
|
||||
Init Short Y Flag Exits Without Error
|
||||
[Documentation] agents init -y should complete with exit code 0 (short-form alias)
|
||||
[Tags] wip
|
||||
${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='init_y_')
|
||||
${result}= Run Process ${PYTHON} -m cleveragents init -y
|
||||
... timeout=60s cwd=${tmpdir}
|
||||
Should Be Equal As Integers ${result.rc} 0
|
||||
... msg=Expected exit code 0 but got ${result.rc}. stderr: ${result.stderr}
|
||||
[Teardown] Remove Directory ${tmpdir} recursive=True
|
||||
F2 -- Bug (Low):
_last_exit_codenot defensively initialized insetup()self._last_exit_codeis only assigned inside_invoke_with_mock()(line 58). Whiletrack_exit_code()calls_invoke_with_mock()first so under normal execution it works, if the mock setup orrunner.invoke()raises an exception before line 58, the subsequentreturn self._last_exit_codewill raiseAttributeErrorrather than a meaningful error.Consider initializing it here:
This follows the defensive pattern seen in other benchmark suites and ensures ASV always gets a numeric value.