Add comprehensive tests for ComplexFormComponent creation#2204
Draft
Add comprehensive tests for ComplexFormComponent creation#2204
Conversation
Tests cover: - Reusing a Harmony entity ID for a different component relationship throws - Changing a property ID (e.g. ComponentEntryId) produces a new entity ID - Idempotent create returns existing entity's ID, not newly provided one - BetweenPosition works with components lacking CRDT IDs (as from LibLCM) Remove the corresponding TODO comments from CrdtMiniLcmApi.cs. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
Remove ChangingPropertyId_ProducesNewEntityId (trivially implied by the idempotent create test's contrapositive). Strip comments that restate what the code already shows. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
github-actions
bot
added
the
💻 FW Lite
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
- ReusingEntityId: Harmony doesn't throw on duplicate entity IDs (no-op), so the test documents the real pitfall — mutating a property without resetting the ID returns the original entity with stale properties. This is what EntrySync guards against with `after.Id = Guid.NewGuid()`. - Create_WithoutPresetId: documents the LibLCM path where MaybeId is null and the system assigns a new entity ID. - BetweenComponents: improved variable naming for readability. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
…tests - CrdtMiniLcmApi: Always assign a new Guid for the entity ID, matching FwData behavior and eliminating the Harmony duplicate-ID pitfall that the sync code previously had to work around. - Base class: Add CreateComplexFormComponent_ChangingPropertyAndCreatingAgain_CreatesBoth to verify that mutating a property and creating again produces two distinct components (the sync diff's remove+add pattern). - CRDT subclass: Replace obsolete ReusingEntityId test with Create_AlwaysAssignsNewEntityId verifying the provided ID is never used. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
CrdtMiniLcmApi.CreateComplexFormComponent: - Add throw when caller provides an entity ID matching an existing component — this documents that the ID is internal and should never be reused (Harmony would silently no-op the duplicate). - Add explanatory comments on the Guid.NewGuid() assignment. Tests reorganized to cover all 4 original TODO comments: 1. Create_WithExistingEntityId_Throws (CRDT) — reusing an already-created component throws, documenting that entity IDs are internal. 2. Create_ChangingProperty_ProducesNewEntityId (CRDT) — mutating a property and creating again produces distinct Harmony entities (the sync diff remove+add pattern). 3. Create_AlwaysAssignsNewEntityId (CRDT) — the provided ID is never used. 4. CreateComplexFormComponent_WithBetweenComponentsLackingIds_PositionsCorrectly (base) — BetweenPosition anchors without entity IDs are resolved by property lookup, as they'd arrive from LibLCM. Also moves BetweenPosition test from CRDT subclass to base (FwData supports between positioning too). https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
- Remove redundant `is not null` guard — existing.MaybeId is always non-null in CRDT, so equality alone suffices. - Add Create_DuplicateByProperties_WithDifferentEntityId_ReturnsExisting to document that two calls with the same properties but different entity IDs (the normal case) are idempotent. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
Sync can be interrupted and replayed, so the same object (with the same entity ID) may be passed to CreateComplexFormComponent again. Instead of throwing, we rely on the property-based match to find and return the existing component — same as any other duplicate-by-properties call. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
Order is [MiniLcmInternal] and its representation differs between implementations (FwData uses sequential ints, CRDT uses doubles). Asserting on raw Order values breaks in FwData because inserting between A and B shifts B's Order, making the stale captured value incorrect. Instead, read the entry back and verify the component sequence — this tests the actual observable behavior. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
- Remove unused createdA, createdB, insertedBetween variables - Use Equal instead of ContainInOrder for exact sequence assertion https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
EntrySync's ComplexFormsDiffApi.Add and ComplexFormComponentsDiffApi.Add both set after.Id = Guid.NewGuid() before calling CreateComplexFormComponent. This is dead code: CRDT always generates its own ID, and FwData ignores the ID entirely. Remove both assignments and their comments. Also remove Create_WithExistingEntityId_IsIdempotent from the CRDT tests — it duplicates CreateComplexFormComponent_UsingTheSameComponentWithNullSenseDoesNothing in the shared base class (both test: create, create again → get same thing back). https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
Documents that passing the exact returned object back (same internal entity ID) is idempotent — a distinct scenario from creating with fresh FromEntries inputs, since sync interruptions can replay the same object. https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f
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
2 participants
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.
Summary
This PR adds three new test cases to validate the behavior of
ComplexFormComponentcreation in the CRDT system, and cleans up related code comments.Key Changes
Added three new test methods to
ComplexFormComponentTests:ReusingEntityId_ForDifferentComponent_Throws: Validates that reusing the same entity ID for different components throws an exceptionIdempotentCreate_ReturnsExistingId_NotNewlyProvidedId: Verifies that creating a duplicate component returns the existing ID rather than the newly provided oneCreate_WithBetweenComponentsLackingIds_PositionsCorrectly: Tests that components without CRDT entity IDs (from LibLCM) are correctly positioned using property-based lookupAdded helper methods to support the new tests:
CreateEntry(string headword): Creates a new entry with the given headwordGetEntry(Guid id): Retrieves an entry by IDComponentWithoutId(Guid, Guid): Creates a component without a CRDT entity ID to simulate LibLCM dataCleaned up code:
CrdtMiniLcmApi.CreateComplexFormComponentthat documented these test casesNewApi()method to directly return the fixture APINotable Implementation Details
The tests validate important edge cases in the component creation logic:
https://claude.ai/code/session_01Dz9V2fkKzPgAkSpFP2NE5f