diff --git a/code-reviews/Driver.Modbus.Cli/findings.md b/code-reviews/Driver.Modbus.Cli/findings.md index 3eb3792..822595d 100644 --- a/code-reviews/Driver.Modbus.Cli/findings.md +++ b/code-reviews/Driver.Modbus.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 6 | +| Open findings | 0 | ## Checklist coverage @@ -87,7 +87,7 @@ message explaining coils carry a single bit. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs:14-24` | -| Status | Open | +| Status | Resolved | **Description:** `Port` (`int`) and `TimeoutMs` (`int`) accept any 32-bit value, including negatives and ports above 65535. `UnitId` is a `byte`, so it accepts @@ -103,7 +103,13 @@ error. None of these are validated at parse time. message — consistent with how `WriteCommand` already rejects bad regions and boolean strings. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added `ModbusCommandBase.ValidateEndpoint()` +that throws `CliFx.Exceptions.CommandException` for `--port` outside 1..65535, +non-positive `--timeout-ms`, and `--unit-id` outside 1..247 (Modbus spec unicast +range — rejects the broadcast 0 and reserved 248-255). Each of `ProbeCommand`, +`ReadCommand`, `WriteCommand`, and `SubscribeCommand` now calls it at the top of +`ExecuteAsync` after `ConfigureLogging()`. Regression tests live in +`ModbusCommandBaseTests` (range + boundary cases for all three knobs). ### Driver.Modbus.Cli-004 @@ -113,7 +119,7 @@ boolean strings. | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:61-67` | -| Status | Open | +| Status | Resolved | **Description:** The `OnDataChange` handler is invoked from the driver's `PollGroupEngine` background thread and calls `console.Output.WriteLine` @@ -127,7 +133,11 @@ any synchronization, so overlapping poll ticks could interleave partial lines. write failures so a transient console-write error cannot tear down the poll loop. A single `lock` around the write also removes the interleave risk. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — wrapped the `OnDataChange` handler in +`try/catch (Exception)` that logs to Serilog at Warning and swallows the failure +so a transient console-write error cannot fault the `PollGroupEngine` background +loop. The console write is also serialized through a local `lock` object, removing +the partial-line interleave risk when multiple poll ticks overlap. ### Driver.Modbus.Cli-005 @@ -136,7 +146,7 @@ A single `lock` around the write also removes the interleave risk. | Severity | Low | | Category | Error handling & resilience | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:21-54`; `Commands/ReadCommand.cs:46-75`; `Commands/WriteCommand.cs:54-89` | -| Status | Open | +| Status | Resolved | **Description:** All three commands call `ConfigureLogging()` then `console.RegisterCancellationHandler()`, but if the operator presses Ctrl+C @@ -152,7 +162,14 @@ commands do not catch it around their driver calls. the noisy trace on Ctrl+C-during-connect is acceptable. Consistency with `SubscribeCommand`'s handling is the cleaner choice. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added a +`catch (OperationCanceledException) when (ct.IsCancellationRequested)` clause +around the driver-call bodies in `ProbeCommand`, `ReadCommand`, and `WriteCommand` +that prints `Cancelled.` and falls through to the existing `finally`-block +shutdown. Matches `SubscribeCommand`'s existing handling so all four commands now +exit quietly on Ctrl+C. Regression tests in `CommandCancellationTests` pre-cancel +the CliFx `FakeInMemoryConsole` before calling `ExecuteAsync` and assert no +exception escapes. ### Driver.Modbus.Cli-006 @@ -161,7 +178,7 @@ the noisy trace on Ctrl+C-during-connect is acceptable. Consistency with | Severity | Low | | Category | Error handling & resilience | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:35-53` | -| Status | Open | +| Status | Resolved | **Description:** `probe` reports `Health: {health.State}` from `GetHealth()`. After a successful `InitializeAsync` the driver sets state to `Healthy` @@ -179,7 +196,14 @@ snapshot's `StatusCode` (Good vs Bad) rather than — or in addition to — the `State`, or print a single combined verdict line so the two cannot contradict each other. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `ProbeCommand` now prints a single combined +`Verdict:` line above the bare driver-state line; the headline is computed by a +new `ProbeCommand.ComputeVerdict(DriverState, statusCode)` helper that combines +the driver state with the probe snapshot's OPC UA quality class (top 2 bits of +the StatusCode). Verdict is `FAIL` whenever the driver is not Healthy OR the +snapshot is Bad, `DEGRADED` when the driver is Healthy but the snapshot is +Uncertain, and `OK` only when both are Good. Regression tests in +`ProbeCommandTests` pin the verdict-grid behaviour. ### Driver.Modbus.Cli-007 @@ -188,7 +212,7 @@ other. | Severity | Low | | Category | Design-document adherence | | Location | `docs/Driver.Modbus.Cli.md:124-156`; `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs` | -| Status | Open | +| Status | Resolved | **Description:** `docs/Driver.Modbus.Cli.md` devotes a whole "v2 addressing grammar" section to the industry-standard tag-address strings (`40001:F:CDAB`, @@ -205,7 +229,14 @@ driver's address-string parser (and `--family` for the DL205/MELSEC native syntax), or scope the "v2 addressing grammar" section of the doc to note it applies to `DriverConfig` JSON and is not a CLI flag. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — chose the doc-scoping fix (an +`--address-string` flag is a bigger feature that warrants its own design +discussion). Added a "CLI scope" callout to the top of the +`docs/Driver.Modbus.Cli.md` "v2 addressing grammar" section stating the CLI +accepts only the structured `--region` + `--address` + `--type` triple and that +the address-string grammar is a `DriverConfig` JSON feature. The pre-existing +closing paragraph already said the same thing; the new callout makes it visible +before the grammar examples instead of after them. Code surface left unchanged. ### Driver.Modbus.Cli-008 @@ -214,7 +245,7 @@ applies to `DriverConfig` JSON and is not a CLI flag. | Severity | Low | | Category | Testing coverage | | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** The test project covers only the two pure-function seams: `ReadCommand.SynthesiseTagName` and `WriteCommand.ParseValue`. There is no coverage @@ -231,4 +262,19 @@ likely to regress and is currently untested. The validation gaps in findings setters and assert the produced `ModbusDriverOptions`). Once findings 002/003 are fixed, add tests for the new validation paths. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added four new test classes covering the +previously untested branch logic: +- `ModbusCommandBaseTests` — six tests over `BuildOptions` (probe disabled, + `TimeoutMs` → `Timeout` mapping, `AutoReconnect` tracking + `--disable-reconnect` in both directions, host/port/unit flow-through, tag + pass-through) plus the new `ValidateEndpoint` range-check tests for finding + 003 (port 1..65535, timeout-ms > 0, unit-id 1..247). +- `WriteCommandRegionValidationTests` — read-only region rejection + (DiscreteInputs / InputRegisters) and the Coils-non-Bool guard for finding + 002. +- `ProbeCommandTests` — the new `ComputeVerdict` helper for finding 006 + (OK / DEGRADED / FAIL grid). +- `CommandCancellationTests` — Ctrl+C-during-initialize for `ProbeCommand` / + `ReadCommand` / `WriteCommand` for finding 005. + +Total test count grew from 18 to 64; all pass. diff --git a/docs/Driver.Modbus.Cli.md b/docs/Driver.Modbus.Cli.md index 0127bed..9e55f01 100644 --- a/docs/Driver.Modbus.Cli.md +++ b/docs/Driver.Modbus.Cli.md @@ -122,6 +122,14 @@ gives plausible values is the correct one for that device. ## v2 addressing grammar +> **CLI scope:** the `read` / `write` / `subscribe` commands accept only +> the structured `--region` + `--address` + `--type` triple. The +> address-string grammar below is a **`DriverConfig` JSON** feature +> consumed by the driver itself; it is not reachable from this CLI's +> flags. To experiment with it via the CLI, use the structured flags; +> to deploy spreadsheets as-is, hand-author a `DriverConfig` and run +> the server. + The driver accepts the industry-standard tag-address grammar so you can paste tag spreadsheets from Wonderware / Kepware / Ignition without per-row manual translation. Full reference + grammar rules: diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs index f3fdb59..b419f49 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs @@ -1,5 +1,6 @@ using CliFx.Attributes; using CliFx.Infrastructure; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands; @@ -21,6 +22,7 @@ public sealed class ProbeCommand : ModbusCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); // Build with one probe tag + Probe.Enabled=false so InitializeAsync connects the @@ -39,17 +41,59 @@ public sealed class ProbeCommand : ModbusCommandBase var snapshot = await driver.ReadAsync(["__probe"], ct); var health = driver.GetHealth(); + // Driver.Modbus.Cli-006: derive the headline verdict from BOTH the driver state + // AND the probe-read StatusCode so the operator never sees the previous + // contradictory pair (`Health: Healthy` over a Bad snapshot line). The bare + // driver state is still printed below for diagnostics, but the verdict is what + // the operator scans first. + var verdict = ComputeVerdict(health.State, snapshot[0].StatusCode); + await console.Output.WriteLineAsync($"Host: {Host}:{Port} (unit {UnitId})"); - await console.Output.WriteLineAsync($"Health: {health.State}"); + await console.Output.WriteLineAsync($"Verdict: {verdict}"); + await console.Output.WriteLineAsync($"Driver state: {health.State}"); if (health.LastError is { } err) await console.Output.WriteLineAsync($"Last error: {err}"); await console.Output.WriteLineAsync(); await console.Output.WriteLineAsync( SnapshotFormatter.Format($"HR[{ProbeAddress}]", snapshot[0])); } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Driver.Modbus.Cli-005: Ctrl+C during InitializeAsync — exit quietly so CliFx + // does not render a full stack trace for a user-initiated cancellation. + await console.Output.WriteLineAsync("Cancelled."); + } finally { await driver.ShutdownAsync(CancellationToken.None); } } + + /// + /// Driver.Modbus.Cli-006: combine the driver-side with the + /// probe snapshot's OPC UA StatusCode into a single headline verdict. Order + /// of precedence: + /// + /// FAIL — driver did not reach + /// (Faulted / Reconnecting / Unknown) OR the snapshot reports Bad quality. + /// DEGRADED — driver Healthy but the snapshot quality is Uncertain. + /// OK — driver Healthy and snapshot Good. + /// + /// + public static string ComputeVerdict(DriverState state, uint statusCode) + { + // OPC UA StatusCode top 2 bits encode the quality class: + // 0x00xxxxxx → Good, 0x40xxxxxx → Uncertain, 0x80xxxxxx / 0xC0xxxxxx → Bad. + var qualityClass = statusCode & 0xC0000000u; + var snapshotGood = qualityClass == 0x00000000u; + var snapshotUncertain = qualityClass == 0x40000000u; + + if (state != DriverState.Healthy || !snapshotGood && !snapshotUncertain) + return $"FAIL (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})"; + + if (snapshotUncertain) + return $"DEGRADED (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})"; + + return $"OK (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})"; + } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs index 544076e..b70dc15 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs @@ -46,6 +46,7 @@ public sealed class ReadCommand : ModbusCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); var tagName = SynthesiseTagName(Region, Address, DataType); @@ -68,6 +69,12 @@ public sealed class ReadCommand : ModbusCommandBase var snapshot = await driver.ReadAsync([tagName], ct); await console.Output.WriteLineAsync(SnapshotFormatter.Format(tagName, snapshot[0])); } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Driver.Modbus.Cli-005: Ctrl+C during driver connect/read — exit quietly so + // CliFx does not render a full stack trace for a user-initiated cancellation. + await console.Output.WriteLineAsync("Cancelled."); + } finally { await driver.ShutdownAsync(CancellationToken.None); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs index 5f9168a..e1f1f74 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs @@ -53,6 +53,7 @@ public sealed class SubscribeCommand : ModbusCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Region, Address, DataType); @@ -69,6 +70,9 @@ public sealed class SubscribeCommand : ModbusCommandBase var options = BuildOptions([tag]); await using var driver = new ModbusDriver(options, DriverInstanceId); + // Driver.Modbus.Cli-004: serialize console writes from the PollGroupEngine background + // thread so overlapping poll ticks can't interleave partial lines. + var writeLock = new object(); ISubscriptionHandle? handle = null; try { @@ -78,10 +82,26 @@ public sealed class SubscribeCommand : ModbusCommandBase // analyzer flags it + IConsole is the testable abstraction). driver.OnDataChange += (_, e) => { - var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + - $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + - $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; - console.Output.WriteLine(line); + // Driver.Modbus.Cli-004: swallow + log write failures so a transient stdout + // error (closed pipe, IO exception on a redirected stream) cannot tear down + // the poll-engine background loop. Without this guard the unhandled + // exception would fault the long-running subscribe. + try + { + var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + + $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + + $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; + lock (writeLock) + { + console.Output.WriteLine(line); + } + } + catch (Exception ex) + { + Serilog.Log.Logger.Warning(ex, + "SubscribeCommand: console write failed for {Tag}; continuing poll loop.", + e.FullReference); + } }; handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs index a605bf3..86ce8a4 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs @@ -54,6 +54,7 @@ public sealed class WriteCommand : ModbusCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); if (Region is not (ModbusRegion.Coils or ModbusRegion.HoldingRegisters)) @@ -92,6 +93,12 @@ public sealed class WriteCommand : ModbusCommandBase var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(tagName, results[0])); } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Driver.Modbus.Cli-005: Ctrl+C during driver connect/write — exit quietly so + // CliFx does not render a full stack trace for a user-initiated cancellation. + await console.Output.WriteLineAsync("Cancelled."); + } finally { await driver.ShutdownAsync(CancellationToken.None); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs index b01ccdd..f7bb531 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs @@ -1,4 +1,5 @@ using CliFx.Attributes; +using CliFx.Exceptions; using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli; @@ -57,4 +58,33 @@ public abstract class ModbusCommandBase : DriverCommandBase /// multiple endpoints in parallel can distinguish the logs. /// protected string DriverInstanceId => $"modbus-cli-{Host}:{Port}"; + + /// + /// Driver.Modbus.Cli-003: validate the endpoint flags at parse time so the operator + /// gets a clear CliFx error instead of an opaque socket / argument exception thrown + /// deep inside the driver. Ranges: + /// + /// --port: 1..65535 (IANA TCP port space, excludes the + /// "any" sentinel 0 and rejects negative / overflowed values). + /// --timeout-ms: strictly positive — a non-positive + /// would propagate as an immediate-timeout into the + /// driver and surface as a confusing TimeoutException. + /// --unit-id: 1..247 — the Modbus spec unicast unit-id range. + /// 0 is the broadcast address and not valid for read/write requests; 248-255 + /// are reserved. (Documented in docs/Driver.Modbus.Cli.md.) + /// + /// + protected void ValidateEndpoint() + { + if (Port < 1 || Port > 65535) + throw new CommandException( + $"--port must be in 1..65535 (got {Port})."); + if (TimeoutMs <= 0) + throw new CommandException( + $"--timeout-ms must be strictly positive (got {TimeoutMs})."); + if (UnitId < 1 || UnitId > 247) + throw new CommandException( + $"--unit-id must be in 1..247 per the Modbus spec (got {UnitId}); " + + $"0 is the broadcast address, 248-255 are reserved."); + } } diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/CommandCancellationTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/CommandCancellationTests.cs new file mode 100644 index 0000000..5ca2e69 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/CommandCancellationTests.cs @@ -0,0 +1,66 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests; + +/// +/// Covers Driver.Modbus.Cli-005: probe / read / write must swallow +/// so a Ctrl+C during InitializeAsync exits +/// cleanly instead of dumping a full stack trace through CliFx. SubscribeCommand +/// already handles this around its Task.Delay; these tests pin the same behaviour +/// to the connect/read/write commands. +/// The test pre-cancels the CliFx ; the driver's +/// ConnectAsync observes the token via Dns.GetHostAddressesAsync and throws +/// OCE before any socket I/O happens, so the test is hermetic — no real PLC needed. +/// +[Trait("Category", "Unit")] +public sealed class CommandCancellationTests +{ + [Fact] + public async Task ProbeCommand_swallows_cancellation_during_initialize() + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); // simulate Ctrl+C before ExecuteAsync runs + + var sut = new ProbeCommand { Host = "127.0.0.1" }; + + await Should.NotThrowAsync(async () => await sut.ExecuteAsync(console)); + } + + [Fact] + public async Task ReadCommand_swallows_cancellation_during_initialize() + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); + + var sut = new ReadCommand + { + Host = "127.0.0.1", + Region = ModbusRegion.HoldingRegisters, + Address = 0, + DataType = ModbusDataType.UInt16, + }; + + await Should.NotThrowAsync(async () => await sut.ExecuteAsync(console)); + } + + [Fact] + public async Task WriteCommand_swallows_cancellation_during_initialize() + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); + + var sut = new WriteCommand + { + Host = "127.0.0.1", + Region = ModbusRegion.HoldingRegisters, + Address = 0, + DataType = ModbusDataType.UInt16, + Value = "42", + }; + + await Should.NotThrowAsync(async () => await sut.ExecuteAsync(console)); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ModbusCommandBaseTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ModbusCommandBaseTests.cs new file mode 100644 index 0000000..6141d9d --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ModbusCommandBaseTests.cs @@ -0,0 +1,164 @@ +using CliFx.Attributes; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests; + +/// +/// Covers — the pure, deterministic mapping +/// from the base's host / port / unit-id / timeout / disable-reconnect flags onto a +/// ModbusDriverOptions. The CLI is one-shot so the background connectivity probe +/// must be disabled; AutoReconnect is the inverse of --disable-reconnect. +/// Also covers the input-range validation introduced for Driver.Modbus.Cli-003. +/// +[Trait("Category", "Unit")] +public sealed class ModbusCommandBaseTests +{ + // Test-only ModbusCommandBase concrete subclass that exposes the protected BuildOptions + // helper + ValidateEndpoint. The [Command] attribute is required by the CliFx analyzer + // (CliFx_CommandMustBeAnnotated) — this command is never registered with the CLI app + // but the analyzer rule fires for every ICommand implementor in the compilation. + [Command("noop-test", Description = "Test-only probe of ModbusCommandBase.BuildOptions.")] + private sealed class ProbeOnly : ModbusCommandBase + { + public override ValueTask ExecuteAsync(IConsole console) => default; + public ModbusDriverOptions Invoke(IReadOnlyList tags) => BuildOptions(tags); + public void InvokeValidate() => ValidateEndpoint(); + } + + [Fact] + public void BuildOptions_disables_probe_for_one_shot_cli_runs() + { + var sut = new ProbeOnly { Host = "10.0.0.5" }; + + var options = sut.Invoke([]); + + options.Probe.ShouldNotBeNull(); + options.Probe.Enabled.ShouldBeFalse(); + } + + [Fact] + public void BuildOptions_maps_TimeoutMs_to_Timeout_TimeSpan() + { + var sut = new ProbeOnly { Host = "h", TimeoutMs = 7500 }; + + var options = sut.Invoke([]); + + options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500)); + } + + [Fact] + public void BuildOptions_AutoReconnect_defaults_to_true_when_flag_unset() + { + var sut = new ProbeOnly { Host = "h" }; + + var options = sut.Invoke([]); + + options.AutoReconnect.ShouldBeTrue(); + } + + [Fact] + public void BuildOptions_AutoReconnect_becomes_false_when_disable_reconnect_flag_set() + { + var sut = new ProbeOnly { Host = "h", DisableAutoReconnect = true }; + + var options = sut.Invoke([]); + + options.AutoReconnect.ShouldBeFalse(); + } + + [Fact] + public void BuildOptions_flows_host_port_unit_through() + { + var sut = new ProbeOnly { Host = "plc.shop.local", Port = 5020, UnitId = 17, TimeoutMs = 3000 }; + + var options = sut.Invoke([]); + + options.Host.ShouldBe("plc.shop.local"); + options.Port.ShouldBe(5020); + options.UnitId.ShouldBe((byte)17); + } + + [Fact] + public void BuildOptions_forwards_tag_list_verbatim() + { + var sut = new ProbeOnly { Host = "h" }; + var tag = new ModbusTagDefinition( + Name: "T", Region: ModbusRegion.HoldingRegisters, Address: 0, DataType: ModbusDataType.UInt16); + + var options = sut.Invoke([tag]); + + options.Tags.Count.ShouldBe(1); + options.Tags[0].ShouldBeSameAs(tag); + } + + // --- Driver.Modbus.Cli-003: parse-time endpoint validation ------------------------------- + + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(65536)] + [InlineData(int.MinValue)] + [InlineData(int.MaxValue)] + public void ValidateEndpoint_rejects_port_outside_1_to_65535(int port) + { + var sut = new ProbeOnly { Host = "h", Port = port }; + + Should.Throw(() => sut.InvokeValidate()); + } + + [Theory] + [InlineData(1)] + [InlineData(502)] + [InlineData(65535)] + public void ValidateEndpoint_accepts_port_in_range(int port) + { + var sut = new ProbeOnly { Host = "h", Port = port }; + + Should.NotThrow(() => sut.InvokeValidate()); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(-2000)] + public void ValidateEndpoint_rejects_non_positive_timeout(int timeoutMs) + { + var sut = new ProbeOnly { Host = "h", TimeoutMs = timeoutMs }; + + Should.Throw(() => sut.InvokeValidate()); + } + + [Theory] + [InlineData(0)] // broadcast — disallowed for unicast read/write requests + [InlineData(248)] + [InlineData(255)] + public void ValidateEndpoint_rejects_unit_id_outside_1_to_247(byte unitId) + { + var sut = new ProbeOnly { Host = "h", UnitId = unitId }; + + Should.Throw(() => sut.InvokeValidate()); + } + + [Theory] + [InlineData(1)] + [InlineData(247)] + [InlineData(50)] + public void ValidateEndpoint_accepts_unit_id_in_range(byte unitId) + { + var sut = new ProbeOnly { Host = "h", UnitId = unitId }; + + Should.NotThrow(() => sut.InvokeValidate()); + } + + [Fact] + public void ValidateEndpoint_accepts_default_options() + { + // Defaults: Port=502, UnitId=1, TimeoutMs=2000. All inside the valid ranges. + var sut = new ProbeOnly { Host = "h" }; + + Should.NotThrow(() => sut.InvokeValidate()); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ProbeCommandTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ProbeCommandTests.cs new file mode 100644 index 0000000..922c78f --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ProbeCommandTests.cs @@ -0,0 +1,60 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests; + +/// +/// Covers — the headline-string helper that +/// combines the driver-side with the probe snapshot's OPC UA +/// so the operator never sees the previous +/// contradictory pair (`Health: Healthy` above a `Status: Bad...` snapshot line — see +/// Driver.Modbus.Cli-006). +/// +[Trait("Category", "Unit")] +public sealed class ProbeCommandTests +{ + [Fact] + public void ComputeVerdict_returns_OK_when_state_is_Healthy_and_status_is_Good() + { + var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode: 0x00000000u); + + verdict.ShouldContain("OK"); + } + + [Theory] + [InlineData(DriverState.Faulted)] + [InlineData(DriverState.Reconnecting)] + [InlineData(DriverState.Unknown)] + public void ComputeVerdict_returns_FAIL_when_driver_state_is_not_Healthy(DriverState state) + { + var verdict = ProbeCommand.ComputeVerdict(state, statusCode: 0x00000000u); + + verdict.ShouldContain("FAIL"); + } + + [Theory] + [InlineData(0x80050000u)] // BadCommunicationError + [InlineData(0x800A0000u)] // BadTimeout + [InlineData(0x80740000u)] // BadTypeMismatch + [InlineData(0x80000000u)] // generic Bad + public void ComputeVerdict_returns_FAIL_when_snapshot_status_is_Bad_even_if_driver_state_Healthy(uint statusCode) + { + // Driver.Modbus.Cli-006: a successful InitializeAsync sets DriverState.Healthy, but + // the FC03 probe read may still fail (snapshot.StatusCode != Good). Previously the + // headline reported Healthy while the snapshot line below showed Bad. The verdict + // must reflect the actual probe-read outcome. + var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode); + + verdict.ShouldContain("FAIL"); + } + + [Fact] + public void ComputeVerdict_returns_DEGRADED_for_uncertain_status_with_healthy_driver() + { + var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode: 0x40000000u); + + verdict.ShouldContain("DEGRADED"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/WriteCommandRegionValidationTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/WriteCommandRegionValidationTests.cs new file mode 100644 index 0000000..9fa0a0e --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/WriteCommandRegionValidationTests.cs @@ -0,0 +1,63 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests; + +/// +/// Covers the branch validation inside : +/// 1. Driver.Modbus.Cli-002 — write to must use +/// --type Bool. +/// 2. Read-only regions (DiscreteInputs / InputRegisters) reject any write. +/// The actual driver call is never reached for these guard cases — they throw a +/// before the driver is constructed, +/// so we can exercise ExecuteAsync against an unreachable host. +/// +[Trait("Category", "Unit")] +public sealed class WriteCommandRegionValidationTests +{ + [Theory] + [InlineData(ModbusRegion.DiscreteInputs, ModbusDataType.Bool, "0")] + [InlineData(ModbusRegion.InputRegisters, ModbusDataType.UInt16, "1")] + public async Task ExecuteAsync_rejects_read_only_regions( + ModbusRegion region, ModbusDataType type, string value) + { + var sut = new WriteCommand + { + // Host is required, but the guard fires before any socket use. + Host = "127.0.0.1", + Region = region, + Address = 0, + DataType = type, + Value = value, + }; + using var console = new FakeInMemoryConsole(); + + await Should.ThrowAsync( + async () => await sut.ExecuteAsync(console)); + } + + [Theory] + [InlineData(ModbusDataType.UInt16)] + [InlineData(ModbusDataType.Int16)] + [InlineData(ModbusDataType.Float32)] + [InlineData(ModbusDataType.Int32)] + public async Task ExecuteAsync_rejects_non_Bool_type_for_Coils_region(ModbusDataType type) + { + var sut = new WriteCommand + { + Host = "127.0.0.1", + Region = ModbusRegion.Coils, + Address = 5, + DataType = type, + Value = "42", + }; + using var console = new FakeInMemoryConsole(); + + var ex = await Should.ThrowAsync( + async () => await sut.ExecuteAsync(console)); + ex.Message.ShouldContain("Coils"); + ex.Message.ShouldContain("Bool"); + } +}