From 44deee1470e4ab136e7eb2ceb6d951581e1b161c Mon Sep 17 00:00:00 2001 From: ancplua Date: Fri, 26 Jun 2026 07:19:29 +0200 Subject: [PATCH] test: drop best-effort catches from container-fixture teardown The integration-fixture DisposeAsync wrapped every container Dispose and the SUT teardown in `try { ... } catch { /* best-effort */ }`, swallowing all teardown faults to keep a failed InitializeAsync from being masked by a secondary cleanup error. That masking risk is gone: both DisposeSutAsync overrides already null-guard their SUT (_factory / _host), so a failed init cannot NRE during cleanup. The blanket catches only hid genuine teardown bugs from ever surfacing. Replace them with plain null-guarded awaits, matching the Microsoft Agent Framework test-fixture idiom (ConformanceTestBase / SessionPersistenceTests: `client?.Dispose(); if (_app != null) await _app.DisposeAsync();`). Teardown order is SUT-first (it consumes the containers) then infra; Testcontainers' Ryuk reaper cleans up anything a mid-teardown throw leaves running. Also drop the same swallow from SharedContainerFixture.DisposeSutAsync (_host.StopAsync). Co-Authored-By: Claude Opus 4.8 (1M context) --- Paperless.TestSupport/ContainerFixtureBase.cs | 21 ++++++++++--------- .../Integration/WorkerTestBase.cs | 11 +++++----- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Paperless.TestSupport/ContainerFixtureBase.cs b/Paperless.TestSupport/ContainerFixtureBase.cs index fb493ba..12e8ce9 100644 --- a/Paperless.TestSupport/ContainerFixtureBase.cs +++ b/Paperless.TestSupport/ContainerFixtureBase.cs @@ -4,7 +4,7 @@ namespace Paperless.TestSupport; /// Template-method base for the integration-test container fixtures. Owns the /// shared container lifecycle (RabbitMQ + MinIO + Elasticsearch, plus an optional /// Postgres), MinIO bucket creation, Elasticsearch readiness polling, the -/// Elasticsearch document/search polling helpers, and guarded teardown. +/// Elasticsearch document/search polling helpers, and ordered teardown. /// /// Derived fixtures supply the system-under-test by overriding /// (which must assign ) @@ -73,17 +73,18 @@ public async ValueTask InitializeAsync() public async ValueTask DisposeAsync() { - // Guard SUT teardown and swallow per-container Dispose failures so that an - // exception thrown during InitializeAsync (e.g. a wait-strategy timeout) is - // not masked by a secondary NRE/dispose error during xUnit fixture cleanup. - try { await DisposeSutAsync(); } catch { /* best-effort */ } - - try { await _rabbit.DisposeAsync(); } catch { /* best-effort */ } - try { await _minio.DisposeAsync(); } catch { /* best-effort */ } - try { await _elastic.DisposeAsync(); } catch { /* best-effort */ } + // SUT first (it consumes the containers), then the infra. Plain awaits, no + // best-effort catch: a teardown failure is a real fault that must surface. + // DisposeSutAsync overrides null-guard their own SUT, so a failed + // InitializeAsync cannot NRE here, and Testcontainers' Ryuk reaper cleans up + // any container a mid-teardown throw leaves running. + await DisposeSutAsync(); + await _rabbit.DisposeAsync(); + await _minio.DisposeAsync(); + await _elastic.DisposeAsync(); if (_postgres is not null) { - try { await _postgres.DisposeAsync(); } catch { /* best-effort */ } + await _postgres.DisposeAsync(); } } diff --git a/PaperlessServices.Tests/Integration/WorkerTestBase.cs b/PaperlessServices.Tests/Integration/WorkerTestBase.cs index cc2ffe2..fc43fc1 100644 --- a/PaperlessServices.Tests/Integration/WorkerTestBase.cs +++ b/PaperlessServices.Tests/Integration/WorkerTestBase.cs @@ -61,14 +61,13 @@ protected override async ValueTask ConfigureSutAsync() protected override async ValueTask DisposeSutAsync() { - // _host is assigned in ConfigureSutAsync. If init throws before that line - // (e.g. a container wait-strategy times out), _host is still null; the base - // guards this call so a naive _host.StopAsync() NRE cannot mask the real - // InitializeAsync exception in xUnit's collection-fixture cleanup report. + // Null-guarded because a failed InitializeAsync (e.g. a container wait-strategy + // timeout) returns before _host is assigned. When the host exists, stop it + // gracefully then dispose it — no best-effort catch: a shutdown fault is real + // and must surface, not hide behind the init exception. if (_host is not null) { - try { await _host.StopAsync(); } - catch { /* best-effort: don't mask the InitializeAsync exception */ } + await _host.StopAsync(); _host.Dispose(); } }