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
2026-10-08 06:53:57 -04:00
parent 3baa332d6b
commit 48bd266e19
18 changed files with 363 additions and 211 deletions
@@ -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)]
@@ -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);
+
+    /// <summary>Verifies that a successful comment prints a success message and passes correct arguments to the service.</summary>
+    [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");
+    }
+
+    /// <summary>Verifies that a bad StatusCode from the service prints a failure message.</summary>
+    [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");
+    }
+
+    /// <summary>Verifies that an invalid hex event-id throws CommandException.</summary>
+    [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<CommandException>(async () => await command.ExecuteAsync(console));
+        ex.Message.ShouldContain("event-id", Case.Insensitive);
+    }
+
+    /// <summary>Verifies that the command disconnects in the finally block.</summary>
+    [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();
+    }
+}
@@ -264,6 +264,20 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
        return Task.FromResult(ConfirmAlarmResult);
    }

+    /// <summary>Gets the list of (conditionNodeId, eventId, comment) tuples from AddCommentAsync calls.</summary>
+    public List<(string ConditionNodeId, byte[] EventId, string Comment)> AddCommentCalls { get; } = [];
+
+    /// <summary>Gets or sets the status code returned by AddCommentAsync.</summary>
+    public StatusCode AddCommentResult { get; set; } = StatusCodes.Good;
+
+    /// <inheritdoc />
+    public Task<StatusCode> AddCommentAsync(string conditionNodeId, byte[] eventId, string comment,
+        CancellationToken ct = default)
+    {
+        AddCommentCalls.Add((conditionNodeId, eventId, comment));
+        return Task.FromResult(AddCommentResult);
+    }
+
    /// <summary>Gets the list of (conditionNodeId, kind, durationSeconds) tuples from ShelveAlarmAsync calls.</summary>
    public List<(string ConditionNodeId, ShelveKind Kind, double DurationSeconds)> ShelveAlarmCalls { get; } = [];

@@ -17,8 +17,8 @@ internal sealed class FakeEndpointDiscovery : IEndpointDiscovery
    /// <param name="endpointUrl">The endpoint URL to select.</param>
    /// <param name="requestedMode">The requested security mode.</param>
    /// <returns>The selected endpoint description.</returns>
-    public EndpointDescription SelectEndpoint(ApplicationConfiguration config, string endpointUrl,
-        MessageSecurityMode requestedMode)
+    public Task<EndpointDescription> 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"
            }
-        };
+        });
    }
-}
+}
@@ -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) ---

    /// <summary>
-    /// 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
-    /// <c>InAlarm</c>/<c>Acked</c> 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
+    /// <c>&lt;condition&gt;.InAlarm</c>/<c>.Acked</c> (those attributes only existed on the retired Galaxy v2 server).
    /// </summary>
    [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
-    }
-
-    /// <summary>
-    /// 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).
-    /// </summary>
-    [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 ---
@@ -282,6 +282,10 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
        return Task.FromResult(ConfirmResult);
    }

+    /// <inheritdoc />
+    public Task<StatusCode> AddCommentAsync(string conditionNodeId, byte[] eventId, string comment,
+        CancellationToken ct = default) => Task.FromResult<StatusCode>(StatusCodes.Good);
+
    /// <summary>Gets or sets the status code returned by shelve operations in UI tests.</summary>
    public StatusCode ShelveResult { get; set; } = StatusCodes.Good;
    /// <summary>Gets or sets the exception thrown to simulate alarm shelve failures in the UI.</summary>