test: one shared TestHost fixture for RestAdapter integration tests, temp-file leaks closed #91
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "test/shared-testhost-fixture"
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?
Adding an endpoint test no longer means copying a 40-line TestHost boot: every TestHost-based class in
RestAdapter.Tests(11 endpoint classes, 3 middleware classes) derives from oneSupport/IntegrationTestBase, and the temp files they used to leave behind are gone.What's in
IntegrationTestBaseboots the host the wayProgram.csdoes: a per-instance temp root/tmp/dm-rest-<guid>/(SQLite file with migrations applied, images, thumbnails),AddRestAdapter(token),UseRestAdapterMiddleware, all six endpoint groups mapped inProgram.csorder, an authenticatedClient. Hooks only where classes actually differed:AuthToken/AuthenticateClient,ConfigureServices,MapEndpoints,SeedAsync, a virtualDisposeAsync,CreateAnonymousClient().SqliteConnection.ClearPoolthen a recursive delete of the temp root. The leak was wider than the backlog said — every DB-backed class left.db-wal+.db-shm(only the.dbwas deleted) and most left their image/thumbnail dirs.main→ the same 365 plus 9 fixture self-tests.docs/DEVELOPMENT.mdstep 6 names the fixture instead of "copy a sibling".Verification
scripts/verify.sh backend: green (RestAdapter 374 = the unchanged 365 plus 9 fixture self-tests);ls /tmp | grep '^dm-rest-'empty after the run.f8cbe54): cleanup removed → the temp-root self-test red; migration step removed → 21 tests red; auth header never added → the authenticated tests red;ClearPoolremoved → the open-handle self-test red (Linux/proc/self/fdscan lists the leaked descriptors); a mapped-but-404 route vs an unmapped one is now distinguished for the variant and backup groups.UsePersistencetoggle is a 10-line change if you want them lean; every endpoint class maps all six groups (no test hit a route outside its old subset). Backup tests keep the process-wideDOUJIN_MANAGER_BACKUP_DIRhandling class-local because xUnit runs classes in parallel.Directory.Delete) — Linux only; the fd probe is Linux-only by design, on Windows the same regression surfaces asDirectory.Deletethrowing.Integration note
Two sibling PRs (ADR 0021 enforcement, stats endpoint) add integration tests with their own copied boot; they move onto this fixture in a follow-up once this lands. The only overlap with the ADR 0021 PR is two adjacent line removals in
docs/BACKLOG.md; whichever merges second gets a one-line rebase from me.🤖 Generated with Claude Code
Fourteen test classes each carried a ~40-line copy of the TestHost boot (temp SQLite + migrations, image/thumbnail dirs, token, endpoint groups, authenticated client). They now derive from Support/IntegrationTestBase, which boots the host the way Program.cs does — every endpoint group mapped by default — and exposes hooks where a class deviates: AuthToken / AuthenticateClient (auth middleware), ConfigureServices (capturing logger provider), MapEndpoints (ad-hoc middleware routes), SeedAsync (search / sorting seed data), a virtual DisposeAsync (backup env var), and CreateAnonymousClient() for the 401 tests. Everything the host writes lands under one per-instance temp root (/tmp/dm-rest-<guid>/) that DisposeAsync deletes recursively. Before, only the .db file was deleted: every DB-backed instance left its SQLite -wal/-shm side files, and every class except the two image ones left its images/thumbs directories — 876 entries in /tmp per full run, not only SearchIntegrationTests. Test names and assertions are unchanged; the no-op Configure<ImageOptions>(_ => { }) in the image classes was dropped (IOptions<ImageOptions> resolves its defaults without it). Verified: scripts/verify.sh backend green — RestAdapter.Tests 365 -> 372 (365 existing unchanged + 7 fixture tests in Support/IntegrationTestBaseTests), backend total 459 -> 466. Mutation probe: commenting out the Directory.Delete(TempRoot) line turns DisposeAsync_RemovesTempRoot_IncludingSqliteSideFiles red ("temp root still exists"). After the full run `ls /tmp | grep '^dm-rest-'` prints nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Review findings on the shared TestHost fixture: - SqliteConnection.ClearPool in IntegrationTestBase.DisposeAsync was an untested branch: on Linux unlinking open files succeeds, so removing it stayed green (on Windows Directory.Delete would throw). DisposeAsync_RemovesTempRoot_IncludingSqliteSideFiles now also asserts, on Linux, that no /proc/self/fd entry links to a path under the probe's temp root after disposal; the failure message lists the offenders. - The same test's side-file guard matched "*.db*", which test.db alone satisfies; it now asserts test.db-wal and test.db-shm exist before disposal (EF Core creates the database in WAL mode and the pooled connection keeps both files alive until ClearPool). - ServesEveryEndpointGroup covered four of six groups: /api/backups joins the 200 theory (ListBackups treats a missing directory as empty, so no DOUJIN_MANAGER_BACKUP_DIR is needed), and DefaultHost_ServesVariantGroup distinguishes a mapped-but-404 route (application/problem+json body) from an unmapped one (bare 404 without a content type) — every variant route needs seeded data to answer 200. Verified: scripts/verify.sh backend green — RestAdapter.Tests 372 -> 374, backend total 466 -> 468. Mutation probes, each restored afterwards: ClearPool removed -> red ("file descriptors still open under ...: test.db (deleted), test.db-wal (deleted), test.db-shm (deleted)"); MapVariantEndpoints removed -> red (content type null instead of application/problem+json); MapBackupEndpoints removed -> red (NotFound instead of OK). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 86.7%
DoujinManager.Infrastructure - 95.1%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 86.8%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ Fourteen little copies of the same 40-line boot ritual, each one leaking temp files like a careless guest tracking snow through my library… and you swept all of them into one elegant fixture and cleaned up after yourself to the last
-shmfile. −1,106 lines and the suite got smarter. This is my favorite kind of PR~ ♡Verdict: ✅ Looks good to me~
I read the full diff (
db23a75...f8cbe54), all 14 converted classes line by line, and held the fixture up againstProgram.csitself:Program.csdoes — same registration order (AddRestAdapter→ logging →AddDbContext→GuidIdGenerator→ImageInfrastructureRegistration→UseCaseRegistrationHelper), same middleware, all sixMap*Endpoints()groups inProgram.csorder. Deliberately not copying prod-only concerns (ForwardedHeaders, Scalar,/health) — correct call, tests shouldn't inherit deployment plumbing.AuthToken/AuthenticateClient(auth class),ConfigureServices(logging class),MapEndpoints(3 classes map ad-hoc endpoints),SeedAsync(sorting/search), plus the virtualDisposeAsync(backup) andCreateAnonymousClient(). Every hook has a real consumer; nothing speculative. And the base relies onImageInfrastructureRegistration.Registercreating the storage dirs — I checked, it does (Server/ImageInfrastructureRegistration.cs:21-22)._client→Client,_app→App, local_imageDir/_thumbnailDir→ the protected properties. I diffed every class: not a single assertion changed. 365 tests in, 365 tests out (374 total confirmed via--list-tests, 9 fixture self-tests included).SearchIntegrationTests.DisposeAsyncdeleted only the.db— I checked base — leaving-wal/-shmand both storage dirs behind, and most siblings did the same.ClearPool(new SqliteConnection(ConnectionString))is the canonical way to drain Microsoft.Data.Sqlite's static pool (keyed by connection string — the unopened instance is fine), and the self-test doesn't just assert the directory is gone: it forces WAL mode on with a real POST first, then scans/proc/self/fdfor descriptors still pointing under the deleted root. That's a probe with teeth~InitializeAsyncboots anything), andDisposeAsyncrunsbase.DisposeAsync()first (recursive delete of the temp root, backups included) and only then nullsDOUJIN_MANAGER_BACKUP_DIR. And since only that one class touches the env var, the now-ubiquitousGET /api/backupsin the group self-test returns 200 whether or not the backup class is running in parallel — no cross-class flake. The class-local-keeping decision was the right one.problem+jsonvs the router's bodyless 404 for an unmapped path. That distinguishes "mapped but empty" from "never mapped" without seeding anything. Oh, that's lovely~ ♪UsePersistenceescape hatch means the decision is reversible if it ever grates. That's how a tradeoff should be shipped — consciously, written down.💡 Little ideas (non-blocking)~
IntegrationTestBaseTests.cs:98— inOpenDescriptorsUnder, thetry/catchguards onlyLinkTarget, butDirectory.EnumerateFileSystemEntries("/proc/self/fd")is lazy and/proc/self/fdchurns constantly — the enumeration itself can throwIOExceptionif a descriptor vanishes mid-iteration (the catch would never see it). Wrapping the enumerator move-next too (or enumerating with a tolerant manual loop) would make the probe immune to that rare transient. Pure polish for an already-green test~✅ What I liked~
TempRootPrefixconstant doubles as thels /tmp | grep '^dm-rest-'tripwire named in the docs. Self-documenting cleanup contract ♡App/ClientthrowingInvalidOperationExceptionwith a "use it from a test or SeedAsync" hint instead of an NRE — small kindness to the next author.DEVELOPMENT.mdstep 6 rewritten to name the fixture instead of "copy a sibling” — the docs now point at the pattern instead of perpetuating it.ClearPool→ fd-probe red, mapped-vs-unmapped) is exactly how I'd have attacked it myself. Fufu~ great minds~Merge it, and the two sibling PRs can come home to the fixture. I'll be watching~ ♡
Automated review by Jibril · 2026-09-06
CI/CD: passed for head
f8cbe54(coverage bot 7043, window 08:09–08:10 UTC, post-push; 90.8% line / 65.7% branch — production assemblies untouched by this test-only diff) · Local checks: build 0 errors; 54/54 green subset (all 9 fixture self-tests incl. disposal/WAL/fd probe, 3 middleware classes, backup, sorting); 374-test count independently confirmed; ClearPool mutation probe environmentally inconclusive (sandbox OOM under concurrent review sessions) — mechanism statically verified