Skip to content

LT-22725: Cover natural class abbreviation display with tests - #1167

Draft
thejambi wants to merge 1 commit into
mainfrom
LT-22725-natural-class-abbreviation-tests
Draft

thejambi wants to merge 1 commit into
mainfrom
LT-22725-natural-class-abbreviation-tests

Conversation

@thejambi

@thejambi thejambi commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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 real IVwEnv needing no window. It captures drawn text, records NoteDependency calls, 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^1 fails 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

  • The width oracle. AssertCellMatchesDrawing is load-bearing; if per-character measurement is wrong, nine tests are vacuous.
  • RuleNamedClass_AbbreviatedC_DrawsFeature. Pins a change review did not flag: a class abbreviated C drew [C] regardless of name, and now draws features unless also named.
  • First native load in this assembly. A view constructor reaches views.dll via VwPropertyStoreManaged P/Invoke. Passes locally; CI has not run it.

Deliberately not here

  • RegularRule_DoesNotRegisterASegmentClass asserts a current production gap, not a fix: a segment-based class is sized to its abbreviation, but CollectFeatureNaturalClasses matches only IPhNCFeatures. Follow-up ticket to come; the fix is one line.
  • No production change, and no CONTEXT.md entries.

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:

  1. git checkout b18c6013a6b4d52c680c80c998e4e54c0d976455^1 -- Src/LexText/Morphology/
  2. Move RuleFormulaControlNaturalClassNameTests.cs out — it cannot compile pre-change.
  3. .\test.ps1 -TestProject MorphologyEditorDllTests -TestFilter "RuleFormulaVcNaturalClass" -SkipNative
  4. git 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

CollectorEnv is a real IVwEnv implementation used elsewhere in the repo for export and measurement. Four of its behaviors make it a formula oracle:

Behavior Consequence
AddProp calls vc.DisplayVariant Brackets and *** land in the collected text
AddStringAltMember reads the real multistring The abbreviation lands in the collected text
get_StringWidth returns tss.Length Widths are deterministic, in characters, with no graphics context
NoteDependency is virtual Registrations can be recorded by a subclass

Two doubles sit on top: TestRuleFormulaVc, exposing the protected line-count and width methods, and RecordingCollectorEnv, 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 into views.dll rather than COM activation, so registration-free COM is not engaged and no manifest work is needed. An empty PropertyTable yields a null stylesheet, which that path already tolerates.

Decisions, and why

Dependency tests drive the concrete view constructors, not the new helper. Calling NoteNaturalClassDependencies directly 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. Driving Display on 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 kfragNCAbbreviation or kfragStars would break the red build for the same reason.

Characterization tests are segregated into their own file. RuleFormulaControlNaturalClassNameTests covers the new public static IsFeatureBasedNCNameUserDefined overload. 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 uses Contains rather than StartsWith — 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.ksRuleNCFeatsName rather 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 asserting 4004 would 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.md entries 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. IsFeatureBasedNCNameUserDefined reads Name.UserDefaultWritingSystem while the natural class editor writes names into analysis writing systems (ws="all analysis" in MorphologyParts.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: UserDefaultWritingSystem falls back to BestAnalysisVernacularAlternative when the user writing system is empty. Reading liblcm settled it. That fallback is also why the check needs its AvailableWritingSystemIds guard — 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 C or V drew 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 abbreviated C changed 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:

Case Pre-change Current
Named class, 3 features ⎡⎢⎣+ vd+ cons- cont⎤⎥⎦ [Vd]
Named class, no abbreviation [+ vd] [***]
Rule-named class abbreviated C [C] [+ cons]
Any class, dependency registration none registered name and abbreviation

Green run: 57 passed, 0 failed, 0 skipped. Total tests: 57 confirms 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; DependencyCall now 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 breaking comment-hygiene.ps1 mid-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 Reviewable

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>
@github-actions

Copy link
Copy Markdown

NUnit Tests

    1 files  ± 0      1 suites  ±0   13m 19s ⏱️ ±0s
6 354 tests +41  6 269 ✅ +41  85 💤 ±0  0 ❌ ±0 
6 363 runs  +41  6 278 ✅ +41  85 💤 ±0  0 ❌ ±0 

Results for commit c3beecc. ± Comparison against base commit 506c2a6.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.30%. Comparing base (506c2a6) to head (c3beecc).

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     

see 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants