Make CanSyncRandomEntries more robust #2075
Conversation
- Runs against FwApi as well - Include moves and positional creates (components, example, complex-forms) - Round-trip data through each api so it's realistic
github-actions
bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR refactors test infrastructure by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
UI unit Tests 1 files 45 suites 18s ⏱️ Results for commit 21d0695. ♻️ This comment has been updated with latest results. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (1)
42-67: Fix invariant: ensure all RichSpans use the key writing system.Some spans come from AutoFaker and may have Ws ≠ writingSystemId, producing inconsistent RichMultiString states. This can leak cross‑WS spans into a single alternative and cause round‑trip surprises. Force Ws to writingSystemId for every span.
- else - { - spans.Add(context.AutoFaker.Generate<RichSpan>()); - } + else + { + var span = context.AutoFaker.Generate<RichSpan>(); + span.Ws = writingSystemId; // keep spans consistent with the key + spans.Add(span); + }Optionally also de‑dupe WS picks here as in MultiStringOverride.
🧹 Nitpick comments (4)
backend/FwLite/MiniLcm/Models/Entry.cs (1)
36-41: Consider aligning headword selection and trimming with LCM helpers.MiniLcm Entry.Headword() picks the first string ordered by Ws code and uses default Trim, while LcmHelpers prefers BestVernacularAlternative and trims with an extended whitespace set. For cross-API consistency, consider mirroring the selection/trim semantics here (or centralize in a shared helper).
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
24-31: Lexeme form path mirrors citation fix — LGTM.Null-safe access and trimming look right. Minor: consider caching entry.LexemeFormOA?.Form into a local to avoid double property access.
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (1)
18-24: Deduplicate selected writing systems to avoid silent overwrites.Random.ListItems may return duplicates; duplicates just overwrite in the dictionary and reduce variability. Apply Distinct() when selecting WS.
- var writingSystems = context.Faker.Random.ListItems(validWritingSystems, count); + var writingSystems = context.Faker.Random.ListItems(validWritingSystems, count) + .Distinct() + .ToArray();backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (1)
77-216: CanSyncRandomEntries: strong, realistic coverage; a couple of nits.
- Round-trip path first prepares both before/after — smart to avoid duplicate-related creation conflicts.
- Assertions account for current API gaps (headwords in CRDT; MorphType in FWData). Good.
Nits:
- When shuffling and ListItems’ing, duplicates can slip in; consider Distinct() to reduce flakiness.
- If headword semantics change (e.g., aligning MiniLcm with LCM helpers), revisit the exclusions around ComponentHeadword/ComplexFormHeadword for CRDT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs(3 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs(3 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(0 hunks)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.cs(2 hunks)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs(2 hunks)backend/FwLite/MiniLcm/Models/Entry.cs(1 hunks)
💤 Files with no reviewable changes (1)
- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
🧰 Additional context used
🧬 Code graph analysis (5)
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs (1)
CitationForm(84-90)
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs (2)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (12)
Task(48-53)Task(55-59)Task(67-76)Task(78-89)Task(91-99)Task(101-105)Task(107-115)Task(117-121)Task(132-136)Task(138-143)Task(145-152)Task(154-158)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (1)
Task(309-313)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (4)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
MultiString(796-818)RichMultiString(820-839)backend/LfClassicData/LfClassicMiniLcmApi.cs (2)
MultiString(347-357)RichMultiString(359-369)backend/FwLite/MiniLcm.Tests/WritingSystemCodes.cs (1)
WritingSystemCodes(5-8)backend/FwLite/MiniLcm/Models/RichMultiString.cs (4)
RichMultiString(13-15)RichMultiString(17-19)RichMultiString(27-37)RichMultiString(171-184)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.cs (3)
backend/FwLite/MiniLcm/Models/Entry.cs (2)
Entry(44-70)Headword(34-42)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (9)
Entry(643-672)ExampleSentence(780-794)Sense(763-778)IList(1204-1214)ComplexFormComponent(716-726)ComplexFormComponent(730-741)Order(743-761)ComplexFormType(489-492)Publication(352-361)backend/FwLite/MiniLcm/Models/Sense.cs (2)
Sense(7-54)Sense(38-53)
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (4)
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs (8)
ExtraWritingSystemsSyncFixture(13-44)SyncFixture(46-121)SyncFixture(60-60)SyncFixture(62-71)SyncFixture(73-75)Task(21-43)Task(77-105)Task(107-111)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
MorphType(88-117)backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (1)
EntrySync(8-327)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/AutoFakerDefault.cs (4)
AutoFakerDefault(10-82)Generate(72-80)Generate(88-91)Generate(103-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: frontend
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (9)
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (2)
16-21: Good switch to BestVernacularAlternative for citation form.This is safer than GetStringFromIndex(0) and matches user WS intent. Ensure BestVernacularAlternative is non-null when only non‑vernacular strings exist; current null‑aware handling looks correct.
33-37: Non-null return for LexEntryHeadwordOrUnknown — good API tightening.Returning UnknownHeadword eliminates nullable churn at call sites. Confirm all usages compiled cleanly after the signature change.
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (1)
11-17: Preinit pattern and WS guard look good.Nice use of pattern matching and explicit check against empty WS arrays.
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.cs (1)
21-61: Helper extraction reads well.Centralizing entry preparation reduces duplication and makes the tests clearer.
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs (2)
13-44: Extra writing systems fixture — nice enhancement.Creating ES/FR in FW and ensuring a default vernacular exists in CRDT unblocks entry queries and broadens coverage.
53-54: Minor fixture polish.DefaultVernacularWritingSystem constant and making InitializeAsync virtual improve reuse. Using DefaultVernacularWritingSystem in NewProject keeps setup consistent.
Also applies to: 77-97
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (3)
13-27: Fixture swap to ExtraWritingSystemsSyncFixture — LGTM.Keeps CRDT and FWData cases aligned and simplifies setup.
28-49: FwDataApiDoesNotUpdateMorphType test is clear and valuable.Good sentinel to catch future behavior changes.
68-76: Enum ApiType for round-trips — good clarity.Simple and self-documenting.
CanSyncRandomEntries()now:93cfc05(#2072). The changes in this PR would have picked those all up).LcmHelpers.LexEntryHeadwordneeded to be made "stable" by usingBestVernacularAlternativeinstead of the first string, which is not strictly related to the configured order of Writing-Systems.This only made the test consistently green, because the WSs created by the fixture are explicitly ordered alphabetically. fwdata respects the actual order while crdts depend on alphabetical order. Yeah, not so good.
We should probably be using
HeadWordinstead ofBestVernacularAlternative, but null handling in the case of empty ws-values should be uniform across fwdata and crdt. So,BestVernacularAlternativeseemed like a slightly more uniform choice for the time being.This PR exposed the fact that there are big holes in our current handling of headwords. So, I at least added a link to an issue in comments here and here.