Add various serialization and snapshot tests #2014
Conversation
|
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 📝 WalkthroughWalkthroughAdds Verify.SystemJson dependency; introduces BaseSerializationTest and extensive test data for serialization/deserialization and migration; refactors several tests, adds new snapshot tests, and updates fixtures to use a TestProject wrapper; makes SaveProjectSnapshot static; exposes MiniLcm.ProjectSnapshot.Empty and LcmCrdtKernel.AllObjectTypes; adjusts namespaces/usings; updates csproj references and a submodule. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
github-actions
bot
added
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
C# Unit Tests130 tests 130 ✅ 19s ⏱️ Results for commit 4d06e8f. ♻️ This comment has been updated with latest results. |
UI unit Tests 1 files 45 suites 29s ⏱️ Results for commit 4d06e8f. ♻️ 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
🧹 Nitpick comments (10)
backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs (1)
1-1: Verify that the System.Buffers import is necessary.The
System.Buffersimport doesn't appear to be used directly in this file. It may be inherited fromBaseSerializationTest, but if not actively referenced here, consider removing it to keep imports clean.backend/FwLite/FwLiteProjectSync.Tests/Fixtures/TestProject.cs (1)
7-13: Consider renaming_cleanupto align with public parameter conventions.The record consolidates test fixture components cleanly. However, the
_cleanupparameter uses a leading underscore typically reserved for private fields, while positional record parameters are public by convention. Consider renaming toCleanupfor consistency.Apply this diff if you'd like to align with naming conventions:
public record TestProject( CrdtMiniLcmApi CrdtApi, FwDataMiniLcmApi FwDataApi, CrdtProject CrdtProject, FwDataProject FwDataProject, - IServiceProvider Services, IDisposable _cleanup) : IDisposable + IServiceProvider Services, IDisposable Cleanup) : IDisposable { - public void Dispose() { _cleanup.Dispose(); } + public void Dispose() { Cleanup.Dispose(); } }backend/FwLite/LcmCrdt.Tests/Data/RegressionTestHelper.cs (1)
73-77: Consider adding documentation for RegressionVersion enum.The
RegressionVersionenum valuesv1andv2could benefit from XML documentation comments explaining what each version represents (e.g., which schema or migration state each corresponds to).backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs (1)
264-275: Ensure database connections are properly managed in BackupDatabase.The method opens the source connection if not already open but doesn't restore the original state. While this may be acceptable for test scenarios, consider whether leaving the connection open could affect other tests or cleanup.
Apply this diff to restore the original connection state:
private static void BackupDatabase(DbContext sourceContext, string destinationPath) { var source = (SqliteConnection)sourceContext.Database.GetDbConnection(); + var wasOpen = source.State == System.Data.ConnectionState.Open; if (source.State != System.Data.ConnectionState.Open) source.Open(); using var destination = new SqliteConnection($"Data Source={destinationPath}"); destination.Open(); source.BackupDatabase(destination); - source.Close(); + if (!wasOpen) + source.Close(); }backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs (2)
52-56: Address TODO: use TestJsonOptions instead.The TODO comments indicate that
TestJsonOptionsshould be used instead of creatingJsonSerializerOptionsfromCrdtConfig.JsonSerializerOptions. This suggests an incomplete refactor or a planned improvement.If
TestJsonOptionsexists elsewhere in the codebase, I can help refactor these usages. Would you like me to search for it and propose a refactor?Also applies to: 108-112
148-167: TakeProjectSnapshot helper duplicates existing API functionality.The private
TakeProjectSnapshotmethod manually constructs aProjectSnapshotby querying all entities. However, line 80 of the test shows thatapi.TakeProjectSnapshot()exists. Consider whether this helper is necessary or if the existing API method should be used instead.If the difference is that this version applies custom ordering for deterministic verification, consider documenting that rationale.
#!/bin/bash # Check if IMiniLcmApi.TakeProjectSnapshot exists and how it differs ast-grep --pattern $'interface IMiniLcmApi { $$$ TakeProjectSnapshot($$$) { $$$ } $$$ }' # Also check the implementation in CrdtMiniLcmApi ast-grep --pattern $'TakeProjectSnapshot($$$) { $$$ }'backend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.cs (2)
23-42: Consider using Path.Combine for cross-platform compatibility.Line 33 uses a hardcoded backslash in the path string, which may cause issues on non-Windows platforms:
File.Copy(RelativePath($"Snapshots\\{sourceSnapshotName}"), snapshotPath, overwrite: true);Consider using
Path.Combineinstead:-File.Copy(RelativePath($"Snapshots\\{sourceSnapshotName}"), snapshotPath, overwrite: true); +File.Copy(RelativePath(Path.Combine("Snapshots", sourceSnapshotName)), snapshotPath, overwrite: true);
63-69: Minor duplication with BaseSerializationTest.GetJsonFilePath.This
RelativePathhelper is functionally identical toGetJsonFilePathin the base class. Consider reusing the inherited method to reduce duplication:-private static string RelativePath(string name, [CallerFilePath] string sourceFile = "") -{ - return Path.Combine( - Path.GetDirectoryName(sourceFile) ?? - throw new InvalidOperationException("Could not get directory of source file"), - name); -} +// Remove this method and use GetJsonFilePath from BaseSerializationTest insteadbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationTests.cs (1)
93-173: Complex but well-structured regression data maintenance test.The three-step approach is logical:
- Validate legacy snapshot outputs are current
- Identify and migrate non-round-tripping "latest" snapshots to legacy
- Ensure all object types are represented
One minor observation: Lines 145-147 generate an additional snapshot when moving an item to legacy. The inline comment acknowledges this might be noisy. Consider monitoring whether this creates too many snapshots over time and remove if it becomes unwieldy.
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (1)
78-82: Consider defensive copying for Tags and ObjData.Unlike
RichSpan.Copy()(in RichString.cs:287-290) which clonesTagsandObjData, this code assigns them directly. If the minimal representation is intended to avoid shared references, consider copying these collections.Apply this diff if defensive copying is desired:
var minimalSpan = new RichSpan { Text = existing.Text, Ws = existing.Ws, - Tags = existing.Tags, + Tags = existing.Tags?.ToArray(), Bold = existing.Bold, FontSize = existing.FontSize, ForeColor = existing.ForeColor, - ObjData = existing.ObjData, + ObjData = existing.ObjData is null ? null : existing.ObjData with { }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
backend/Directory.Packages.props(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/.gitignore(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs(2 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/TestProject.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/FwLiteProjectSync.Tests.csproj(2 hunks)backend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs(5 hunks)backend/FwLite/FwLiteProjectSync.Tests/WritingSystemSyncTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txt(5 hunks)backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.legacy.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs(3 hunks)backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs(2 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.ChangeEntities.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.ProjectSnapshot.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.Snapshots.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.ChangeEntities.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.ProjectSnapshot.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.Snapshots.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/RegressionTestHelper.cs(3 hunks)backend/FwLite/LcmCrdt.Tests/Data/Scripts/generating-versions.MD(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/Scripts/v2.sql(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.legacy.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/VerifyRegeneratedSnapshotsAfterMigrationFromScriptedDb.v1.verified.json(1 hunks)backend/FwLite/LcmCrdt.Tests/Data/VerifyRegeneratedSnapshotsAfterMigrationFromScriptedDb.v2.verified.json(1 hunks)backend/FwLite/LcmCrdt.Tests/LcmCrdt.Tests.csproj(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs(1 hunks)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/AutoFakerDefault.cs(3 hunks)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs(1 hunks)backend/FwLite/MiniLcm/MiniLcm.csproj(1 hunks)backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs(1 hunks)backend/FwLite/MiniLcm/ProjectSnapshot.cs(2 hunks)backend/harmony(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-22T03:51:17.255Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1692
File: backend/FwLite/MiniLcm/Models/RichString.cs:62-238
Timestamp: 2025-05-22T03:51:17.255Z
Learning: In the RichMultiString implementation, the RichSpan record properly compares equality of all fields including styling attributes like Bold, not just the Text property, as verified by dedicated tests in RichMultiStringTests.cs.
Applied to files:
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs
🧬 Code graph analysis (13)
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (1)
backend/FwLite/FwDataMiniLcmBridge/FwDataProject.cs (1)
FwDataProject(13-27)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (2)
backend/FwLite/MiniLcm/Models/RichString.cs (1)
RichSpan(288-291)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/ObjectWithIdOverride.cs (1)
Generate(14-20)
backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs (2)
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (8)
CrdtFwdataProjectSyncService(16-155)Task(25-28)Task(30-50)Task(52-101)Task(119-125)Task(127-132)Task(134-140)SnapshotPath(142-147)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/TestProject.cs (1)
Dispose(12-12)
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (2)
backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs (3)
IEnumerable(16-46)IEnumerable(48-57)IEnumerable(59-65)backend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.cs (1)
IEnumerable(14-21)
backend/FwLite/LcmCrdt.Tests/Data/RegressionTestHelper.cs (1)
backend/FwLite/LcmCrdt.Tests/MiniLcmTests/QueryEntryTests.cs (1)
Task(17-22)
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs (3)
backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs (4)
Task(31-40)Task(42-46)Task(66-81)Task(84-89)backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (6)
Task(25-28)Task(30-50)Task(52-101)Task(119-125)Task(127-132)Task(134-140)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs (3)
Task(43-72)Task(74-78)Task(93-113)
backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationTests.cs (3)
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (5)
BaseSerializationTest(13-108)GetJsonFilePath(57-63)JsonArray(96-107)ToNormalizedIndentedJsonString(83-94)SerializeRegressionData(65-75)backend/FwLite/LcmCrdt/Objects/MiniLcmCrdtAdapter.cs (4)
IObjectBase(36-39)MiniLcmCrdtAdapter(8-60)MiniLcmCrdtAdapter(12-15)MiniLcmCrdtAdapter(56-59)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (1)
LcmCrdtKernel(36-292)
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (2)
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (2)
LcmCrdtKernel(36-292)ConfigureCrdt(139-271)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/AutoFakerDefault.cs (3)
AutoFakerConfig(11-48)AutoFakerDefault(7-64)SimpleOverride(66-74)
backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs (3)
backend/FwLite/LcmCrdt/Json.cs (1)
Json(12-206)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (12)
Task(49-54)Task(56-60)Task(68-80)Task(82-93)Task(95-103)Task(105-109)Task(111-119)Task(121-125)Task(136-140)Task(142-147)Task(149-156)Task(158-162)backend/LexData/Entities/CommitEntityConfiguration.cs (1)
Serialize(38-38)
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/TestProject.cs (1)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
CrdtMiniLcmApi(25-773)
backend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.cs (2)
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (1)
BaseSerializationTest(13-108)backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (8)
Task(25-28)Task(30-50)Task(52-101)Task(119-125)Task(127-132)Task(134-140)CrdtFwdataProjectSyncService(16-155)SnapshotPath(142-147)
backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs (3)
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (5)
BaseSerializationTest(13-108)GetJsonFilePath(57-63)JsonArray(96-107)ToNormalizedIndentedJsonString(83-94)SerializeRegressionData(65-75)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (3)
IEnumerable(273-278)IEnumerable(280-285)LcmCrdtKernel(36-292)backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs (1)
IEnumerable(125-228)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/AutoFakerDefault.cs (2)
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (1)
AutoFakerConfig(28-51)backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (4)
RichSpanOverride(65-87)Generate(10-23)Generate(30-62)Generate(69-86)
⏰ 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). (6)
- GitHub Check: Build API / publish-api
- 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 (58)
backend/FwLite/FwLiteProjectSync.Tests/.gitignore (1)
1-1: LGTM! Correctly un-ignores verified snapshot.The negation pattern correctly ensures that
sena-3-live.verified.sqliteis tracked by Git despite broader ignore rules. This is standard practice for snapshot testing with Verify, where verified snapshots serve as reference data for tests.backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs (1)
1-3: LGTM! Namespace consolidation is clean.The namespace change from
FwLiteProjectSynctoMiniLcmand the updated using statement are consistent with the broader namespace reorganization mentioned in the PR summary.backend/FwLite/MiniLcm/ProjectSnapshot.cs (2)
3-3: LGTM! Namespace consolidation is consistent.The namespace change from
FwLiteProjectSynctoMiniLcmaligns with the consolidation inMiniLcmApiExtensions.cs.
13-13: LGTM! Increased visibility supports test scenarios.Changing the
Emptyproperty frominternaltopublicmakes sense given the PR's focus on adding serialization and snapshot tests. This provides a convenient singleton for test initialization.backend/harmony (1)
1-1: Manual verification required: harmony submodule update (backend/harmony)
The commits 17334b30f2fd311bb551fb79e2cf740eac397c25 → 28bbb2cb3d7ccb91ff3fbd0cffcee1582b1854e8 couldn’t be fetched automatically due to missing refs. Please run:git submodule sync git submodule update --init --recursivethen confirm the new commit exists in the remote harmony repo and manually inspect its changes for compatibility and alignment with PR objectives.
backend/FwLite/FwLiteProjectSync.Tests/WritingSystemSyncTests.cs (1)
3-3: LGTM!The addition of the
using MiniLcm;directive is appropriate and enables direct usage ofWritingSystemId(line 30) without full qualification.backend/FwLite/MiniLcm/MiniLcm.csproj (1)
15-15: LGTM!The addition of
System.Linq.Asyncpackage reference is appropriate and follows the project's centralized package management pattern. This package enables asynchronous LINQ operations that complement the testing enhancements in this PR.backend/FwLite/LcmCrdt.Tests/Data/VerifyRegeneratedSnapshotsAfterMigrationFromScriptedDb.v1.verified.json (1)
1-431: LGTM!This verified JSON snapshot data is well-structured and provides comprehensive test coverage for migration scenarios. The use of
Guid_Nplaceholders ensures deterministic test execution, and the data includes a representative sample of all major entity types (ComplexFormType, Entry, ExampleSentence, PartOfSpeech, SemanticDomain, Sense, WritingSystem).backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.Snapshots.verified.txt (1)
1-1728: LGTM!This v2 migration snapshot data is comprehensive and well-structured. The inclusion of both root and non-root snapshots (IsRoot field) appropriately represents CRDT change history, and the
DeletedAtfield presence indicates support for soft-delete semantics. The data covers all major entity types and their relationships through theReferencesarrays.backend/FwLite/LcmCrdt.Tests/Data/VerifyRegeneratedSnapshotsAfterMigrationFromScriptedDb.v2.verified.json (1)
1-1065: LGTM!This v2 regenerated snapshot data complements the other migration test fixtures and uses
Guid_Nplaceholders for deterministic test execution. The data structure is consistent with the v2 migration format, includingDeletedAtandMaybeIdfields, and appropriately represents both root and non-root entity states for comprehensive migration testing.backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txt (1)
1-1461: LGTM! Verified regression test data.This is a static JSON data file containing serialized test fixtures for snapshot deserialization regression tests. The structure appears valid and aligns with the MiniLcm domain model entities (Entry, Sense, SemanticDomain, WritingSystem, etc.).
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (5)
15-27: LGTM! Proper lazy initialization of JSON serializer options.The lazy initialization pattern is appropriate for the expensive CrdtConfig setup, and exposing both standard and indented options is useful for test scenarios. Setting
ReadCommentHandling.Skipallows test data files to include comments.
28-55: LGTM! AutoFaker configuration aligns with snapshot serialization requirements.The configuration appropriately empties collections (Senses, ComplexForms, Components, ExampleSentences) that aren't persisted in snapshots, preventing generation of nested data that would be handled separately. The use of
repeatCount: 1andminimalRichSpans: trueis suitable for test data generation.
57-63: LGTM! CallerFilePath usage is appropriate for test file paths.The method correctly uses
[CallerFilePath]to build paths relative to the test file location, with proper null checking and error handling.
65-94: LGTM! Proper normalization for DateTimeOffset serialization.Both serialization methods correctly apply normalization for DateTimeOffset encoding (replacing
\u002Bwith+). The comments clearly explain the discrepancy between the standard CRDT serializer andJsonSerializer.Serialize. The use ofUtf8JsonWriterwithArrayBufferWriterinToNormalizedIndentedJsonStringis efficient and properly disposed.
96-107: LGTM! Robust JSON array reading with appropriate defaults.The method handles missing files gracefully by returning an empty array, uses forgiving parse options (trailing commas, comment handling) suitable for test data, and properly disposes the file stream. The null check ensures parsing succeeded before calling
AsArray().backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txt (1)
1-501: LGTM! Verified regression data updated to Spans-based format.The changes systematically convert fields (LiteralMeaning, Note, Sentence, Translation, Reference, Definition) from flat strings to Spans-based representations with Text and Ws properties. This aligns with the RichMultiString model and the broader serialization test infrastructure added in this PR. The addition of new change entries (CreateEntryChange with MorphType, AddEntryComponentChange) expands test coverage.
backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs (5)
16-46: LGTM! Well-structured change generation with appropriate special cases.The method correctly handles:
SetComplexFormComponentChangevia factory methods (no public constructor)- Generic
JsonPatchChange<T>via reflection- Other changes via AutoFaker with helpful error context
The assertion ensures all paths produce valid
IChangeinstances.
48-65: LGTM! Clean enumeration of all change types.The methods appropriately delegate to
LcmCrdtKernel.AllChangeTypes()andGeneratedChangesForType, ensuring comprehensive coverage.
101-121: LGTM! Clear separation of latest vs. legacy regression data.The test correctly validates that all change types are represented in the latest regression data file, with helpful comments explaining the update mechanism.
123-141: LGTM! Well-designed legacy data validation.The
LegacyChangeRecordstructure clearly pairs input/output, and the test appropriately validates both fields are present and non-null. The comments explain the migration strategy from latest to legacy.
143-227: LGTM! Comprehensive regression data maintenance logic.This test orchestrates a sophisticated three-step process:
- Re-validates legacy change round-trip outputs
- Detects when "latest" changes no longer round-trip stably and migrates them to legacy
- Ensures all change types are represented
The automatic generation of additional changes when migrating to legacy (lines 195-199) is well-justified by the comment—it's pragmatic to over-generate and let developers prune rather than require manual insertion.
The use of
Verifyfor snapshot assertions makes the test maintainable.backend/FwLite/LcmCrdt.Tests/Data/Scripts/generating-versions.MD (1)
1-6: LGTM! Clear and helpful documentation.The documentation effectively explains how the SQL test fixtures were generated. The example command is correct and provides a clear template for regenerating or creating similar files.
backend/FwLite/LcmCrdt.Tests/LcmCrdt.Tests.csproj (1)
15-15: LGTM! Appropriate addition for serialization testing.The
Verify.SystemJsonpackage supports the new serialization and snapshot tests added in this PR, complementing the existingVerify.Xunitinfrastructure.backend/Directory.Packages.props (1)
126-126: LGTM! Package addition supports JSON verification tests.The addition of Verify.SystemJson 1.4.1 enables System.Text.Json-based verification in tests, aligning with the PR's objective to add serialization and snapshot tests.
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (1)
280-285: LGTM! Consistent API expansion for test infrastructure.The new
AllObjectTypes()method mirrors the existingAllChangeTypes()pattern and provides a clean way to expose the configured object types for test validation.backend/FwLite/FwLiteProjectSync.Tests/FwLiteProjectSync.Tests.csproj (1)
38-38: LGTM! Project reference addition enables MiniLcm test scenarios.The addition of the MiniLcm project reference supports the expanded test infrastructure for project synchronization scenarios.
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (1)
134-134: LGTM! Making method static simplifies test usage.The method doesn't rely on instance state, so making it static is appropriate and enables cleaner test code that doesn't require instantiating the service.
backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.Snapshots.verified.txt (1)
1-479: LGTM! Well-structured test data.This verified snapshot test data is well-formed with consistent structure across all entity types. The data provides comprehensive coverage of migration scenarios including entities, references, and nested structures.
backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.ProjectSnapshot.verified.txt (1)
1-723: LGTM! Comprehensive project snapshot test data.This v2 project snapshot fixture provides rich test data with multiple entries, writing systems, semantic domains, and complex relationships. The data structure is consistent and suitable for migration and serialization testing.
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs (3)
37-37: LGTM! Return type simplification improves clarity.The change from returning a tuple to returning a TestProject wrapper improves encapsulation and makes the API cleaner. The TestProject type properly aggregates all necessary components (CrdtApi, FwDataApi, projects, services, cleanup).
52-53: LGTM! Path resolution using ProjectFolder property.Using
fwDataProject.ProjectFolderdirectly is cleaner than manually constructing the path. This change relies on the FwDataProject class to provide the correct folder path, which is a better encapsulation of path logic.
59-59: LGTM! TestProject instantiation includes all required components.The TestProject constructor receives all necessary components: both APIs, both projects, services, and cleanup handler. This matches the usage patterns shown in Sena3SyncTests where consumers access individual properties like
_project.CrdtApi.backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.ChangeEntities.verified.txt (1)
1-299: LGTM! Comprehensive change entity test data.This v1 change entities fixture provides thorough coverage of different change types and entity mutations. The hierarchical semantic domain structure and variety of change operations make this suitable for testing migration scenarios.
backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.legacy.verified.txt (1)
1-1057: LGTM! Thorough legacy deserialization regression data.This legacy regression data comprehensively tests deserialization transformations including:
- Removal of legacy
DbObjectfields- Normalization of simple strings to
MultiStringwithSpans- Addition of default values for new fields
- String-to-null conversions for references
The Input/Output pairs provide clear expected behavior for backward compatibility testing.
backend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.ProjectSnapshot.verified.txt (1)
166-251: Verify duplicate WsId "en" for Analysis and Vernacular writing systems.Both Analysis and Vernacular writing systems use
WsId: "en"(lines 171 and 213), but have different Ids and Type values (1 for Analysis, 0 for Vernacular). While this may be intentional for testing scenarios where the same language is used for both purposes, confirm this is the expected test data configuration and won't cause issues in code that expects unique WsIds.backend/FwLite/LcmCrdt.Tests/Data/RegressionTestHelper.cs (2)
26-29: LGTM! PRAGMA fix for virtual tables.The addition of
PRAGMA writable_schema=RESET;addresses the SQLite virtual table initialization issue. The comment references the SQLite forum discussion, which provides good context for this workaround.
40-43: Verify default RegressionVersion.v2 is correct
All parameterless calls toInitializeAsync()(e.g., in backend/Testing//* and FwLite/LcmCrdt.Tests//*) now default to v2 instead of the previous v1. Confirm no existing tests depend on v1 script behavior—either update those tests to specify v1 explicitly or adjust their expectations.backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs (3)
80-80: LGTM! Updated to use static SaveProjectSnapshot.The call correctly reflects the signature change of
SaveProjectSnapshotfrom instance to static method inCrdtFwdataProjectSyncService.
191-254: Well-designed live sync test with comprehensive verification.This test provides valuable regression protection by maintaining a "live" database and snapshot in the repository. The atomic verification pattern (lines 246-251) ensures both artifacts are updated together, which is excellent for catching serialization changes.
The exception-handling pattern (lines 222-236) using a captured
verifyExceptionis unconventional but functional—it allows the test to complete all actions before asserting, which is useful for generating the backup database even when verification fails.
204-213: Test data files present. Verified that bothsena-3-live.verified.sqliteandsena-3-live_snapshot.verified.txtexist inbackend/FwLite/FwLiteProjectSync.Tests/. No further action required.backend/FwLite/LcmCrdt.Tests/Data/MigrationTests.cs (2)
17-20: InitializeAsync pattern change needs verification.The method changed from
async Taskto non-asyncTaskreturningTask.CompletedTask. Since_helper.DisposeAsync()is still called inDisposeAsync(), verify that initialization is now deferred until the actual test methods call_helper.InitializeAsync(regressionVersion). This changes the test lifecycle pattern from "initialize once per test class" to "initialize per test method with specific version".This is likely intentional for the parameterized theory tests, but confirm this doesn't break any test isolation or setup assumptions.
169-196: Clever no-op commit pattern for snapshot regeneration.The
NewNoOpCommithelper creates a commit that both creates and immediately deletes an entity, resulting in a no-op from a data perspective while triggering snapshot regeneration. The comment correctly notes this is "slightly hacky" (line 118).The excluded entity ID check on line 128 (
s.EntityId != noOpEntityId) ensures the no-op entry doesn't pollute the verification results.backend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.cs (4)
1-10: LGTM!The imports and class declaration are appropriate. The test class correctly inherits from
BaseSerializationTestto leverage shared serialization test infrastructure.
11-21: LGTM!The test data enumeration methods are well-structured. The separation between
GetSena3SnapshotNames()(for xUnit theory) andGetSena3SnapshotPaths()(for actual file discovery) is clean and maintainable.
44-61: LGTM!The round-trip test correctly validates that the latest snapshot serializes and deserializes consistently. The use of
OrderDescending()on file paths assumes a lexicographic ordering convention for snapshot versioning, which appears intentional.
71-91: LGTM!The round-trip helper correctly:
- Sets up a test project with the snapshot
- Loads and saves the snapshot through the production code paths
- Re-parses and normalizes the JSON for comparison
The use of
AllowTrailingCommasduring re-parsing ensures flexibility in the input format.backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationTests.cs (5)
1-10: LGTM!The class now correctly inherits from
BaseSerializationTestto leverage shared serialization options and helpers. The new imports support JSON node manipulation and scoped assertions used in the new tests.
12-38: LGTM!The
GenerateSnapshotForTypehelper correctly handles bothIObjectWithId(wrapping inMiniLcmCrdtAdapter) andIObjectBasetypes. The exception handling provides clear diagnostics when generation fails.
40-48: LGTM!The renamed test method more clearly indicates it's testing project dump data specifically. The assertions ensure the regression data deserializes correctly without null entries.
50-71: LGTM!The test ensures comprehensive coverage by validating that all types from
LcmCrdtKernel.AllObjectTypes()are represented in the regression data. UsingAssertionScopeallows all missing types to be reported in a single test failure, improving developer experience.
73-91: LGTM!The
LegacySnapshotRecordtype clearly captures the input/output structure for legacy snapshots. The test validates that both fields are present and non-null for each record, ensuring the legacy data format is maintained.backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (1)
65-87: LGTM! Clean implementation of minimal RichSpan generation.The implementation correctly uses
Preinitialize = trueto post-process the auto-generated instance and creates a streamlined representation with essential fields for testing.backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/AutoFakerDefault.cs (3)
11-11: LGTM! Well-designed parameter addition.The
minimalRichSpansparameter with a sensible default offalsemaintains backward compatibility while enabling minimal RichSpan generation for snapshot tests.
20-20: LGTM! Correct integration with RichSpanOverride.The
minimalRichSpansparameter is properly threaded through to theRichSpanOverrideconstructor, enabling the minimal representation feature when requested.
66-74: LGTM! Good refactor to promote reusability.Elevating
SimpleOverride<T>to a public top-level class enables external consumers likeBaseSerializationTestto use this utility for custom override logic. The implementation remains clean and focused.backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.legacy.verified.txt (2)
1-224: LGTM! Well-structured regression test data.The first four test cases comprehensively cover legacy-to-current format migrations:
- String-to-Spans transformations for multilingual fields
- Addition of default values (MorphType, Order)
- Preservation of complex nested structures (SemanticDomains)
225-246: Confirm regression test correctness.The private
[JsonConstructor]on CreateExampleSentenceChange only bindsentityIdandsenseId; all other properties default to zero/null when deserializing legacy JSON with lowercase field names—this test accurately captures the intended legacy behavior.
myieye
force-pushed
the
add-various-serialization-and-snapshot-tests
branch
from
ec73615 to
96636a4
Compare
myieye
force-pushed
the
add-various-serialization-and-snapshot-tests
branch
from
96636a4 to
4d06e8f
Compare
This was initially added in #2000.
It introduces a lot of snapshot testing.
So, I want this to get merged first so that Git history shows how #2000 causes all of the snapshot validation to kick in.
A lot of this code was already reviewed in #2000.