Split test_regex_sync.py at the seam it already had - #353
Merged
Conversation
One 1306-line module held two test suites that shared, by AST, exactly one symbol: _SANCTIONED_EXTRAS, which is about the differential rules' character classes and belongs on the ledger side anyway. Nothing else crossed in either direction. The module docstring's first three paragraphs existed to explain why the two lived together, which is a filename's job. test_regex_sync.py keeps what its name says: the pipeline's hand copies of nameparser.config patterns, 160 lines. test_ledger_guards.py takes the differential half unchanged -- the hand copies made inside expected_since_*.toml, and the corpus-derived checks on what each rule may claim. _differential_fixtures.py holds the handles on tools/differential/ that both it and test_differential.py need: the ledger glob, the corpus, and the compare.py path-loader. That loader existed twice; now once. A pure move, and checked as one: pytest collects 2387 tests in tests/v2 before and after, and every node id is identical once the module name is stripped. No test added, removed or changed. Deliberately NOT split further. Spans, vocabularies and claims stay in one file, because that three-way category axis is the one six review rounds proved wrong -- every widening they found landed just outside whichever category the last guard covered, and two findings existed because no reader could see two guards' coverage side by side. `grep '^def test_'` on one file answers "what is guarded"; across three files it does not. Also repoints nine cross-references that named test_regex_sync.py for things that moved -- four ledger comments, the corpus builder, the harness README, and test_differential.py's own note about the loader. The four references left pointing at test_regex_sync.py are the pipeline-copy ones, which are still right.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 41 41
Lines 2845 2845
=======================================
Hits 2802 2802
Misses 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The mechanical half of the split held up: byte-identical ASTs, same 2387 node ids, every recorded number reproducing. The prose did not. The worst of it was self-inflicted in a way worth recording. I extracted load_compare so two modules could share it, ruff then flagged it unused in test_ledger_guards.py, I deleted the import -- and never revisited the claim that had justified the extraction. So both the docstring and the commit message said a loader had been deduplicated when nothing had: there was one compare.py loader and one build_cjk_corpus.py loader, of two different files. The fix is the one the reviewer's own finding pointed at. There really were two path-loaders of tools/ modules, written out six lines each, so load_tool(stem) now serves both -- and test_ledger_guards.py stops recomputing _TOOLS locally when it already imports it. The claim is true now instead of being withdrawn. _SANCTIONED_EXTRAS lost the two opening lines that said what it IS: the one sanctioned divergence between the ledger classes and _SCRIPT_RANGES. Restored, and it matters -- the membership test's docstring points at "its definition above for why U+FF65 is in it", and naming U+FF65 as the halfwidth middle dot was exactly what the move cut. Three positional references crossed the seam and now point into another file: two "the guards below" in the fixtures module, and _SOURCES, which lives in test_regex_sync.py now. Every remaining above/below in both modules was checked programmatically; none is mis-oriented, and neither module names an unqualified symbol from the other. The new module docstring also claimed "two kinds of guard" and "six review rounds" -- the body says four and five throughout, and three tests are neither kind (the glob, the roster staleness check, and the generated-corpus pin). Says five now, and names the third kind. AGENTS.md release step 8 lost ", so" along with its #333 citation when repointed, fusing two clauses into a run-on in text an agent follows at release time. Imports are relative now, matching the four existing sibling imports in tests/v2/. Also drops two pre-existing wrong names the move surfaced: a "guard A" label nothing defines, and _no_top_level_alternation for a function called _top_level_alternation.
Three and six blank lines where the two halves were cut apart, and a word dropped from the fold comment during an earlier reflow -- "five character classes", which is the count the sentence then lists. Both are cosmetic, and both are the kind of thing a pure-move claim should not be leaving behind: ruff does not check blank-line runs (E3 is outside its default select), so nothing would have caught either.
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
1 participant
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.
Closes #352.
tests/v2/test_regex_sync.pywas 1306 lines holding two test suites that shared, by AST, exactly one symbol —_SANCTIONED_EXTRAS, which is about the differential rules' character classes and belongs on the ledger side anyway. Nothing crossed in the other direction at all.tests/v2/test_regex_sync.pynameparser.configpatterns — what the name saystests/v2/test_ledger_guards.pyexpected_since_*.toml, plus the corpus-derived checks on what each rule may claimtests/v2/_differential_fixtures.pytools/differential/both it andtest_differential.pyneed: ledger glob, corpus, and thecompare.pypath-loaderThe loader existed twice (
test_differential.pyhad its own_load_compare); now once.Verified as a pure move
No test added, removed, or changed. Full suite 3055 passed; mypy and ruff clean; the differential harness still reports 0 unexplained at baseline 2.1.0 and 89 intentional / 0 unexplained at 2.0.0.
Deliberately NOT split further
Spans, vocabularies and claims stay in one file. That three-way category axis is precisely the one six review rounds proved wrong: every widening those rounds found landed just outside whichever category the last guard covered, and two of the findings existed because no reader could see two guards' coverage side by side.
grep '^def test_'on one file answers "what is guarded"; across three files it does not.The reviewers were split on whether to do this at all, and the argument against was good — co-location is what let a reader find the later defects by reading straight through, no finding was caused by file length, and the module runs in 0.15s. What tipped it is that the two halves are genuinely unrelated and the guard set has now stopped moving.
Cross-references
Nine comments named
test_regex_sync.pyfor things that moved — four ledger comments,build_cjk_corpus.py, the harness README, andtest_differential.py's own note about the loader. All repointed. The four left namingtest_regex_sync.py(_render.py,_policy.py, two in_vocab.py) are the pipeline-copy ones and are still correct.AGENTS.mdrelease step 8 and the test-module list are updated for both new modules.🤖 Generated with Claude Code