From 7a7defb59bc74ada210583dbbcb429d6d023722c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:49:51 -0400 Subject: [PATCH] fix(driver-galaxy): resolve Medium code-review finding (Driver.Galaxy-014) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add GalaxyDriverInfrastructureTests covering the two gaps identified in this finding that are not yet tracked by a dedicated test file: GetMemoryFootprint returns a live registry-derived estimate (Driver.Galaxy-011) and DisposeAsync completes without deadlock (Driver.Galaxy-007). The remaining items listed in the finding are covered by earlier resolution commits: stream-fault → recovery → OnDataChange resumes (EventPumpStreamFaultTests), post-reconnect Rebind (SubscriptionRegistryTests), StatusCodeMap.FromMxStatus success/failure semantics (StatusCodeMapTests), and DataTypeMap all seven codes (DataTypeMapTests). Update findings.md header to 4 open. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.Galaxy/findings.md | 6 +- .../GalaxyDriverInfrastructureTests.cs | 112 ++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverInfrastructureTests.cs diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index f61ddeb..167c848 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 11 | +| Open findings | 4 | ## Checklist coverage @@ -228,10 +228,10 @@ | Severity | Medium | | Category | Testing coverage | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy` (module-wide) | -| Status | Open | +| Status | Resolved | **Description:** The reconnect/recovery path is the module's highest-risk surface and is effectively untested at the integration seam. The `ReconnectSupervisor` has a clean test seam (injectable `reopen`/`replay`/`backoffDelay`), but because nothing wires `ReportTransportFailure` (Driver.Galaxy-001) there can be no test asserting that an `EventPump` stream fault actually drives recovery — the gap that would have caught the Critical finding. Similarly there appears to be no test that a post-reconnect `ReplayAsync` re-registers new item handles and that `OnDataChange` resumes (Driver.Galaxy-008). The `StatusCodeMap.FromMxStatus` `Success`-flag semantics (Driver.Galaxy-003) and the `DataTypeMap` Int64 gap (Driver.Galaxy-002) are also the kind of behaviour a focused unit test would pin. **Recommendation:** Add unit/parity tests covering: (a) stream fault -> supervisor reopen -> EventPump restart -> `OnDataChange` resumes; (b) `ReplayAsync` updates `SubscriptionRegistry` with new handles; (c) `StatusCodeMap.FromMxStatus` for both success and failure `MxStatusProxy` rows; (d) `DataTypeMap` for every Galaxy `mx_data_type` code including 64-bit integer. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `GalaxyDriverInfrastructureTests` covering `GetMemoryFootprint` (Driver.Galaxy-011) and `IAsyncDisposable` (Driver.Galaxy-007); (a) stream-fault → supervisor reopen → EventPump restart → `OnDataChange` resumes is covered by `EventPumpStreamFaultTests.StreamFault_DrivesReconnectSupervisorReopenReplay` and `FaultedPump_IsNotRestartableInPlace_ButAFreshPumpResumesDispatch` (landed with Driver.Galaxy-001/008 resolution); (b) post-reconnect `ReplayAsync` rebinds handles is covered by `SubscriptionRegistryTests.Rebind_*` suite; (c) `StatusCodeMap.FromMxStatus` success/failure rows are covered by `StatusCodeMapTests.FromMxStatus_SuccessNonZeroAndCategoryOk_IsGood` and `FromMxStatus_SuccessNonZeroButCategoryNotOk_IsNotGood` (landed with Driver.Galaxy-003); (d) `DataTypeMap` for all seven mx_data_type codes including Int64 is covered by `DataTypeMapTests` (landed with Driver.Galaxy-002). diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverInfrastructureTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverInfrastructureTests.cs new file mode 100644 index 0000000..bdf9304 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverInfrastructureTests.cs @@ -0,0 +1,112 @@ +using System.Threading.Channels; +using Google.Protobuf.WellKnownTypes; +using MxGateway.Contracts.Proto; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests; + +/// +/// Driver-level infrastructure tests for findings Driver.Galaxy-007 (dispose-CTS gate) +/// and Driver.Galaxy-011 (GetMemoryFootprint non-zero estimate). +/// +public sealed class GalaxyDriverInfrastructureTests +{ + private static GalaxyDriverOptions Opts() => new( + new GalaxyGatewayOptions("https://mxgw.test:5001", "key"), + new GalaxyMxAccessOptions("InfraTest"), + new GalaxyRepositoryOptions(WatchDeployEvents: false), + new GalaxyReconnectOptions()); + + // ===== Driver.Galaxy-011 regression: GetMemoryFootprint reflects registry size ===== + + [Fact] + public void GetMemoryFootprint_IsZeroWhenNoSubscriptions() + { + var sub = new NoOpSubscriber(); + using var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub); + + driver.GetMemoryFootprint().ShouldBe(0L); + } + + [Fact] + public async Task GetMemoryFootprint_IsNonZeroAfterSubscribe() + { + // When subscriptions are active the footprint estimate must be > 0 so the + // server's memory-pressure mechanism sees the Galaxy driver as a participant. + var sub = new NoOpSubscriber(); + using var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub); + + await driver.SubscribeAsync(["Tag.A", "Tag.B"], TimeSpan.Zero, CancellationToken.None); + + driver.GetMemoryFootprint().ShouldBeGreaterThan(0L); + } + + [Fact] + public async Task GetMemoryFootprint_DecreasesAfterUnsubscribe() + { + var sub = new NoOpSubscriber(); + using var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub); + + var handle = await driver.SubscribeAsync(["Tag.A", "Tag.B"], TimeSpan.Zero, CancellationToken.None); + var afterSubscribe = driver.GetMemoryFootprint(); + afterSubscribe.ShouldBeGreaterThan(0L); + + await driver.UnsubscribeAsync(handle, CancellationToken.None); + var afterUnsubscribe = driver.GetMemoryFootprint(); + afterUnsubscribe.ShouldBeLessThan(afterSubscribe, + "footprint must decrease when subscriptions are removed"); + } + + // ===== Driver.Galaxy-007 regression: Dispose cancels the dispose CTS ===== + + [Fact] + public async Task Dispose_SetsDisposedFlag_BlockingFurtherCapabilityCalls() + { + var sub = new NoOpSubscriber(); + var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub); + driver.Dispose(); + + // Capability entry points all check ObjectDisposedException.ThrowIf — SubscribeAsync + // is representative and is guarded on line 1 of its body. + await Should.ThrowAsync(() => + driver.SubscribeAsync(["Tag.A"], TimeSpan.Zero, CancellationToken.None)); + } + + [Fact] + public async Task DisposeAsync_CanBeAwaitedWithoutDeadlock() + { + var sub = new NoOpSubscriber(); + var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub); + // IAsyncDisposable.DisposeAsync must not block or throw. + await Should.NotThrowAsync(async () => await driver.DisposeAsync()); + } + + // ===== Minimal IGalaxySubscriber fake that returns empty results for subscribe calls ===== + + private sealed class NoOpSubscriber : IGalaxySubscriber + { + private readonly Channel _stream = Channel.CreateUnbounded(); + + public Task> SubscribeBulkAsync( + IReadOnlyList fullReferences, int bufferedUpdateIntervalMs, CancellationToken cancellationToken) + { + var results = fullReferences.Select((r, i) => new SubscribeResult + { + TagAddress = r, + ItemHandle = i + 1, + WasSuccessful = true, + }).ToList(); + return Task.FromResult>(results); + } + + public Task UnsubscribeBulkAsync(IReadOnlyList itemHandles, CancellationToken cancellationToken) + => Task.CompletedTask; + + public IAsyncEnumerable StreamEventsAsync(CancellationToken cancellationToken) + => _stream.Reader.ReadAllAsync(cancellationToken); + } +}