From 48bd266e1905d2b15a9d19386281406fb15e4532 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 06:53:57 -0400 Subject: [PATCH] feat(client): AddComment + comment CLI, drop Galaxy-era alarm fallback, serialize CLI tests (#598); Client.Shared off obsolete sync SDK APIs (#621) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/Client.CLI.md | 17 ++- .../Commands/CommentCommand.cs | 85 ++++++++++++ .../Adapters/ClientTelemetry.cs | 39 ++++++ .../DefaultApplicationConfigurationFactory.cs | 4 +- .../Adapters/DefaultEndpointDiscovery.cs | 18 +-- .../Adapters/DefaultSessionAdapter.cs | 2 +- .../Adapters/DefaultSessionFactory.cs | 8 +- .../Adapters/DefaultSubscriptionAdapter.cs | 2 +- .../Adapters/IEndpointDiscovery.cs | 5 +- .../IOpcUaClientService.cs | 10 ++ .../OpcUaClientService.cs | 101 +++++--------- .../AssemblyInfo.cs | 6 + .../CommentCommandTests.cs | 106 ++++++++++++++ .../Fakes/FakeOpcUaClientService.cs | 14 ++ .../Fakes/FakeEndpointDiscovery.cs | 10 +- .../OpcUaClientServiceTests.cs | 131 ++---------------- .../Fakes/FakeOpcUaClientService.cs | 4 + .../ClientAlarmMethodTests.cs | 12 +- 18 files changed, 363 insertions(+), 211 deletions(-) create mode 100644 src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/CommentCommand.cs create mode 100644 src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ClientTelemetry.cs create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AssemblyInfo.cs create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommentCommandTests.cs diff --git a/docs/Client.CLI.md b/docs/Client.CLI.md index 1b3a7dd6..7c6f1883 100644 --- a/docs/Client.CLI.md +++ b/docs/Client.CLI.md @@ -4,7 +4,7 @@ `ZB.MOM.WW.OtOpcUa.Client.CLI` is a cross-platform command-line client for the OtOpcUa OPC UA server. It targets .NET 10 and uses the shared `IOpcUaClientService` from `Client.Shared` for all OPC UA operations. Commands are routed and parsed by [CliFx](https://github.com/Tyrrrz/CliFx). -The CLI is the primary tool for operators and developers to test and interact with the server from a terminal. It supports all core operations: connectivity testing, browsing, reading, writing, subscriptions, alarm monitoring, alarm operator actions (acknowledge / confirm / shelve / enable / disable), history reads, and redundancy queries. Any driver surface exposed by the server (Galaxy, Modbus, S7, AB CIP, AB Legacy, TwinCAT, FOCAS, OPC UA Client) is reachable through these commands — the CLI is driver-agnostic because everything below the OPC UA endpoint is. +The CLI is the primary tool for operators and developers to test and interact with the server from a terminal. It supports all core operations: connectivity testing, browsing, reading, writing, subscriptions, alarm monitoring, alarm operator actions (acknowledge / confirm / comment / shelve / enable / disable), history reads, and redundancy queries. Any driver surface exposed by the server (Galaxy, Modbus, S7, AB CIP, AB Legacy, TwinCAT, FOCAS, OPC UA Client) is reachable through these commands — the CLI is driver-agnostic because everything below the OPC UA endpoint is. ## Build and Run @@ -284,6 +284,21 @@ Example output: Confirm successful: ns=1;s=TestMachine_001 ``` +### comment + +Attaches an operator comment to an alarm condition (OPC UA Part 9 `AddComment`) without changing its acknowledge/confirm state. Works on scripted and native conditions alike. The `--node` and `--event-id` are the same values `ack`/`confirm` take. + +```bash +otopcua-cli comment -u opc.tcp://localhost:4840/OtOpcUa \ + -n "ns=2;s=Plant/Line1/Pump1/HighTemp" -e 0A1B2C -c "Checked on site" +``` + +| Flag | Description | +|---|---| +| `-n` / `--node` | Condition node ID of the alarm to comment on (**required**) | +| `-e` / `--event-id` | Hex-encoded EventId from the alarm notification (**required**) | +| `-c` / `--comment` | Operator comment to attach (**required**) | + ### shelve Shelves or unshelves an active alarm condition via the OPC UA Part 9 `ShelvedStateMachine` (`OneShotShelve` / `TimedShelve` / `Unshelve`). Gated on the `AlarmAck` role. `--duration` is given in **seconds** (converted to milliseconds for the OPC UA call) and is required only for `--kind Timed`. The `--node` is the condition node itself (the `SourceNode`/condition id an `alarms` notification reports): `ack`, `confirm`, `shelve`, `enable` and `disable` call the Part 9 method ids directly on that node — no `.Condition` / `.ShelvingState` suffix is appended (those child nodes do not exist on the OtOpcUa server). diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/CommentCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/CommentCommand.cs new file mode 100644 index 00000000..858e6d9e --- /dev/null +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/CommentCommand.cs @@ -0,0 +1,85 @@ +using CliFx.Attributes; +using CliFx.Exceptions; +using CliFx.Infrastructure; +using Opc.Ua; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; +using ZB.MOM.WW.OtOpcUa.Client.Shared; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; + +[Command("comment", Description = "Add an operator comment to an alarm condition (OPC UA Part 9 AddComment)")] +public class CommentCommand : CommandBase +{ + /// + /// Creates the comment command used to attach an operator comment to an OPC UA condition from the terminal. + /// + /// The factory that creates the shared client service for the command run. + public CommentCommand(IOpcUaClientServiceFactory factory) : base(factory) + { + } + + /// + /// Gets the condition node ID of the alarm event to comment on. + /// + [CommandOption("node", 'n', Description = "Condition node ID of the alarm to comment on", IsRequired = true)] + public string NodeId { get; init; } = default!; + + /// + /// Gets the event ID (hex-encoded) returned by the server in the alarm notification. + /// + [CommandOption("event-id", 'e', Description = "EventId from the alarm notification (hex-encoded byte array)", IsRequired = true)] + public string EventId { get; init; } = default!; + + /// + /// Gets the operator comment to attach to the condition. + /// + [CommandOption("comment", 'c', Description = "Operator comment to attach", IsRequired = true)] + public string Comment { get; init; } = string.Empty; + + /// + public override async ValueTask ExecuteAsync(IConsole console) + { + ConfigureLogging(); + + try + { + NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + + byte[] eventId; + try + { + eventId = Convert.FromHexString(EventId); + } + catch (FormatException ex) + { + throw new CommandException($"Invalid --event-id value: {ex.Message} (expected hex-encoded bytes, e.g. 0A1B2C)"); + } + + IOpcUaClientService? service = null; + try + { + var ct = console.RegisterCancellationHandler(); + (service, _) = await CreateServiceAndConnectAsync(ct); + + var statusCode = await service.AddCommentAsync(NodeId, eventId, Comment, ct); + + if (StatusCode.IsGood(statusCode)) + await console.Output.WriteLineAsync($"Comment added: {NodeId}"); + else + await console.Output.WriteLineAsync($"Comment failed: {statusCode}"); + } + finally + { + if (service != null) + { + await service.DisconnectAsync(); + service.Dispose(); + } + } + } +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ClientTelemetry.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ClientTelemetry.cs new file mode 100644 index 00000000..d574bb18 --- /dev/null +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ClientTelemetry.cs @@ -0,0 +1,39 @@ +using Opc.Ua; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters; + +/// +/// The one the shared client hands the SDK's non-obsolete (telemetry-taking) +/// overloads. An empty builder keeps SDK internals off any logging provider; the client logs through Serilog. +/// +internal static class ClientTelemetry +{ + private static readonly Lazy Instance = new(() => DefaultTelemetry.Create(_ => { })); + + /// Gets the shared telemetry context. + public static ITelemetryContext Context => Instance.Value; +} + +/// +/// Bounded sync-over-async for the Dispose paths, which cannot await. Runs on the thread pool so a +/// caller on a synchronization context (the Avalonia UI thread) cannot deadlock, and waits at most +/// so a dead server cannot hang teardown. +/// +internal static class DisposeBridge +{ + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(5); + + /// Runs to completion (or the timeout), swallowing every failure. + public static void Run(Func work) + { + using var cts = new CancellationTokenSource(Timeout); + try + { + Task.Run(() => work(cts.Token), cts.Token).Wait(Timeout); + } + catch + { + // Teardown is best-effort, exactly as the synchronous Close/Delete it replaces. + } + } +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs index 3165f322..ffdaffd6 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultApplicationConfigurationFactory.cs @@ -58,14 +58,14 @@ internal sealed class DefaultApplicationConfigurationFactory : IApplicationConfi } }; - await config.Validate(ApplicationType.Client); + await config.ValidateAsync(ApplicationType.Client, ct); if (settings.AutoAcceptCertificates) config.CertificateValidator.CertificateValidation += (_, e) => e.Accept = true; if (settings.SecurityMode != SecurityMode.None) { - var app = new ApplicationInstance + var app = new ApplicationInstance(ClientTelemetry.Context) { ApplicationName = "OtOpcUaClient", ApplicationType = ApplicationType.Client, diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs index 0add4d9c..4ed0ad71 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs @@ -12,18 +12,14 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery private static readonly ILogger Logger = Log.ForContext(); /// - public EndpointDescription SelectEndpoint(ApplicationConfiguration config, string endpointUrl, - MessageSecurityMode requestedMode) + public async Task SelectEndpointAsync(ApplicationConfiguration config, string endpointUrl, + MessageSecurityMode requestedMode, CancellationToken ct = default) { - if (requestedMode == MessageSecurityMode.None) - { -#pragma warning disable CS0618 // Acceptable for endpoint selection - return CoreClientUtils.SelectEndpoint(config, endpointUrl, false); -#pragma warning restore CS0618 - } - - using var client = DiscoveryClient.Create(new Uri(endpointUrl)); - var allEndpoints = client.GetEndpoints(null); + // One path for every mode: discover, filter by mode, rewrite the host. (The None branch used to call the + // obsolete synchronous CoreClientUtils.SelectEndpoint, which does the same filter + rewrite.) + using var client = await DiscoveryClient.CreateAsync( + config, new Uri(endpointUrl), DiagnosticsMasks.None, ct).ConfigureAwait(false); + var allEndpoints = await client.GetEndpointsAsync(null, ct).ConfigureAwait(false); return EndpointSelector.SelectBest(allEndpoints, endpointUrl, requestedMode); } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs index 41440590..9b4bbe05 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs @@ -264,7 +264,7 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter { try { - if (_session.Connected) _session.Close(); + if (_session.Connected) DisposeBridge.Run(ct => _session.CloseAsync(ct)); } catch { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionFactory.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionFactory.cs index 44dc77f8..2cab8d81 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionFactory.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionFactory.cs @@ -23,14 +23,18 @@ internal sealed class DefaultSessionFactory : ISessionFactory var endpointConfig = EndpointConfiguration.Create(config); var configuredEndpoint = new ConfiguredEndpoint(null, endpoint, endpointConfig); - var session = await Session.Create( + var created = await new Opc.Ua.Client.DefaultSessionFactory(ClientTelemetry.Context).CreateAsync( config, configuredEndpoint, false, sessionName, sessionTimeoutMs, identity, - null); + null, + ct); + var session = created as Session + ?? throw new InvalidOperationException( + $"Session factory returned {created.GetType().Name}, expected {nameof(Session)}."); Logger.Information("Session created: {SessionName} -> {EndpointUrl}", sessionName, endpoint.EndpointUrl); return new DefaultSessionAdapter(session); diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs index b5b3c4ab..deb15dd5 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs @@ -123,7 +123,7 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter { try { - _subscription.Delete(true); + DisposeBridge.Run(ct => _subscription.DeleteAsync(true, ct)); } catch { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/IEndpointDiscovery.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/IEndpointDiscovery.cs index b3fe2456..4b40e945 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/IEndpointDiscovery.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/IEndpointDiscovery.cs @@ -14,7 +14,8 @@ internal interface IEndpointDiscovery /// The OPC UA application configuration. /// The endpoint URL to discover. /// The requested message security mode. + /// Cancellation token. /// The best-matching endpoint description, with its URL hostname rewritten to match when they differ. - EndpointDescription SelectEndpoint(ApplicationConfiguration config, string endpointUrl, - MessageSecurityMode requestedMode); + Task SelectEndpointAsync(ApplicationConfiguration config, string endpointUrl, + MessageSecurityMode requestedMode, CancellationToken ct = default); } \ No newline at end of file diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs index e0d31b9e..6874816f 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/IOpcUaClientService.cs @@ -127,6 +127,16 @@ public interface IOpcUaClientService : IDisposable /// Task ConfirmAlarmAsync(string conditionNodeId, byte[] eventId, string comment, CancellationToken ct = default); + /// + /// Attaches an operator comment to a condition (Part 9 AddComment) using the event identifier of an alarm notification. + /// + /// The condition node the comment is attached to. + /// The event identifier returned by the OPC UA server for the alarm event. + /// The operator comment. + /// The cancellation token that aborts the request. + /// on success, or the server's bad when the call is refused. + Task AddCommentAsync(string conditionNodeId, byte[] eventId, string comment, CancellationToken ct = default); + /// /// Shelves or unshelves an active alarm condition (OPC UA Part 9 ShelvedStateMachine). /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index 595d15b5..fc9ee3b7 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -428,6 +428,35 @@ public sealed class OpcUaClientService : IOpcUaClientService return StatusCodes.Good; } + /// + public async Task AddCommentAsync(string conditionNodeId, byte[] eventId, string comment, + CancellationToken ct = default) + { + ThrowIfDisposed(); + ThrowIfNotConnected(); + + // AddComment is a ConditionType method, invoked on the condition node itself. + var conditionObjId = NodeId.Parse(conditionNodeId); + + try + { + await _session!.CallMethodAsync( + conditionObjId, + MethodIds.ConditionType_AddComment, + [eventId, new LocalizedText(comment)], + ct); + } + catch (ServiceResultException ex) + { + Logger.Warning(ex, "Failed to add comment on {ConditionId} (status {Status})", + conditionNodeId, ex.StatusCode); + return ex.StatusCode; + } + + Logger.Debug("Added comment on {ConditionId}", conditionNodeId); + return StatusCodes.Good; + } + /// public async Task ShelveAlarmAsync(string conditionNodeId, ShelveKind kind, double shelvingTimeSeconds = 0, CancellationToken ct = default) @@ -633,7 +662,7 @@ public sealed class OpcUaClientService : IOpcUaClientService var config = await _configFactory.CreateAsync(effectiveSettings, ct); var requestedMode = SecurityModeMapper.ToMessageSecurityMode(settings.SecurityMode); - var endpoint = _endpointDiscovery.SelectEndpoint(config, endpointUrl, requestedMode); + var endpoint = await _endpointDiscovery.SelectEndpointAsync(config, endpointUrl, requestedMode, ct); var identity = settings.Username != null ? new UserIdentity(settings.Username, Encoding.UTF8.GetBytes(settings.Password ?? "")) @@ -824,74 +853,8 @@ public sealed class OpcUaClientService : IOpcUaClientService var ackedState = ackedField ?? false; var activeState = activeField ?? false; - // Fallback: read InAlarm/Acked from condition node Galaxy attributes - // when the server doesn't populate standard event fields. - // Must run on a background thread to avoid deadlocking the notification thread. - // Capture the session reference now; skip supplemental reads if the session has - // been replaced by a concurrent failover before the Task.Run body executes. - if (ackedField == null && activeField == null && conditionNodeId != null && _session != null) - { - var session = _session; - var capturedConditionNodeId = conditionNodeId; - var capturedMessage = message; - _ = Task.Run(async () => - { - // If the session was replaced by a failover before we started reading, - // skip the supplemental reads to avoid hitting a disposed session. - if (!ReferenceEquals(session, _session)) - return; - - try - { - var inAlarmValue = await session.ReadValueAsync(NodeId.Parse($"{capturedConditionNodeId}.InAlarm")); - if (inAlarmValue.Value is bool inAlarm) activeState = inAlarm; - - var ackedValue = await session.ReadValueAsync(NodeId.Parse($"{capturedConditionNodeId}.Acked")); - if (ackedValue.Value is bool acked) ackedState = acked; - - if (time == DateTime.MinValue && activeState) - { - var timeValue = await session.ReadValueAsync(NodeId.Parse($"{capturedConditionNodeId}.TimeAlarmOn")); - if (timeValue.Value is DateTime alarmTime && alarmTime != DateTime.MinValue) - time = alarmTime; - } - - // Read alarm description to use as message - try - { - var descValue = await session.ReadValueAsync(NodeId.Parse($"{capturedConditionNodeId}.DescAttrName")); - if (descValue.Value is string desc && !string.IsNullOrEmpty(desc)) - capturedMessage = desc; - } - catch { /* DescAttrName may not exist */ } - } - catch (ObjectDisposedException) - { - // Session was disposed during supplemental reads (concurrent failover); - // skip the event rather than delivering stale/default states. - Logger.Debug("Supplemental alarm read skipped — session disposed during failover for {ConditionNodeId}", capturedConditionNodeId); - return; - } - catch - { - // Other supplemental read failure; deliver event with defaults - } - - // OperatorComment is intentionally left null (default) here. - // Two concrete reasons make it unrecoverable in this Galaxy sub-attribute fallback path: - // (a) Galaxy's AckMsg attribute is WRITE-ONLY — there is no readable attribute that - // exposes the last operator comment, so no supplemental read can recover it. - // (b) The OPC UA event SelectClause (CreateAlarmEventFilter) carries no comment field; - // the OPC UA event notification itself never delivers a comment string. - // The operator comment is only available via the gateway's native alarm feed - // (the Galaxy driver path, GalaxyAlarmTransition.OperatorComment), not this - // OpcUaClient sub-attribute fallback. - AlarmEvent?.Invoke(this, new AlarmEventArgs( - sourceName, conditionName, severity, capturedMessage, retain, activeState, ackedState, time, - eventId, capturedConditionNodeId)); - }); - return; - } + // No Galaxy-era supplemental reads (.InAlarm/.Acked/...): the server publishes the + // standard Part 9 fields. A foreign server that leaves both null reports Active=false/Acked=false. AlarmEvent?.Invoke(this, new AlarmEventArgs( sourceName, conditionName, severity, message, retain, activeState, ackedState, time, diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AssemblyInfo.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AssemblyInfo.cs new file mode 100644 index 00000000..b1a9c8cb --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AssemblyInfo.cs @@ -0,0 +1,6 @@ +using Xunit; + +// Every command run reassigns the process-global Serilog Log.Logger (CommandBase.ConfigureLogging), and +// LoggerLifecycleTests asserts on that global's dispose lifecycle. A collection attribute on that one class +// cannot serialize it against the ~20 other classes that run commands, so the whole (small) suite runs serially. (#598) +[assembly: CollectionBehavior(DisableTestParallelization = true)] diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommentCommandTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommentCommandTests.cs new file mode 100644 index 00000000..eb92e7e8 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommentCommandTests.cs @@ -0,0 +1,106 @@ +using CliFx.Exceptions; +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +public class CommentCommandTests +{ + private static string HexOf(byte[] bytes) => Convert.ToHexString(bytes); + + /// Verifies that a successful comment prints a success message and passes correct arguments to the service. + [Fact] + public async Task Execute_Success_PrintsSuccessMessage() + { + var fakeService = new FakeOpcUaClientService + { + AddCommentResult = StatusCodes.Good + }; + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var eventId = new byte[] { 0x0A, 0x1B, 0x2C }; + var command = new CommentCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=MyAlarm", + EventId = HexOf(eventId), + Comment = "operator note" + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("Comment added"); + fakeService.AddCommentCalls.Count.ShouldBe(1); + fakeService.AddCommentCalls[0].ConditionNodeId.ShouldBe("ns=2;s=MyAlarm"); + fakeService.AddCommentCalls[0].EventId.ShouldBe(eventId); + fakeService.AddCommentCalls[0].Comment.ShouldBe("operator note"); + } + + /// Verifies that a bad StatusCode from the service prints a failure message. + [Fact] + public async Task Execute_BadStatus_PrintsFailureMessage() + { + var fakeService = new FakeOpcUaClientService + { + AddCommentResult = StatusCodes.BadNotSupported + }; + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new CommentCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=MyAlarm", + EventId = HexOf(new byte[] { 0xFF }), + Comment = "" + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("Comment failed"); + } + + /// Verifies that an invalid hex event-id throws CommandException. + [Fact] + public async Task Execute_InvalidEventId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new CommentCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=MyAlarm", + EventId = "ZZZ-not-hex", + Comment = "" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("event-id", Case.Insensitive); + } + + /// Verifies that the command disconnects in the finally block. + [Fact] + public async Task Execute_DisconnectsInFinally() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new CommentCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=MyAlarm", + EventId = HexOf(new byte[] { 0x01 }), + Comment = "" + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.DisconnectCalled.ShouldBeTrue(); + fakeService.DisposeCalled.ShouldBeTrue(); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/Fakes/FakeOpcUaClientService.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/Fakes/FakeOpcUaClientService.cs index 4c3f308c..aeb22446 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/Fakes/FakeOpcUaClientService.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/Fakes/FakeOpcUaClientService.cs @@ -264,6 +264,20 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService return Task.FromResult(ConfirmAlarmResult); } + /// Gets the list of (conditionNodeId, eventId, comment) tuples from AddCommentAsync calls. + public List<(string ConditionNodeId, byte[] EventId, string Comment)> AddCommentCalls { get; } = []; + + /// Gets or sets the status code returned by AddCommentAsync. + public StatusCode AddCommentResult { get; set; } = StatusCodes.Good; + + /// + public Task AddCommentAsync(string conditionNodeId, byte[] eventId, string comment, + CancellationToken ct = default) + { + AddCommentCalls.Add((conditionNodeId, eventId, comment)); + return Task.FromResult(AddCommentResult); + } + /// Gets the list of (conditionNodeId, kind, durationSeconds) tuples from ShelveAlarmAsync calls. public List<(string ConditionNodeId, ShelveKind Kind, double DurationSeconds)> ShelveAlarmCalls { get; } = []; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeEndpointDiscovery.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeEndpointDiscovery.cs index 59cb537d..ccbc3f99 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeEndpointDiscovery.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeEndpointDiscovery.cs @@ -17,8 +17,8 @@ internal sealed class FakeEndpointDiscovery : IEndpointDiscovery /// The endpoint URL to select. /// The requested security mode. /// The selected endpoint description. - public EndpointDescription SelectEndpoint(ApplicationConfiguration config, string endpointUrl, - MessageSecurityMode requestedMode) + public Task SelectEndpointAsync(ApplicationConfiguration config, string endpointUrl, + MessageSecurityMode requestedMode, CancellationToken ct = default) { SelectCallCount++; LastEndpointUrl = endpointUrl; @@ -26,7 +26,7 @@ internal sealed class FakeEndpointDiscovery : IEndpointDiscovery if (ThrowOnSelect) throw new InvalidOperationException($"No endpoint found for {endpointUrl}"); - return new EndpointDescription + return Task.FromResult(new EndpointDescription { EndpointUrl = endpointUrl, SecurityMode = requestedMode, @@ -38,6 +38,6 @@ internal sealed class FakeEndpointDiscovery : IEndpointDiscovery ApplicationName = "FakeServer", ApplicationUri = "urn:localhost:FakeServer" } - }; + }); } -} \ No newline at end of file +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs index d74a1bfe..a604fb08 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs @@ -1263,52 +1263,27 @@ public class OpcUaClientServiceTests : IDisposable result.Code.ShouldBe(StatusCodes.BadConditionAlreadyDisabled); } - // --- Alarm fallback path (Client.Shared-011) --- + // --- Missing standard alarm fields (#598: the Galaxy-era supplemental reads are gone) --- /// - /// Regression for Client.Shared-011: when standard AckedState/Id and ActiveState/Id - /// fields are missing (null Value) but the SourceNode (ConditionId) field at index 12 - /// is populated, the client launches the Task.Run fallback that reads - /// InAlarm/Acked from the condition node's Galaxy attributes. Verify - /// the alarm event is delivered with the values from the supplemental reads. + /// When the standard AckedState/Id and ActiveState/Id fields arrive null the event is delivered with + /// Active=false / Acked=false and the client issues NO supplemental reads of + /// <condition>.InAlarm/.Acked (those attributes only existed on the retired Galaxy v2 server). /// [Fact] - public async Task OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent() + public async Task OnAlarmEvent_MissingAckedActive_DeliversDefaultsWithoutExtraReads() { var fakeSub = new FakeSubscriptionAdapter(); - var session = new FakeSessionAdapter - { - NextSubscription = fakeSub, - ReadResponseFunc = nodeId => - { - var key = nodeId.ToString(); - if (key.EndsWith(".InAlarm")) - return new DataValue(new Variant(true), StatusCodes.Good); - if (key.EndsWith(".Acked")) - return new DataValue(new Variant(false), StatusCodes.Good); - if (key.EndsWith(".TimeAlarmOn")) - return new DataValue(new Variant(new DateTime(2026, 1, 1, 12, 0, 0)), StatusCodes.Good); - if (key.EndsWith(".DescAttrName")) - return new DataValue(new Variant("Fallback message"), StatusCodes.Good); - return new DataValue(StatusCodes.BadNodeIdUnknown); - } - }; + var session = new FakeSessionAdapter { NextSubscription = fakeSub }; _sessionFactory.EnqueueSession(session); await _service.ConnectAsync(ValidSettings()); AlarmEventArgs? received = null; - var raised = new TaskCompletionSource(); - _service.AlarmEvent += (_, e) => - { - received = e; - raised.TrySetResult(); - }; + _service.AlarmEvent += (_, e) => received = e; await _service.SubscribeAlarmsAsync(); var handle = fakeSub.ActiveHandles.First(); - // AckedState/Id (8) and ActiveState/Id (9) are present but Variant.Value is null, - // which triggers the supplemental Galaxy-attribute fallback; SourceNode (12) is set. var fields = new EventFieldList { EventFields = @@ -1321,8 +1296,8 @@ public class OpcUaClientServiceTests : IDisposable new Variant((ushort)400), // 5: Severity new Variant("CondName"), // 6: ConditionName new Variant(true), // 7: Retain - Variant.Null, // 8: AckedState/Id — missing - Variant.Null, // 9: ActiveState/Id — missing + Variant.Null, // 8: AckedState/Id - missing + Variant.Null, // 9: ActiveState/Id - missing new Variant(true), // 10: EnabledState/Id new Variant(false), // 11: SuppressedOrShelved new Variant("ns=2;s=ConditionId") // 12: SourceNode @@ -1330,90 +1305,14 @@ public class OpcUaClientServiceTests : IDisposable }; fakeSub.SimulateEvent(handle, fields); - // The fallback runs on a background Task.Run continuation — wait briefly for it. - await Task.WhenAny(raised.Task, Task.Delay(500)); - + // Delivery is now synchronous (no background fallback), so the event is already here. received.ShouldNotBeNull(); - received!.ActiveState.ShouldBeTrue(); // from InAlarm read - received.AckedState.ShouldBeFalse(); // from Acked read + received!.ActiveState.ShouldBeFalse(); + received.AckedState.ShouldBeFalse(); received.ConditionNodeId.ShouldBe("ns=2;s=ConditionId"); - received.Message.ShouldBe("Fallback message"); // from DescAttrName read - } - - /// - /// Pins the accepted limitation that OperatorComment is always null on the Galaxy - /// sub-attribute fallback path. Two concrete reasons make it unrecoverable: - /// (a) Galaxy's AckMsg attribute is WRITE-ONLY — no readable attribute exposes the - /// last operator comment — and (b) the OPC UA event SelectClause carries no comment - /// field. The operator comment is only available via the gateway's native alarm feed - /// (GalaxyAlarmTransition.OperatorComment in the Galaxy driver path, not here). - /// - [Fact] - public async Task OnAlarmEvent_GalaxyFallback_LeavesOperatorCommentNull() - { - var fakeSub = new FakeSubscriptionAdapter(); - var session = new FakeSessionAdapter - { - NextSubscription = fakeSub, - ReadResponseFunc = nodeId => - { - var key = nodeId.ToString(); - if (key.EndsWith(".InAlarm")) - return new DataValue(new Variant(true), StatusCodes.Good); - if (key.EndsWith(".Acked")) - return new DataValue(new Variant(true), StatusCodes.Good); - if (key.EndsWith(".TimeAlarmOn")) - return new DataValue(new Variant(new DateTime(2026, 1, 1, 12, 0, 0)), StatusCodes.Good); - if (key.EndsWith(".DescAttrName")) - return new DataValue(new Variant("Ack message"), StatusCodes.Good); - return new DataValue(StatusCodes.BadNodeIdUnknown); - } - }; - _sessionFactory.EnqueueSession(session); - await _service.ConnectAsync(ValidSettings()); - - AlarmEventArgs? received = null; - var raised = new TaskCompletionSource(); - _service.AlarmEvent += (_, e) => - { - received = e; - raised.TrySetResult(); - }; - - await _service.SubscribeAlarmsAsync(); - - var handle = fakeSub.ActiveHandles.First(); - // AckedState/Id (8) and ActiveState/Id (9) are null, triggering the Galaxy - // sub-attribute fallback; SourceNode (12) is set so conditionNodeId is populated. - var fields = new EventFieldList - { - EventFields = - [ - new Variant(new byte[] { 4, 5, 6 }), // 0: EventId - new Variant(ObjectTypeIds.AlarmConditionType), // 1: EventType - new Variant("Source2"), // 2: SourceName - new Variant(DateTime.MinValue), // 3: Time - new Variant(new LocalizedText("Ack")), // 4: Message - new Variant((ushort)200), // 5: Severity - new Variant("CondName2"), // 6: ConditionName - new Variant(true), // 7: Retain - Variant.Null, // 8: AckedState/Id — missing - Variant.Null, // 9: ActiveState/Id — missing - new Variant(true), // 10: EnabledState/Id - new Variant(false), // 11: SuppressedOrShelved - new Variant("ns=2;s=ConditionId2") // 12: SourceNode - ] - }; - fakeSub.SimulateEvent(handle, fields); - - // The fallback runs on a background Task.Run continuation — wait briefly for it. - await Task.WhenAny(raised.Task, Task.Delay(500)); - - received.ShouldNotBeNull(); - // OperatorComment must be null: AckMsg is WRITE-ONLY in Galaxy and the OPC UA - // event SelectClause carries no comment field — both reasons are unrecoverable - // on this client sub-attribute fallback path. - received!.OperatorComment.ShouldBeNull(); + received.Message.ShouldBe("Initial"); + received.OperatorComment.ShouldBeNull(); + session.ReadCount.ShouldBe(0); } // --- Failover tests --- diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/Fakes/FakeOpcUaClientService.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/Fakes/FakeOpcUaClientService.cs index 210abbb8..a44d7a08 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/Fakes/FakeOpcUaClientService.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/Fakes/FakeOpcUaClientService.cs @@ -282,6 +282,10 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService return Task.FromResult(ConfirmResult); } + /// + public Task AddCommentAsync(string conditionNodeId, byte[] eventId, string comment, + CancellationToken ct = default) => Task.FromResult(StatusCodes.Good); + /// Gets or sets the status code returned by shelve operations in UI tests. public StatusCode ShelveResult { get; set; } = StatusCodes.Good; /// Gets or sets the exception thrown to simulate alarm shelve failures in the UI. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs index 418a48fc..615ed60a 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs @@ -18,7 +18,7 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; /// /// The condition id is passed exactly as a CLI user passes --node: the condition node's own id. /// Each call must return Good and move the condition — read back from the condition events the -/// server reports, not from the call result alone. The client exposes no AddComment API, so it is not covered. +/// server reports, not from the call result alone. AddComment is covered on both the scripted and the native condition (the native one supports Acknowledge and AddComment only). /// public sealed class ClientAlarmMethodTests { @@ -66,6 +66,10 @@ public sealed class ClientAlarmMethodTests conditionId, events.LatestEventId(item, "base"), "confirm", ct)).ShouldBeTrue(); (await Next(++n))[RecordedEvents.ConfirmedField].Value.ShouldBe(true); + StatusCode.IsGood(await client.AddCommentAsync( + conditionId, events.LatestEventId(item, "base"), "note", ct)).ShouldBeTrue(); + ((await Next(++n))[RecordedEvents.CommentField].Value as LocalizedText).ShouldNotBeNull().Text.ShouldBe("note"); + StatusCode.IsGood(await client.ShelveAlarmAsync(conditionId, ShelveKind.OneShot, 0, ct)).ShouldBeTrue(); (await Next(++n))[RecordedEvents.ShelvingField].Value.ShouldBe(ObjectIds.ShelvedStateMachineType_OneShotShelved); @@ -120,6 +124,12 @@ public sealed class ClientAlarmMethodTests ack[RecordedEvents.ConfirmedField].Value.ShouldBe(true); ack[RecordedEvents.ShelvingField].Value.ShouldBe(ObjectIds.ShelvedStateMachineType_Unshelved); + StatusCode.IsGood(await client.AddCommentAsync( + conditionId, events.LatestEventId(item, "base"), "note", ct)).ShouldBeTrue(); + await rig.DrainProjectionsAsync(); + await RecordedEvents.WaitUntilAsync(() => events.ConditionEvents(item, "base").Count >= 3); + (events.ConditionEvents(item, "base")[2][RecordedEvents.CommentField].Value as LocalizedText).ShouldNotBeNull().Text.ShouldBe("note"); + await subscription.DeleteAsync(true, ct); } finally { await rig.DisposeAsync(); }