test: add CLI optimize parser tests - #28
Conversation
mverab
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested (1 warning that should be fixed in the test, plus process gaps)
Thank you for the first contribution — these parser tests are a useful start. I ran them locally: 2 passed against current egeo.cli.build_parser. They are parser-only (no pipeline execution, no writes, no shell). The asserted defaults match egeo/cli.py (out_dir=geo-output, schema_type=Article, runtime=python).
Please address the items below, then we can re-review. Do not treat this as a request to implement issue #2 (markdown frontmatter); this PR is test-only.
Warning
- tests/test_cli.py —
test_optimize_command_custom_optionspasses--runtime python, which is already the default. That assertion cannot catch a regression that ignores the flag. Use a non-default value (any string is accepted; argparse does not validate runtime names) or drop the--runtimepair from this test.
Process (needed for an auditable merge)
- Fill in the PR template: what changed, why, and that this is not the frontmatter issue. Branch
fix-markdown-frontmatterdoes not match the diff. - Add the missing trailing newline at EOF.
Looks good
- Happy-path defaults are real assertions, not tautologies.
--out-dir outputand--schema-type FAQPagedo exercise non-default parsing.- Scope is correctly limited to argparse.
Out of scope for this PR (maintainer follow-up)
CI today does not run pytest, and pytest is not a project dependency. That is a pre-existing repo gap. Wiring pytest tests/ into CI is welcome if you want to include it, but it is not required of this contribution.
PR #29 is the same commit (85fa56b). We will treat #28 as the canonical PR.
Reviewed on SHA 85fa56b
| assert args.input == "content.md" | ||
| assert args.out_dir == "output" | ||
| assert args.schema_type == "FAQPage" | ||
| assert args.runtime == "python" No newline at end of file |
There was a problem hiding this comment.
This asserts the default, not an override. --runtime defaults to python in egeo/cli.py (_add_optimize_parser). A parse that ignored the flag would still pass. Please pass a non-default value (e.g. --runtime other) or remove this pair from the custom-options test.
The custom-options case was passing --runtime python, which is already the default, so it could not catch a flag-ignore regression.
mverab
left a comment
There was a problem hiding this comment.
Maintainer follow-up landed on this branch:
--runtime othernow exercises a non-default override- trailing newline added
- PR description filled
Local pytest tests/test_cli.py: 2 passed on SHA f568669.
CI on first-time forks is still action_required (workflow approval). The tests themselves are parser-only and do not change product behavior.
Approve for merge when you want it on main. I am not merging.
Description
Adds unit tests for the
egeo optimizeargparse contract (build_parser).This is a test-only PR. It does not implement markdown frontmatter support (issue #2); the source branch name is a leftover.
Maintainer follow-up on top of the original commit: the custom-options test now uses a non-default
--runtimevalue, and the file has a trailing newline.Type of Change
GEO Impact
Testing
pytest tests/test_cli.py— 2 passed locally against currentegeo.cliChecklist
Additional Notes
Canonical PR for this commit. #29 was closed as a duplicate (same SHA).