test(cli): cover action and plan extensions #420

Closed
brent.edwards wants to merge 0 commits from feature/m4-cli-extension-tests into master
Member

Summary

  • Added comprehensive Behave test coverage (27 new scenarios) for CLI extension features: automation profile resolution across all builtin profiles, invariant ordering preservation, actor override error cases, and output snapshot assertions for JSON/YAML/table formats
  • Added Robot Framework integration tests (4 new test cases) for action show with optional actors/invariants and profile resolution
  • Created benchmarks/cli_extension_tests_bench.py with ASV performance baselines across 4 suites (12 benchmarks)
  • Updated docs/development/testing.md with CLI extension test fixture documentation

Closes #326

## Summary - Added comprehensive Behave test coverage (27 new scenarios) for CLI extension features: automation profile resolution across all builtin profiles, invariant ordering preservation, actor override error cases, and output snapshot assertions for JSON/YAML/table formats - Added Robot Framework integration tests (4 new test cases) for `action show` with optional actors/invariants and profile resolution - Created `benchmarks/cli_extension_tests_bench.py` with ASV performance baselines across 4 suites (12 benchmarks) - Updated `docs/development/testing.md` with CLI extension test fixture documentation Closes #326
brent.edwards added this to the v3.1.0 milestone 2026-02-24 20:12:38 +00:00
test(cli): cover action and plan extensions
All checks were successful
CI / lint (pull_request) Successful in 25s
CI / quality (pull_request) Successful in 37s
CI / benchmark-publish (pull_request) Has been skipped
CI / security (pull_request) Successful in 55s
CI / typecheck (pull_request) Successful in 1m1s
CI / build (pull_request) Successful in 33s
CI / integration_tests (pull_request) Successful in 5m13s
CI / unit_tests (pull_request) Successful in 12m53s
CI / docker (pull_request) Successful in 9s
CI / benchmark-regression (pull_request) Successful in 21m31s
CI / coverage (pull_request) Successful in 51m7s
2c21611ad5
Added comprehensive test coverage for CLI extension features from #325:
- Behave scenarios for automation_profile resolution, invariant ordering,
  and actor override error cases
- Output snapshot assertions for JSON/YAML/table formats
- Robot integration test for action show with optional actors/invariants
- ASV benchmark for extended CLI scenario runtime baseline
- Updated testing.md with CLI extension test fixture documentation

ISSUES CLOSED: #326
Merge branch 'master' into feature/m4-cli-extension-tests
All checks were successful
CI / lint (pull_request) Successful in 25s
CI / benchmark-publish (pull_request) Has been skipped
CI / quality (pull_request) Successful in 35s
CI / security (pull_request) Successful in 50s
CI / typecheck (pull_request) Successful in 1m2s
CI / build (pull_request) Successful in 36s
CI / integration_tests (pull_request) Successful in 5m6s
CI / unit_tests (pull_request) Successful in 21m56s
CI / docker (pull_request) Successful in 15s
CI / benchmark-regression (pull_request) Successful in 22m7s
CI / coverage (pull_request) Successful in 56m13s
c8434e21f3
freemo requested changes 2026-02-24 22:26:03 +00:00
Dismissed
freemo left a comment

Code Review: PR #420test(cli): cover action and plan extensions

This is a well-organized testing PR with good Behave/Robot/ASV coverage structure. The description clearly summarizes what was done and references the correct issue. A few items need attention before merge.

Required Changes

1. Missing CHANGELOG update (CONTRIBUTING.md §6)
No CHANGELOG entry was added for this test suite. Per guidelines: "The PR must include an update to the changelog file. Add one new entry per commit in the PR that describes the change from the user's perspective."

2. Missing CONTRIBUTORS.md update (CONTRIBUTING.md §8)
Brent Edwards is not listed in CONTRIBUTORS.md and was not added in this PR.

Advisory (Non-Blocking)

3. Broad except Exception: pass in benchmarks
benchmarks/cli_extension_tests_bench.py (around line 233) contains except Exception: pass which silently swallows all errors. Per CONTRIBUTING.md error handling guidelines: "Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic." While benchmarks are less critical than production code, consider catching a more specific exception or at minimum logging the error.

4. Import inside function body
robot/helper_cli_extensions.py contains import json inside a function body. Per the project's Import Guidelines: "Ensure all imports are at the top of the Python file. Do not scatter imports throughout the file or bury them inside functions or methods."

What's Good

  • Proper Closes #326 linking in description
  • Commit message follows Conventional Changelog format
  • Issue #326 referenced in commit footer
  • Correct milestone (v3.1.0)
  • All files in correct directories
  • Appropriate Type/Testing label

Please address items 1-2 (and ideally 3-4) and push updated commits.

