feat(cli): harden test inventory discovery and UX (#172)#1066
Conversation
- Use re.MULTILINE regex in discover.py for reliable Makefile target detection - Add 'synthetic' to valid --category choices in tests list - Convert RuntimeError from interactive picker to structured OpenSREError with actionable suggestion - Guard non-runnable suite IDs in tests run with a clear error message - Print skip message in run_catalog_items instead of silently dropping items - Expand test_cli_inventory.py from 2 to 16 tests covering filtering, stable IDs, JSON output, unknown-ID errors, missing-TUI graceful degradation, and command rendering
Greptile SummaryThis PR hardens the Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions with no correctness impact All four bug-fix targets (MULTILINE regex, synthetic category, TUI degradation, suite guard) are correctly implemented and well-tested. The only open comments are a misplaced analytics call and a stdout-vs-stderr placement, neither of which affects correctness or data integrity. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["opensre tests (no subcommand)"] --> B{is_yes or is_json_output?}
B -- Yes --> C[raise OpenSREError\n'No subcommand provided']
B -- No --> D[run_interactive_picker]
D -- RuntimeError\nmissing TUI / no tty --> E[catch RuntimeError\nraise OpenSREError\nwith suggestion]
D -- returns int --> F[raise SystemExit]
G["opensre tests list"] --> H[load_test_catalog]
H --> I{--category filter}
I -- synthetic ✅ --> J[filter catalog]
I -- rca / ci-safe / etc --> J
J --> K[echo or JSON output]
L["opensre tests run <id>"] --> M[find_test_item]
M -- None --> N[OpenSREError: Unknown id]
M -- found --> O{item.is_runnable?}
O -- No --> P[OpenSREError: is a suite]
O -- Yes --> Q[run_catalog_item]
R[run_catalog_items] --> S{item.is_runnable?}
S -- No --> T["print: Skipping '<id>'"]
S -- Yes --> U[run_catalog_item]
Reviews (1): Last reviewed commit: "feat(cli): harden test inventory discove..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| worst = 0 | ||
| for item in items: | ||
| if not item.is_runnable: | ||
| print(f"Skipping '{item.id}' — no runnable command defined.") |
There was a problem hiding this comment.
print() writes to stdout, so the skip notice will appear in the output stream alongside any real test output. If a caller pipes or captures stdout (e.g., opensre tests run … | jq), the diagnostic text will pollute the data stream. Writing to stderr keeps the message visible to the user without contaminating captured stdout.
| print(f"Skipping '{item.id}' — no runnable command defined.") | |
| import sys | |
| print(f"Skipping '{item.id}' — no runnable command defined.", file=sys.stderr) |
- Write skip notice to stderr instead of stdout in run_catalog_items so it doesn't pollute captured output when callers pipe or capture stdout - Move capture_tests_picker_opened() inside the try block so the analytics event only fires when the interactive picker actually opens, not on degraded/non-tty paths that immediately raise RuntimeError - Update test assertion to check stderr for the skip message
Davidson3556
force-pushed
the
feat/harden-cli-test-inventory-172
branch
from
ce30fed to
e0d1cae
Compare
|
@muddlebee kindly review |
- Move capture_tests_picker_opened() after picker returns so analytics only fires on successful open, not on RuntimeError degraded paths - Replace 'select a specific child' suggestion with clearer wording that matches what the CLI actually exposes - Drop make:test-grafana assertion from stable-IDs test since that target is Makefile-dependent and not universally present in CI - Replace output-length comparison in search filter test with explicit ID presence/absence assertions
|
@muddlebee kindly review |
|
@muddlebee please review |
|
@Davidson3556 thank you. I will just review a bit more so edge cases aren't missed. thank you for patience |
|
@Davidson3556 can you add a video demo using
the screenshots doesnt tell much |
- Add regression test for re.MULTILINE fix in discover_make_targets: mocks MAKEFILE_PATH with a target at line 1 (no preceding newline) and asserts it is discovered — catches any revert to the old string check - Strengthen synthetic category test to assert a known stable ID (synthetic:001-replication-lag) is present, not just exit_code == 0
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
i made the video with loom but it looks like loom is down.
@muddlebee kindly review |
|
i drop all the screenshot instead @muddlebee |
|
@Davidson3556 thank you for sharing demo. you can just record in your system and push the video to github or dropdown here. that also works |
|
and the demo looks smooth 👍 great, nice work |
|
@davincios @rrajan94 please kindly review |
|
🎉 MERGED! @Davidson3556 just shipped something. The diff gods are pleased. 🙌
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
4 participants
Fixes #172
Describe the changes you have made in this PR -
Hardened the
opensre testsCLI command to be more reliable and give clearer errors in every environment.discover.py — replaced the fragile
"\n{target}:"string search withre.search(..., re.MULTILINE)so Makefile target detection works correctly regardless of position in the file.commands/tests.py — three improvements:
syntheticto the valid--categorychoices soopensre tests list --category syntheticno longer exits with "invalid choice"run_interactive_pickerso aRuntimeError(raised when questionary is missing or there is no tty) becomes a structuredOpenSREErrorwith an actionable suggestion instead of a raw tracebackis_runnablecheck intests runso passing a suite ID gives a clear error ("is a suite and cannot be run directly") with a suggestion to useopensre tests listrunner.py —
run_catalog_itemsnow prints"Skipping '<id>' — no runnable command defined."instead of silently dropping non-runnable items.test_cli_inventory.py — expanded from 2 tests to 16, covering: always-on list, stable IDs, category filtering, search filtering, empty-search result, JSON output structure, JSON + category combined, unknown-ID helpful error, no-subcommand non-interactive fallback, missing-TUI graceful degradation,
format_commandrendering for make targets and opensre subcommands, and non-runnable skip message.Demo/Screenshot for feature changes and bug fixes -
https://www.loom.com/share/530c7020bbcc46ea8ef14cb1dc00b6ac
Test results: 18/18 new tests pass, 3806/3806 suite tests pass, lint and typecheck clean.
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The core problem was that the CLI had three silent failure modes: (1) discovery used a brittle string search that could miss targets at the start of the Makefile, (2) missing TUI dependencies produced a raw Python traceback instead of a helpful message, and (3) passing an unknown or non-runnable ID gave no guidance on what to do next.
For discovery I switched to
re.searchwithre.MULTILINE— the simplest fix that matches how Makefile targets are actually defined. I considered parsing the Makefile more deeply but that would be over-engineering for what is a simple presence check.For error handling I chose to catch
RuntimeErrorat the command layer (commands/tests.py) and re-raise asOpenSREError, which already has the project's standard formatting and suggestion field. This keeps error-handling logic out of the interactive module and in the place that owns CLI output.The test expansion follows the existing pattern in the file (CliRunner invocations) and adds one direct unit test for
run_catalog_itemsusing a manually constructedTestCatalogItemwith no command, which is the simplest way to exercise the skip path without needing a real subprocess.Checklist before requesting a review