Conversation
A rule formula draws a feature-based natural class the user has named as its abbreviation in brackets, and any other one as its feature list. Tests now pin that choice, the line count and cell width that follow from it, and the display dependencies that rebuild a formula when a name or abbreviation is edited. A collector environment drives the view constructors with no window. It captures the drawn text and the dependencies registered, and it measures one unit per character, so a context's cell width can be asserted against the text that context draws. A registration naming more pairs than it supplies is rejected rather than trimmed, so the harness cannot absorb a count that disagrees with its arrays. Two cases are pinned as current behavior rather than as a fix. A class abbreviated "C" shows its features unless it also carries a name, so the abbreviation alone does not decide the drawing. A segment-based class is sized to its abbreviation while registration covers feature-based classes, so editing that abbreviation leaves the cell at its earlier width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1167 +/- ##
==========================================
+ Coverage 39.03% 39.30% +0.26%
==========================================
Files 1522 1522
Lines 353035 353035
Branches 40744 40744
==========================================
+ Hits 137792 138745 +953
+ Misses 185926 185023 -903
+ Partials 29317 29267 -50 🚀 New features to boost your workflow:
|
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.
Start here:
RuleFormulaVcTestDoubles.cs— the two test doubles the whole suite rests on. Everything else is data setup and assertions.What it does
Adds 41 tests for #1158, which merged with no coverage. That change made a rule formula draw a user-named feature-based natural class as its abbreviation in brackets instead of as its feature list, and registered dependencies so editing either resizes the cell. No production code changes here.
The suite drives all three rule formula view constructors through
StringCollectorEnv, a realIVwEnvneeding no window. It captures drawn text, recordsNoteDependencycalls, and measures one unit per character — so a cell's width is asserted against the very text it draws, not a copied constant. That turns #1158's own claim, that sizing follows the same check as drawing, into one invariant.The unknown you start with
Tests written after a merge usually prove nothing: they pass because they were written against what the code already does. So the question is not "do these pass" but "would they have caught it".
Reverting the five production files to
b18c6013a^1fails 20 of 33 red-capable tests. The other 13 pass deliberately, guarding against overreach. A further 8 characterize a static overload that does not exist pre-change and so cannot fail — evidence of intent, not of a fix.Where to look
AssertCellMatchesDrawingis load-bearing; if per-character measurement is wrong, nine tests are vacuous.RuleNamedClass_AbbreviatedC_DrawsFeature. Pins a change review did not flag: a class abbreviatedCdrew[C]regardless of name, and now draws features unless also named.views.dllviaVwPropertyStoreManagedP/Invoke. Passes locally; CI has not run it.Deliberately not here
RegularRule_DoesNotRegisterASegmentClassasserts a current production gap, not a fix: a segment-based class is sized to its abbreviation, butCollectFeatureNaturalClassesmatches onlyIPhNCFeatures. Follow-up ticket to come; the fix is one line.CONTEXT.mdentries.Verification
57 passed, 0 failed (16 pre-existing, 41 new). Comment and token hygiene clean. No FLEx session run.
To repeat the red/green check:
git checkout b18c6013a6b4d52c680c80c998e4e54c0d976455^1 -- Src/LexText/Morphology/RuleFormulaControlNaturalClassNameTests.csout — it cannot compile pre-change..\test.ps1 -TestProject MorphologyEditorDllTests -TestFilter "RuleFormulaVcNaturalClass" -SkipNativegit checkout HEAD -- Src/LexText/Morphology/, then restore the file.Next: review, or tell me to fold the segment-class fix into this branch instead of a follow-up.
Reading this a year from now — start here
This branch added tests for a change that had already merged. The reasoning below was produced during the review that preceded this PR and lives here because the branch itself carries no working documents — there was nothing to delete.
The durable point: rule formula drawing, line counting and cell sizing are three separate code paths that must agree about the same class, and the only cheap way to prove they agree is to measure the drawn text with the same environment that drew it.
How the harness works
CollectorEnvis a realIVwEnvimplementation used elsewhere in the repo for export and measurement. Four of its behaviors make it a formula oracle:AddPropcallsvc.DisplayVariant***land in the collected textAddStringAltMemberreads the real multistringget_StringWidthreturnstss.LengthNoteDependencyisvirtualTwo doubles sit on top:
TestRuleFormulaVc, exposing the protected line-count and width methods, andRecordingCollectorEnv, which captures both the text and the dependency calls.Font measurement was the feasibility risk, since a view constructor's base class measures every writing system on construction. It resolves to
VwPropertyStoreManaged, which is managed P/Invoke intoviews.dllrather than COM activation, so registration-free COM is not engaged and no manifest work is needed. An emptyPropertyTableyields a null stylesheet, which that path already tolerates.Decisions, and why
Dependency tests drive the concrete view constructors, not the new helper. Calling
NoteNaturalClassDependenciesdirectly would have been simpler, but that method does not exist pre-change, so such a test could not compile in the red state, let alone fail. DrivingDisplayon each view constructor instead makes the tests both red-capable and a truer test of the real call sites.The suite asserts on collected strings, never on the new fragment constants. Naming
kfragNCAbbreviationorkfragStarswould break the red build for the same reason.Characterization tests are segregated into their own file.
RuleFormulaControlNaturalClassNameTestscovers the newpublic static IsFeatureBasedNCNameUserDefinedoverload. Those 8 tests could never have failed, so mixing them into the red/green count would have overstated the evidence.One test input comes from real project data.
Phonemes s and t - Created automatically for rule "s to n"appears in a phonology test project in the liblcm tree. It explains why the production check usesContainsrather thanStartsWith— real auto-generated names carry a prefix. That case would not have been invented from the code alone.The auto-generated-name tests use
MEStrings.ksRuleNCFeatsNamerather than a hardcoded English copy, so they fail loudly if the resource text changes.Paths not taken
A rendered view or UI automation. The change is about what glyphs appear and how wide a cell is, which looks like it needs a window. It does not: the collector environment settles both, in milliseconds, with no UI thread.
Hardcoding expected widths.
[Vd]is 4 units plus margins, and asserting4004would have passed while documenting nothing. Asserting width against the drawn string's length states the actual invariant.Checking out the pre-merge branch to get a red run. Reverting only the five production files is narrower and leaves the new tests in place. Nothing under
Src/LexText/Morphology/has changed since the merge, so the two are equivalent today and the narrow revert stays correct if that stops being true.Adding
CONTEXT.mdentries for "view constructor" and phonological rule "context." Both are overloaded FieldWorks terms central to this branch and neither is defined there. Judged out of scope for a test-only branch; the gap is real and still open.Surprising findings
A claim I retracted mid-review.
IsFeatureBasedNCNameUserDefinedreadsName.UserDefaultWritingSystemwhile the natural class editor writes names into analysis writing systems (ws="all analysis"inMorphologyParts.xml), which looks like the feature would silently not apply in any project whose analysis writing system differs from the UI language. It does apply:UserDefaultWritingSystemfalls back toBestAnalysisVernacularAlternativewhen the user writing system is empty. Reading liblcm settled it. That fallback is also why the check needs itsAvailableWritingSystemIdsguard — without it, an unnamed class reads back as the***placeholder and would count as named.A behavior change that review did not surface. Before #1158, any single-feature class abbreviated
CorVdrew as its abbreviation. Now it must also carry a name. The red run shows it concretely:[C]pre-change,[+ cons]after. Standard FLEx classes carry real names and are unaffected, but a class both auto-named and abbreviatedCchanged silently.Evidence
Red run, five production files at
b18c6013a^1, characterization file set aside: 18 of 30 failed, then 2 of 3 more after the metathesis tests were added — 20 of 33, 13 passing deliberately.Captured pre-change drawings, each now pinned by a test:
⎡⎢⎣+ vd+ cons- cont⎤⎥⎦[Vd][+ vd][***]C[C][+ cons]Green run: 57 passed, 0 failed, 0 skipped.
Total tests: 57confirms the project selection is not vacuous — a filter matching nothing still exits 0.Test distribution: display 11, dependency 13, sizing 9, name characterization 8.
Review fixes applied before this PR: a literal U+200B zero-width space replaced with its escape (invisible in diffs, and comment hygiene does not scan string literals); two bracket glyphs replaced with escapes;
DependencyCallnow rejects a declared count that runs past its arrays instead of silently skipping the excess — which incidentally proves production's count matches, since all 57 still pass; and five files normalized from LF to CRLF, whose git warning on stderr had been breakingcomment-hygiene.ps1mid-run.Not verified: whether a natural class abbreviation can be edited in place inside a rule formula. The view is editable and the context cells carry no not-editable property, but no running app was checked. That question sets the severity of the segment-class gap and belongs to the follow-up ticket.
This change is