## Code Review: PR #420 — `test(cli): cover action and plan extensions` This is a well-organized testing PR with good Behave/Robot/ASV coverage structure. The description clearly summarizes what was done and references the correct issue. A few items need attention before merge. ### Required Changes **1. Missing CHANGELOG update (CONTRIBUTING.md §6)** No CHANGELOG entry was added for this test suite. Per guidelines: "The PR must include an update to the changelog file. Add one new entry per commit in the PR that describes the change from the user's perspective." **2. Missing CONTRIBUTORS.md update (CONTRIBUTING.md §8)** Brent Edwards is not listed in `CONTRIBUTORS.md` and was not added in this PR. ### Advisory (Non-Blocking) **3. Broad `except Exception: pass` in benchmarks** `benchmarks/cli_extension_tests_bench.py` (around line 233) contains `except Exception: pass` which silently swallows all errors. Per CONTRIBUTING.md error handling guidelines: "Do not use bare catch-all exception handlers without re-raising unless you have specific recovery logic." While benchmarks are less critical than production code, consider catching a more specific exception or at minimum logging the error. **4. Import inside function body** `robot/helper_cli_extensions.py` contains `import json` inside a function body. Per the project's Import Guidelines: "Ensure all imports are at the top of the Python file. Do not scatter imports throughout the file or bury them inside functions or methods." ### What's Good - Proper `Closes #326` linking in description - Commit message follows Conventional Changelog format - Issue #326 referenced in commit footer - Correct milestone (v3.1.0) - All files in correct directories - Appropriate `Type/Testing` label Please address items 1-2 (and ideally 3-4) and push updated commits.
freemo dismissed freemo's review 2026-02-24 22:45:57 +00:00
Reason:

dont want hard block

chore(pr): address code review feedback for PR #420
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 18s
CI / build (pull_request) Successful in 17s
CI / typecheck (pull_request) Successful in 32s
CI / security (pull_request) Successful in 33s
CI / unit_tests (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / benchmark-regression (pull_request) Has been cancelled
CI / coverage (pull_request) Has been cancelled
CI / docker (pull_request) Has been cancelled
f3ddb48faf
Added CHANGELOG entry for CLI extension test suite. Added Brent Edwards
to CONTRIBUTORS.md. Replaced broad except Exception with specific
ValidationError catch in benchmark. Moved import json to module
top-level in robot helper script.

ISSUES CLOSED: #326
Author
Member

Thanks for the thorough review, Jeff. All four items have been addressed in commit f3ddb48:

  1. CHANGELOG — Added entry under Unreleased.
  2. CONTRIBUTORS.md — Added Brent E. Edwards.
  3. Broad exception catch — Replaced except Exception with except ValidationError in benchmarks/cli_extension_tests_bench.py. The validate_namespaced_actor function raises cleveragents.core.exceptions.ValidationError, so this is the correct specific type.
  4. Import placement — Moved import json from inside action_show_json() to module-level imports in robot/helper_cli_extensions.py.

All nox stages pass (lint, typecheck, unit_tests, integration_tests, coverage_report at 97.2%).

Thanks for the thorough review, Jeff. All four items have been addressed in commit `f3ddb48`: 1. **CHANGELOG** — Added entry under Unreleased. 2. **CONTRIBUTORS.md** — Added Brent E. Edwards. 3. **Broad exception catch** — Replaced `except Exception` with `except ValidationError` in `benchmarks/cli_extension_tests_bench.py`. The `validate_namespaced_actor` function raises `cleveragents.core.exceptions.ValidationError`, so this is the correct specific type. 4. **Import placement** — Moved `import json` from inside `action_show_json()` to module-level imports in `robot/helper_cli_extensions.py`. All nox stages pass (lint, typecheck, unit_tests, integration_tests, coverage_report at 97.2%).
Merge branch 'master' into feature/m4-cli-extension-tests
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 18s
CI / build (pull_request) Successful in 20s
CI / typecheck (pull_request) Successful in 32s
CI / security (pull_request) Successful in 33s
CI / integration_tests (pull_request) Successful in 3m47s
CI / unit_tests (pull_request) Successful in 17m13s
CI / benchmark-regression (pull_request) Successful in 20m7s
CI / docker (pull_request) Successful in 20s
CI / coverage (pull_request) Successful in 1h5m29s
3a66ce4d02
Author
Member

⚠️ Do not merge this PR individually. This branch exists for review convenience only.

All changes from this branch are consolidated in PR #431 (develop-brent-4), which should be the one merged into master.

#431

⚠️ **Do not merge this PR individually.** This branch exists for review convenience only. All changes from this branch are consolidated in **PR #431** (`develop-brent-4`), which should be the one merged into `master`. → https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/431
brent.edwards closed this pull request 2026-02-25 19:45:48 +00:00
brent.edwards deleted branch feature/m4-cli-extension-tests 2026-02-25 19:46:01 +00:00
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
Required
Details
CI / quality (pull_request) Successful in 18s
Required
Details
CI / build (pull_request) Successful in 20s
Required
Details
CI / typecheck (pull_request) Successful in 32s
Required
Details
CI / security (pull_request) Successful in 33s
Required
Details
CI / integration_tests (pull_request) Successful in 3m47s
Required
Details
CI / unit_tests (pull_request) Successful in 17m13s
Required
Details
CI / benchmark-regression (pull_request) Successful in 20m7s
CI / docker (pull_request) Successful in 20s
Required
Details
CI / coverage (pull_request) Successful in 1h5m29s
Required
Details

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!420
No description provided.