From 8761f867acdd777f95cf6a1e4fbe633756c9475a Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 16:12:48 +0700 Subject: [PATCH 1/7] Cover change converter and config/adapter guard paths Adds coverage for the bespoke IChange converter and the config/adapter guard branches, which were largely untested: - PeekThenConcreteChangeConverter malformed-input throws (non-object root, empty object, non-string $type) and OpaqueChange EntityId parsing (present, missing-defaults-to-empty, nested-json round-trip). - OpaqueChange runtime contract: EntityType and NewEntity throw, ApplyChange is a no-op. - ChangeTypeListBuilder duplicate-add idempotency and frozen guard. - ObjectTypeListBuilder frozen guard, DefaultAdapter singleton reuse, and Adapt dispatch (success, non-IObjectBase throw, no-provider-matches throw). - DerivedTypeHelper duplicate-type throw and wrong-base throw. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 100 ++++++++++++++++++ src/SIL.Harmony.Tests/ConfigTests.cs | 80 ++++++++++++++ .../Helpers/DerivedTypeHelperTests.cs | 38 +++++++ 3 files changed, 218 insertions(+) create mode 100644 src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index 039252b..929623b 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -106,4 +106,104 @@ public void Requires_type_as_first_property() var act = () => JsonSerializer.Deserialize(json, options); act.Should().Throw().WithMessage("*first property*"); } + + [Fact] + public void Deserialize_NonObjectToken_ThrowsExpectedStartObject() + { + var options = SampleOptions(); + + var act = () => JsonSerializer.Deserialize("[1,2,3]", options); + act.Should().Throw().WithMessage("*StartObject*"); + } + + [Fact] + public void Deserialize_EmptyObject_ThrowsExpectedPropertyName() + { + var options = SampleOptions(); + + var act = () => JsonSerializer.Deserialize("{}", options); + act.Should().Throw().WithMessage("*property name*"); + } + + [Fact] + public void Deserialize_NonStringDiscriminator_Throws() + { + var options = SampleOptions(); + var json = """{"$type":5,"EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}"""; + + var act = () => JsonSerializer.Deserialize(json, options); + act.Should().Throw().WithMessage("*string*discriminator*"); + } + + [Fact] + public void OpaqueChange_ParsesEntityIdFromRawJson() + { + var options = SampleOptions(); + var entityId = Guid.Parse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); + var json = $$"""{"$type":"SetWordPriorityChange","EntityId":"{{entityId}}","Priority":7}"""; + + var change = JsonSerializer.Deserialize(json, options); + + change.Should().BeOfType().Which.EntityId.Should().Be(entityId); + } + + [Fact] + public void OpaqueChange_MissingEntityId_DefaultsToEmpty() + { + var options = SampleOptions(); + var json = """{"$type":"SetWordPriorityChange","Priority":7}"""; + + var change = JsonSerializer.Deserialize(json, options); + + change.Should().BeOfType().Which.EntityId.Should().Be(Guid.Empty); + } + + [Fact] + public void OpaqueChange_PreservesNestedJson_OnRoundTrip() + { + var options = SampleOptions(); + var json = """ + {"$type":"SetWordPriorityChange","EntityId":"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee","Data":{"nested":[1,2],"flag":true}} + """; + + var change = JsonSerializer.Deserialize(json, options)!; + var opaque = change.Should().BeOfType().Subject; + opaque.RawJson.GetProperty("Data").GetProperty("flag").GetBoolean().Should().BeTrue(); + + var rewritten = JsonSerializer.Serialize(change, options); + rewritten.Should().Contain("\"nested\":[1,2]"); + rewritten.Should().Contain("\"flag\":true"); + } + + private static OpaqueChange NewOpaque() => new() + { + TypeName = "UnknownChange", + RawJson = JsonDocument.Parse("{}").RootElement.Clone(), + }; + + [Fact] + public void OpaqueChange_EntityType_Throws() + { + var act = () => NewOpaque().EntityType; + act.Should().Throw(); + } + + [Fact] + public async Task OpaqueChange_NewEntity_Throws() + { + var commit = new Commit + { + ClientId = Guid.NewGuid(), + HybridDateTime = new HybridDateTime(DateTimeOffset.UtcNow, 0), + }; + var act = async () => await NewOpaque().NewEntity(commit, null!); + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task OpaqueChange_ApplyChange_IsNoOp() + { + //an unknown change applied via sync must not mutate anything or throw + await NewOpaque().ApplyChange(null!, null!); + } } diff --git a/src/SIL.Harmony.Tests/ConfigTests.cs b/src/SIL.Harmony.Tests/ConfigTests.cs index d9608b7..df0ef48 100644 --- a/src/SIL.Harmony.Tests/ConfigTests.cs +++ b/src/SIL.Harmony.Tests/ConfigTests.cs @@ -69,4 +69,84 @@ public void ConfigureJsonOptions_throws_after_freeze() act.Should().Throw() .WithMessage("*JsonOptionsBuilder* frozen*"); } + + [Fact] + public void ChangeTypeListBuilder_DuplicateAdd_IsIdempotent() + { + var config = new HarmonyConfig(); + config.ChangeTypeListBuilder.Add(); + config.ChangeTypeListBuilder.Add(); + + config.ChangeTypes.Should().ContainSingle(t => t.Type == typeof(SetWordTextChange)); + } + + [Fact] + public void ChangeTypeListBuilder_AddAfterFreeze_Throws() + { + var config = new HarmonyConfig(); + //building the serializer options freezes the change type builder + _ = config.JsonSerializerOptions; + + var act = () => config.ChangeTypeListBuilder.Add(); + + act.Should().Throw().WithMessage("*ChangeTypeListBuilder*frozen*"); + } + + [Fact] + public void ObjectTypeListBuilder_AddAfterFreeze_Throws() + { + var config = new HarmonyConfig(); + config.ObjectTypeListBuilder.DefaultAdapter().Add(); + config.ObjectTypeListBuilder.Freeze(); //happens during EF model build in a real setup + + var act = () => config.ObjectTypeListBuilder.DefaultAdapter(); + + act.Should().Throw().WithMessage("*ObjectTypeListBuilder*frozen*"); + } + + [Fact] + public void DefaultAdapter_CalledTwice_ReturnsSameInstance() + { + var config = new HarmonyConfig(); + var first = config.ObjectTypeListBuilder.DefaultAdapter(); + var second = config.ObjectTypeListBuilder.DefaultAdapter(); + + second.Should().BeSameAs(first); + } + + [Fact] + public void Adapt_ObjectImplementingIObjectBase_ReturnsIt() + { + var config = new HarmonyConfig(); + config.ObjectTypeListBuilder.DefaultAdapter().Add(); + var word = new Word { Id = Guid.NewGuid(), Text = "hello" }; + + config.ObjectTypeListBuilder.Adapt(word).Should().BeSameAs(word); + } + + [Fact] + public void Adapt_NonObjectBase_Throws() + { + var config = new HarmonyConfig(); + config.ObjectTypeListBuilder.DefaultAdapter().Add(); + + var act = () => config.ObjectTypeListBuilder.Adapt("not an entity"); + + act.Should().Throw().WithMessage("*does not implement*IObjectBase*"); + } + + [Fact] + public void Adapt_NoProviderMatches_Throws() + { + var config = new HarmonyConfig(); + //two providers, so Adapt takes the multi-provider dispatch path rather than the single-adapter fast path + config.ObjectTypeListBuilder.DefaultAdapter().Add(); + config.ObjectTypeListBuilder + .CustomAdapter() + .Add(); + + var act = () => config.ObjectTypeListBuilder.Adapt(new object()); + + act.Should().Throw().WithMessage("*Unable to adapt*"); + } } diff --git a/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs b/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs new file mode 100644 index 0000000..e2c9c0c --- /dev/null +++ b/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs @@ -0,0 +1,38 @@ +using System.Text.Json.Serialization.Metadata; +using SIL.Harmony.Entities; +using SIL.Harmony.Helpers; +using SIL.Harmony.Sample.Models; + +namespace SIL.Harmony.Tests.Helpers; + +public class DerivedTypeHelperTests +{ + [Fact] + public void AddDerivedType_Duplicate_Throws() + { + var types = new Dictionary>(); + types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word"); + + var act = () => types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word"); + + act.Should().Throw().WithMessage("*already added*"); + } + + [Fact] + public void AddDerivedType_DifferentTypesUnderSameBase_Succeeds() + { + var types = new Dictionary>(); + types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word"); + types.AddDerivedType(typeof(IObjectBase), typeof(Definition), "Definition"); + + types[typeof(IObjectBase)].Should().HaveCount(2); + } + + [Fact] + public void GetEntityDiscriminator_WhenInstanceTypeIsNotAssignableToBase_Throws() + { + var act = () => DerivedTypeHelper.GetEntityDiscriminator(typeof(string)); + + act.Should().Throw().WithMessage("*must implement IObjectBase*"); + } +} From 0a1014451fa1453d67435a09421eb8d56c55ddf1 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 16:45:27 +0700 Subject: [PATCH 2/7] Fix OpaqueChange_EntityType_Throws to invoke the getter reliably The lambda inferred Func, which FluentAssertions did not reliably invoke (passed in Debug, failed in Release CI with 'no exception thrown'). Use an explicit Action with a discard so the throwing getter always runs. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index 929623b..167c6b4 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -184,7 +184,8 @@ public void OpaqueChange_PreservesNestedJson_OnRoundTrip() [Fact] public void OpaqueChange_EntityType_Throws() { - var act = () => NewOpaque().EntityType; + var opaque = NewOpaque(); + Action act = () => _ = opaque.EntityType; act.Should().Throw(); } From 4fcd47e55af523665f3a1f34c899195fbeeb50d4 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 16:51:16 +0700 Subject: [PATCH 3/7] Use Assert.Throws for OpaqueChange.EntityType throwing getter The FluentAssertions delegate form did not reliably invoke the getter under the CI Release SDK (passed locally, failed in CI). Assert.Throws' Func overload returns and consumes the property value, guaranteeing the throwing getter runs. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index 167c6b4..8ff55be 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -185,8 +185,9 @@ public void OpaqueChange_PreservesNestedJson_OnRoundTrip() public void OpaqueChange_EntityType_Throws() { var opaque = NewOpaque(); - Action act = () => _ = opaque.EntityType; - act.Should().Throw(); + //Assert.Throws' Func overload returns and consumes the property value, so the throwing + //getter is reliably invoked (a discarded read can be optimized away under Release). + Assert.Throws(() => opaque.EntityType); } [Fact] From 83c9fee60d257e1516043c000cee3453e4036be7 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 16:53:58 +0700 Subject: [PATCH 4/7] Strengthen DerivedTypeHelper assertions per review - duplicate-registration test now uses a different discriminator for the second call, proving the guard rejects a duplicate DerivedType rather than a duplicate discriminator. - different-types test asserts the stored JsonDerivedType values instead of only the count. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs b/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs index e2c9c0c..e62d7d7 100644 --- a/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs +++ b/src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs @@ -13,7 +13,8 @@ public void AddDerivedType_Duplicate_Throws() var types = new Dictionary>(); types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word"); - var act = () => types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word"); + //different discriminator, same DerivedType: proves the guard rejects the duplicate type, not the discriminator + var act = () => types.AddDerivedType(typeof(IObjectBase), typeof(Word), "WordAgain"); act.Should().Throw().WithMessage("*already added*"); } @@ -25,7 +26,10 @@ public void AddDerivedType_DifferentTypesUnderSameBase_Succeeds() types.AddDerivedType(typeof(IObjectBase), typeof(Word), "Word"); types.AddDerivedType(typeof(IObjectBase), typeof(Definition), "Definition"); - types[typeof(IObjectBase)].Should().HaveCount(2); + types[typeof(IObjectBase)].Should().BeEquivalentTo([ + new JsonDerivedType(typeof(Word), "Word"), + new JsonDerivedType(typeof(Definition), "Definition"), + ]); } [Fact] From 135c15490fa7d047b91019ba8341e59e09ae2dfa Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 16:58:53 +0700 Subject: [PATCH 5/7] Access OpaqueChange.EntityType directly (no lambda) in throw test Every delegate-wrapped access of the throw-bodied getter (FluentAssertions Func/Action and Assert.Throws' Func overload) failed to observe the exception under the CI SDK. Access the getter directly in the method body inside a try/catch and consume the value via GC.KeepAlive so the call cannot be elided. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index 8ff55be..fdb62c7 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -185,9 +185,20 @@ public void OpaqueChange_PreservesNestedJson_OnRoundTrip() public void OpaqueChange_EntityType_Throws() { var opaque = NewOpaque(); - //Assert.Throws' Func overload returns and consumes the property value, so the throwing - //getter is reliably invoked (a discarded read can be optimized away under Release). - Assert.Throws(() => opaque.EntityType); + //Access the throw-bodied getter directly in the method body (no lambda) and consume the + //result via GC.KeepAlive, so the call cannot be elided and the exception is observed. + NotSupportedException? thrown = null; + try + { + var entityType = opaque.EntityType; + GC.KeepAlive(entityType); + } + catch (NotSupportedException e) + { + thrown = e; + } + + thrown.Should().NotBeNull(); } [Fact] From 5714fcfd6acceb879f0368080f9883bd652eaec8 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 7 Aug 2026 13:30:36 +0700 Subject: [PATCH 6/7] Merge main and align OpaqueChange.EntityType test with new contract main changed OpaqueChange.EntityType to return null (Type?) instead of throwing, and added UnknownChangeHandling. The branch was based on the older throwing behavior, which is why the getter test failed only in CI (CI builds the PR merged with main). Update the test to assert EntityType is null, matching the new contract. --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index a99b1c8..b4476c0 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -198,23 +198,10 @@ public void OpaqueChange_PreservesNestedJson_OnRoundTrip() }; [Fact] - public void OpaqueChange_EntityType_Throws() + public void OpaqueChange_EntityType_IsNull() { - var opaque = NewOpaque(); - //Access the throw-bodied getter directly in the method body (no lambda) and consume the - //result via GC.KeepAlive, so the call cannot be elided and the exception is observed. - NotSupportedException? thrown = null; - try - { - var entityType = opaque.EntityType; - GC.KeepAlive(entityType); - } - catch (NotSupportedException e) - { - thrown = e; - } - - thrown.Should().NotBeNull(); + //an unknown change has no known entity type on this client + NewOpaque().EntityType.Should().BeNull(); } [Fact] From d3a24f47c87d0c955c4f666aaa6721f1c06b096b Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 7 Aug 2026 14:17:21 +0700 Subject: [PATCH 7/7] Address Devin review: harden opaque EntityId parsing and fix guard tests Production: - PeekThenConcreteChangeConverter.ReadOpaque now uses TryGetGuid so a malformed (non-GUID) EntityId from a newer client defaults to empty instead of throwing FormatException, which had defeated the opaque fallback (flagged by Devin). Tests: - Add OpaqueChange_MalformedEntityId_DefaultsToEmpty covering that path. - ObjectTypeListBuilder_AddAfterFreeze_Throws now actually exercises Add() after freeze (it previously called DefaultAdapter, not matching its name); added a separate DefaultAdapterAfterFreeze test to keep that coverage. - SampleOptions disposes the ServiceProvider it builds. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/ChangeConverterTests.cs | 24 +++++++++++++++---- src/SIL.Harmony.Tests/ConfigTests.cs | 16 ++++++++++++- .../PeekThenConcreteChangeConverter.cs | 8 +++++-- 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/SIL.Harmony.Tests/ChangeConverterTests.cs b/src/SIL.Harmony.Tests/ChangeConverterTests.cs index b4476c0..c2d81a6 100644 --- a/src/SIL.Harmony.Tests/ChangeConverterTests.cs +++ b/src/SIL.Harmony.Tests/ChangeConverterTests.cs @@ -9,12 +9,16 @@ namespace SIL.Harmony.Tests; public class ChangeConverterTests { - private static JsonSerializerOptions SampleOptions(UnknownChangeHandling handling = UnknownChangeHandling.Fallback) => - new ServiceCollection() + private static JsonSerializerOptions SampleOptions(UnknownChangeHandling handling = UnknownChangeHandling.Fallback) + { + //dispose the provider once the options are built; JsonSerializerOptions is self-contained + //(the change converter holds its own type maps) so it stays valid after disposal + using var services = new ServiceCollection() .AddCrdtDataSample(":memory:") .Configure(c => c.UnknownChangeHandling = handling) - .BuildServiceProvider() - .GetRequiredService(); + .BuildServiceProvider(); + return services.GetRequiredService(); + } [Fact] public void Happy_path_deserializes_to_concrete_change() @@ -174,6 +178,18 @@ public void OpaqueChange_MissingEntityId_DefaultsToEmpty() change.Should().BeOfType().Which.EntityId.Should().Be(Guid.Empty); } + [Fact] + public void OpaqueChange_MalformedEntityId_DefaultsToEmpty() + { + var options = SampleOptions(); + //a newer client could send a non-GUID EntityId; the opaque fallback must not throw + var json = """{"$type":"SetWordPriorityChange","EntityId":"not-a-guid","Priority":7}"""; + + var change = JsonSerializer.Deserialize(json, options); + + change.Should().BeOfType().Which.EntityId.Should().Be(Guid.Empty); + } + [Fact] public void OpaqueChange_PreservesNestedJson_OnRoundTrip() { diff --git a/src/SIL.Harmony.Tests/ConfigTests.cs b/src/SIL.Harmony.Tests/ConfigTests.cs index df0ef48..c9423f9 100644 --- a/src/SIL.Harmony.Tests/ConfigTests.cs +++ b/src/SIL.Harmony.Tests/ConfigTests.cs @@ -96,9 +96,23 @@ public void ChangeTypeListBuilder_AddAfterFreeze_Throws() public void ObjectTypeListBuilder_AddAfterFreeze_Throws() { var config = new HarmonyConfig(); - config.ObjectTypeListBuilder.DefaultAdapter().Add(); + //capture the adapter before freezing so we can exercise Add (not DefaultAdapter) after freeze + var adapter = config.ObjectTypeListBuilder.DefaultAdapter(); + adapter.Add(); config.ObjectTypeListBuilder.Freeze(); //happens during EF model build in a real setup + var act = () => adapter.Add(); + + act.Should().Throw().WithMessage("*ObjectTypeListBuilder*frozen*"); + } + + [Fact] + public void ObjectTypeListBuilder_DefaultAdapterAfterFreeze_Throws() + { + var config = new HarmonyConfig(); + config.ObjectTypeListBuilder.DefaultAdapter().Add(); + config.ObjectTypeListBuilder.Freeze(); + var act = () => config.ObjectTypeListBuilder.DefaultAdapter(); act.Should().Throw().WithMessage("*ObjectTypeListBuilder*frozen*"); diff --git a/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs b/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs index 0c13bed..296c852 100644 --- a/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs +++ b/src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs @@ -82,8 +82,12 @@ private static OpaqueChange ReadOpaque(ref Utf8JsonReader reader, string typeNam return new OpaqueChange { TypeName = typeName, - EntityId = element.TryGetProperty(nameof(IChange.EntityId), out var id) && id.ValueKind == JsonValueKind.String - ? id.GetGuid() + //use TryGetGuid so a malformed (non-GUID) EntityId from a newer client defaults to empty + //instead of throwing FormatException, which would defeat the opaque fallback + EntityId = element.TryGetProperty(nameof(IChange.EntityId), out var id) + && id.ValueKind == JsonValueKind.String + && id.TryGetGuid(out var entityId) + ? entityId : default, RawJson = element };