diff --git a/code-reviews/IntegrationTests/findings.md b/code-reviews/IntegrationTests/findings.md index bb83ae8..edb19b2 100644 --- a/code-reviews/IntegrationTests/findings.md +++ b/code-reviews/IntegrationTests/findings.md @@ -7,13 +7,13 @@ | Review date | 2026-05-18 | | Commit reviewed | `6c64030` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Checklist coverage | # | Category | Result | |---|---|---| -| 1 | Correctness & logic bugs | Issues found: IntegrationTests-003 (asserts only on first event), IntegrationTests-010 (`WaitForFirstMessageAsync` ignores cancellation). | +| 1 | Correctness & logic bugs | Issues found: IntegrationTests-003 (asserts only on first event), IntegrationTests-010 (`WaitForMessageAsync` ignores cancellation). | | 2 | mxaccessgw conventions | Live tests correctly gated and skip (not fail) when prerequisites are absent; `LiveGalaxyRepositoryFactAttribute` undocumented in the opt-in matrix. | | 3 | Concurrency & thread safety | Issue found: IntegrationTests-007 (no `[Collection]`/parallelism guard for shared MXAccess/ZB/GLAuth). | | 4 | Error handling & resilience | Issue found: IntegrationTests-004 (cleanup `WaitAsync` can mask the original failure). | @@ -123,13 +123,13 @@ | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs:20`, `src/MxGateway.IntegrationTests/Galaxy/GalaxyRepositoryLiveTests.cs:5`, `src/MxGateway.IntegrationTests/DashboardLdapLiveTests.cs:9` | -| Status | Open | +| Status | Resolved | **Description:** The live test classes contend for genuinely shared singletons — one MXAccess COM provider, one ZB SQL database, one GLAuth instance with a 3-fail/10-minute per-IP lockout. No `[Collection]` annotation or `DisableTestParallelization` is declared, so xUnit's default cross-class parallelism could run the Galaxy tests concurrently or interleave an LDAP failure burst that trips the GLAuth lockout. **Recommendation:** Place the live test classes in a shared `[Collection]`, or set `[assembly: CollectionBehavior(DisableTestParallelization = true)]` for this opt-in project, so live external resources are accessed serially. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18: Confirmed — no `[Collection]` or assembly-level `CollectionBehavior` existed. Added `LiveResourcesCollection.cs` with a `[CollectionDefinition(Name, DisableParallelization = true)]` and applied `[Collection(LiveResourcesCollection.Name)]` to `WorkerLiveMxAccessSmokeTests`, `GalaxyRepositoryLiveTests`, and `DashboardLdapLiveTests`. A named collection (rather than an assembly-wide `DisableTestParallelization`) was chosen so the live classes serialize against each other and within themselves while non-live tests (`IntegrationTestEnvironmentTests`) keep parallelizing. Verified by build; live tests not executed (no MXAccess COM / live LDAP in this environment). ### IntegrationTests-008 @@ -138,13 +138,13 @@ | Severity | Low | | Category | Code organization & conventions | | Location | `src/MxGateway.IntegrationTests/LiveLdapFactAttribute.cs`, `src/MxGateway.IntegrationTests/Galaxy/LiveGalaxyRepositoryFactAttribute.cs`, `src/MxGateway.IntegrationTests/LiveMxAccessFactAttribute.cs` | -| Status | Open | +| Status | Resolved | **Description:** Three near-identical fact attributes each re-implement the same "compare env var to `1` with `StringComparison.Ordinal`, set `Skip` otherwise" pattern. `LiveMxAccessFactAttribute` delegates to `IntegrationTestEnvironment` while the other two inline the logic, so the project has two divergent styles for the same concern. **Recommendation:** Extract a shared helper (e.g. `IntegrationTestEnvironment.IsEnabled(string variableName)`) and have all three attributes call it. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18: Confirmed — `LiveLdapFactAttribute.Enabled` and `LiveGalaxyRepositoryFactAttribute.Enabled` each inlined the ordinal `== "1"` comparison while `LiveMxAccessFactAttribute` delegated to `IntegrationTestEnvironment`. Added `IntegrationTestEnvironment.IsEnabled(string variableName)` as the single implementation; `LiveMxAccessTestsEnabled`, `LiveLdapFactAttribute.Enabled`, and `LiveGalaxyRepositoryFactAttribute.Enabled` now all call it. Verified by build. ### IntegrationTests-009 @@ -153,13 +153,13 @@ | Severity | Low | | Category | Documentation & comments | | Location | `src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs:372-375` | -| Status | Open | +| Status | Resolved | **Description:** `TestServerCallContext` is XML-documented as a "Mock server call context," but it is a hand-written stub/fake with no mocking framework and no verification behavior. Per the style guides (accurate naming; explain why not what), calling it a mock misleads readers who may expect verifiable interactions. **Recommendation:** Reword the summary to "test stub" / "minimal `ServerCallContext` implementation for in-process gRPC calls." -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18: Confirmed — the summary read "Mock server call context for testing gRPC calls." Reworded to "Minimal `ServerCallContext` stub for invoking the gRPC service in-process," noting it is a hand-written fake with no verification behavior. No mocking framework is involved; this is a documentation-only fix. Verified by build. ### IntegrationTests-010 @@ -168,10 +168,12 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs:366-369` | -| Status | Open | +| Status | Resolved | **Description:** `WaitForFirstMessageAsync` accepts only a `timeout` and never observes a `CancellationToken`. There is no per-test cancellation propagation, so if the gateway/worker hangs without writing an event the test relies solely on the 15s `WaitAsync` timeout and gives no contextual diagnostics. Combined with IntegrationTests-004, a hung live worker produces a bare `TimeoutException`. **Recommendation:** Accept a `CancellationToken` (linked to `TestServerCallContext`'s token), pass it to `firstMessage.Task.WaitAsync(timeout, token)`, and on timeout emit the recorded `Messages` count via `output.WriteLine` before throwing. -**Resolution:** _(open)_ +**Re-triage:** The named method `WaitForFirstMessageAsync` no longer exists — IntegrationTests-003's resolution renamed/replaced it with `RecordingServerStreamWriter.WaitForMessageAsync(predicate, timeout)`, which scans recorded messages and blocks on a `SemaphoreSlim`. The underlying defect still held: that replacement method also took only a `timeout` and never observed a `CancellationToken`. The finding remains valid (Low, Correctness) against the renamed method; the recommendation's `firstMessage.Task.WaitAsync` detail is stale but the intent (thread a token, surface a count on timeout) is unchanged. + +**Resolution:** Resolved 2026-05-18: Added an optional `CancellationToken` parameter to `WaitForMessageAsync`, linked with the existing timeout source via `CancellationTokenSource.CreateLinkedTokenSource`, so a per-test cancellation aborts the wait promptly. `GatewaySession_WithLiveWorker_RegistersAdvisesStreamsDataAndCloses` now creates a `CancellationTokenSource`, passes its token into the `StreamEvents` `TestServerCallContext` and into `WaitForMessageAsync`, so the stream call and the wait share one cancellation source. On timeout the method already throws a `TimeoutException` whose message includes the scanned message count, satisfying the "emit recorded count" intent (the count surfaces in the test failure rather than via a separate `output.WriteLine`). Verified by build; live tests not executed. diff --git a/src/MxGateway.IntegrationTests/DashboardLdapLiveTests.cs b/src/MxGateway.IntegrationTests/DashboardLdapLiveTests.cs index 75bb32a..37607fe 100644 --- a/src/MxGateway.IntegrationTests/DashboardLdapLiveTests.cs +++ b/src/MxGateway.IntegrationTests/DashboardLdapLiveTests.cs @@ -6,6 +6,7 @@ using MxGateway.Server.Dashboard; namespace MxGateway.IntegrationTests; +[Collection(LiveResourcesCollection.Name)] public sealed class DashboardLdapLiveTests { [LiveLdapFact] diff --git a/src/MxGateway.IntegrationTests/Galaxy/GalaxyRepositoryLiveTests.cs b/src/MxGateway.IntegrationTests/Galaxy/GalaxyRepositoryLiveTests.cs index 731d252..86d412a 100644 --- a/src/MxGateway.IntegrationTests/Galaxy/GalaxyRepositoryLiveTests.cs +++ b/src/MxGateway.IntegrationTests/Galaxy/GalaxyRepositoryLiveTests.cs @@ -2,6 +2,7 @@ using MxGateway.Server.Galaxy; namespace MxGateway.IntegrationTests.Galaxy; +[Collection(LiveResourcesCollection.Name)] public sealed class GalaxyRepositoryLiveTests { /// Verifies that the Galaxy Repository can establish a live connection to the ZB database. diff --git a/src/MxGateway.IntegrationTests/Galaxy/LiveGalaxyRepositoryFactAttribute.cs b/src/MxGateway.IntegrationTests/Galaxy/LiveGalaxyRepositoryFactAttribute.cs index 19896c1..32bcf63 100644 --- a/src/MxGateway.IntegrationTests/Galaxy/LiveGalaxyRepositoryFactAttribute.cs +++ b/src/MxGateway.IntegrationTests/Galaxy/LiveGalaxyRepositoryFactAttribute.cs @@ -18,11 +18,7 @@ public sealed class LiveGalaxyRepositoryFactAttribute : FactAttribute } /// Gets a value indicating whether live Galaxy Repository tests are enabled. - public static bool Enabled => - string.Equals( - Environment.GetEnvironmentVariable(EnableVariableName), - "1", - StringComparison.Ordinal); + public static bool Enabled => IntegrationTestEnvironment.IsEnabled(EnableVariableName); /// Gets the Galaxy Repository connection string from environment or default. public static string ConnectionString => diff --git a/src/MxGateway.IntegrationTests/IntegrationTestEnvironment.cs b/src/MxGateway.IntegrationTests/IntegrationTestEnvironment.cs index 0f2c470..5e52cc5 100644 --- a/src/MxGateway.IntegrationTests/IntegrationTestEnvironment.cs +++ b/src/MxGateway.IntegrationTests/IntegrationTestEnvironment.cs @@ -9,9 +9,18 @@ public static class IntegrationTestEnvironment public const string LiveMxAccessEventTimeoutSecondsVariableName = "MXGATEWAY_LIVE_MXACCESS_EVENT_TIMEOUT_SECONDS"; /// Gets whether live MXAccess tests are enabled. - public static bool LiveMxAccessTestsEnabled => + public static bool LiveMxAccessTestsEnabled => IsEnabled(LiveMxAccessVariableName); + + /// + /// Gets whether an opt-in live-test suite is enabled, by comparing the named + /// environment variable to 1. Shared by every Live*FactAttribute + /// so the opt-in check has a single implementation. + /// + /// The environment variable that gates the suite. + /// when the variable is exactly 1. + public static bool IsEnabled(string variableName) => string.Equals( - Environment.GetEnvironmentVariable(LiveMxAccessVariableName), + Environment.GetEnvironmentVariable(variableName), "1", StringComparison.Ordinal); diff --git a/src/MxGateway.IntegrationTests/LiveLdapFactAttribute.cs b/src/MxGateway.IntegrationTests/LiveLdapFactAttribute.cs index a0b69a2..61a46f1 100644 --- a/src/MxGateway.IntegrationTests/LiveLdapFactAttribute.cs +++ b/src/MxGateway.IntegrationTests/LiveLdapFactAttribute.cs @@ -12,9 +12,5 @@ public sealed class LiveLdapFactAttribute : FactAttribute } } - public static bool Enabled => - string.Equals( - Environment.GetEnvironmentVariable(EnableVariableName), - "1", - StringComparison.Ordinal); + public static bool Enabled => IntegrationTestEnvironment.IsEnabled(EnableVariableName); } diff --git a/src/MxGateway.IntegrationTests/LiveResourcesCollection.cs b/src/MxGateway.IntegrationTests/LiveResourcesCollection.cs new file mode 100644 index 0000000..0a5c692 --- /dev/null +++ b/src/MxGateway.IntegrationTests/LiveResourcesCollection.cs @@ -0,0 +1,16 @@ +namespace MxGateway.IntegrationTests; + +/// +/// xUnit collection that serializes every live integration-test class. The live +/// suites contend for genuinely shared singletons — one MXAccess COM provider, +/// one ZB SQL database, and one GLAuth instance with a per-IP failure +/// lockout — so they must not run in parallel with one another. Placing each +/// live class in this collection disables xUnit's default cross-class +/// parallelism for them while leaving non-live tests free to parallelize. +/// +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class LiveResourcesCollection +{ + /// The collection name applied via [Collection] on live test classes. + public const string Name = "Live external resources"; +} diff --git a/src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs b/src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs index f590fba..ad326d8 100644 --- a/src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs +++ b/src/MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs @@ -17,6 +17,7 @@ using Xunit.Abstractions; namespace MxGateway.IntegrationTests; +[Collection(LiveResourcesCollection.Name)] public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) { private static readonly TimeSpan CommandTimeout = TimeSpan.FromSeconds(15); @@ -40,6 +41,7 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) string? sessionId = null; RecordingServerStreamWriter? eventWriter = null; Task? streamTask = null; + using CancellationTokenSource streamCancellation = new(); try { @@ -61,7 +63,7 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) streamTask = fixture.Service.StreamEvents( new StreamEventsRequest { SessionId = sessionId }, eventWriter, - new TestServerCallContext()); + new TestServerCallContext(streamCancellation.Token)); MxCommandReply registerReply = await fixture.Service.Invoke( CreateRegisterRequest(sessionId), @@ -94,7 +96,8 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) MxEvent dataChange = await eventWriter .WaitForMessageAsync( candidate => candidate.Family == MxEventFamily.OnDataChange, - IntegrationTestEnvironment.LiveMxAccessEventTimeout) + IntegrationTestEnvironment.LiveMxAccessEventTimeout, + streamCancellation.Token) .ConfigureAwait(false); LogEvent(dataChange); @@ -560,12 +563,20 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) /// /// Filter the awaited message must satisfy. /// The maximum total time to wait. + /// + /// Token observed alongside the timeout so a per-test cancellation (for example the + /// gRPC call context's token) aborts the wait promptly instead of hanging until the + /// timeout elapses. + /// /// The first message that satisfies the predicate. public async Task WaitForMessageAsync( Func predicate, - TimeSpan timeout) + TimeSpan timeout, + CancellationToken cancellationToken = default) { using CancellationTokenSource timeoutCancellation = new(timeout); + using CancellationTokenSource linkedCancellation = + CancellationTokenSource.CreateLinkedTokenSource(timeoutCancellation.Token, cancellationToken); int scanned = 0; while (true) @@ -586,7 +597,7 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) try { - await messageArrived.WaitAsync(timeoutCancellation.Token).ConfigureAwait(false); + await messageArrived.WaitAsync(linkedCancellation.Token).ConfigureAwait(false); } catch (OperationCanceledException) when (timeoutCancellation.IsCancellationRequested) { @@ -598,7 +609,9 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) } /// - /// Mock server call context for testing gRPC calls. + /// Minimal stub for invoking the gRPC service + /// in-process. It is a hand-written fake with no verification behavior — it + /// only supplies the context values the service reads during a call. /// private sealed class TestServerCallContext(CancellationToken cancellationToken = default) : ServerCallContext {