diff --git a/code-reviews/Client.CLI/findings.md b/code-reviews/Client.CLI/findings.md index 7ce8c2e..2278a6d 100644 --- a/code-reviews/Client.CLI/findings.md +++ b/code-reviews/Client.CLI/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 8 | +| Open findings | 0 | ## Checklist coverage @@ -62,7 +62,7 @@ assumption precisely. | Severity | Low | | Category | Correctness & logic bugs | | Location | `Commands/SubscribeCommand.cs:129-137` | -| Status | Open | +| Status | Resolved | **Description:** The summary computes `neverWentBad` as every target whose node-id key is absent from the `everBad` dictionary. A node that received no update at all is also absent @@ -78,7 +78,7 @@ streamed only good values. "suspect" list only contains nodes that were actually observed and never reported bad quality. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `neverWentBad` now requires the node to be present in `lastStatus` (i.e. it received at least one update) before being counted, so the "suspect" bucket only contains nodes that were actually observed and never reported bad quality. ### Client.CLI-003 @@ -87,7 +87,7 @@ quality. | Severity | Low | | Category | Correctness & logic bugs | | Location | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` | -| Status | Open | +| Status | Resolved | **Description:** Numeric command options accept any value with no range validation. `--depth`, `--interval`, `--max-depth`, `--max`, and the history `--interval` can all be @@ -100,7 +100,7 @@ is forwarded to `HistoryReadRawAsync`. None of these produce a clear operator-fa `CliFx.Exceptions.CommandException` with an actionable message when a value is out of range. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — every command's `ExecuteAsync` now validates the numeric option ranges (`--interval`, `--depth`, `--max-depth`, `--max`, `--duration`) and throws `CliFx.Exceptions.CommandException` with the offending value when a non-positive (or otherwise out-of-range) value is supplied. Pinned by `CommandRangeValidationTests`. ### Client.CLI-004 @@ -109,7 +109,7 @@ range. | Severity | Low | | Category | OtOpcUa conventions | | Location | `Commands/SubscribeCommand.cs:13-37` | -| Status | Open | +| Status | Resolved | **Description:** `SubscribeCommand` is the only command in the module whose constructor and all `[CommandOption]` properties have no XML doc comments. Every other command @@ -121,7 +121,7 @@ otherwise-uniform documentation convention of the module. **Recommendation:** Add `` XML docs to the `SubscribeCommand` constructor and to each of its option properties, matching the style used by the sibling commands. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` now carries `` XML docs on the type, the constructor, every `[CommandOption]` property, and `ExecuteAsync`, matching the style used by the sibling commands. ### Client.CLI-005 @@ -156,7 +156,7 @@ callback. | Severity | Low | | Category | Error handling & resilience | | Location | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` | -| Status | Open | +| Status | Resolved | **Description:** Operator input-format errors surface as raw .NET exceptions rather than clean CLI errors. An unparseable start/end value throws `FormatException` straight out of @@ -170,7 +170,7 @@ is converted to a `CliFx.Exceptions.CommandException` with a clean exit code. `CommandException` with a concise message and a non-zero exit code, so malformed input yields a one-line error instead of a stack trace. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `HistoryReadCommand` parses `--start`/`--end` with `CultureInfo.InvariantCulture` + `AssumeUniversal`/`AdjustToUniversal`, catches `FormatException`, and rethrows as `CommandException` with the offending value. Every command's call to `NodeIdParser.ParseRequired` is wrapped in a `catch (FormatException or ArgumentException)` block that surfaces the underlying message as a clean CLI error. Pinned by `InputValidationErrorsTests`. ### Client.CLI-007 @@ -179,7 +179,7 @@ yields a one-line error instead of a stack trace. | Severity | Low | | Category | Performance & resource management | | Location | `CommandBase.cs:112-123` | -| Status | Open | +| Status | Resolved | **Description:** `ConfigureLogging` builds a new Serilog `LoggerConfiguration`, creates a logger, and assigns it to the static `Log.Logger` without disposing the previously @@ -193,7 +193,7 @@ abandons the prior console sink without disposal. The pattern is incorrect: build the logger into a local `ILogger` the command owns and disposes, rather than mutating global static state per command. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `CommandBase.ConfigureLogging` now calls `Log.CloseAndFlush()` before assigning a new `Log.Logger`, so a prior logger's console sink is disposed before the next one is installed. Pinned by `LoggerLifecycleTests`. ### Client.CLI-008 @@ -202,7 +202,7 @@ global static state per command. | Severity | Low | | Category | Documentation & comments | | Location | `docs/Client.CLI.md:158-217` | -| Status | Open | +| Status | Resolved | **Description:** `docs/Client.CLI.md` is stale relative to the code at this commit. (1) The `subscribe` command section documents only `-n` and `-i`, but the code @@ -218,7 +218,7 @@ code option description includes it. `docs/Client.CLI.md` from the current option set, including the five new subscribe flags and the `StandardDeviation` aggregate row. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — rewrote the `subscribe` section of `docs/Client.CLI.md` to document every flag (`-r/--recursive`, `--max-depth`, `-q/--quiet`, `--duration`, `--summary-file`) plus the summary-bucket vocabulary, and added the `StandardDeviation` row plus the UTC `--start`/`--end` convention note to the `historyread` section. ### Client.CLI-009 @@ -227,7 +227,7 @@ and the `StandardDeviation` aggregate row. | Severity | Low | | Category | Code organization & conventions | | Location | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` | -| Status | Open | +| Status | Resolved | **Description:** Both long-running commands attach an event handler (`service.DataChanged += ...`, `service.AlarmEvent += ...`) with a lambda and never detach @@ -243,7 +243,7 @@ but never the .NET event. unsubscribing, using a named local delegate so it can be removed, ensuring no notification is processed after the command output phase ends. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` and `AlarmsCommand` declare named local handlers (`DataChangedHandler` / `AlarmEventHandler`) and detach them via `service.DataChanged -= ...` / `service.AlarmEvent -= ...` right after `UnsubscribeAsync` so no notification reaches the console once the command's output phase ends. Pinned by `EventHandlerLifecycleTests`. ### Client.CLI-010 @@ -252,7 +252,7 @@ is processed after the command output phase ends. | Severity | Low | | Category | Testing coverage | | Location | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** The new `SubscribeCommand` capabilities are largely untested. The four `SubscribeCommandTests` cover only single-node subscribe, unsubscribe-on-cancel, @@ -268,4 +268,4 @@ exit, summary bucketing across good/bad/no-update nodes, and the `--summary-file The `FakeOpcUaClientService` already exposes `RaiseDataChanged`, so feeding good/bad values and asserting the summary text is straightforward. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added `SubscribeCommandSummaryTests` (covering recursive collection via `FakeOpcUaClientService.AddDiscoveredVariable`, `--duration` auto-exit, summary bucketing for good/bad/never/never-went-bad, and the `--summary-file` write), `CommandRangeValidationTests`, `EventHandlerLifecycleTests`, `InputValidationErrorsTests`, and `LoggerLifecycleTests` to pin the other Low findings; `FakeOpcUaClientService` was extended with `AddDiscoveredVariable` / `RaiseDataChanged` helpers. diff --git a/docs/Client.CLI.md b/docs/Client.CLI.md index bb99539..8ef3193 100644 --- a/docs/Client.CLI.md +++ b/docs/Client.CLI.md @@ -149,53 +149,77 @@ otopcua-cli browse -u opc.tcp://localhost:4840/OtOpcUa -U admin -P admin123 -r - ### subscribe -Monitors a node for value changes using OPC UA subscriptions. Prints each data change notification with timestamp, value, and status code. Runs until Ctrl+C, then unsubscribes and disconnects cleanly. +Monitors a node (or every Variable in its subtree) for value changes using OPC UA subscriptions. +Prints each data-change notification with timestamp, value, and status code, then prints a +summary on exit. Exits on Ctrl+C, or automatically after `--duration` seconds. ```bash +# Subscribe to a single node otopcua-cli subscribe -u opc.tcp://localhost:4840 -n "ns=2;s=MyNode" -i 500 + +# Browse a subtree and subscribe to every Variable, run for 60 seconds, write the summary to disk +otopcua-cli subscribe -u opc.tcp://localhost:4840 -n "ns=3;s=ZB" -r --max-depth 4 \ + --duration 60 --quiet --summary-file C:\Temp\subscribe-summary.txt ``` | Flag | Description | |------|-------------| -| `-n` / `--node` | Node ID to monitor (required) | -| `-i` / `--interval` | Sampling/publishing interval in milliseconds (default: 1000) | +| `-n` / `--node` | Node ID to monitor (required). When `--recursive` is set, this is the browse root. | +| `-i` / `--interval` | Sampling interval in milliseconds (default: 1000) | +| `-r` / `--recursive` | Browse recursively from `--node` and subscribe to every Variable found | +| `--max-depth` | Maximum recursion depth when `--recursive` is set (default: 10) | +| `-q` / `--quiet` | Suppress per-update output; only print the final summary | +| `--duration` | Auto-exit after N seconds and print the summary (0 = run until Ctrl+C, default: 0) | +| `--summary-file` | Also write the summary to this file path on exit | + +#### Summary buckets + +The summary prints per-node counts across these buckets: + +- **Ever went BAD during window** — node received at least one notification whose status was not Good. +- **NEVER went bad (suspect)** — node received at least one notification and every one was Good. +- **Last status GOOD / NOT-GOOD** — final observed status across nodes that received any update. +- **No update received at all** — node was subscribed but no notification arrived during the window. ### historyread Reads historical data from a node. Supports raw history reads and aggregate (processed) history reads. +`--start` and `--end` are parsed with `CultureInfo.InvariantCulture` and treated as UTC; supply +them in ISO 8601 UTC form (`YYYY-MM-DDTHH:MM:SSZ`) for unambiguous behaviour across hosts. ```bash # Raw history otopcua-cli historyread -u opc.tcp://localhost:4840/OtOpcUa \ -n "ns=1;s=TestMachine_001.TestHistoryValue" \ - --start "2026-03-25" --end "2026-03-30" + --start "2026-03-25T00:00:00Z" --end "2026-03-30T00:00:00Z" # Aggregate: 1-hour average otopcua-cli historyread -u opc.tcp://localhost:4840/OtOpcUa \ -n "ns=1;s=TestMachine_001.TestHistoryValue" \ - --start "2026-03-25" --end "2026-03-30" \ + --start "2026-03-25T00:00:00Z" --end "2026-03-30T00:00:00Z" \ --aggregate Average --interval 3600000 ``` | Flag | Description | |------|-------------| | `-n` / `--node` | Node ID to read history for (required) | -| `--start` | Start time, ISO 8601 or date string (default: 24 hours ago) | -| `--end` | End time, ISO 8601 or date string (default: now) | +| `--start` | Start time in ISO 8601 UTC format, e.g. `2026-01-15T08:00:00Z` (default: 24 hours ago) | +| `--end` | End time in ISO 8601 UTC format, e.g. `2026-01-15T09:00:00Z` (default: now) | | `--max` | Maximum number of values (default: 1000) | -| `--aggregate` | Aggregate function: Average, Minimum, Maximum, Count, Start, End | +| `--aggregate` | Aggregate function: Average, Minimum, Maximum, Count, Start, End, StandardDeviation | | `--interval` | Processing interval in milliseconds for aggregates (default: 3600000) | #### Aggregate mapping -| Name | OPC UA Node ID | -|------|---------------| -| `Average` | `AggregateFunction_Average` | -| `Minimum` | `AggregateFunction_Minimum` | -| `Maximum` | `AggregateFunction_Maximum` | -| `Count` | `AggregateFunction_Count` | -| `Start` | `AggregateFunction_Start` | -| `End` | `AggregateFunction_End` | +| Name | Aliases | OPC UA Node ID | +|------|---------|---------------| +| `Average` | `avg` | `AggregateFunction_Average` | +| `Minimum` | `min` | `AggregateFunction_Minimum` | +| `Maximum` | `max` | `AggregateFunction_Maximum` | +| `Count` | | `AggregateFunction_Count` | +| `Start` | `first` | `AggregateFunction_Start` | +| `End` | `last` | `AggregateFunction_End` | +| `StandardDeviation` | `stddev`, `stdev` | `AggregateFunction_StandardDeviationSample` | ### alarms diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/CommandBase.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/CommandBase.cs index f3473f3..7aca25f 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/CommandBase.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/CommandBase.cs @@ -109,8 +109,16 @@ public abstract class CommandBase : ICommand /// /// Configures Serilog based on the verbose flag. /// + /// + /// Disposes the previously assigned via + /// before installing the new one, so repeated CLI invocations (e.g. in the test suite) do not + /// leak the prior logger's console sink. + /// protected void ConfigureLogging() { + // Dispose any previously installed logger before swapping in a new one. + Log.CloseAndFlush(); + var config = new LoggerConfiguration(); if (Verbose) config.MinimumLevel.Debug() diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs index 2dde493..a0a10f7 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs @@ -1,6 +1,8 @@ using System.Threading.Channels; 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; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; @@ -43,14 +45,25 @@ public class AlarmsCommand : CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + if (Interval <= 0) + throw new CommandException($"--interval must be greater than 0 (was {Interval})."); + + NodeId? sourceNodeId; + try + { + sourceNodeId = NodeIdParser.Parse(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var sourceNodeId = NodeIdParser.Parse(NodeId); - // Channel serialises SDK notification-thread writes to the main async loop so // that concurrent alarm callbacks never interleave on the shared TextWriter. var outputChannel = Channel.CreateUnbounded( diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/BrowseCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/BrowseCommand.cs index f56c4ac..fda8c05 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/BrowseCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/BrowseCommand.cs @@ -1,4 +1,5 @@ using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; @@ -42,13 +43,25 @@ public class BrowseCommand : CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + if (Depth <= 0) + throw new CommandException($"--depth must be greater than 0 (was {Depth})."); + + NodeId? startNode; + try + { + startNode = NodeIdParser.Parse(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var startNode = NodeIdParser.Parse(NodeId); var maxDepth = Recursive ? Depth : 1; await BrowseNodeAsync(service, console, startNode, maxDepth, 0, ct); diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs index 0a364f3..61926bd 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs @@ -1,5 +1,6 @@ using System.Globalization; using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; @@ -62,22 +63,65 @@ public class HistoryReadCommand : CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + if (MaxValues <= 0) + throw new CommandException($"--max must be greater than 0 (was {MaxValues})."); + if (!string.IsNullOrEmpty(Aggregate) && IntervalMs <= 0) + throw new CommandException($"--interval must be greater than 0 (was {IntervalMs})."); + + NodeId nodeId; + try + { + nodeId = NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + + DateTime start, end; + try + { + start = string.IsNullOrEmpty(StartTime) + ? DateTime.UtcNow.AddHours(-24) + : DateTime.Parse(StartTime, CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal); + } + catch (FormatException ex) + { + throw new CommandException($"Invalid --start value '{StartTime}': {ex.Message}. Expected ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z."); + } + + try + { + end = string.IsNullOrEmpty(EndTime) + ? DateTime.UtcNow + : DateTime.Parse(EndTime, CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal); + } + catch (FormatException ex) + { + throw new CommandException($"Invalid --end value '{EndTime}': {ex.Message}. Expected ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z."); + } + + AggregateType aggregateType = default; + if (!string.IsNullOrEmpty(Aggregate)) + { + try + { + aggregateType = ParseAggregateType(Aggregate); + } + catch (ArgumentException ex) + { + throw new CommandException($"Invalid --aggregate value: {ex.Message}"); + } + } + IOpcUaClientService? service = null; try { var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var nodeId = NodeIdParser.ParseRequired(NodeId); - var start = string.IsNullOrEmpty(StartTime) - ? DateTime.UtcNow.AddHours(-24) - : DateTime.Parse(StartTime, CultureInfo.InvariantCulture, - DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal); - var end = string.IsNullOrEmpty(EndTime) - ? DateTime.UtcNow - : DateTime.Parse(EndTime, CultureInfo.InvariantCulture, - DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal); - IReadOnlyList values; if (string.IsNullOrEmpty(Aggregate)) @@ -88,7 +132,6 @@ public class HistoryReadCommand : CommandBase } else { - var aggregateType = ParseAggregateType(Aggregate); await console.Output.WriteLineAsync( $"History for {NodeId} ({Aggregate}, interval={IntervalMs}ms)"); values = await service.HistoryReadAggregateAsync( diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs index bb8b666..6889278 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ReadCommand.cs @@ -1,5 +1,7 @@ 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; @@ -29,13 +31,23 @@ public class ReadCommand : CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + + NodeId nodeId; + try + { + nodeId = NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var nodeId = NodeIdParser.ParseRequired(NodeId); var value = await service.ReadValueAsync(nodeId, ct); await console.Output.WriteLineAsync($"Node: {NodeId}"); diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs index cf6d8fd..28cccd5 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs @@ -1,6 +1,7 @@ using System.Collections.Concurrent; using System.Threading.Channels; using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; @@ -12,42 +13,92 @@ namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; [Command("subscribe", Description = "Monitor a node for value changes")] public class SubscribeCommand : CommandBase { + /// + /// Creates the subscribe command used to monitor a node (or a subtree of nodes) for data-change + /// notifications. + /// + /// The factory that creates the shared client service for the command run. public SubscribeCommand(IOpcUaClientServiceFactory factory) : base(factory) { } + /// + /// Gets the node ID to monitor. When is set, this node is the browse root + /// and every Variable child it reaches is subscribed. + /// [CommandOption("node", 'n', Description = "Node ID to monitor", IsRequired = true)] public string NodeId { get; init; } = default!; + /// + /// Gets the sampling interval, in milliseconds, requested for every monitored item. + /// [CommandOption("interval", 'i', Description = "Sampling interval in milliseconds")] public int Interval { get; init; } = 1000; + /// + /// Gets a value indicating whether the command should browse from + /// and subscribe to every Variable in the subtree. + /// [CommandOption("recursive", 'r', Description = "Browse recursively from --node and subscribe to every Variable found")] public bool Recursive { get; init; } + /// + /// Gets the maximum recursion depth applied while collecting variables when is set. + /// [CommandOption("max-depth", Description = "Maximum recursion depth when --recursive is set")] public int MaxDepth { get; init; } = 10; + /// + /// Gets a value indicating whether per-update lines should be suppressed in favour of the final summary only. + /// [CommandOption("quiet", 'q', Description = "Suppress per-update output; only print a final summary on Ctrl+C")] public bool Quiet { get; init; } + /// + /// Gets the duration, in seconds, before the command auto-exits and prints its summary. + /// A value of 0 means the command runs until Ctrl+C. + /// [CommandOption("duration", Description = "Auto-exit after N seconds and print summary (0 = run until Ctrl+C)")] public int DurationSeconds { get; init; } = 0; + /// + /// Gets the optional path that the command should write the final summary to on exit, in addition to stdout. + /// [CommandOption("summary-file", Description = "Write summary to this file path on exit (in addition to stdout)")] public string? SummaryFile { get; init; } + /// + /// Connects to the server, subscribes to (or its subtree when recursive), + /// streams data-change notifications to the console, and prints a summary when the command exits. + /// + /// The CLI console used for output and cancellation handling. public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + + if (Interval <= 0) + throw new CommandException($"--interval must be greater than 0 (was {Interval})."); + if (Recursive && MaxDepth <= 0) + throw new CommandException($"--max-depth must be greater than 0 (was {MaxDepth})."); + if (DurationSeconds < 0) + throw new CommandException($"--duration must be 0 or a positive number (was {DurationSeconds})."); + + NodeId rootNodeId; + try + { + rootNodeId = NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var rootNodeId = NodeIdParser.ParseRequired(NodeId); - var targets = new List<(NodeId nodeId, string displayPath)>(); if (Recursive) { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/WriteCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/WriteCommand.cs index 044df88..d92fa2d 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/WriteCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/WriteCommand.cs @@ -1,4 +1,5 @@ using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; @@ -37,14 +38,23 @@ public class WriteCommand : CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + + NodeId nodeId; + try + { + nodeId = NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { var ct = console.RegisterCancellationHandler(); (service, _) = await CreateServiceAndConnectAsync(ct); - var nodeId = NodeIdParser.ParseRequired(NodeId); - // Read current value to determine type for conversion var currentValue = await service.ReadValueAsync(nodeId, ct); var typedValue = ValueConverter.ConvertValue(Value, currentValue.Value); diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommandRangeValidationTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommandRangeValidationTests.cs new file mode 100644 index 0000000..7139021 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/CommandRangeValidationTests.cs @@ -0,0 +1,165 @@ +using CliFx.Exceptions; +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; + +/// +/// Regression tests for Client.CLI-003: numeric command options must validate their ranges +/// and report a clean operator-facing error instead of silently passing bad values to the SDK. +/// +public class CommandRangeValidationTests +{ + [Fact] + public async Task BrowseCommand_NegativeDepth_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new BrowseCommand(factory) + { + Url = "opc.tcp://localhost:4840", + Depth = -1 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--depth"); + } + + [Fact] + public async Task BrowseCommand_ZeroDepth_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new BrowseCommand(factory) + { + Url = "opc.tcp://localhost:4840", + Depth = 0 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--depth"); + } + + [Fact] + public async Task SubscribeCommand_ZeroInterval_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + Interval = 0 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--interval"); + } + + [Fact] + public async Task SubscribeCommand_NegativeInterval_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + Interval = -50 + }; + + using var console = TestConsoleHelper.CreateConsole(); + await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + } + + [Fact] + public async Task SubscribeCommand_RecursiveZeroMaxDepth_ThrowsCommandException() + { + // --max-depth only matters when --recursive is set; validation is scoped to that combination. + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + Recursive = true, + MaxDepth = 0 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--max-depth"); + } + + [Fact] + public async Task SubscribeCommand_NegativeDuration_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + DurationSeconds = -5 + }; + + using var console = TestConsoleHelper.CreateConsole(); + await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + } + + [Fact] + public async Task AlarmsCommand_ZeroInterval_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new AlarmsCommand(factory) + { + Url = "opc.tcp://localhost:4840", + Interval = 0 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--interval"); + } + + [Fact] + public async Task HistoryReadCommand_NegativeMax_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new HistoryReadCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + MaxValues = -10 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--max"); + } + + [Fact] + public async Task HistoryReadCommand_ZeroInterval_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new HistoryReadCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + Aggregate = "Average", + IntervalMs = 0 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--interval"); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/EventHandlerLifecycleTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/EventHandlerLifecycleTests.cs new file mode 100644 index 0000000..29ab363 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/EventHandlerLifecycleTests.cs @@ -0,0 +1,59 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +/// +/// Regression tests for Client.CLI-009: long-running commands must detach their event handlers +/// from the service before the command finishes, so notifications fired during teardown don't +/// land in a disposed console. +/// +public class EventHandlerLifecycleTests +{ + [Fact] + public async Task SubscribeCommand_AfterExit_DataChangedEventHasNoSubscribers() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=Node", + DurationSeconds = 1 + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + // The fake's event should have no subscribers after the command completes — every + // handler attached by the command must have been detached during teardown. + fakeService.HasDataChangedSubscribers.ShouldBeFalse( + "SubscribeCommand must detach its DataChanged handler before returning."); + } + + [Fact] + public async Task AlarmsCommand_AfterExit_AlarmEventHasNoSubscribers() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new AlarmsCommand(factory) + { + Url = "opc.tcp://localhost:4840" + }; + + using var console = TestConsoleHelper.CreateConsole(); + + var task = Task.Run(async () => await command.ExecuteAsync(console)); + + await Task.Delay(150); + console.RequestCancellation(); + await task; + + fakeService.HasAlarmEventSubscribers.ShouldBeFalse( + "AlarmsCommand must detach its AlarmEvent handler before returning."); + } +} 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 f7d75fd..f41abb6 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 @@ -55,6 +55,14 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService new("ns=2;s=Node2", "Node2", "Variable", false) }; + /// + /// Optional per-parent-node browse results. When a key matches the requested parent's + /// , this dictionary takes precedence over . + /// Tests exercising recursive walks (Client.CLI-010) use it to model a real subtree whose + /// child node ids do not collide on descent. + /// + public Dictionary> BrowseResultsByParent { get; } = new(); + public IReadOnlyList HistoryReadResult { get; set; } = new List { new(new Variant(10.0), StatusCodes.Good, DateTime.UtcNow.AddHours(-1), DateTime.UtcNow), @@ -68,6 +76,7 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService public Exception? ReadException { get; set; } public Exception? WriteException { get; set; } public Exception? ConditionRefreshException { get; set; } + public Exception? SubscribeException { get; set; } /// public bool IsConnected => ConnectCalled && !DisconnectCalled; @@ -84,6 +93,12 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService /// public event EventHandler? ConnectionStateChanged; + /// True when at least one handler is attached to . + public bool HasDataChangedSubscribers => DataChanged != null; + + /// True when at least one handler is attached to . + public bool HasAlarmEventSubscribers => AlarmEvent != null; + /// public Task ConnectAsync(ConnectionSettings settings, CancellationToken ct = default) { @@ -120,6 +135,9 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService public Task> BrowseAsync(NodeId? parentNodeId = null, CancellationToken ct = default) { BrowseNodeIds.Add(parentNodeId); + var key = parentNodeId?.ToString(); + if (key != null && BrowseResultsByParent.TryGetValue(key, out var keyed)) + return Task.FromResult(keyed); return Task.FromResult(BrowseResults); } @@ -127,6 +145,7 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService public Task SubscribeAsync(NodeId nodeId, int intervalMs = 1000, CancellationToken ct = default) { SubscribeCalls.Add((nodeId, intervalMs)); + if (SubscribeException != null) throw SubscribeException; return Task.CompletedTask; } diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadCommandTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadCommandTests.cs index c981a49..816e2dc 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadCommandTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadCommandTests.cs @@ -105,7 +105,7 @@ public class HistoryReadCommandTests } [Fact] - public async Task Execute_InvalidAggregate_ThrowsArgumentException() + public async Task Execute_InvalidAggregate_ThrowsCommandException() { var fakeService = new FakeOpcUaClientService(); var factory = new FakeOpcUaClientServiceFactory(fakeService); @@ -117,7 +117,10 @@ public class HistoryReadCommandTests }; using var console = TestConsoleHelper.CreateConsole(); - await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + // Operator-input errors now surface as CliFx CommandException (was ArgumentException) + // per Client.CLI-006 so malformed input prints a clean CLI error instead of a stack trace. + await Should.ThrowAsync( + async () => await command.ExecuteAsync(console)); } [Fact] diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/InputValidationErrorsTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/InputValidationErrorsTests.cs new file mode 100644 index 0000000..56291bd --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/InputValidationErrorsTests.cs @@ -0,0 +1,98 @@ +using CliFx.Exceptions; +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; + +/// +/// Regression tests for Client.CLI-006: predictable operator-input errors must surface as +/// CliFx CommandException with a clean message, not raw FormatException/ArgumentException +/// with a stack trace. +/// +public class InputValidationErrorsTests +{ + [Fact] + public async Task HistoryReadCommand_InvalidStartTime_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new HistoryReadCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + StartTime = "not-a-date" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--start"); + } + + [Fact] + public async Task HistoryReadCommand_InvalidEndTime_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new HistoryReadCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + EndTime = "garbage" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("--end"); + } + + [Fact] + public async Task HistoryReadCommand_InvalidAggregate_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new HistoryReadCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=N", + Aggregate = "NotARealAggregate" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("aggregate", Case.Insensitive); + } + + [Fact] + public async Task ReadCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new ReadCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } + + [Fact] + public async Task SubscribeCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/LoggerLifecycleTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/LoggerLifecycleTests.cs new file mode 100644 index 0000000..16382a7 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/LoggerLifecycleTests.cs @@ -0,0 +1,55 @@ +using Serilog; +using Serilog.Core; +using Serilog.Events; +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; + +/// +/// Regression test for Client.CLI-007: ConfigureLogging must dispose the previously assigned +/// Log.Logger before replacing it, so repeated CLI invocations do not leak sinks. +/// +public class LoggerLifecycleTests +{ + [Fact] + public async Task ConfigureLogging_DisposesPreviousLogger_BeforeReassigning() + { + // Install a tracker logger before the command runs. + var trackerSink = new DisposeTrackingSink(); + var trackerLogger = new LoggerConfiguration() + .WriteTo.Sink(trackerSink) + .CreateLogger(); + Log.Logger = trackerLogger; + + try + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new ConnectCommand(factory) + { + Url = "opc.tcp://localhost:4840" + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + // The command's ConfigureLogging should have disposed the tracker logger we installed. + trackerSink.Disposed.ShouldBeTrue( + "Previous Log.Logger should be disposed via Log.CloseAndFlush() before ConfigureLogging assigns a new one."); + } + finally + { + Log.CloseAndFlush(); + } + } + + private sealed class DisposeTrackingSink : ILogEventSink, IDisposable + { + public bool Disposed { get; private set; } + public void Emit(LogEvent logEvent) { } + public void Dispose() => Disposed = true; + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandSummaryTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandSummaryTests.cs new file mode 100644 index 0000000..74a4af3 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandSummaryTests.cs @@ -0,0 +1,258 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; +using BrowseResult = ZB.MOM.WW.OtOpcUa.Client.Shared.Models.BrowseResult; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +/// +/// Regression tests for SubscribeCommand summary bucketing, --duration, --quiet, --summary-file, +/// and recursive collection. Covers Client.CLI-002, -009, and -010. +/// +public class SubscribeCommandSummaryTests +{ + [Fact] + public async Task Summary_NodeWithNoUpdate_IsCountedAsNeverNotAsNeverWentBad() + { + // Client.CLI-002: A node that received no updates at all is "never received an update", + // NOT a "suspect that never went bad". + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=SilentNode", + DurationSeconds = 1 + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("No update received at all: 1"); + output.ShouldContain("NEVER went bad (suspect): 0"); + // The "suspect" detail header should not appear when the suspect list is empty. + output.ShouldNotContain("--- Nodes that NEVER received a bad-quality update (suspect) ---"); + // The "never received update" detail header should appear. + output.ShouldContain("--- Nodes that never received an update at all ---"); + } + + [Fact] + public async Task Summary_NodeReceivedOnlyGoodValues_IsCountedAsNeverWentBad() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=GoodNode", + // Use --duration so the command auto-exits. + DurationSeconds = 2 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var runTask = Task.Run(async () => await command.ExecuteAsync(console)); + + // Wait until the subscription is registered by the fake. + await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1); + + fakeService.RaiseDataChanged( + "ns=2;s=GoodNode", + new DataValue(new Variant(42), StatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow)); + + await runTask; + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("NEVER went bad (suspect): 1"); + output.ShouldContain("Last status GOOD: 1"); + output.ShouldContain("No update received at all: 0"); + } + + [Fact] + public async Task Summary_NodeReceivedBadValue_IsCountedAsEverWentBad() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=BadNode", + DurationSeconds = 2 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var runTask = Task.Run(async () => await command.ExecuteAsync(console)); + + await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1); + + fakeService.RaiseDataChanged( + "ns=2;s=BadNode", + new DataValue(Variant.Null, StatusCodes.BadDeviceFailure, DateTime.UtcNow, DateTime.UtcNow)); + + await runTask; + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("Ever went BAD during window: 1"); + output.ShouldContain("NEVER went bad (suspect): 0"); + } + + [Fact] + public async Task Duration_ZeroOrPositive_AutoExits() + { + // --duration > 0 should make the command exit on its own without needing cancellation. + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=AutoExit", + DurationSeconds = 1 + }; + + using var console = TestConsoleHelper.CreateConsole(); + + var sw = System.Diagnostics.Stopwatch.StartNew(); + await command.ExecuteAsync(console); + sw.Stop(); + + sw.Elapsed.ShouldBeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(900)); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(10)); + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("==================== SUMMARY ===================="); + } + + [Fact] + public async Task Quiet_SuppressesPerUpdateOutputButPrintsSummary() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=Quiet", + Quiet = true, + DurationSeconds = 2 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var runTask = Task.Run(async () => await command.ExecuteAsync(console)); + + await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1); + fakeService.RaiseDataChanged( + "ns=2;s=Quiet", + new DataValue(new Variant(1.0), StatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow)); + + await runTask; + + var output = TestConsoleHelper.GetOutput(console); + // No per-update "value = ..." line should appear because --quiet was set. + output.ShouldNotContain(" = 1 ("); + // Summary section is still printed. + output.ShouldContain("==================== SUMMARY ===================="); + } + + [Fact] + public async Task SummaryFile_WritesSummaryToDisk() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var tempFile = Path.Combine(Path.GetTempPath(), $"otopcua-cli-summary-{Guid.NewGuid():N}.txt"); + try + { + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=SummaryFileNode", + DurationSeconds = 1, + SummaryFile = tempFile + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + File.Exists(tempFile).ShouldBeTrue(); + var contents = await File.ReadAllTextAsync(tempFile); + contents.ShouldContain("SUMMARY"); + contents.ShouldContain("Total subscribed: 1"); + } + finally + { + if (File.Exists(tempFile)) File.Delete(tempFile); + } + } + + [Fact] + public async Task Recursive_BrowsesSubtreeAndSubscribesEveryVariable() + { + // Root has a Folder child and a top-level Variable; the Folder contains a second Variable. + // Each node id is distinct so the ToDictionary(t => t.nodeId.ToString()) collapse + // doesn't trip a duplicate-key error. + var fakeService = new FakeOpcUaClientService(); + fakeService.BrowseResultsByParent["ns=2;s=Root"] = new List + { + new("ns=2;s=Folder", "Folder", "Object", true), + new("ns=2;s=Tag1", "Tag1", "Variable", false) + }; + fakeService.BrowseResultsByParent["ns=2;s=Folder"] = new List + { + new("ns=2;s=Tag2", "Tag2", "Variable", false) + }; + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=Root", + Recursive = true, + MaxDepth = 3, + DurationSeconds = 1 + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + // Both Variables (depth 1 + depth 2) should have been subscribed. + fakeService.SubscribeCalls.Count.ShouldBeGreaterThanOrEqualTo(2); + fakeService.SubscribeCalls.ShouldContain(c => c.NodeId.Identifier.ToString() == "Tag1"); + fakeService.SubscribeCalls.ShouldContain(c => c.NodeId.Identifier.ToString() == "Tag2"); + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("Browsing subtree of ns=2;s=Root"); + } + + [Fact] + public async Task SubscribeFailure_PrintsFailedMessage_DoesNotCrash() + { + var fakeService = new FakeOpcUaClientService + { + SubscribeException = new InvalidOperationException("forced failure") + }; + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new SubscribeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=FailNode", + DurationSeconds = 1 + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + var output = TestConsoleHelper.GetOutput(console); + output.ShouldContain("FAILED to subscribe"); + output.ShouldContain("forced failure"); + // The summary block is still printed. + output.ShouldContain("==================== SUMMARY ===================="); + } + + private static async Task WaitForAsync(Func predicate, int timeoutMs = 5000) + { + var deadline = DateTime.UtcNow.AddMilliseconds(timeoutMs); + while (!predicate() && DateTime.UtcNow < deadline) + await Task.Delay(25); + if (!predicate()) + throw new TimeoutException("Condition not met within timeout."); + } +}