Strengthen test hygiene and fix vacuous assertions

[hahn-kev-bot]

---

AI-generated summary

This PR fixes tests that passed without actually verifying behavior — the first batch from a broader test-suite review.

• ModelSnapshotTests.CanGetEmptyModelSnapshot was a non-async void test that called DataModel.GetProjectSnapshot().Should().NotBeNull(). That asserted the returned Task was non-null (always true) and never awaited it, so it verified nothing. It now awaits and asserts the empty-model shape (Snapshots empty; LastChange/LastCommitId/LastCommitHash null).
• Smoke-only tests given real assertions: DontAddTheSameSnapshotTwice (final converged value + word count), CanRecreateUniqueConstraintConflictingValueInOneCommit (original tag deleted, recreated tag present), and CanCreate2EntriesOutOfOrder (both entities present with correct values and ordering).
• RepositoryTests.DeleteStaleSnapshots_Works had no assertion. Reworked into DeleteStaleSnapshots_KeepsSnapshotsOlderThanTheCommit, which covers the skip-when-the-new-commit-is-newer-than-all-snapshots branch that previously had no coverage.
• MultiThreadingTests previously asserted only that concurrent writes threw no exception. It now returns each thread's entity id and final value and asserts the model converged to that final write, so a lost update or corruption that doesn't throw would now fail.

No production code changed — test-only.

Test plan

• dotnet test on the affected tests passes (6/6 locally).
• No production code changed; behavior of the suite is unchanged except stronger assertions.

Summary by CodeRabbit

• Tests
  • Improved validation of out-of-order commits, including timestamp ordering and entity values.
  • Added checks for empty project snapshots.
  • Strengthened multithreading tests to detect lost updates and data corruption.
  • Updated stale-snapshot tests to confirm snapshots are preserved appropriately.
  • Enhanced unique-constraint tests to verify replacement records remain active and retain expected values.

[coderabbitai]

Warning

Review limit reached

@hahn-kev-bot, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 20250ce4-e599-4ef7-a47d-d279308aae8f

📥 Commits

Reviewing files that changed from the base of the PR and between 225cb7d and 295cf4e.

📒 Files selected for processing (2)

• src/SIL.Harmony.Tests/MultiThreadingTests.cs
• src/SIL.Harmony.Tests/RepositoryTests.cs

📝 Walkthrough

Walkthrough

The PR strengthens tests for commit ordering, empty model snapshots, snapshot retention, unique-constraint recreation, and concurrent entity writes.

Changes

Persistence and concurrency test coverage

Layer / File(s)	Summary
Model state and commit assertions src/SIL.Harmony.Tests/DataModelSimpleChanges.cs, src/SIL.Harmony.Tests/ModelSnapshotTests.cs	The tests verify out-of-order commit timestamps and all empty project snapshot fields.
Repository and constraint assertions src/SIL.Harmony.Tests/RepositoryTests.cs, src/SIL.Harmony.Tests/SnapshotTests.cs	The tests verify snapshot retention after cleanup and the state of recreated unique-constraint entities.
Concurrent write persistence src/SIL.Harmony.Tests/MultiThreadingTests.cs	The multithreading test tracks each entity’s final value and verifies the persisted database record.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: myieye

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the test-only changes that replace vacuous assertions and improve test hygiene.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch tests/hygiene-weak-assertions

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/SIL.Harmony.Tests/DataModelSimpleChanges.cs (1)

180-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Make the out-of-order assertion order-sensitive.

commit1 and commit2 create different entities. Both GetLatest assertions pass whether the model applies commits by insertion order or timestamp order. The timestamp assertion verifies only the test setup.

Assert that DataModel.GetProjectSnapshot().LastChange equals commit1.DateTime. The existing snapshot contract is checked in src/SIL.Harmony.Tests/ModelSnapshotTests.cs, Line [38]. Alternatively, add an ordering-sensitive update to the same entity.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/SIL.Harmony.Tests/DataModelSimpleChanges.cs` around lines 180 - 184,
Update the out-of-order test around commit1, commit2, and the existing GetLatest
assertions to verify ordering through DataModel.GetProjectSnapshot().LastChange,
asserting it equals commit1.DateTime. Keep the current timestamp and entity
assertions unless necessary, and do not rely on updates to separate entities as
evidence of commit ordering.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/SIL.Harmony.Tests/MultiThreadingTests.cs`:
- Around line 23-24: Update CanApplyChangesWithoutError and its worker-thread
fixture creation to dispose all four DataModelTestBase instances, including the
read fixture and each per-thread fixture, by awaiting or synchronously
completing DisposeAsync before each fixture’s scope ends.
- Around line 23-24: Move DataModelTestBase construction and InitializeAsync
execution inside the worker thread’s existing try/catch so setup failures are
captured and returned as the Run result exception. After awaiting Task.WhenAll,
assert result.exception.Should().BeNull() to report any captured failure through
the test.

---

Nitpick comments:
In `@src/SIL.Harmony.Tests/DataModelSimpleChanges.cs`:
- Around line 180-184: Update the out-of-order test around commit1, commit2, and
the existing GetLatest assertions to verify ordering through
DataModel.GetProjectSnapshot().LastChange, asserting it equals commit1.DateTime.
Keep the current timestamp and entity assertions unless necessary, and do not
rely on updates to separate entities as evidence of commit ordering.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 26612e22-175d-426a-8a9a-1049edf6d684

📥 Commits

Reviewing files that changed from the base of the PR and between 03f609f and 0aa56f7.

📒 Files selected for processing (5)

• src/SIL.Harmony.Tests/DataModelSimpleChanges.cs
• src/SIL.Harmony.Tests/ModelSnapshotTests.cs
• src/SIL.Harmony.Tests/MultiThreadingTests.cs
• src/SIL.Harmony.Tests/RepositoryTests.cs
• src/SIL.Harmony.Tests/SnapshotTests.cs

[hahn-kev-bot]

Addressed the remaining CodeRabbit nitpick on DataModelSimpleChanges.cs (CanCreate2EntriesOutOfOrder): the two GetLatest checks are on different entities and pass under either insertion- or timestamp-ordering, so they didn't prove ordering. Added an order-sensitive assertion that GetProjectSnapshot().LastChange equals commit1.DateTime (the later timestamp), verifying commits are applied in timestamp order regardless of insertion order.

🤖 Addressed by Claude Code

[hahn-kev-bot]

Reviewed the Devin findings:

• Worker fixture disposal could throw unhandled on the raw thread (MultiThreadingTests.cs:49-54) — fixed: the finally disposal is now wrapped so a cleanup failure is logged via ITestOutputHelper instead of crashing the test host.
• Reworked stale-snapshot test dropped the empty-snapshots branch (RepositoryTests.cs) — fixed: added DeleteStaleSnapshots_WithNoSnapshots_DoesNothing to restore that coverage.
• Convergence assertion may be sensitive to interleaved timestamps (flag) — reviewed, no change needed: each worker thread writes to its own entity id, so there is no cross-thread LWW conflict; each entity's final value is the sole writer's last sequential write (test99) regardless of how commits interleave across threads.
• Shared in-memory DB lifetime relies on the test-level fixture's open connection (info) — this is intentional and documented in a comment; the test-level fixture is deliberately kept open (and disposed last) so the shared DB survives while worker fixtures dispose.

🤖 Addressed by Claude Code