From 0aa56f7f6a0c71b536e836442b9d01f8a1716f9e Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 15:59:03 +0700 Subject: [PATCH 1/4] Strengthen test hygiene and fix vacuous assertions Fixes tests that asserted less than they appeared to: - CanGetEmptyModelSnapshot asserted the Task was non-null (always true) and never awaited it; now awaits and asserts the empty-model shape. - Add missing assertions to smoke-only tests: DontAddTheSameSnapshotTwice, CanRecreateUniqueConstraintConflictingValueInOneCommit, CanCreate2EntriesOutOfOrder. - Turn DeleteStaleSnapshots_Works into a meaningful test of the skip-when-newer branch (renamed to DeleteStaleSnapshots_KeepsSnapshotsOlderThanTheCommit). - MultiThreading test now asserts each thread's entity converged to its final write instead of only checking that no exception was thrown. Co-Authored-By: Claude Opus 4.8 --- .../DataModelSimpleChanges.cs | 6 ++++- src/SIL.Harmony.Tests/ModelSnapshotTests.cs | 9 +++++-- src/SIL.Harmony.Tests/MultiThreadingTests.cs | 26 ++++++++++++++----- src/SIL.Harmony.Tests/RepositoryTests.cs | 12 +++++++-- src/SIL.Harmony.Tests/SnapshotTests.cs | 11 +++++++- 5 files changed, 52 insertions(+), 12 deletions(-) diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs b/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs index 39f9907..42a326a 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs @@ -177,7 +177,11 @@ public async Task CanTrackMultipleEntries() public async Task CanCreate2EntriesOutOfOrder() { var commit1 = await WriteNextChange(SetWord(_entity1Id, "entity1")); - await WriteChangeBefore(commit1, SetWord(_entity2Id, "entity2")); + var commit2 = await WriteChangeBefore(commit1, SetWord(_entity2Id, "entity2")); + + commit2.DateTime.Should().BeBefore(commit1.DateTime); + (await DataModel.GetLatest(_entity1Id))!.Text.Should().Be("entity1"); + (await DataModel.GetLatest(_entity2Id))!.Text.Should().Be("entity2"); } [Fact] diff --git a/src/SIL.Harmony.Tests/ModelSnapshotTests.cs b/src/SIL.Harmony.Tests/ModelSnapshotTests.cs index adc509f..951af8a 100644 --- a/src/SIL.Harmony.Tests/ModelSnapshotTests.cs +++ b/src/SIL.Harmony.Tests/ModelSnapshotTests.cs @@ -6,9 +6,14 @@ namespace SIL.Harmony.Tests; public class ModelSnapshotTests : DataModelTestBase { [Fact] - public void CanGetEmptyModelSnapshot() + public async Task CanGetEmptyModelSnapshot() { - DataModel.GetProjectSnapshot().Should().NotBeNull(); + var snapshot = await DataModel.GetProjectSnapshot(); + snapshot.Should().NotBeNull(); + snapshot.Snapshots.Should().BeEmpty(); + snapshot.LastChange.Should().BeNull(); + snapshot.LastCommitId.Should().BeNull(); + snapshot.LastCommitHash.Should().BeNull(); } [Fact] diff --git a/src/SIL.Harmony.Tests/MultiThreadingTests.cs b/src/SIL.Harmony.Tests/MultiThreadingTests.cs index b6d6855..f94ca70 100644 --- a/src/SIL.Harmony.Tests/MultiThreadingTests.cs +++ b/src/SIL.Harmony.Tests/MultiThreadingTests.cs @@ -1,24 +1,28 @@ using Microsoft.Data.Sqlite; +using SIL.Harmony.Sample.Models; namespace SIL.Harmony.Tests; public class MultiThreadingTests(ITestOutputHelper output) { private const string _connectionString = "Data Source=file:MultiThreadingTests.db?mode=memory&cache=shared"; - private static async Task Run(ITestOutputHelper output, + private const int _changesPerThread = 100; + + private static async Task<(Guid id, string lastValue, Exception? exception)> Run(ITestOutputHelper output, CancellationTokenSource cancellationTokenSource, bool debug) { return await Task.Run(() => { Exception? exception = null; + var id = Guid.NewGuid(); + var lastValue = ""; var t = new Thread(() => { var random = new Random(); var fixture = new DataModelTestBase(new SqliteConnection(_connectionString)); fixture.InitializeAsync().GetAwaiter().GetResult(); - var id = Guid.NewGuid(); - for (var i = 0; i < 100; i++) + for (var i = 0; i < _changesPerThread; i++) { var value = "test" + i; try @@ -26,6 +30,7 @@ public class MultiThreadingTests(ITestOutputHelper output) Thread.Sleep(random.Next(1, 10)); _ = fixture.WriteNextChange(fixture.SetWord(id, value)).Result; + lastValue = value; if (debug) output.WriteLine($"id: {id}, value:{value}"); if (cancellationTokenSource.IsCancellationRequested) return; @@ -41,7 +46,7 @@ public class MultiThreadingTests(ITestOutputHelper output) }); t.Start(); t.Join(); - return exception; + return (id, lastValue, exception); }); } @@ -49,7 +54,7 @@ public class MultiThreadingTests(ITestOutputHelper output) public async Task CanApplyChangesWithoutError() { //ensure the database is created before running the tests - _ = new DataModelTestBase(new SqliteConnection(_connectionString)); + var fixture = new DataModelTestBase(new SqliteConnection(_connectionString)); bool debug = false; var cancellationTokenSource = new CancellationTokenSource(); var results = await Task.WhenAll( @@ -57,10 +62,19 @@ public async Task CanApplyChangesWithoutError() Run(output, cancellationTokenSource, debug), Run(output, cancellationTokenSource, debug) ); - foreach (var exception in results) + foreach (var (_, _, exception) in results) { exception.Should().BeNull(); } + //every thread wrote to its own entity; assert the model converged to each thread's final write + //(a lost update or corruption that doesn't throw would otherwise pass unnoticed) + foreach (var (id, lastValue, _) in results) + { + lastValue.Should().Be("test" + (_changesPerThread - 1)); + var word = await fixture.DataModel.GetLatest(id); + word.Should().NotBeNull(); + word.Text.Should().Be(lastValue); + } } } diff --git a/src/SIL.Harmony.Tests/RepositoryTests.cs b/src/SIL.Harmony.Tests/RepositoryTests.cs index 45acfd8..27ec1c9 100644 --- a/src/SIL.Harmony.Tests/RepositoryTests.cs +++ b/src/SIL.Harmony.Tests/RepositoryTests.cs @@ -245,9 +245,17 @@ await _repository.AddSnapshots([ } [Fact] - public async Task DeleteStaleSnapshots_Works() + public async Task DeleteStaleSnapshots_KeepsSnapshotsOlderThanTheCommit() { - await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(1, 0))); + await _repository.AddSnapshots([ + Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(1, 0)), + Snapshot(Guid.NewGuid(), Guid.NewGuid(), Time(2, 0)), + ]); + + //the new commit is newer than every existing snapshot, so none are stale + await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(3, 0))); + + _crdtDbContext.Snapshots.Should().HaveCount(2); } [Fact] diff --git a/src/SIL.Harmony.Tests/SnapshotTests.cs b/src/SIL.Harmony.Tests/SnapshotTests.cs index b136841..617e213 100644 --- a/src/SIL.Harmony.Tests/SnapshotTests.cs +++ b/src/SIL.Harmony.Tests/SnapshotTests.cs @@ -88,18 +88,27 @@ await AddCommitsViaSync([ await WriteNextChange(SetWord(Guid.NewGuid(), "test 1"), add: false), await WriteNextChange(SetWord(entityId, "test 2"), add: false), ]); + + (await DataModel.GetLatest(entityId))!.Text.Should().Be("test 2"); + DbContext.Set().Should().HaveCount(2); } [Fact] public async Task CanRecreateUniqueConstraintConflictingValueInOneCommit() { var entityId = Guid.NewGuid(); + var recreatedTagId = Guid.NewGuid(); await WriteNextChange(SetTag(entityId, "tag-1")); await WriteNextChange( [ DeleteTag(entityId), - SetTag(Guid.NewGuid(), "tag-1"), + SetTag(recreatedTagId, "tag-1"), ]); + + (await DataModel.GetLatest(entityId))!.DeletedAt.Should().NotBeNull("the original tag was deleted"); + var recreatedTag = await DataModel.GetLatest(recreatedTagId); + recreatedTag!.Text.Should().Be("tag-1"); + recreatedTag.DeletedAt.Should().BeNull(); } [Fact] From 08e92f4698b1f579f306ca74b33ec7814fe6a737 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 7 Aug 2026 13:38:09 +0700 Subject: [PATCH 2/4] Dispose DataModelTestBase fixtures in MultiThreadingTests Per CodeRabbit review: the test created four DataModelTestBase instances (one in the test, one per worker thread) and never disposed them, leaking each DI ServiceProvider. Each worker thread now disposes its own fixture in a finally block, and the test-level fixture is an 'await using' so it is disposed after assertions. The test-level fixture keeps the shared in-memory SQLite database alive while the worker fixtures dispose. Also merges latest main (MTP runner migration) to keep the branch current. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/MultiThreadingTests.cs | 46 ++++++++++++-------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/src/SIL.Harmony.Tests/MultiThreadingTests.cs b/src/SIL.Harmony.Tests/MultiThreadingTests.cs index f94ca70..4dad848 100644 --- a/src/SIL.Harmony.Tests/MultiThreadingTests.cs +++ b/src/SIL.Harmony.Tests/MultiThreadingTests.cs @@ -21,28 +21,37 @@ public class MultiThreadingTests(ITestOutputHelper output) { var random = new Random(); var fixture = new DataModelTestBase(new SqliteConnection(_connectionString)); - fixture.InitializeAsync().GetAwaiter().GetResult(); - for (var i = 0; i < _changesPerThread; i++) + try { - var value = "test" + i; - try + fixture.InitializeAsync().GetAwaiter().GetResult(); + for (var i = 0; i < _changesPerThread; i++) { - Thread.Sleep(random.Next(1, 10)); + var value = "test" + i; + try + { + Thread.Sleep(random.Next(1, 10)); - _ = fixture.WriteNextChange(fixture.SetWord(id, value)).Result; - lastValue = value; + _ = fixture.WriteNextChange(fixture.SetWord(id, value)).Result; + lastValue = value; - if (debug) output.WriteLine($"id: {id}, value:{value}"); - if (cancellationTokenSource.IsCancellationRequested) return; - } - catch (Exception e) - { - output.WriteLine($"id: {id}, value:{value}, error: {e}"); - cancellationTokenSource.Cancel(); - exception = e; - return; + if (debug) output.WriteLine($"id: {id}, value:{value}"); + if (cancellationTokenSource.IsCancellationRequested) return; + } + catch (Exception e) + { + output.WriteLine($"id: {id}, value:{value}, error: {e}"); + cancellationTokenSource.Cancel(); + exception = e; + return; + } } } + finally + { + //dispose this thread's fixture (and its DI ServiceProvider); the test-level fixture + //keeps the shared in-memory database alive until assertions complete + fixture.DisposeAsync().AsTask().GetAwaiter().GetResult(); + } }); t.Start(); t.Join(); @@ -53,8 +62,9 @@ public class MultiThreadingTests(ITestOutputHelper output) [Fact] public async Task CanApplyChangesWithoutError() { - //ensure the database is created before running the tests - var fixture = new DataModelTestBase(new SqliteConnection(_connectionString)); + //ensure the database is created before running the tests, and keep this fixture alive (and its + //connection open) so the shared in-memory database survives while the worker fixtures dispose + await using var fixture = new DataModelTestBase(new SqliteConnection(_connectionString)); bool debug = false; var cancellationTokenSource = new CancellationTokenSource(); var results = await Task.WhenAll( From 225cb7d192832fa0df9b361e33b9a03a3d7cb84a Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 7 Aug 2026 14:07:12 +0700 Subject: [PATCH 3/4] Make CanCreate2EntriesOutOfOrder assert timestamp ordering Per CodeRabbit review: the two GetLatest assertions are on different entities and pass under either insertion-order or timestamp-order application, so they don't prove ordering. Assert GetProjectSnapshot().LastChange equals commit1.DateTime (the later timestamp) to verify commits are ordered by timestamp, not insertion order. Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/DataModelSimpleChanges.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs b/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs index 42a326a..d01b599 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs @@ -182,6 +182,10 @@ public async Task CanCreate2EntriesOutOfOrder() commit2.DateTime.Should().BeBefore(commit1.DateTime); (await DataModel.GetLatest(_entity1Id))!.Text.Should().Be("entity1"); (await DataModel.GetLatest(_entity2Id))!.Text.Should().Be("entity2"); + //commit1 has the later timestamp, so it must be the model's most recent change even though + //commit2 was written after it — proving commits are ordered by timestamp, not insertion order + var snapshot = await DataModel.GetProjectSnapshot(); + snapshot.LastChange.Should().Be(commit1.DateTime); } [Fact] From 295cf4e41220fbdc0ea53bdb957ec9e12ead564a Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 7 Aug 2026 14:15:20 +0700 Subject: [PATCH 4/4] Address Devin review on MultiThreading and stale-snapshot tests - Guard worker-thread fixture disposal so a cleanup failure is logged instead of surfacing as an unhandled exception on the raw thread. - Restore coverage of the empty-snapshots branch that the reworked DeleteStaleSnapshots test had dropped (DeleteStaleSnapshots_WithNoSnapshots_DoesNothing). Co-Authored-By: Claude Opus 4.8 --- src/SIL.Harmony.Tests/MultiThreadingTests.cs | 13 +++++++++++-- src/SIL.Harmony.Tests/RepositoryTests.cs | 9 +++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/SIL.Harmony.Tests/MultiThreadingTests.cs b/src/SIL.Harmony.Tests/MultiThreadingTests.cs index 4dad848..2f36d7a 100644 --- a/src/SIL.Harmony.Tests/MultiThreadingTests.cs +++ b/src/SIL.Harmony.Tests/MultiThreadingTests.cs @@ -49,8 +49,17 @@ public class MultiThreadingTests(ITestOutputHelper output) finally { //dispose this thread's fixture (and its DI ServiceProvider); the test-level fixture - //keeps the shared in-memory database alive until assertions complete - fixture.DisposeAsync().AsTask().GetAwaiter().GetResult(); + //keeps the shared in-memory database alive until assertions complete. + //guard the cleanup so a disposal failure can't surface as an unhandled exception on + //this raw thread (which would crash the test host rather than fail the test). + try + { + fixture.DisposeAsync().AsTask().GetAwaiter().GetResult(); + } + catch (Exception disposeException) + { + output.WriteLine($"error disposing fixture: {disposeException}"); + } } }); t.Start(); diff --git a/src/SIL.Harmony.Tests/RepositoryTests.cs b/src/SIL.Harmony.Tests/RepositoryTests.cs index 27ec1c9..3ad69d3 100644 --- a/src/SIL.Harmony.Tests/RepositoryTests.cs +++ b/src/SIL.Harmony.Tests/RepositoryTests.cs @@ -244,6 +244,15 @@ await _repository.AddSnapshots([ commit.Id.Should().Be(commitIds[1], $"commit order: [{string.Join(", ", commitIds)}]"); } + [Fact] + public async Task DeleteStaleSnapshots_WithNoSnapshots_DoesNothing() + { + //the empty-repository branch: nothing to delete, must not throw + await _repository.DeleteStaleSnapshots(Commit(Guid.NewGuid(), Time(1, 0))); + + _crdtDbContext.Snapshots.Should().BeEmpty(); + } + [Fact] public async Task DeleteStaleSnapshots_KeepsSnapshotsOlderThanTheCommit() {