Give the validation scripts real argparse surfaces and write down the refresh runbook - #425
Open
Nitjsefnie wants to merge 5 commits into
Open
Give the validation scripts real argparse surfaces and write down the refresh runbook#425Nitjsefnie wants to merge 5 commits into
Nitjsefnie wants to merge 5 commits into
Conversation
build_validation.py hand-rolled sys.argv parsing, so --help raised KeyError and generate_validation_report.py/fetch_test_assets.py ignored argv entirely (--help silently rewrote docs/parser-validation.md / ran the fetch loop). Each script now has a testable build_parser() (description=__doc__, matching serve_compare.py's local convention): --help exits 0, and an unknown argument or slug exits 2 with a usage message instead of performing real work. Co-Authored-By: Kimi K3 <noreply@kimi.com>
The end-to-end refresh procedure existed only as scattered fragments. Put the ordered runbook (fetch+rebuild fixtures, regenerate the report, verify with the slow gate) in scripts/README.md under the Validation and accuracy table, state that both regeneration steps must happen together and that these scripts write committed files, and point at it from TESTING.md and validation_sources.py instead of restating it. Also document build_validation.py's slug filter in its table row, which only mentioned --fetch. Co-Authored-By: Kimi K3 <noreply@kimi.com>
Assert per script, via build_parser(): --help exits 0 with a usage line, an unknown argument exits 2 with a usage message, and for build_validation.py an unknown slug also exits 2 (no more KeyError) while the real forms ([], --fetch, --fetch <slug>) still parse to the right namespace. Co-Authored-By: Kimi K3 <noreply@kimi.com>
The parser tests call build_parser() directly, so they stayed green if main() never invoked it — the exact pre-AgoraDMV#297 defect (argv silently ignored, real work performed on --nope). Add one test per script that runs main() under a monkeypatched sys.argv: the two no-arg scripts must exit 2 on an unknown argument before touching any asset or the report, and build_validation.py must act on args.fetch (fetch first) and args.slugs (restrict to exactly those jurisdictions), verified against a fake jurisdiction rooted in tmp_path so no committed fixture is written. Co-Authored-By: Kimi K3 <noreply@kimi.com>
build_validation.py prints the fixture path absolute, not relative, and generate_validation_report.py ends its line with '; skipped: none'. Quote both the way a reader will actually see them. Co-Authored-By: Kimi K3 <noreply@kimi.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
Closes #297
What does this change?
The three external-validation scripts get real
argparsesurfaces, and the end-to-end refreshsequence gets written down in
scripts/README.md.Two of the three were worse than the issue recorded, so the fix is not only about discoverability:
generate_validation_report.py --helpexited 0 and overwrotedocs/parser-validation.md—asking a script for help silently rewrote a committed file.
fetch_test_assets.py --helpexited 0 and ran its entire fetch loop.build_validation.py --helpraisedKeyError: '--help', which is the one the issue names.Now all three parse argv first:
--helpprints usage and exits 0 touching nothing, an unknown flagexits 2 with a usage message, and an unknown slug exits 2 naming all twelve valid ones instead of
raising a
KeyError.The runbook lives in
scripts/README.md(the shape PR #343 used for the same defect in the otherarea), with pointers from
TESTING.mdandtests/validation_sources.py. Item 3 of the issue — CLIgating — is deliberately not here; it read as one decision covering both areas rather than
something to settle inside a docs PR.
How to test
Everything below was run, not eyeballed.
The behaviour that matters, on a clean tree:
On
develop, that same sequence leavesM docs/parser-validation.mdand prints the full13-asset fetch loop.
Errors:
--nope→ exit 2,error: unrecognized arguments: --nope;build_validation.py nosuchslug→ exit 2,
invalid choice: 'nosuchslug' (choose from 'agriculture', …).The runbook was executed verbatim from the project root — step 1 rewrote the twelve fixtures, step 2
regenerated the report (
774/822 recalled, 94.2%; skipped: none), step 3 gave33 passed in 11.61s.Gates:
uv run ruff check .clean ·uv run ruff format --check .178 files · fast suite1415 passed, 15 skipped, 15 xfailedagainst a1403/15/15baseline ·uv run pytest -m browser6 passed ·
uv run pytest -m slow --deselect tests/test_govinfo_corpus_parity.py554 passed, 7 skipped, unchanged. Those counts are from this machine and are higher than yours;they are offered as a delta, not as your numbers.
Every new test was checked in both directions. Reverting the scripts to
developmakes all of themfail. Separately, the wiring tests were checked against a subtler mutation — replacing the
parse_args()call inmain()withpass, which restores the exact original defect while leavingbuild_parser()intact — and they go red, with the captured stdout showingwrote docs/parser-validation.md …on a--nopeinvocation.Checklist
Closes #...)AI assistance
Generated by Kimi K3 (implementation, verification), Claude Opus 5 (1M context) (brief, review)
Follow-ups / Known Limitations
Two things this PR deliberately leaves alone, both worth knowing before you run the runbook yourself:
77 deletions) — the fixture
noteprose no longer matches whatbuild_fixture()emits. Itreproduces identically on
develop, so it is not from this branch. Worse than the prose churn:some
match_pathvalues that are resolved paths in the committed fixtures now regenerate asnull, which silently degrades those accounts to the agency-scoped fallback, concentrated indefense / energy_water / transportation_hud. I did not touch the fixtures. Happy to file it or fix
it, whichever you prefer.
docs/parser-validation.mdis stale relative to its own generator — filed separately as docs/parser-validation.md says do not edit by hand, but regenerating it on develop produces a diff #423 andkept out of this diff.