Repository navigation
LT-22672: Cover the relevance rule with the most rows riding on it - #1145
Merged
Merged
Conversation
Raised by Jason on #1135, after the merge. MoStemMsa.IsFieldRelevant withholds FromPartsOfSpeech ("Attaches to Categories") unless the owning entry has a proclitic or enclitic, and Morphology.fwlayout:39 declares that part visibility="always" -- so before the relevance gate the row composed on every stem MSA, and now it disappears for every entry without a clitic. That is the gate's most visible consequence and nothing pinned it. The test is Jason's, run and confirmed to bite: suppressing the gate fails its negative half with "Expected: False, But was: True", while the positive control -- add a proclitic allomorph, change nothing else -- keeps passing. Also corrects two comments that claimed more than the code does. The composer implements the SECOND of SliceFilter.IncludeSlice's two gates; the first looks the slice id up in the tool's filter list, and the id never reaches the composer, so that half is LT-22802. The fixture summary said it covered the remainder of the overrides, which was untrue while this test was missing, and still excludes the InflectionClass limb that withholds the row from a compound rule's left/right MSA. xWorksTests filter Avalonia 1642 passed, 0 failed. 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 #1145 +/- ##
=======================================
Coverage 38.99% 38.99%
=======================================
Files 1520 1520
Lines 352711 352711
Branches 40681 40681
=======================================
Hits 137532 137532
Misses 185883 185883
Partials 29296 29296
🚀 New features to boost your workflow:
|
The summary described IsIrrelevantForObject but sat on _propsToMonitor, a field declared between it and the method, which already carries its own comment where it is used. It also asserted that the other gate was absent and named it as tracked elsewhere. That reads as true only until the other gate lands, and the change that lands it branched from a base without this comment, so it would not have corrected the claim. Saying which gate this one IS leaves nothing to fall out of date; LT-22802 is still named, since a Jira id is the pointer that keeps working. Two uses of the banned word went with it, in this file and in the test fixture's summary, along with the "legacy resolves no flid either" aside, which describes this code's own behaviour perfectly well without it. Comments only. DetailFieldRelevanceTests 4 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thejambi
marked this pull request as ready for review
September 24, 2026 22:21
mark-sil
approved these changes
Sep 26, 2026
mark-sil
left a comment
mark-sil
left a comment
Contributor
There was a problem hiding this comment.
@mark-sil reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on thejambi).
thejambi
pushed a commit
that referenced
this pull request
Sep 28, 2026
…-lists Main now has #1145, which this branch also carried as its first commit. Squash-merging gave it a different identity, so both files it touched conflicted with that commit's own later edits there. Resolved toward main, whose comments had been corrected, with one change: the relevance doc named LT-22802 as covering the other gate. This branch is that gate, so the doc now points at IsFilteredOutByTool, in the same class. The fixture summary keeps its pointer to DetailSliceFilterTests, where that gate is tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thejambi
added a commit
that referenced
this pull request
Sep 29, 2026
#1146) * test: pin the relevance rule with the most rows riding on it Raised by Jason on #1135, after the merge. MoStemMsa.IsFieldRelevant withholds FromPartsOfSpeech ("Attaches to Categories") unless the owning entry has a proclitic or enclitic, and Morphology.fwlayout:39 declares that part visibility="always" -- so before the relevance gate the row composed on every stem MSA, and now it disappears for every entry without a clitic. That is the gate's most visible consequence and nothing pinned it. The test is Jason's, run and confirmed to bite: suppressing the gate fails its negative half with "Expected: False, But was: True", while the positive control -- add a proclitic allomorph, change nothing else -- keeps passing. Also corrects two comments that claimed more than the code does. The composer implements the SECOND of SliceFilter.IncludeSlice's two gates; the first looks the slice id up in the tool's filter list, and the id never reaches the composer, so that half is LT-22802. The fixture summary said it covered the remainder of the overrides, which was untrue while this test was missing, and still excludes the InflectionClass limb that withholds the row from a compound rule's left/right MSA. xWorksTests filter Avalonia 1642 passed, 0 failed. * LT-22802: Apply a tool's slice filter list in the Avalonia detail view Legacy SliceFilter.IncludeSlice has two gates: look the slice's authored id up in the filter list the tool's filterPath names and withhold the row when it is listed, then ask IsFieldRelevant. LT-22672 added the second. This adds the first. The id was discarded at import, so three pieces: - XmlLayoutImporter carries the slice's id onto ViewNode.SliceId, and id leaves the unhandled-attribute report. - DetailComposer.Walk withholds a node whose SliceId the tool lists, checked before the node kind is dispatched so a withheld node takes its subtree with it -- where legacy checks it, at the top of ProcessSubpartNode. - RecordEditView reads filterPath from its own configuration and parses the same file into the id set, memoized per view. A tool with no filterPath, or a file that cannot be read, filters nothing: an extra row beats a view that will not open. NOTHING VISIBLE CHANGES TODAY, and the ticket's symptom section overstates it. The only shipped filter entries that still resolve to a real slice are the five CmPossibility ids in basicPlusFilter.xml, and the only tool they reach is Exception "Features" (ProdRestrictEdit) -- which is not in the Avalonia tool catalog, so it renders legacy even under UIMode=New. The other 27 ids across the six filter files name nodes that no longer exist. Category Edit is in the catalog and carries the same filterPath, but its layout reaches none of those ids. So this is parity landed before it is needed: the day ProdRestrictEdit joins LexiconFeatureCatalog, the view already withholds what legacy withholds instead of growing five rows nobody looks for. Tested at the mechanism, not the wiring. Suppressing the composer gate fails the filter tests with the full row list -- Name, Abbreviation, Description, Status, Discussion, Confidence, Researchers, Restrictions -- and the importer tests pin the id. Reading filterPath in RecordEditView has NO test and cannot be checked in the app while the tool renders legacy; it becomes verifiable, by Jason's acceptance steps, when that tool is activated. FwAvaloniaTests 751 passed, xWorksTests filter Avalonia 1645 passed, 0 failed. * LT-22802: Cover the step that reads a tool's filter list The filter-list read was the one part with no test, and the part that could quietly make the whole feature a no-op: every other test in this area supplies the id set by hand, so a broken read leaves them all green. Demonstrated rather than assumed. Pointing the XPath at the wrong element name fails only the new test; the three composer tests still pass. The read moves to an internal static taking the tool's configuration node, so it is reachable without a mediator or property table -- the same way the reference-vector menu entry points are tested. The property keeps only its memoization. One test drives the whole chain against a shipped filter file: the attribute name, the path resolution, the XPath and the id attribute. It asserts the ids are PRESENT rather than exhaustive, so editing that file leaves it alone while breaking any link in the chain does not. xWorksTests filter Avalonia 1650 passed, 0 failed. * Merge remote-tracking branch 'origin/main' into LT-22802-slice-filter-lists ViewNode gained an optional parameter on each side -- sliceId here, and helpTopicId on main -- with the same conflict at its one call site in the importer. Both are kept; they are independent. * Merge remote-tracking branch 'origin/main' into LT-22802-slice-filter-lists Main now has #1145, which this branch also carried as its first commit. Squash-merging gave it a different identity, so both files it touched conflicted with that commit's own later edits there. Resolved toward main, whose comments had been corrected, with one change: the relevance doc named LT-22802 as covering the other gate. This branch is that gate, so the doc now points at IsFilteredOutByTool, in the same class. The fixture summary keeps its pointer to DetailSliceFilterTests, where that gate is tested. * LT-22802: Rejoin a doc line the comment wrapper split The comment-hygiene wrapper broke the new pointer to IsFilteredOutByTool so that one word sat alone on the last line. Reflowed. Comments only. * Merge remote-tracking branch 'origin/main' into LT-22802-slice-filter-lists * LT-22802: Stop an override from shifting a node's help topic and id sliceId was inserted ahead of helpTopicId in ViewNode's constructor, when resolving a merge conflict between the two. Both override clone helpers pass that tail positionally, and both parameters are strings, so n.HelpTopicId bound to sliceId and helpTopicId fell back to null -- with no warning. Any layout with a Field Visibility or Move Field override lost every authored help topic, and each of its rows took the help id as its slice id. The helpers also never carried SliceId at all. Moving the parameter back alone would have restored help topics and left SliceId null after every override, so a tool's filter list would stop withholding rows in exactly the layouts users have customised. sliceId goes last again, and both helpers name the tail and pass both fields, so a parameter added ahead of them cannot shift either one. The existing test for fields an override must preserve now covers both, and a duplicated node is checked for them too. Rebuilding the merged code fails both tests exactly as reported: help topic null, slice id the help id. Dropping only the SliceId carry fails them on SliceId. Reported in review. * LT-22802: Carry a node's slice id through the canonical JSON The serializer wrote and read helpTopicID but had no slice id, so a layout loaded from canonical JSON would lose the id a tool's filter list names. No shipped path loads JSON today; this keeps that path from springing the same trap if it is enabled. Written as "sliceId", not "id": "id" is already the stable id's key, and sharing it would overwrite every node's stable id on the way out. The round-trip test that sets every property now sets helpTopicId and sliceId, which it had not been covering, and checks the two ids stay apart. Dropping the write fails it on SliceId. * LT-22802: Test that a tool's filter list reaches the composer The file reader and the composer's gate were each tested, but not the join between them: deleting the view's hiddenSliceIds argument left the whole suite green. No shipped tool reaches a live filter id today, so nothing else would have noticed. The view's composition moves out of ShowAvaloniaEntry into ComposeDetail, unchanged, so a test can reach it without the settle, adapter and message work around it. The test installs the shipped ProdRestrictEdit configuration, whose filter list is basicPlusFilter.xml, and composes a production restriction through the view. The same view with the filterPath removed is the control, and shows the Status row. Removing the argument fails the test, with Status among the composed rows. Reported in review. * LT-22802: Put each summary back on the member it describes Extracting ComposeDetail left it under ShowAvaloniaEntry's summary as well as its own, and ShowAvaloniaEntry with none. That summary is back on its method. The memoized filter list caches a failed read too, which reads like an oversight; the comment now says it is deliberate, since a missing filter file is an install fault and re-reading it would log the same failure for every record shown. The reading tests' summary argued for their own existence, and its claim that every other test supplies the id set by hand stopped being true with the wiring test. It now says what the fixture reads. Reported in review. Comments only. Co-Authored-By: Zachary Burnham <zachary_burnham@sil.org> Co-Authored-By: Claude Opus 5 <noreply@anthropic.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.
Adds the one composer test that was missing for the field-relevance gate, raised by
@jasonleenaylor after #1135 merged.
MoStemMsa.IsFieldRelevantwithholds Attaches to Categories (FromPartsOfSpeech)unless the owning entry has a proclitic or enclitic allomorph.
Morphology.fwlayoutdeclares that part
visibility="always", so before the relevance gate landed the rowcomposed on every stem MSA. That makes it the gate's most visible consequence, and it
was the only one of its rules with no test.
The test bites
It is Jason's test, run and falsified rather than taken on trust:
Expected: False, But was: TrueThe positive control adds a proclitic allomorph and changes nothing else, so the same
row on the same object is shown to appear as well as to vanish.
Also
Two comments claimed more than the code does, and are corrected:
IsIrrelevantForObjectdescribed itself as the whole ofSliceFilter.IncludeSlice.It implements one of that method's two gates.
_propsToMonitor, a field declared between it and the method itdescribes -- and which already carries its own comment where it is used. Moved onto
IsIrrelevantForObject.true only until LT-22802: Apply a tool's slice filter list in the Avalonia detail view #1146 lands it, and LT-22802: Apply a tool's slice filter list in the Avalonia detail view #1146 branched from a base without this comment,
so it would not have corrected the claim. Saying which gate this one is leaves
nothing to fall out of date, and the two can merge in either order.
LT-22802isstill named: a Jira id is the pointer that keeps working.
IsFieldRelevantoverrides,which was untrue while this test was missing. It now names the one limb still
uncovered: the
InflectionClasscase that withholds the row from a compound rule'sleft or right MSA.
Verification
No behaviour change -- one test and the comments around it.
Merged with
main(8 commits) and re-verified, since a clean merge is not necessarilya compiling one.
TestCategory!=DesktopRequired)build.ps1 -CommentHygiene -TokenHygienecleanAddAllomorphToEntryTest, passes on its own: the SLDRwriting-system repository intermittently cannot write its cache directory during
BackendProvider.EnsureWritingSystemsExist. Seen twice this week on unrelatedtests, including a CI run that went green on a re-run.
The
LexiconFirstSliceEditContextEdgeCaseTeststeardown error in that run ispre-existing and unrelated:
Validate_WhitespaceOnlyLexeme_IsAnErrorstages an edit andnever commits or cancels it, so the unit-of-work write lock is still held when the cache
is disposed.
git blameputs it in LT-22625 (#964). Nothing here touches that file.🤖 Generated with Claude Code
This change is