From b4f5e8eb480d3dcdb38b1a2d05fe687afc4bce8a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 18 May 2026 22:59:18 -0400 Subject: [PATCH] Resolve IntegrationTests-007..010 code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IntegrationTests-007: the three live test classes contend for shared singletons (one MXAccess COM, one ZB SQL DB, one GLAuth). Added LiveResourcesCollection with DisableParallelization and applied it to all three so they no longer run concurrently. IntegrationTests-008: the three live fact attributes each re-implemented the env-var check. Added IntegrationTestEnvironment.IsEnabled and all three now delegate to it. IntegrationTests-009: reworded the misleading "Mock server call context" XML doc — it is a hand-written stub with no verification behavior. IntegrationTests-010: WaitForMessageAsync ignored cancellation. It now takes an optional CancellationToken linked with the timeout; the smoke test shares one cancellation source with the StreamEvents call context. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/IntegrationTests/findings.md | 22 ++++++++++-------- .../DashboardLdapLiveTests.cs | 1 + .../Galaxy/GalaxyRepositoryLiveTests.cs | 1 + .../LiveGalaxyRepositoryFactAttribute.cs | 6 +---- .../IntegrationTestEnvironment.cs | 13 +++++++++-- .../LiveLdapFactAttribute.cs | 6 +---- .../LiveResourcesCollection.cs | 16 +++++++++++++ .../WorkerLiveMxAccessSmokeTests.cs | 23 +++++++++++++++---- 8 files changed, 61 insertions(+), 27 deletions(-) create mode 100644 src/MxGateway.IntegrationTests/LiveResourcesCollection.cs 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 {