fix(cli): address reviewer feedback on format_output rich/color fix
- Move function-level imports to module top level in formatting.py:
* Remove redundant OutputSession import inside _format_rich()
* Remove redundant OutputSession import inside _format_color()
* Move `import sys` from inside format_output() to module level
- Fix robot/helper_cli_formats.py:
* Remove redundant `import json as _json` inside format_output_rich();
use the top-level `json` module directly
* Replace non-deterministic datetime.now() calls in _mock_action() and
_mock_plan() with fixed datetime(2025, 1, 15, 10, 0, 0)
- Split cli_output_formats_steps.py to comply with 500-line limit:
* Extract all @then step definitions into new file
features/steps/cli_output_format_validation_steps.py
* Behave auto-discovers steps from any .py file in steps/
ISSUES CLOSED: #2921
This commit is contained in:
@@ -0,0 +1,127 @@
|
||||
"""Step definitions for CLI output format validation (rich / color formats)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
|
||||
import yaml
|
||||
from behave import then
|
||||
from behave.runner import Context
|
||||
|
||||
|
||||
@then("the format result should be valid JSON dict")
|
||||
def step_format_result_json_dict(context: Context) -> None:
|
||||
parsed = json.loads(context.format_result)
|
||||
assert isinstance(parsed, dict)
|
||||
# Verify spec-required envelope fields are present
|
||||
for field in ("command", "status", "exit_code", "data", "timing", "messages"):
|
||||
assert field in parsed, f"Envelope field '{field}' missing from JSON result"
|
||||
# Verify data field contains the original dict
|
||||
assert isinstance(parsed["data"], dict)
|
||||
|
||||
|
||||
@then("the format result should be valid YAML dict")
|
||||
def step_format_result_yaml_dict(context: Context) -> None:
|
||||
parsed = yaml.safe_load(context.format_result)
|
||||
assert isinstance(parsed, dict)
|
||||
# Verify spec-required envelope fields are present
|
||||
for field in ("command", "status", "exit_code", "data", "timing", "messages"):
|
||||
assert field in parsed, f"Envelope field '{field}' missing from YAML result"
|
||||
# Verify data field contains the original dict
|
||||
assert isinstance(parsed["data"], dict)
|
||||
|
||||
|
||||
@then("the format result should contain plain key-value pairs")
|
||||
def step_format_result_plain(context: Context) -> None:
|
||||
assert "name: test" in context.format_result
|
||||
assert "nested:" in context.format_result
|
||||
assert "items:" in context.format_result
|
||||
|
||||
|
||||
@then("the format result should contain table output")
|
||||
def step_format_result_table(context: Context) -> None:
|
||||
assert "name" in context.format_result
|
||||
assert "count" in context.format_result
|
||||
|
||||
|
||||
@then("the format result should contain separator lines")
|
||||
def step_format_result_separator(context: Context) -> None:
|
||||
assert "---" in context.format_result
|
||||
|
||||
|
||||
@then("the serialized result should have string enum values")
|
||||
def step_serialized_enum_values(context: Context) -> None:
|
||||
assert context.serialized["state"] == "available"
|
||||
assert isinstance(context.serialized["nested"], dict)
|
||||
assert context.serialized["items"][0] == "archived"
|
||||
assert isinstance(context.serialized["ts"], str)
|
||||
|
||||
|
||||
@then("the format result should contain rich styled output")
|
||||
def step_format_result_rich_styled(context: Context) -> None:
|
||||
"""Assert that rich format produces styled output (ANSI codes or key-value lines).
|
||||
|
||||
The rich format must NOT produce raw JSON — it must produce styled terminal
|
||||
output via the RichMaterializer (ANSI escape codes or structured key-value
|
||||
lines with panel headers).
|
||||
"""
|
||||
result = context.format_result
|
||||
# Rich output must contain the data keys
|
||||
assert "name" in result, f"Rich output missing 'name': {result!r}"
|
||||
assert "count" in result, f"Rich output missing 'count': {result!r}"
|
||||
# Rich output must contain styled panel header or ANSI codes
|
||||
has_ansi = "\033[" in result or "\x1b[" in result
|
||||
has_panel_header = "Output" in result
|
||||
assert has_ansi or has_panel_header, (
|
||||
f"Rich output has no ANSI codes or panel header — got: {result!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the format result should not be valid JSON")
|
||||
def step_format_result_not_json(context: Context) -> None:
|
||||
"""Assert that the format result is NOT valid JSON (i.e. not the JSON fallback)."""
|
||||
try:
|
||||
json.loads(context.format_result)
|
||||
raise AssertionError(
|
||||
f"format_output() with 'rich' returned valid JSON — "
|
||||
f"expected styled terminal output, got: {context.format_result!r}"
|
||||
)
|
||||
except (json.JSONDecodeError, ValueError):
|
||||
pass # Expected: rich output is not JSON
|
||||
|
||||
|
||||
@then("the format result should contain color styled output")
|
||||
def step_format_result_color_styled(context: Context) -> None:
|
||||
"""Assert that color format produces ANSI-colored output (not plain text).
|
||||
|
||||
The color format must NOT produce plain text — it must produce ANSI-colored
|
||||
terminal output via the ColorMaterializer.
|
||||
"""
|
||||
result = context.format_result
|
||||
# Color output must contain the data keys
|
||||
assert "name" in result, f"Color output missing 'name': {result!r}"
|
||||
assert "count" in result, f"Color output missing 'count': {result!r}"
|
||||
# Color output must contain ANSI escape codes or styled panel header
|
||||
has_ansi = "\033[" in result or "\x1b[" in result
|
||||
has_panel_header = "Output" in result
|
||||
assert has_ansi or has_panel_header, (
|
||||
f"Color output has no ANSI codes or panel header — got: {result!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the format result should not be plain text only")
|
||||
def step_format_result_not_plain_text(context: Context) -> None:
|
||||
"""Assert that the color format result is NOT plain text (i.e. not the plain fallback).
|
||||
|
||||
Plain text would look like 'name: test\\ncount: 42' with no ANSI codes.
|
||||
Color output must have ANSI escape codes or styled panel headers.
|
||||
"""
|
||||
result = context.format_result
|
||||
has_ansi = "\033[" in result or "\x1b[" in result
|
||||
has_panel_header = "Output" in result
|
||||
# If it's purely plain text (no ANSI, no panel header), that's the bug
|
||||
is_plain_only = not has_ansi and not has_panel_header
|
||||
assert not is_plain_only, (
|
||||
f"format_output() with 'color' returned plain text — "
|
||||
f"expected ANSI-colored output, got: {result!r}"
|
||||
)
|
||||
@@ -466,124 +466,3 @@ def step_call_serialize_with_enum(context: Context) -> None:
|
||||
"ts": datetime(2025, 6, 1, 12, 0),
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
# ------- Additional Then steps -------
|
||||
|
||||
|
||||
@then("the format result should be valid JSON dict")
|
||||
def step_format_result_json_dict(context: Context) -> None:
|
||||
parsed = json.loads(context.format_result)
|
||||
assert isinstance(parsed, dict)
|
||||
# Verify spec-required envelope fields are present
|
||||
for field in ("command", "status", "exit_code", "data", "timing", "messages"):
|
||||
assert field in parsed, f"Envelope field '{field}' missing from JSON result"
|
||||
# Verify data field contains the original dict
|
||||
assert isinstance(parsed["data"], dict)
|
||||
|
||||
|
||||
@then("the format result should be valid YAML dict")
|
||||
def step_format_result_yaml_dict(context: Context) -> None:
|
||||
parsed = yaml.safe_load(context.format_result)
|
||||
assert isinstance(parsed, dict)
|
||||
# Verify spec-required envelope fields are present
|
||||
for field in ("command", "status", "exit_code", "data", "timing", "messages"):
|
||||
assert field in parsed, f"Envelope field '{field}' missing from YAML result"
|
||||
# Verify data field contains the original dict
|
||||
assert isinstance(parsed["data"], dict)
|
||||
|
||||
|
||||
@then("the format result should contain plain key-value pairs")
|
||||
def step_format_result_plain(context: Context) -> None:
|
||||
assert "name: test" in context.format_result
|
||||
assert "nested:" in context.format_result
|
||||
assert "items:" in context.format_result
|
||||
|
||||
|
||||
@then("the format result should contain table output")
|
||||
def step_format_result_table(context: Context) -> None:
|
||||
assert "name" in context.format_result
|
||||
assert "count" in context.format_result
|
||||
|
||||
|
||||
@then("the format result should contain separator lines")
|
||||
def step_format_result_separator(context: Context) -> None:
|
||||
assert "---" in context.format_result
|
||||
|
||||
|
||||
@then("the serialized result should have string enum values")
|
||||
def step_serialized_enum_values(context: Context) -> None:
|
||||
assert context.serialized["state"] == "available"
|
||||
assert isinstance(context.serialized["nested"], dict)
|
||||
assert context.serialized["items"][0] == "archived"
|
||||
assert isinstance(context.serialized["ts"], str)
|
||||
|
||||
|
||||
@then("the format result should contain rich styled output")
|
||||
def step_format_result_rich_styled(context: Context) -> None:
|
||||
"""Assert that rich format produces styled output (ANSI codes or key-value lines).
|
||||
|
||||
The rich format must NOT produce raw JSON — it must produce styled terminal
|
||||
output via the RichMaterializer (ANSI escape codes or structured key-value
|
||||
lines with panel headers).
|
||||
"""
|
||||
result = context.format_result
|
||||
# Rich output must contain the data keys
|
||||
assert "name" in result, f"Rich output missing 'name': {result!r}"
|
||||
assert "count" in result, f"Rich output missing 'count': {result!r}"
|
||||
# Rich output must contain styled panel header or ANSI codes
|
||||
has_ansi = "\033[" in result or "\x1b[" in result
|
||||
has_panel_header = "Output" in result
|
||||
assert has_ansi or has_panel_header, (
|
||||
f"Rich output has no ANSI codes or panel header — got: {result!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the format result should not be valid JSON")
|
||||
def step_format_result_not_json(context: Context) -> None:
|
||||
"""Assert that the format result is NOT valid JSON (i.e. not the JSON fallback)."""
|
||||
try:
|
||||
json.loads(context.format_result)
|
||||
raise AssertionError(
|
||||
f"format_output() with 'rich' returned valid JSON — "
|
||||
f"expected styled terminal output, got: {context.format_result!r}"
|
||||
)
|
||||
except (json.JSONDecodeError, ValueError):
|
||||
pass # Expected: rich output is not JSON
|
||||
|
||||
|
||||
@then("the format result should contain color styled output")
|
||||
def step_format_result_color_styled(context: Context) -> None:
|
||||
"""Assert that color format produces ANSI-colored output (not plain text).
|
||||
|
||||
The color format must NOT produce plain text — it must produce ANSI-colored
|
||||
terminal output via the ColorMaterializer.
|
||||
"""
|
||||
result = context.format_result
|
||||
# Color output must contain the data keys
|
||||
assert "name" in result, f"Color output missing 'name': {result!r}"
|
||||
assert "count" in result, f"Color output missing 'count': {result!r}"
|
||||
# Color output must contain ANSI escape codes or styled panel header
|
||||
has_ansi = "\033[" in result or "\x1b[" in result
|
||||
has_panel_header = "Output" in result
|
||||
assert has_ansi or has_panel_header, (
|
||||
f"Color output has no ANSI codes or panel header — got: {result!r}"
|
||||
)
|
||||
|
||||
|
||||
@then("the format result should not be plain text only")
|
||||
def step_format_result_not_plain_text(context: Context) -> None:
|
||||
"""Assert that the color format result is NOT plain text (i.e. not the plain fallback).
|
||||
|
||||
Plain text would look like 'name: test\\ncount: 42' with no ANSI codes.
|
||||
Color output must have ANSI escape codes or styled panel headers.
|
||||
"""
|
||||
result = context.format_result
|
||||
has_ansi = "\033[" in result or "\x1b[" in result
|
||||
has_panel_header = "Output" in result
|
||||
# If it's purely plain text (no ANSI, no panel header), that's the bug
|
||||
is_plain_only = not has_ansi and not has_panel_header
|
||||
assert not is_plain_only, (
|
||||
f"format_output() with 'color' returned plain text — "
|
||||
f"expected ANSI-colored output, got: {result!r}"
|
||||
)
|
||||
|
||||
@@ -49,13 +49,13 @@ def _mock_action(name: str = "local/fmt-smoke") -> Action:
|
||||
read_only=False,
|
||||
state=ActionState.AVAILABLE,
|
||||
created_by=None,
|
||||
created_at=datetime.now(),
|
||||
updated_at=datetime.now(),
|
||||
created_at=datetime(2025, 1, 15, 10, 0, 0),
|
||||
updated_at=datetime(2025, 1, 15, 10, 0, 0),
|
||||
)
|
||||
|
||||
|
||||
def _mock_plan(name: str = "local/fmt-smoke-plan") -> Plan:
|
||||
now = datetime.now()
|
||||
now = datetime(2025, 1, 15, 10, 0, 0)
|
||||
return Plan(
|
||||
identity=PlanIdentity(plan_id=_ULID),
|
||||
namespaced_name=NamespacedName.parse(name),
|
||||
@@ -219,8 +219,6 @@ def format_output_rich() -> None:
|
||||
|
||||
Regression test for issue #2921: rich format silently fell back to JSON.
|
||||
"""
|
||||
import json as _json
|
||||
|
||||
from cleveragents.cli.formatting import format_output
|
||||
|
||||
data = {"name": "smoke-test", "status": "active", "count": 42}
|
||||
@@ -228,12 +226,12 @@ def format_output_rich() -> None:
|
||||
|
||||
# Must NOT be valid JSON (that was the bug)
|
||||
try:
|
||||
_json.loads(result)
|
||||
json.loads(result)
|
||||
raise AssertionError(
|
||||
f"format_output(data, 'rich') returned valid JSON — "
|
||||
f"expected styled terminal output, got: {result!r}"
|
||||
)
|
||||
except (_json.JSONDecodeError, ValueError):
|
||||
except (json.JSONDecodeError, ValueError):
|
||||
pass # Expected: rich output is not JSON
|
||||
|
||||
# Must contain the data keys
|
||||
|
||||
@@ -14,6 +14,7 @@ Based on v3_spec.md implementation plan Stage A4b / G5b.render.
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import sys
|
||||
import time
|
||||
from datetime import datetime
|
||||
from enum import Enum, StrEnum
|
||||
@@ -156,8 +157,6 @@ def _format_rich(data: dict[str, Any] | list[dict[str, Any]]) -> str:
|
||||
that the output is consistent with the newer session-based rendering path.
|
||||
The data passed here has already been redacted by the caller.
|
||||
"""
|
||||
from cleveragents.cli.output.session import OutputSession
|
||||
|
||||
strategy = RichMaterializer()
|
||||
with OutputSession(
|
||||
format="rich", command="format_output", strategy=strategy
|
||||
@@ -184,8 +183,6 @@ def _format_color(data: dict[str, Any] | list[dict[str, Any]]) -> str:
|
||||
that the output is consistent with the newer session-based rendering path.
|
||||
The data passed here has already been redacted by the caller.
|
||||
"""
|
||||
from cleveragents.cli.output.session import OutputSession
|
||||
|
||||
strategy = ColorMaterializer()
|
||||
with OutputSession(
|
||||
format="color", command="format_output", strategy=strategy
|
||||
@@ -339,8 +336,6 @@ def format_output(
|
||||
The rendered string. For machine-readable formats the output
|
||||
is written directly to stdout and an empty string is returned.
|
||||
"""
|
||||
import sys
|
||||
|
||||
t_start = time.monotonic()
|
||||
safe_data = _redact_data(data)
|
||||
fmt = format_type.lower()
|
||||
|
||||
Reference in New Issue
Block a user