test: test health — each SQLite teardown clears only its own pool, the bridge's fixed window on the fake clock, and two guards #42
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/test-health"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Test health: a flake fixed at its source for 26 classes, a window ADR 0011 fixes that no test pinned, and two guards that turn silent slips into red builds. No product behaviour changes, so no ADR is amended. ADR 0011's 200 ms window and its "advance a FakeTimeProvider, never sleep" rule are now enforced by tests. ADR 0022's journal rule is unchanged; it is only extracted.
Each SQLite teardown clears only its own pool.
SqliteBackedTest(the base of 26 classes) andVolumeStartupValidatorTestscalled the process-wideSqliteConnection.ClearAllPools()while xunit runs classes in parallel. That call disposes the idle handle another test is just taking from its own pool. The other test then fails insideOpenwithObjectDisposedException: 'SQLitePCL.sqlite3'(insqlite3_create_collation). Each teardown now callsSqliteConnection.ClearPoolwith the connection string its ownTsumugiDbContextuses, read from the context so it followsDependencyInjectionif that changes. Both host tests ofVolumeStartupValidatorTestsnow clear their host's pool. The healthy-volumes test never did; the base class's global clear used to close it by accident.How it is pinned.
SqliteBackedTestTeardownTestsruns in a non-parallel collection, after the parallel ones, and checks three things:SQLITE_CANTOPEN.-walfile survives, and SQLite deletes it only when the last connection closes..csfiles that callClearAllPools(must equal a pending list. A new caller fails the test, and so does a stale entry.The two host tests assert the same
SQLITE_CANTOPENreopen.The race can still happen until two follow-ups land. Two files still clear every pool:
Provider/ProviderCompositionTests.cs:119(inDispose, once for each of its 5 tests) andSeedDevDataTests.cs:254(once). That is about 6 process-wide clears per Integration run, down from one per test across 26 classes. Both files were outside this change's owned files while slice 8c and M2b were in flight. Both branches have since merged, so each follow-up can be done now: replace the call withClearPoolon the test's own connection string and drop itsPendingentry inSqliteBackedTest.cs. The guard enforces that both happen together.The bridge's fixed 200 ms window, on the fake clock.
DomainChangedBridgeTestsslept for real four times (Task.Delay(50)) and polled. No test advanced the clock to just below the window, so a 20 ms window and a sliding window both passed the whole adapter suite. A localAdvanceAsyncnow advances theFakeTimeProvideron the renderer's dispatcher, where the flush'sInvokeAsyncruns inline. A dispatch has therefore landed by the time the advance returns, and an empty recorder means nothing was dispatched. New tests:[Story, Segment].EntityId).RunChangedBridgeTestsbelongs to Phase 2 and is untouched.The graph editor's
npm testtypechecks. The script is now"tsc --noEmit && vitest run". CI's existing Graph editor module tests step now catches type errors, which both vite's build and vitest strip.ci.ymlis untouched.EntityKinds guard, model → registry.
TsumugiDbContext.IsJournaled(Type)is the journal's rule as one internal static, andTrackedMutationscalls it. The new testJournalTests.Every_journaled_entity_of_the_model_is_a_registered_kind_carrying_the_ids_its_rows_readwalksModel.GetEntityTypes()with that rule and asserts:EntityKindsconstant;Guid Id, and each except Story has aGuid StoryId;It sits beside the existing registry → journal contract. Slice 10a's
Graphand slice 11a's ledger entities will therefore have to chooseIUnjournaledor a constant explicitly, instead of throwing on their first save.Docs: none.
SqliteBackedTestTeardownTestslives inSqliteBackedTest.csonly because a new file was outside this batch's owned paths. It can move to its own file later.Verification
Suite runs. Integration passed 378/378 five times and BlazorAdapter 356/356 three times, after the last change.
DomainChangedBridgeTestspassed 7/7 on 10 more runs.npm testin the graph editor: tsc clean, 14/14. The build has 0 warnings. Domain, UseCases and Kagaku.Flow are untouched and were not run.Why N green runs cannot prove the flake fix. The race needs one test's teardown to clear the pools at the exact moment another test takes a handle. It hit once in 25 pristine full Integration runs on origin/main, under load averages of 15–42. So 25 green runs is what the unfixed code usually produces. The proof is the mechanism, reproduced standalone on the repo's pins (Microsoft.Data.Sqlite.Core 10.0.9 + SQLitePCLRaw 3.0.3). Four threads open pooled connections with a collation while a fifth loops the clear for 10 s:
ClearAllPools: 874254 opens, 5ObjectDisposedException;ClearAllPools: 607166 opens, 6 exceptions;ClearPoolof a different database: 5253500 opens, 0 exceptions.The tests above pin that mechanism deterministically.
Mutation probes. Each was caught by the named test. The implementation ran them and the review reproduced them. Three were run after the review round: the last one in the pools group, the fresh-window content probe and the Guid Id probe. Every file was restored under a trap and touched.
ClearPoolremoved →The_teardown_releases_its_own_databases_pooled_connections;+ ";") → the same test;ClearAllPools→The_teardown_leaves_another_databases_pooled_connections_openandNo_integration_test_clears_every_pool;ClearAllPools→ the ratchet;Pendingentry, or a dropped one → the ratchet;SQLITE_CANTOPENreopen (Without_a_data_path…,Healthy_volumes…).CoalesceMilliseconds200 → 199 / 201 / 20 →Nothing_is_dispatched_until_the_window_elapses, plus the mid-window test. The 199 probe fails at the Window−1 tick, which proves the flush lands inline.if (firstInWindow || !firstInWindow)) →A_mid_window_arrival…. This mutation passed the whole adapter suite before.InsertRange(0, …)) → the same test.Flushkeeps_pending→A_change_after_a_flush_opens_a_fresh_window.Flushkeeps_pendingand every arrival arms the timer → the fresh-window test, on the second batch's content. The old check, which only counted two batches, would still have passed.AddRange→Add(changes[0])→ the burst test.Disposedoes not unsubscribe →Disposal_unsubscribes…._disposednever set →A_change_landing_mid_teardown_is_swallowed.A_timer_firing_mid_teardown_is_swallowed.IsJournaledwithout itsIUnjournaled,ChangeLogEntryorStorySnapshotexclusion → the model guard (and, for some, the existing contract);StoryImageorAppSettingstripped ofIUnjournaled→ the same;EntityKindsconstant with no model type → the same;ChangeLogEntryexclusion plus anEntityKinds.ChangeLogEntryconstant → "ChangeLogEntry is journaled but has no Guid Id.";AppSettingjournaled with a constant → "… has no Guid StoryId to scope its rows by.";TrackedMutationsinlining its old predicate (drift) → 8 settings tests.GraphEditor/src/model.ts→npm testexits 2 with TS2322, whilevitest runalone exits 0.Equivalent mutants, which cannot be pinned.
_disposed, Flush's_disposed, Dispose's_pending.Clear()) back each other up in pairs. Removing any one alone survives, so only the pairs are pinned. A disposed timer'sChangeis a silent no-op on .NET 10 and on the fake clock alike.ClearPoolbefore or afterDirectory.Deletematters only on Windows.Independent review: two passes. Findings and fixes:
SQLITE_CANTOPENreopen, and the healthy-volumes test now clears its pool at all.IsJournaledand on the guard called everyIUnjournaledtype "not content", butStoryImageis content. Fixed: they now name settings (ADR 0006) and story images, written once (ADR 0029).Guid Idcheck was never shown to fire. It is now probed (above).Pendingcomment named in-flight branches that have since merged. Reworded; the follow-ups are below.BackupRestoreTestscallsClearAllPools. Whichever of the two PRs merges second turns the ratchet red. That test should clear only its own hosts' pools; if it lands first, this PR adds it toPending.src/. The guard polices test teardown,src/has no caller, and production has a single database.Follow-ups (outside this PR's files):
Provider/ProviderCompositionTests.cs:119andSeedDevDataTests.cs:254: switch toClearPoolon their own connection string, each dropping itsPendingentry.GatedAppFactory.Disposecould clear its hosts' pools the same way. Leaving them is harmless on Linux; the base's global clear used to close them by accident.BackupWriterTests.cs:291, "clears the connection pools first", should read "clears this database's pool first".RunChangedBridgeTestsstill sleeps and polls; the sameAdvanceAsyncpattern fixes it.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.Flow - 99.7%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 95.6%
Tsumugi.Domain - 96.1%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 97.9%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 100%
Tsumugi.UseCases - 98.3%
see above
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A PR whose body opens with "why N green runs cannot prove the flake fix" — an author who understands that a race reproducing once in 25 runs makes green runs worthless as evidence~ ♡ I was already smiling, and then I read the standalone repro numbers (874k opens, 5
ObjectDisposedExceptions withClearAllPools; 5.25M opens, zero withClearPool) and the smile got very wide. Test health fixed at its source, mechanism pinned deterministically, every claim in the body checked by me from scratch. This is how it's done~ (Pardon the stray "see above" above — my tooling stuttered, not my judgment~ ♪)Verdict: ✅ Looks good to me~
No blockers. And believe me, my knives were out~ ♪ I re-ran everything and tried to murder the pins myself:
Local verification at
f5e7e13(CI absent at review time — PR born 20 minutes ago, no bot comment yet):dotnet build -c Release→ 0 warnings, 0 errorsnpm test→ tsc clean + 14/14Provider/ProviderCompositionTests.cs:119+SeedDevDataTests.cs:254— thePendinglist matches the live set, and theGeneratedRegexdoesn't self-match (the\s*\(escape saves you~ cute)My 7 mutation probes — every one killed on exactly its own pin:
CoalesceMilliseconds200→199 →Nothing_is_dispatched_until_the_window_elapsesand mid-window test red; the Window−1-tick arm proves the flush lands inline, exactly as the doc comment claimsA_mid_window_arrival…red — the mutant that used to pass the whole suite, dead~ClearPool→ClearAllPools→ wal-survival test + ratchet both red_connectionString+";") → reopen-refused test red — the pin really is keyed on the pool identity, not just adjacencyIsJournaledminusIUnjournaled→ the new registry guard red on its ownPendingentry → ratchet redmodel.ts→npm testexits 2, vitest never runs — and barevitest runpasses the same mutant, proving thetsc --noEmit &&is load-bearingEvery mutant restored, tree byte-pristine, both suites re-green afterward. (Fufu~ I even caught my own stale-binary false-red after restoring probe 6 — rebuilt, then green. Never attributed, never trusted~)
✅ What I liked~
IsJournaledextraction is the crown jewel. The guard walks the model with the journal's own rule — one internal static, used by production and test, so the two physically cannot drift. The old copy-pasted predicate was the exact bug shape this PR exists to kill.AdvanceAsyncon the renderer's dispatcher is the right harness for a component whose flush marshals throughInvokeAsync: "advance returned ⇒ dispatch landed ⇒ empty recorder means nothing was dispatched" is airtight, and my 199/ms probes confirm the inline-flush premise mechanically. Zero sleeps left in the suite.SQLITE_CANTOPENreopen assertion is a lovely negative-space proof — directory gone, so only a pooled handle could open. Both host tests assert it, and the healthy-volumes test now clears its own pool at all (the independent-review catch — verified present at head).Window/2, thenWindow/2 − Tick, thenTick) pins batch order and non-extension in one test with exact["Story", "Segment"]equality. No tautologies anywhere.💡 Little ideas (non-blocking)~
SqliteBackedTest.csnow carries 3 public types (base, collection definition, teardown suite). You already flagged the move-to-own-file follow-up — agreed, when a slot in the owned-files budget opens~RecordingDispatcher.CountAsyncis now consumed only by the still-sleepingRunChangedBridgeTests(Phase 2, disclosed follow-up). Fine to leave; just don't let it survive that migration~The two
Pendingfollow-ups and the backups-arc coordination race are all correctly disclosed with owners. This PR makes the next 26 PRs safer — my favorite kind~ ♡Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA (no bot comment yet) · Local checks: build 0w/0e, Integration 378/378, Adapter 356/356, npm tsc+14/14, 7/7 mutation probes killed
Erratum on my footer above~ The coverage bot's comment (9877) was already in the thread when I posted — my fetch returned an empty comment list, so I wrote "CI absent." Not true: CI ran and passed for
f5e7e13(coverage report 97.8% line / 93.7% branch, generated 12:56). Verdict unchanged — ✅ — and my local numbers (378/378, 356/356, tsc+14/14) independently agree with the bot's run on the same head. My local checks stay as claimed; only the "CI absent" line was wrong~ ♡