From f7e3e9885e8250c770efab9e5a70433047c31319 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 07:45:31 -0400 Subject: [PATCH] fix(driver-ablegacy): resolve Low code-review findings (Driver.AbLegacy-005,011,013) - Driver.AbLegacy-005: optional ILogger ctor parameter, logged init failure / probe transitions / first non-zero libplctag status per device. - Driver.AbLegacy-011: Dispose() runs the synchronous teardown directly instead of bridging via DisposeAsync().AsTask().GetAwaiter().GetResult() to remove the documented sync-over-async deadlock pattern. - Driver.AbLegacy-013: documented the ResolveHost three-tier fallback chain in XML and pointed DiscoverAsync's IsArray=false comment at the Modbus ArrayCount pattern for the eventual multi-element follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.AbLegacy/findings.md | 45 ++++- .../AbLegacyDriver.cs | 103 +++++++++++- .../AbLegacyDriverFactoryExtensions.cs | 20 ++- .../AbLegacyDisposeAndResolveHostTests.cs | 158 ++++++++++++++++++ .../AbLegacyLoggerInjectionTests.cs | 90 ++++++++++ 5 files changed, 404 insertions(+), 12 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDisposeAndResolveHostTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyLoggerInjectionTests.cs diff --git a/code-reviews/Driver.AbLegacy/findings.md b/code-reviews/Driver.AbLegacy/findings.md index 3d3f5fa..2f10d5c 100644 --- a/code-reviews/Driver.AbLegacy/findings.md +++ b/code-reviews/Driver.AbLegacy/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -141,7 +141,7 @@ decode the full 16-bit word and test bit 0. | Severity | Low | | Category | OtOpcUa conventions | | Location | `AbLegacyDriver.cs` (whole file) | -| Status | Open | +| Status | Resolved | **Description:** The driver uses no `ILogger`/Serilog at all. Probe-loop failures, runtime initialisation failures, libplctag non-zero statuses, and read/write @@ -155,7 +155,16 @@ string that the next read or write immediately clobbers. log probe transitions, runtime-init failures, and the first occurrence of a non-zero libplctag status per device. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `AbLegacyDriver` now accepts an optional +`ILogger` (falls back to `NullLogger`), mirroring the Modbus / S7 / +Galaxy driver pattern. `InitializeAsync` catch-path logs the init failure at Error +level; `TransitionDeviceState` logs every probe transition (Warning on downgrade to +Stopped, Information on recovery); `ReadAsync` logs the first non-zero libplctag +status per device at Warning level via a re-armable `DeviceState.FirstNonZeroStatusLogged` +latch so a permanently-bad PLC doesn't flood the rolling file. `AbLegacyDriverFactoryExtensions.Register` +gains an optional `ILoggerFactory` parameter so the Server bootstrap can wire DI +logging when it chooses; the legacy single-arg `CreateInstance` overload stays for +back-compat. Regression coverage in `AbLegacyLoggerInjectionTests`. ### Driver.AbLegacy-006 @@ -293,7 +302,7 @@ into a real PCCC-STS path or delete it as dead code. The same defect exists in | Severity | Low | | Category | Performance & resource management | | Location | `AbLegacyDriver.cs:440` | -| Status | Open | +| Status | Resolved | **Description:** `Dispose()` is implemented as `DisposeAsync().AsTask().GetAwaiter().GetResult()` - sync-over-async. `ShutdownAsync` @@ -306,7 +315,16 @@ single-threaded synchronization context. must exist, perform the synchronous teardown directly (cancel CTSs, dispose runtimes) rather than blocking on the async path. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `Dispose()` now performs the synchronous +teardown directly (cancel probe CTSs, dispose runtimes, clear maps) rather than +wrapping `DisposeAsync().AsTask().GetAwaiter().GetResult()`. The poll engine's +`DisposeAsync` is drained with `.ConfigureAwait(false).GetAwaiter().GetResult()` so a +captured single-threaded `SynchronizationContext` can never be the resumption target — +the classic sync-over-async deadlock is structurally ruled out. Regression test +`Dispose_under_single_threaded_sync_context_does_not_deadlock` drives the path +through a cooperative single-threaded `SynchronizationContext` with a 2s pump timeout; +`Dispose_runs_teardown_without_blocking_on_async_wait` and `Dispose_is_idempotent` +cover the cleanup invariants. ### Driver.AbLegacy-012 @@ -345,7 +363,7 @@ unused fields and the doc comments that imply they are load-bearing. | Severity | Low | | Category | Code organization & conventions | | Location | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` | -| Status | Open | +| Status | Resolved | **Description:** Two minor organisational issues: 1. `ResolveHost` returns `_options.Devices.FirstOrDefault()?.HostAddress ?? @@ -362,4 +380,17 @@ unused fields and the doc comments that imply they are load-bearing. document why falling back to the instance id is acceptable. For (2), record the array-addressing gap as a tracked follow-up. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — +(1) `ResolveHost` carries a new XML-doc block that documents the three-step fallback +chain (known tag → first device → `DriverInstanceId`) and explicitly cites the +`IPerCallHostResolver` contract which requires implementations to return the driver's +default-host string rather than throw on an unknown reference. The instance-id +fallback is therefore the documented single-host behaviour, not a leaky fake. Three +regression tests in `AbLegacyDisposeAndResolveHostTests` pin each branch of the chain +(`ResolveHost_known_reference_returns_tag_device`, +`ResolveHost_unknown_reference_with_devices_returns_first_device`, +`ResolveHost_unknown_reference_no_devices_returns_driver_instance_id`). +(2) `DiscoverAsync` now carries an inline tracked-follow-up comment that calls out +the PCCC-file-as-array gap, notes the consistency with the PR-staged scope in +`docs/v2/driver-specs.md`, and points to the Modbus `ArrayCount` flow as the pattern +to mirror when multi-element addressing lands. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 4bf1413..1da9775 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -1,3 +1,5 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; @@ -14,6 +16,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover private readonly AbLegacyDriverOptions _options; private readonly string _driverInstanceId; private readonly IAbLegacyTagFactory _tagFactory; + private readonly ILogger _logger; private readonly PollGroupEngine _poll; private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); @@ -29,12 +32,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover public event EventHandler? OnHostStatusChanged; public AbLegacyDriver(AbLegacyDriverOptions options, string driverInstanceId, - IAbLegacyTagFactory? tagFactory = null) + IAbLegacyTagFactory? tagFactory = null, + ILogger? logger = null) { ArgumentNullException.ThrowIfNull(options); _options = options; _driverInstanceId = driverInstanceId; _tagFactory = tagFactory ?? new LibplctagLegacyTagFactory(); + _logger = logger ?? NullLogger.Instance; _poll = new PollGroupEngine( reader: ReadAsync, onChange: (handle, tagRef, snapshot) => @@ -92,6 +97,11 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover catch (Exception ex) { _health = new DriverHealth(DriverState.Faulted, null, ex.Message); + // Driver.AbLegacy-005 — structured log of the init failure so a field operator sees + // the exception in the rolling Serilog file rather than only as a transient Detail + // string on DriverHealth. + _logger.LogError(ex, + "AbLegacy driver initialise failed. Driver={DriverInstanceId}", _driverInstanceId); // Tear down any probe loops and cached state that were created before the failure so // that a caller who catches and abandons (rather than retrying via ReinitializeAsync) // doesn't leave orphaned background tasks, CancellationTokenSources, and libplctag @@ -192,8 +202,23 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover AbLegacyStatusMapper.MapLibplctagStatus(status), null, now); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, $"libplctag status {status} reading {reference}"); + // Driver.AbLegacy-005 — log the FIRST non-zero libplctag status per device so + // a field operator can correlate a comms problem with a structured log + // entry. Detail on DriverHealth is overwritten by the very next read; the + // log entry persists. Subsequent occurrences on the same device stay quiet so + // a permanently-bad PLC doesn't flood the rolling file. + if (!device.FirstNonZeroStatusLogged) + { + device.FirstNonZeroStatusLogged = true; + _logger.LogWarning( + "AbLegacy non-zero libplctag status. Driver={DriverInstanceId} Device={DeviceHostAddress} Reference={Reference} Status={Status}", + _driverInstanceId, def.DeviceHostAddress, reference, status); + } continue; } + // Healthy read — re-arm the per-device first-failure log so a future non-zero + // status logs again rather than being suppressed by an old flag from a prior outage. + device.FirstNonZeroStatusLogged = false; results[i] = new DataValueSnapshot(value, AbLegacyStatusMapper.Good, now, now); _health = new DriverHealth(DriverState.Healthy, now, null); @@ -313,6 +338,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover string.Equals(t.DeviceHostAddress, device.HostAddress, StringComparison.OrdinalIgnoreCase)); foreach (var tag in tagsForDevice) { + // Driver.AbLegacy-013 (tracked follow-up) — PCCC files are inherently arrays of + // elements (a single N7 file is up to 256 words), but the current tag-definition + // surface only addresses one element. IsArray/ArrayDim are hard-wired false/null + // until multi-element addressing lands; tags that genuinely span a range have to + // be enumerated one element at a time today. This is consistent with the + // PR-staged scope documented in docs/v2/driver-specs.md (AbLegacy ships with thin + // array coverage); when array support is added, ArrayCount on the tag definition + // will flow through here as it already does on the Modbus driver. deviceFolder.Variable(tag.Name, tag.Name, new DriverAttributeInfo( FullName: tag.Name, DriverDataType: tag.DataType.ToDriverDataType(), @@ -397,12 +430,38 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover state.HostState = newState; state.HostStateChangedUtc = DateTime.UtcNow; } + // Driver.AbLegacy-005 — structured log of every probe-driven transition. Operators can + // grep the rolling Serilog file for the device address to see when a PLC was last + // reachable. Downgrades to Stopped log as Warning; recoveries log as Information. + if (newState == HostState.Stopped) + _logger.LogWarning( + "AbLegacy probe transition. Driver={DriverInstanceId} Device={DeviceHostAddress} From={Old} To={New}", + _driverInstanceId, state.Options.HostAddress, old, newState); + else + _logger.LogInformation( + "AbLegacy probe transition. Driver={DriverInstanceId} Device={DeviceHostAddress} From={Old} To={New}", + _driverInstanceId, state.Options.HostAddress, old, newState); OnHostStatusChanged?.Invoke(this, new HostStatusChangedEventArgs(state.Options.HostAddress, old, newState)); } // ---- IPerCallHostResolver ---- + /// + /// Map a full reference to the host string used as the resilience-pipeline breaker key. + /// Driver.AbLegacy-013 — the contract on requires that + /// implementations never throw on an unknown reference. The fallback chain is therefore: + /// + /// Known tag → its DeviceHostAddress. + /// Unknown reference but devices configured → the first device's host address + /// (multi-device drivers degrade to single-host behaviour rather than failing). + /// Unknown reference and no devices configured → the driver instance id, which + /// the dispatch layer treats as the single-host key per the interface + /// documentation. Reaching this branch indicates a misconfigured driver (no + /// devices) so callers that want to surface that should validate + /// before relying on per-tag routing. + /// + /// public string ResolveHost(string fullReference) { if (_tagsByName.TryGetValue(fullReference, out var def)) @@ -549,7 +608,36 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover } } - public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); + /// + /// Driver.AbLegacy-011 — synchronous teardown. Mirrors the body of + /// but never wraps the async path in + /// .AsTask().GetAwaiter().GetResult(). The poll engine's DisposeAsync is + /// drained with a ConfigureAwait(false) awaiter so a captured single-threaded + /// can never be the resumption target — the + /// classic sync-over-async deadlock cannot occur. Any other awaitable cleanup is + /// translated to direct synchronous calls (cancel probe CTSs, dispose runtimes). + /// + public void Dispose() + { + // ValueTask.ConfigureAwait(false).GetAwaiter().GetResult() — drains the poll engine + // shutdown on the current thread without capturing the SynchronizationContext. The + // engine cancels every loop CTS up-front then either completes immediately (no + // subscriptions, common case for the test fixture) or awaits a bounded WhenAll on the + // already-shutting-down loop tasks. We swallow exceptions so a buggy poll-loop teardown + // can't poison the rest of the disposal chain. + try { _poll.DisposeAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } catch { } + foreach (var state in _devices.Values) + { + try { state.ProbeCts?.Cancel(); } catch { } + state.ProbeCts?.Dispose(); + state.ProbeCts = null; + state.DisposeRuntimes(); + } + _devices.Clear(); + _tagsByName.Clear(); + _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + } + public async ValueTask DisposeAsync() => await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); internal sealed class DeviceState( @@ -626,6 +714,17 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover public CancellationTokenSource? ProbeCts { get; set; } public bool ProbeInitialized { get; set; } + /// + /// Driver.AbLegacy-005 — per-device latch for the structured "first non-zero + /// libplctag status" log. Reset to on a successful read so a + /// future outage re-fires the warning rather than being suppressed by a stale flag. + /// Concurrent readers on the same device may race the unlatched check + set, but the + /// worst case is a small finite number of duplicate warnings at outage onset (one per + /// racing reader) — which is preferable to either silently losing the first warning + /// or paying lock contention on the hot read path. + /// + public bool FirstNonZeroStatusLogged { get; set; } + public void DisposeRuntimes() { foreach (var r in Runtimes.Values) r.Dispose(); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs index 0fbccad..ab30672 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs @@ -1,5 +1,6 @@ using System.Text.Json; using System.Text.Json.Serialization; +using Microsoft.Extensions.Logging; using ZB.MOM.WW.OtOpcUa.Core.Hosting; using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; @@ -15,13 +16,23 @@ public static class AbLegacyDriverFactoryExtensions { public const string DriverTypeName = "AbLegacy"; - public static void Register(DriverFactoryRegistry registry) + /// + /// Register the AbLegacy factory with the driver registry. The optional + /// is captured at registration time and used to + /// construct an per driver instance — without it, + /// the driver runs with the null logger (existing tests and standalone callers stay + /// unchanged). Mirrors the Modbus driver registration pattern. + /// + public static void Register(DriverFactoryRegistry registry, ILoggerFactory? loggerFactory = null) { ArgumentNullException.ThrowIfNull(registry); - registry.Register(DriverTypeName, CreateInstance); + registry.Register(DriverTypeName, (id, json) => CreateInstance(id, json, loggerFactory)); } internal static AbLegacyDriver CreateInstance(string driverInstanceId, string driverConfigJson) + => CreateInstance(driverInstanceId, driverConfigJson, loggerFactory: null); + + internal static AbLegacyDriver CreateInstance(string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); @@ -63,7 +74,10 @@ public static class AbLegacyDriverFactoryExtensions Timeout = TimeSpan.FromMilliseconds(dto.TimeoutMs ?? 2_000), }; - return new AbLegacyDriver(options, driverInstanceId); + return new AbLegacyDriver( + options, driverInstanceId, + tagFactory: null, + logger: loggerFactory?.CreateLogger()); } private static T ParseEnum(string? raw, string driverInstanceId, string field, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDisposeAndResolveHostTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDisposeAndResolveHostTests.cs new file mode 100644 index 0000000..c670f5a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDisposeAndResolveHostTests.cs @@ -0,0 +1,158 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Driver.AbLegacy-011 — synchronous must perform real +/// synchronous teardown rather than blocking via DisposeAsync().AsTask().GetAwaiter().GetResult(). +/// Driver.AbLegacy-013 — fallback when the reference +/// is unknown and no devices are configured is the documented single-host fallback per the +/// IPerCallHostResolver contract. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyDisposeAndResolveHostTests +{ + // ---- Driver.AbLegacy-011 ---- + + [Fact] + public async Task Dispose_runs_teardown_without_blocking_on_async_wait() + { + // Build a driver with a real device + tag so InitializeAsync registers state, then Dispose. + // The teardown must clear the device dictionary just like ShutdownAsync would, but without + // round-tripping through AsTask().GetAwaiter().GetResult() (which would deadlock under a + // single-threaded synchronization context). + var factory = new FakeAbLegacyTagFactory(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0", AbLegacyPlcFamily.Slc500)], + Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-dispose", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + // Materialise one runtime so DisposeRuntimes has work to do. + await drv.ReadAsync(["X"], CancellationToken.None); + factory.Tags["N7:0"].Disposed.ShouldBeFalse(); + + drv.Dispose(); + + // The cached libplctag tag must be disposed and the device map cleared. + factory.Tags["N7:0"].Disposed.ShouldBeTrue(); + drv.DeviceCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + } + + [Fact] + public async Task Dispose_is_idempotent() + { + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1"); + await drv.InitializeAsync("{}", CancellationToken.None); + drv.Dispose(); + Should.NotThrow(() => drv.Dispose()); + } + + [Fact] + public async Task Dispose_under_single_threaded_sync_context_does_not_deadlock() + { + // The legacy sync-over-async pattern (DisposeAsync().AsTask().GetAwaiter().GetResult()) + // can deadlock if any awaited continuation marshals back to a captured single-threaded + // context. Drive a single-threaded SynchronizationContext + Dispose() and ensure it + // returns within a short timeout. + var factory = new FakeAbLegacyTagFactory(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + await drv.ReadAsync(["X"], CancellationToken.None); + + using var ctx = new SingleThreadSynchronizationContext(); + var prior = SynchronizationContext.Current; + SynchronizationContext.SetSynchronizationContext(ctx); + try + { + var disposed = new ManualResetEventSlim(false); + ctx.Post(_ => { drv.Dispose(); disposed.Set(); }, null); + ctx.RunUntil(disposed); + disposed.IsSet.ShouldBeTrue("Dispose must return without blocking on the single-threaded context"); + } + finally + { + SynchronizationContext.SetSynchronizationContext(prior); + } + } + + /// + /// Minimal cooperative single-threaded SynchronizationContext for the deadlock-regression + /// test. The thread that calls pumps queued callbacks until the + /// stop event is set. + /// + private sealed class SingleThreadSynchronizationContext : SynchronizationContext, IDisposable + { + private readonly System.Collections.Concurrent.BlockingCollection<(SendOrPostCallback, object?)> _queue = new(); + + public override void Post(SendOrPostCallback d, object? state) => _queue.Add((d, state)); + public override void Send(SendOrPostCallback d, object? state) => d(state); + + public void RunUntil(ManualResetEventSlim stop) + { + while (!stop.IsSet) + { + if (_queue.TryTake(out var item, TimeSpan.FromSeconds(2))) + item.Item1(item.Item2); + else + throw new TimeoutException("Dispose did not complete — likely sync-over-async deadlock"); + } + } + + public void Dispose() => _queue.Dispose(); + } + + // ---- Driver.AbLegacy-013 ---- + + [Fact] + public void ResolveHost_known_reference_returns_tag_device() + { + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)], + }, "drv-1"); + drv.ResolveHost("X").ShouldBe("ab://10.0.0.5/1,0"); + } + + [Fact] + public void ResolveHost_unknown_reference_with_devices_returns_first_device() + { + // Multi-device fallback: an unknown reference returns the first configured device so the + // resilience pipeline keys on a real ab:// host rather than the instance id. + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = + [ + new AbLegacyDeviceOptions("ab://10.0.0.5/1,0"), + new AbLegacyDeviceOptions("ab://10.0.0.6/"), + ], + }, "drv-1"); + drv.ResolveHost("unknown").ShouldBe("ab://10.0.0.5/1,0"); + } + + [Fact] + public void ResolveHost_unknown_reference_no_devices_returns_driver_instance_id() + { + // Per IPerCallHostResolver: implementations MUST NOT throw on an unknown reference; they + // must return the driver's default-host string. With no devices configured the driver + // instance id is the documented single-host fallback. + var drv = new AbLegacyDriver(new AbLegacyDriverOptions(), "drv-singleton"); + drv.ResolveHost("anything").ShouldBe("drv-singleton"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyLoggerInjectionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyLoggerInjectionTests.cs new file mode 100644 index 0000000..835d565 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyLoggerInjectionTests.cs @@ -0,0 +1,90 @@ +using Microsoft.Extensions.Logging; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Driver.AbLegacy-005 — verifies the driver accepts and uses an optional +/// . Probe transitions, init failures, and the first +/// non-zero libplctag status per device must be logged (rather than only folded into the +/// transient string) so a field operator can correlate a +/// PCCC comms problem with a structured log entry. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyLoggerInjectionTests +{ + private sealed class CapturingLogger : ILogger + { + public readonly List<(LogLevel Level, string Message)> Entries = new(); + public IDisposable BeginScope(TState state) where TState : notnull => NullScope.Instance; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + => Entries.Add((logLevel, formatter(state, exception))); + private sealed class NullScope : IDisposable + { + public static readonly NullScope Instance = new(); + public void Dispose() { } + } + } + + [Fact] + public void Driver_accepts_optional_logger_parameter() + { + // Constructor must accept an ILogger as an optional named arg, matching + // the Modbus/S7/Galaxy driver pattern. The driver runs with NullLogger when omitted. + var logger = new CapturingLogger(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions(), "drv-logged", tagFactory: null, logger: logger); + drv.ShouldNotBeNull(); + } + + [Fact] + public async Task InitializeAsync_failure_emits_error_log() + { + var logger = new CapturingLogger(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("not-a-valid-address")], + }, "drv-logged", tagFactory: null, logger: logger); + + await Should.ThrowAsync( + () => drv.InitializeAsync("{}", CancellationToken.None)); + + var errors = logger.Entries.Where(e => e.Level >= LogLevel.Error).ToList(); + errors.ShouldNotBeEmpty("init failure must surface as an Error-level log"); + errors[0].Message.ShouldContain("drv-logged"); + } + + [Fact] + public async Task First_nonzero_libplctag_status_per_device_is_logged() + { + // The driver should log the first occurrence of a non-zero libplctag status per device, + // so that a field operator can correlate a comms problem with a structured log entry + // even though Detail is overwritten by the next read. + var factory = new FakeAbLegacyTagFactory(); + var logger = new CapturingLogger(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-logged", factory, logger); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = p => new FakeAbLegacyTag(p) { Status = -32 }; // timeout + + await drv.ReadAsync(["X"], CancellationToken.None); + + var warnings = logger.Entries.Where(e => e.Level == LogLevel.Warning).ToList(); + warnings.ShouldNotBeEmpty("first non-zero libplctag status must emit a structured warning"); + warnings.Any(w => w.Message.Contains("ab://10.0.0.5/1,0") || w.Message.Contains("drv-logged")) + .ShouldBeTrue("warning must identify the device or driver instance"); + + // Second read with the same status — the per-device de-dupe should suppress. + var warningCountAfterFirst = warnings.Count; + await drv.ReadAsync(["X"], CancellationToken.None); + var newWarnings = logger.Entries.Where(e => e.Level == LogLevel.Warning).ToList(); + newWarnings.Count.ShouldBe(warningCountAfterFirst, "subsequent same-device non-zero status stays quiet"); + } +}