fix: validation guards must survive python -O; add the missing test module - #12
fix: validation guards must survive python -O; add the missing test module#12JohnRDOrazio wants to merge 2 commits into
Conversation
Closes #11. All three guards in generate_seed.py were `assert`s, which Python strips under -O. Running the generator with that flag skipped validation entirely: it exited 0, printed its usual success line, and wrote a seed carrying unresolvable cross-references. The three checks move into a `validate(entries, known_types)` function that raises ValueError, following the shape coecdr already uses for its own generator. main() catches it and exits via sys.exit with a one-line message, so a data problem reads as a diagnostic rather than a traceback. Every problem is now collected and reported together. Repairing seed data took one run per error before; a run that finds a duplicate id, an unknown ctype and a malformed esi: reference now reports all three at once. Adds scripts/test_generate_seed.py, matching crpdr's and coecdr's test modules — this repository had none, which is why the -O behaviour went unnoticed. Thirteen tests: validate accepts the committed seed and null types, rejects duplicate ids, unknown ctype: values and every malformed esi: form including the bare prefix, and reports multiple problems together. The SurvivesOptimizedMode class is the regression guard proper: it runs validate in a subprocess under -O and -OO and requires it to still raise, so reintroducing `assert` here fails the suite. Also pins slugify, including the Italian arcidiocesi case fixed in #2, which had no test. Verified: exit 1 under -O, -OO and no flags with the seed left byte-identical on disk; a valid run still regenerates data/circumscriptions.json unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe seed generator now validates duplicate IDs, circumscription types, and ChangesSeed validation hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Generator
participant Validator
participant Output
Generator->>Validator: validate(entries, known_types)
Validator-->>Generator: return or collected ValueError
Generator->>Output: write validated entries
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test_generate_seed.py`:
- Around line 96-116: Extend the optimized-mode tests around run_validate and
main() to exercise invalid church_sui_iuris, duplicate-ID, and unknown-type
inputs. For each case, assert a non-zero exit under -O and -OO where applicable,
and verify data/circumscriptions.json remains unchanged or absent, covering
main()’s no-write behavior rather than only validate().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 001d1828-d909-46ff-b70f-9ad36db640cb
📒 Files selected for processing (3)
README.mdscripts/generate_seed.pyscripts/test_generate_seed.py
The suite drove validate() only, so it pinned detection but not the thing that actually went wrong in #11: the generator writing a corrupted seed and exiting 0. Mutating main() to detect the problem and write anyway left the previous suite fully green. Adds MainRefusesToWrite, which runs main() as a subprocess against a synthetic source in a temporary repo and asserts the output file is never created. Covers duplicate ids (two same-named sees with no province, which the generator cannot qualify) and unknown ctype: values (injected through MANUAL), each under no flags, -O and -OO, plus a sound-input case that must still write, and a case proving an existing seed is left byte-intact when a later run fails. church_sui_iuris has no main()-level counterpart: the value is a literal in main() that no source input or MANUAL override can reach, so it is unreachable end-to-end by construction. Noted in the test file and left covered at the validate() level. Also extends the optimized-mode test from one guard to all three, so reintroducing `assert` for any single check fails the suite rather than only for the church_sui_iuris one. 17 tests. Verified by mutation: reverting the guards to asserts fails 23 subtests, and making main() write anyway fails 7 — the latter passing cleanly before this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Verified. Both parts hold, and the second was a real hole — fixed in Fixed — main()'s no-write behaviour was untestedThis is the sharper point. The suite drove Demonstrated by mutation — patching
I had verified the no-write behaviour by hand and then not committed it as a test, which is the same ad-hoc-and-discard habit issue #11 criticises. Now
Fixed — optimized-mode test covered one guard, not threeAlso correct. Partially skipped — main()-level coverage for church_sui_iurisNot reachable, and not for want of trying. The value is a literal in This is the same reachability point from the earlier review round: the malformed- Validation17 tests, green. Mutation-checked both ways: reverting the guards to |
1 participant
Closes #11.
The change
All three guards were
asserts. They move into avalidate(entries, known_types)function raisingValueError, matching the shape coecdr already uses.main()catches it and exits throughsys.exit, so a data problem reads as a diagnostic rather than a traceback:Problems are now collected and reported together. Before, repairing seed data cost one run per error.
Verified, not asserted
Re-running the exact demonstration from #11 — two deliberately invalid values seeded, generator run three ways:
python3 -Opython3 -OOpython3Previously the first row was
exit=0withWrote 2935 circumscriptionsand a corrupted seed. A valid run still regeneratesdata/circumscriptions.jsonbyte-identical.The missing test module
This repository had no tests, which is precisely why the
-Obehaviour went unnoticed —crpdrshipstest_generate_seed.pyandcoecdrshipstest_generate_registry.py. Thirteen tests inscripts/test_generate_seed.py:Validate— accepts the committed seed and nulltype; rejects duplicate ids, unknownctype:values, and every malformedesi:form (esi:,latin,esi:Latin,esi:-latin,esi:latin-,""); accepts well-formed references CESIDR mints but the seed does not yet use; and reports three simultaneous problems in one message.SurvivesOptimizedMode— the regression guard proper. Runsvalidatein a subprocess under-Oand-OOand requires it to still raise. Reintroducingasserthere fails the suite.Slugify— pins the strip rule, including the Italianarcidiocesicase fixed in fix: strip the Italian archdiocesan type word (58 slugs) #2, which had no test. Also pins thatArchdiocese for the Military Servicesis not stripped, since only the leading styled form is generic.Not included
The companion conversion in cesidr is tracked separately at CatholicOS/cesidr#1; its failure mode differs (silent data loss rather than bad references) and it needs its own fix.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation