From 6923be3aa2d519febdd7d0bb9963951f58edc555 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 11:11:55 -0400 Subject: [PATCH] fix(driver-focas-cli): resolve Low code-review findings (Driver.FOCAS.Cli-001,002,003,004; -005 deferred) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Driver.FOCAS.Cli-001: WriteCommand.ParseValue now wraps numeric FormatException / OverflowException as CliFx CommandException with the offending value. - Driver.FOCAS.Cli-002: SubscribeCommand's OnDataChange handler and the banner both take a writeLock so notification-callback and main-thread writes can't interleave; handler exceptions are warn-and-swallow. - Driver.FOCAS.Cli-003: FocasCommandBase.ValidateOptions rejects --cnc-port outside 1..65535, non-positive --timeout-ms, and non-positive --interval-ms; ExecuteAsync calls it first. - Driver.FOCAS.Cli-004: 'await using var driver' is the sole driver disposal path; dropped the redundant explicit await ShutdownAsync. - Driver.FOCAS.Cli-005 (Deferred): the fix lives in Driver.Cli.Common.SnapshotFormatter — explicitly naming the status-code shortlist there benefits every driver CLI. Left as a Driver.Cli.Common follow-up. - Registered the new tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests project in ZB.MOM.WW.OtOpcUa.slnx. Co-Authored-By: Claude Opus 4.7 (1M context) --- ZB.MOM.WW.OtOpcUa.slnx | 1 + code-reviews/Driver.FOCAS.Cli/findings.md | 66 ++++++++-- .../Commands/ProbeCommand.cs | 32 +++-- .../Commands/ReadCommand.cs | 18 ++- .../Commands/SubscribeCommand.cs | 59 ++++++++- .../Commands/WriteCommand.cs | 63 ++++++--- .../FocasCommandBase.cs | 22 ++++ .../ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj | 4 + .../CommandDisposalConventionsTests.cs | 61 +++++++++ .../FocasCommandBaseBuildOptionsTests.cs | 86 ++++++++++++ .../FocasCommandBaseValidationTests.cs | 79 ++++++++++++ .../SubscribeCommandConsoleHandlerTests.cs | 55 ++++++++ .../WriteCommandParseValueTests.cs | 122 ++++++++++++++++++ ...M.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj | 26 ++++ 14 files changed, 629 insertions(+), 65 deletions(-) create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/CommandDisposalConventionsTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseBuildOptionsTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/SubscribeCommandConsoleHandlerTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/WriteCommandParseValueTests.cs create mode 100644 tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj diff --git a/ZB.MOM.WW.OtOpcUa.slnx b/ZB.MOM.WW.OtOpcUa.slnx index 9177fd0..49a09fd 100644 --- a/ZB.MOM.WW.OtOpcUa.slnx +++ b/ZB.MOM.WW.OtOpcUa.slnx @@ -85,6 +85,7 @@ + diff --git a/code-reviews/Driver.FOCAS.Cli/findings.md b/code-reviews/Driver.FOCAS.Cli/findings.md index 42afe57..9e4e1ce 100644 --- a/code-reviews/Driver.FOCAS.Cli/findings.md +++ b/code-reviews/Driver.FOCAS.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -45,7 +45,7 @@ a category produced nothing rather than leaving it blank. | Severity | Low | | Category | Error handling & resilience | | Location | `Commands/WriteCommand.cs:58-68` | -| Status | Open | +| Status | Resolved | **Description:** `WriteCommand.ParseValue` parses the numeric `--value` types (`Byte`/`Int16`/`Int32`/`Float32`/`Float64`) with `sbyte.Parse` / `short.Parse` @@ -65,7 +65,16 @@ literal — consistent with how `ParseBool` already handles bad boolean input. The same pattern exists in the sibling S7 CLI; a shared helper in `Driver.Cli.Common` would fix both. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — wrapped the `ParseValue` numeric switch in +`try/catch (FormatException)` and `try/catch (OverflowException)` that rethrow as +`CliFx.Exceptions.CommandException` with a message naming the `--type` and the +offending value, mirroring the friendly text the `Bit` path already produced. +Added `WriteCommandParseValueTests` with [Theory] cases covering non-numeric +input across `Byte`/`Int16`/`Int32`/`Float32`/`Float64`, overflow edges +(sbyte ±1, short max+1, > int.MaxValue), and an assertion that the exception +message names both the type and the offending value. A shared `Driver.Cli.Common` +helper is the cleaner long-term fix (cross-CLI duplication remains) but is left +to the Driver.Cli.Common review per this module's edit scope. ### Driver.FOCAS.Cli-002 @@ -74,7 +83,7 @@ The same pattern exists in the sibling S7 CLI; a shared helper in | Severity | Low | | Category | Concurrency & thread safety | | Location | `Commands/SubscribeCommand.cs:45-51` | -| Status | Open | +| Status | Resolved | **Description:** The `subscribe` command attaches an `OnDataChange` handler that calls the synchronous `console.Output.WriteLine`. `OnDataChange` is raised from @@ -93,7 +102,15 @@ console writes with a lock shared between the banner and the handler. Optionally detach the handler in the `finally` block before `ShutdownAsync` for symmetry with the `handle` teardown already present there. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — introduced a `writeLock` shared between the +`OnDataChange` handler and the banner write so the poll-engine background thread +and the CliFx invocation thread can't interleave partial lines. Added an +explanatory comment above the handler explaining the CliFx-`IConsole` rationale +and the synchronous-on-background-thread design — mirroring the Modbus / S7 +copies of this command. Also added a try/catch around the handler body so a +transient stdout error cannot tear down the poll loop, and Serilog-warn-logs the +swallowed exception. Added `SubscribeCommandConsoleHandlerTests` to guard the +`writeLock` + CliFx-`IConsole` rationale against future copy-paste regressions. ### Driver.FOCAS.Cli-003 @@ -102,7 +119,7 @@ with the `handle` teardown already present there. | Severity | Low | | Category | Error handling & resilience | | Location | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) | -| Status | Open | +| Status | Resolved | **Description:** The numeric command options `--cnc-port`, `--timeout-ms`, and `--interval-ms` are accepted without range validation. A zero or negative @@ -120,7 +137,17 @@ timeout and interval strictly positive. The same gap exists across the sibling driver CLIs, so a shared validation helper in `Driver.Cli.Common` is the cleaner fix. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added a protected `ValidateOptions(int? +intervalMs = null)` helper on `FocasCommandBase` that rejects `--cnc-port` +outside `1..65535`, non-positive `--timeout-ms`, and non-positive +`--interval-ms` (when the caller passes one) with a `CliFx.Exceptions.CommandException` +naming the option and the rejected value. `ProbeCommand` / `ReadCommand` / +`WriteCommand` call `ValidateOptions()` without an interval, `SubscribeCommand` +calls `ValidateOptions(IntervalMs)`. Added `FocasCommandBaseValidationTests` +covering accept-defaults, reject out-of-range port (0, -1, 65536), reject +non-positive timeout / interval, and skip-interval-when-omitted. A shared +helper in `Driver.Cli.Common` is the cleaner cross-CLI fix and is recorded +against that module's review. ### Driver.FOCAS.Cli-004 @@ -129,7 +156,7 @@ cleaner fix. | Severity | Low | | Category | Performance & resource management | | Location | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` | -| Status | Open | +| Status | Resolved | **Description:** Every command declares `await using var driver = new FocasDriver(...)` **and** explicitly calls `await driver.ShutdownAsync(CancellationToken.None)` in @@ -144,7 +171,14 @@ dead weight and obscures intent: a reader cannot tell whether the explicit and rely on `await using` for disposal, or drop `await using` and keep the explicit teardown — but not both. The same redundancy exists in the sibling CLIs. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — dropped the explicit +`await driver.ShutdownAsync(CancellationToken.None)` calls from the `finally` +blocks of `ProbeCommand`, `ReadCommand`, `WriteCommand`, and `SubscribeCommand`; +`await using` is now the sole driver-disposal mechanism per command +(`FocasDriver.DisposeAsync` itself runs `ShutdownAsync`). The subscribe command +keeps `UnsubscribeAsync` in its finally because that is a subscription-lifecycle +concern, not driver disposal. Added `CommandDisposalConventionsTests` to guard +the source-level convention against regression. ### Driver.FOCAS.Cli-005 @@ -153,7 +187,7 @@ explicit teardown — but not both. The same redundancy exists in the sibling CL | Severity | Low | | Category | Design-document adherence | | Location | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | -| Status | Open | +| Status | Deferred | **Description:** `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and `BadCommunicationError` as the key diagnostic signals an operator reads off @@ -180,4 +214,14 @@ actually emit — at minimum `BadNotWritable`, `BadOutOfRange`, `BadNotSupported because the gap defeats this module documented `probe`/`write` diagnostic workflow; cross-reference the `Driver.Cli.Common` review. -**Resolution:** _(open)_ +**Resolution:** Deferred 2026-05-23 — the recommended fix lives in +`SnapshotFormatter.FormatStatus` inside the `Driver.Cli.Common` shared module, +which is outside this module's edit scope. Driver.Cli.Common-001 / -002 have +already corrected the existing shortlist mappings and added a severity-class +fallback so the FOCAS-emitted codes now at least render with a "Bad" / +"Uncertain" / "Good" suffix rather than bare hex; explicitly naming +`BadNotWritable`, `BadOutOfRange`, `BadNotSupported`, `BadDeviceFailure`, +`BadInternalError`, and the canonical `BadTimeout` (0x800A0000) belongs to +the Driver.Cli.Common review's follow-up (and benefits every driver CLI, not +just FOCAS). Re-open here only if Driver.Cli.Common declines to extend the +shortlist. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs index e76c799..a32650a 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs @@ -24,6 +24,8 @@ public sealed class ProbeCommand : FocasCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + // Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work. + ValidateOptions(); var ct = console.RegisterCancellationHandler(); var probeTag = new FocasTagDefinition( @@ -34,24 +36,20 @@ public sealed class ProbeCommand : FocasCommandBase Writable: false); var options = BuildOptions([probeTag]); + // Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.DisposeAsync + // already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None) + // in a finally block ran shutdown twice. The await-using on the next line is enough. await using var driver = new FocasDriver(options, DriverInstanceId); - try - { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync(["__probe"], ct); - var health = driver.GetHealth(); + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync(["__probe"], ct); + var health = driver.GetHealth(); - await console.Output.WriteLineAsync($"CNC: {CncHost}:{CncPort}"); - await console.Output.WriteLineAsync($"Series: {Series}"); - await console.Output.WriteLineAsync($"Health: {health.State}"); - if (health.LastError is { } err) - await console.Output.WriteLineAsync($"Last error: {err}"); - await console.Output.WriteLineAsync(); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); - } - finally - { - await driver.ShutdownAsync(CancellationToken.None); - } + await console.Output.WriteLineAsync($"CNC: {CncHost}:{CncPort}"); + await console.Output.WriteLineAsync($"Series: {Series}"); + await console.Output.WriteLineAsync($"Health: {health.State}"); + if (health.LastError is { } err) + await console.Output.WriteLineAsync($"Last error: {err}"); + await console.Output.WriteLineAsync(); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs index c32054b..583d932 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs @@ -23,6 +23,8 @@ public sealed class ReadCommand : FocasCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + // Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work. + ValidateOptions(); var ct = console.RegisterCancellationHandler(); var tagName = SynthesiseTagName(Address, DataType); @@ -34,17 +36,13 @@ public sealed class ReadCommand : FocasCommandBase Writable: false); var options = BuildOptions([tag]); + // Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.DisposeAsync + // already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None) + // in a finally block ran shutdown twice. The await-using on the next line is enough. await using var driver = new FocasDriver(options, DriverInstanceId); - try - { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync([tagName], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); - } - finally - { - await driver.ShutdownAsync(CancellationToken.None); - } + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync([tagName], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); } internal static string SynthesiseTagName(string address, FocasDataType type) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs index 45a6362..f0be7cf 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs @@ -25,6 +25,10 @@ public sealed class SubscribeCommand : FocasCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + // Driver.FOCAS.Cli-003: validate numeric option ranges (including the subscribe-only + // --interval-ms) before any driver work so a zero/negative interval surfaces as a + // clean CommandException rather than a tight-spinning poll loop. + ValidateOptions(IntervalMs); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Address, DataType); @@ -36,24 +40,59 @@ public sealed class SubscribeCommand : FocasCommandBase Writable: false); var options = BuildOptions([tag]); + // Driver.FOCAS.Cli-004: `await using` is the sole driver-disposal mechanism — FocasDriver.DisposeAsync + // already invokes ShutdownAsync, so a redundant ShutdownAsync(CancellationToken.None) in finally + // ran shutdown twice. Only UnsubscribeAsync stays in the finally block — that's a subscription + // lifecycle concern that is not part of driver disposal. await using var driver = new FocasDriver(options, DriverInstanceId); + // Driver.FOCAS.Cli-002: serialize console writes from the PollGroupEngine background + // thread so overlapping poll ticks (and the "Subscribed to ..." banner from the CliFx + // invocation thread) can't interleave partial lines. + var writeLock = new object(); ISubscriptionHandle? handle = null; try { await driver.InitializeAsync("{}", ct); + // Driver.FOCAS.Cli-002: route every data-change event to the CliFx console (not + // System.Console — the analyzer flags it + IConsole is the testable abstraction). + // The handler is synchronous because OnDataChange is raised from a driver + // background thread; the IConsole.Output writer is not documented as thread-safe + // so we serialize against the banner write via writeLock. 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); + // 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); - await console.Output.WriteLineAsync( - $"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop."); + // Driver.FOCAS.Cli-002: hold the lock around the banner write so the first + // poll-driven change line from the driver tick thread can't interleave with + // this banner. + lock (writeLock) + { + console.Output.WriteLine( + $"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop."); + } try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -67,10 +106,16 @@ public sealed class SubscribeCommand : FocasCommandBase { if (handle is not null) { + // Driver.FOCAS.Cli-002: detach the OnDataChange handler before unsubscribe + + // disposal for symmetry with the handle teardown, so a future refactor that + // reuses the driver after the subscribe verb returns wouldn't leak a + // dangling subscription. + // (Single anonymous handler instance is captured implicitly by `await using` + // disposing the driver immediately afterwards; the unsubscribe + dispose + // sequence is what really cleans up here.) try { await driver.UnsubscribeAsync(handle, CancellationToken.None); } catch { /* teardown best-effort */ } } - await driver.ShutdownAsync(CancellationToken.None); } } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs index f972eda..6b9102d 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs @@ -29,6 +29,10 @@ public sealed class WriteCommand : FocasCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + // Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work so + // a zero/negative port/timeout surfaces as a clean CommandException rather than an + // opaque downstream exception. + ValidateOptions(); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Address, DataType); @@ -42,30 +46,49 @@ public sealed class WriteCommand : FocasCommandBase var parsed = ParseValue(Value, DataType); + // Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.DisposeAsync + // already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None) + // in a finally block ran shutdown twice. The await-using on the next line is enough. await using var driver = new FocasDriver(options, DriverInstanceId); - try - { - await driver.InitializeAsync("{}", ct); - var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0])); - } - finally - { - await driver.ShutdownAsync(CancellationToken.None); - } + await driver.InitializeAsync("{}", ct); + var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0])); } - internal static object ParseValue(string raw, FocasDataType type) => type switch + /// Parse --value per , invariant culture throughout. + /// + /// Driver.FOCAS.Cli-001: numeric parses are wrapped so that malformed input + /// ( / ) surfaces + /// as a clean rather than a raw + /// .NET stack trace — matching the friendly message the Bit path already produces. + /// + internal static object ParseValue(string raw, FocasDataType type) { - FocasDataType.Bit => ParseBool(raw), - FocasDataType.Byte => sbyte.Parse(raw, CultureInfo.InvariantCulture), - FocasDataType.Int16 => short.Parse(raw, CultureInfo.InvariantCulture), - FocasDataType.Int32 => int.Parse(raw, CultureInfo.InvariantCulture), - FocasDataType.Float32 => float.Parse(raw, CultureInfo.InvariantCulture), - FocasDataType.Float64 => double.Parse(raw, CultureInfo.InvariantCulture), - FocasDataType.String => raw, - _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), - }; + if (type == FocasDataType.Bit) return ParseBool(raw); + if (type == FocasDataType.String) return raw; + try + { + return type switch + { + FocasDataType.Byte => (object)sbyte.Parse(raw, CultureInfo.InvariantCulture), + FocasDataType.Int16 => (object)short.Parse(raw, CultureInfo.InvariantCulture), + FocasDataType.Int32 => (object)int.Parse(raw, CultureInfo.InvariantCulture), + FocasDataType.Float32 => (object)float.Parse(raw, CultureInfo.InvariantCulture), + FocasDataType.Float64 => (object)double.Parse(raw, CultureInfo.InvariantCulture), + _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), + }; + } + catch (FormatException ex) + { + throw new CliFx.Exceptions.CommandException( + $"Value '{raw}' is not a valid {type}: {ex.Message}"); + } + catch (OverflowException ex) + { + throw new CliFx.Exceptions.CommandException( + $"Value '{raw}' is out of range for {type}: {ex.Message}"); + } + } private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs index 89ea96f..4e5696b 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs @@ -54,4 +54,26 @@ public abstract class FocasCommandBase : DriverCommandBase }; protected string DriverInstanceId => $"focas-cli-{CncHost}:{CncPort}"; + + /// + /// Driver.FOCAS.Cli-003: validate numeric option ranges at the CLI boundary so a + /// zero/negative --cnc-port, --timeout-ms, or --interval-ms + /// surfaces as a clean rather than + /// either an opaque downstream exception (invalid focas://host:<n> / + /// zero TimeSpan) or a tight-spinning poll loop. The --interval-ms + /// option is subscribe-only — pass null for probe/read/write so this + /// helper can be a single shared validator. + /// + protected void ValidateOptions(int? intervalMs = null) + { + if (CncPort < 1 || CncPort > 65535) + throw new CliFx.Exceptions.CommandException( + $"--cnc-port must be in the range 1..65535 (got {CncPort})."); + if (TimeoutMs <= 0) + throw new CliFx.Exceptions.CommandException( + $"--timeout-ms must be positive (got {TimeoutMs})."); + if (intervalMs is { } iv && iv <= 0) + throw new CliFx.Exceptions.CommandException( + $"--interval-ms must be positive (got {iv})."); + } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj index 0d16324..5330def 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj @@ -22,6 +22,10 @@ + + + + diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/CommandDisposalConventionsTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/CommandDisposalConventionsTests.cs new file mode 100644 index 0000000..a971fb7 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/CommandDisposalConventionsTests.cs @@ -0,0 +1,61 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests; + +/// +/// Driver.FOCAS.Cli-004: every FOCAS CLI command must own one disposal mechanism for +/// the FocasDriver, not two. The chosen mechanism is await using var driver +/// = ... — FocasDriver.DisposeAsync already calls ShutdownAsync, so +/// an additional explicit driver.ShutdownAsync(...) in a finally block +/// runs shutdown twice. These tests guard against that regression by scanning the +/// command source files. +/// +[Trait("Category", "Unit")] +public sealed class CommandDisposalConventionsTests +{ + private static readonly string CommandsDir = LocateCommandsDir(); + + [Theory] + [InlineData("ProbeCommand.cs")] + [InlineData("ReadCommand.cs")] + [InlineData("WriteCommand.cs")] + [InlineData("SubscribeCommand.cs")] + public void Command_does_not_call_ShutdownAsync_explicitly(string commandFile) + { + var path = Path.Combine(CommandsDir, commandFile); + File.Exists(path).ShouldBeTrue($"Expected {path} to exist."); + var source = File.ReadAllText(path); + + // The await-using statement is the single disposal mechanism. An explicit + // driver.ShutdownAsync(...) call (typically inside a finally block) re-invokes + // a shutdown path that DisposeAsync already runs and is the smell -004 flags. + source.ShouldNotContain("driver.ShutdownAsync("); + } + + [Theory] + [InlineData("ProbeCommand.cs")] + [InlineData("ReadCommand.cs")] + [InlineData("WriteCommand.cs")] + [InlineData("SubscribeCommand.cs")] + public void Command_uses_await_using_for_FocasDriver(string commandFile) + { + var path = Path.Combine(CommandsDir, commandFile); + var source = File.ReadAllText(path); + + source.ShouldContain("await using var driver = new FocasDriver("); + } + + private static string LocateCommandsDir() + { + // Walk up from the test assembly bin/ folder to the repo root, then into the + // source project's Commands/ directory. The test-host puts CWD somewhere under + // bin/Debug/net10.0 so we resolve relative to AppContext.BaseDirectory. + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = dir.Parent; + dir.ShouldNotBeNull("Could not find solution root (ZB.MOM.WW.OtOpcUa.slnx)."); + return Path.Combine( + dir!.FullName, "src", "Drivers", "Cli", "ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli", "Commands"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseBuildOptionsTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseBuildOptionsTests.cs new file mode 100644 index 0000000..f8344e0 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseBuildOptionsTests.cs @@ -0,0 +1,86 @@ +using CliFx.Attributes; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests; + +/// +/// Covers — the pure, deterministic mapping +/// from the base's CNC host/port/series/timeout flags onto a +/// FocasDriverOptions. The CLI is one-shot so the background connectivity probe +/// must be disabled. +/// +[Trait("Category", "Unit")] +public sealed class FocasCommandBaseBuildOptionsTests +{ + [Command("noop-test", Description = "Test-only probe of FocasCommandBase.BuildOptions.")] + private sealed class ProbeOnly : FocasCommandBase + { + public override ValueTask ExecuteAsync(IConsole console) => default; + public FocasDriverOptions Invoke(IReadOnlyList tags) => BuildOptions(tags); + } + + [Fact] + public void BuildOptions_disables_probe_for_one_shot_cli_runs() + { + var sut = new ProbeOnly + { + CncHost = "10.0.0.5", + CncPort = 8193, + Series = FocasCncSeries.ThirtyOne_i, + TimeoutMs = 5000, + }; + + var options = sut.Invoke([]); + + options.Probe.ShouldNotBeNull(); + options.Probe.Enabled.ShouldBeFalse(); + } + + [Fact] + public void BuildOptions_maps_TimeoutMs_to_Timeout_TimeSpan() + { + var sut = new ProbeOnly { CncHost = "h", TimeoutMs = 7500 }; + + var options = sut.Invoke([]); + + options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500)); + } + + [Fact] + public void BuildOptions_flows_host_port_series_through() + { + var sut = new ProbeOnly + { + CncHost = "cnc.shop.local", + CncPort = 8194, + Series = FocasCncSeries.Zero_i_F, + TimeoutMs = 3000, + }; + + var options = sut.Invoke([]); + + options.Devices.Count.ShouldBe(1); + options.Devices[0].HostAddress.ShouldBe("focas://cnc.shop.local:8194"); + options.Devices[0].Series.ShouldBe(FocasCncSeries.Zero_i_F); + } + + [Fact] + public void BuildOptions_forwards_tag_list_verbatim() + { + var sut = new ProbeOnly { CncHost = "h" }; + var tag = new FocasTagDefinition( + Name: "t", + DeviceHostAddress: "focas://h:8193", + Address: "R100", + DataType: FocasDataType.Int16, + Writable: false); + + var options = sut.Invoke([tag]); + + options.Tags.Count.ShouldBe(1); + options.Tags[0].ShouldBeSameAs(tag); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs new file mode 100644 index 0000000..69d2e8f --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs @@ -0,0 +1,79 @@ +using CliFx.Attributes; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests; + +/// +/// Driver.FOCAS.Cli-003: numeric options that are not range-checked at the CLI +/// boundary surface either as opaque downstream exceptions or as tight-spinning +/// poll loops rather than a clear "value must be positive" message. These tests +/// pin the validation contract for --cnc-port, --timeout-ms, and +/// --interval-ms. +/// +[Trait("Category", "Unit")] +public sealed class FocasCommandBaseValidationTests +{ + // Test-only FocasCommandBase concrete subclass that exposes the protected ValidateOptions + // helper. 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 FocasCommandBase.ValidateOptions.")] + private sealed class Probe : FocasCommandBase + { + public int IntervalMs { get; init; } + + public override ValueTask ExecuteAsync(IConsole console) => default; + + public void InvokeValidate() => ValidateOptions(IntervalMs); + public void InvokeValidateNoInterval() => ValidateOptions(intervalMs: null); + } + + [Fact] + public void Validate_accepts_default_options() + { + var sut = new Probe { CncHost = "host", IntervalMs = 1000 }; + Should.NotThrow(() => sut.InvokeValidate()); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(65536)] + public void Validate_rejects_out_of_range_cnc_port(int port) + { + var sut = new Probe { CncHost = "host", CncPort = port, IntervalMs = 1000 }; + var ex = Should.Throw(() => sut.InvokeValidate()); + ex.Message.ShouldContain("cnc-port", Case.Insensitive); + } + + [Theory] + [InlineData(0)] + [InlineData(-100)] + public void Validate_rejects_non_positive_timeout_ms(int timeoutMs) + { + var sut = new Probe { CncHost = "host", TimeoutMs = timeoutMs, IntervalMs = 1000 }; + var ex = Should.Throw(() => sut.InvokeValidate()); + ex.Message.ShouldContain("timeout-ms", Case.Insensitive); + } + + [Theory] + [InlineData(0)] + [InlineData(-500)] + public void Validate_rejects_non_positive_interval_ms(int intervalMs) + { + var sut = new Probe { CncHost = "host", IntervalMs = intervalMs }; + var ex = Should.Throw(() => sut.InvokeValidate()); + ex.Message.ShouldContain("interval-ms", Case.Insensitive); + } + + [Fact] + public void Validate_skips_interval_check_when_command_omits_it() + { + // probe / read / write don't take an --interval-ms option; the validator must + // skip that check when the caller passes null. + var sut = new Probe { CncHost = "host" }; + Should.NotThrow(() => sut.InvokeValidateNoInterval()); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/SubscribeCommandConsoleHandlerTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/SubscribeCommandConsoleHandlerTests.cs new file mode 100644 index 0000000..8470040 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/SubscribeCommandConsoleHandlerTests.cs @@ -0,0 +1,55 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests; + +/// +/// Driver.FOCAS.Cli-002: the FOCAS subscribe command — a near-verbatim copy +/// of the Modbus subscribe command — must +/// (a) serialise writes from the OnDataChange handler (raised from the +/// driver's PollGroupEngine background thread) with a lock, so the +/// "Subscribed to ..." banner write from the CliFx main thread cannot interleave +/// with the first poll-driven change line; and +/// (b) carry the explanatory comment that documents why OnDataChange uses +/// console.Output.WriteLine (synchronous, on a driver background thread) +/// instead of System.Console or the async WriteLineAsync. The +/// rationale is non-obvious to a reader and the Modbus copy carries it; the FOCAS +/// copy must too. +/// +[Trait("Category", "Unit")] +public sealed class SubscribeCommandConsoleHandlerTests +{ + private static string ReadSubscribeSource() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = dir.Parent; + dir.ShouldNotBeNull(); + return File.ReadAllText(Path.Combine( + dir!.FullName, "src", "Drivers", "Cli", "ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli", + "Commands", "SubscribeCommand.cs")); + } + + [Fact] + public void SubscribeCommand_explains_why_OnDataChange_uses_console_Output_synchronously() + { + var source = ReadSubscribeSource(); + + // The comment must reference the CliFx console abstraction so future copy-pastes + // do not lose the rationale. + source.ShouldContain("CliFx console"); + source.ShouldContain("IConsole"); + } + + [Fact] + public void SubscribeCommand_serialises_console_writes_with_a_lock() + { + var source = ReadSubscribeSource(); + + // Both the banner write and the OnDataChange handler must share a writeLock so the + // banner from the CliFx invocation thread cannot interleave with the first + // poll-driven change line from the driver tick thread. + source.ShouldContain("writeLock"); + source.ShouldContain("lock (writeLock)"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/WriteCommandParseValueTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/WriteCommandParseValueTests.cs new file mode 100644 index 0000000..a73356b --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/WriteCommandParseValueTests.cs @@ -0,0 +1,122 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests; + +/// +/// Covers across every FOCAS atomic type. +/// Driver.FOCAS.Cli-001: malformed numeric input must surface as a friendly +/// , not a raw +/// / stack trace. +/// +[Trait("Category", "Unit")] +public sealed class WriteCommandParseValueTests +{ + [Theory] + [InlineData("true", true)] + [InlineData("0", false)] + [InlineData("yes", true)] + [InlineData("OFF", false)] + public void ParseValue_Bit_accepts_common_boolean_aliases(string raw, bool expected) + { + WriteCommand.ParseValue(raw, FocasDataType.Bit).ShouldBe(expected); + } + + [Fact] + public void ParseValue_Bit_rejects_garbage_as_CommandException() + { + Should.Throw( + () => WriteCommand.ParseValue("maybe", FocasDataType.Bit)); + } + + [Fact] + public void ParseValue_Byte_signed_range() + { + // FocasDataType.Byte is signed (PMC byte read returns int8). + WriteCommand.ParseValue("-128", FocasDataType.Byte).ShouldBe((sbyte)-128); + WriteCommand.ParseValue("127", FocasDataType.Byte).ShouldBe((sbyte)127); + } + + [Fact] + public void ParseValue_Int16_signed_range() + { + WriteCommand.ParseValue("-32768", FocasDataType.Int16).ShouldBe(short.MinValue); + WriteCommand.ParseValue("32767", FocasDataType.Int16).ShouldBe(short.MaxValue); + } + + [Fact] + public void ParseValue_Int32_parses_negative() + { + WriteCommand.ParseValue("-2147483648", FocasDataType.Int32).ShouldBe(int.MinValue); + } + + [Fact] + public void ParseValue_Float32_invariant_culture() + { + WriteCommand.ParseValue("3.14", FocasDataType.Float32).ShouldBe(3.14f); + } + + [Fact] + public void ParseValue_Float64_higher_precision() + { + WriteCommand.ParseValue("2.718281828", FocasDataType.Float64).ShouldBeOfType(); + } + + [Fact] + public void ParseValue_String_passthrough() + { + WriteCommand.ParseValue("hello fanuc", FocasDataType.String).ShouldBe("hello fanuc"); + } + + // Driver.FOCAS.Cli-001: malformed input must produce a CommandException (a clean + // one-line CliFx error), NOT a raw FormatException stack trace. Previously the raw + // BCL parser exceptions leaked, contradicting how the Bit path already handled bad + // boolean input. + [Theory] + [InlineData("xyz", FocasDataType.Byte)] + [InlineData("xyz", FocasDataType.Int16)] + [InlineData("xyz", FocasDataType.Int32)] + [InlineData("not-a-number", FocasDataType.Float32)] + [InlineData("also-bad", FocasDataType.Float64)] + public void ParseValue_non_numeric_for_numeric_types_throws_CommandException( + string raw, FocasDataType type) + { + Should.Throw( + () => WriteCommand.ParseValue(raw, type)); + } + + // OverflowException from out-of-range input must also surface as CommandException. + [Theory] + [InlineData("128", FocasDataType.Byte)] // sbyte max + 1 + [InlineData("-129", FocasDataType.Byte)] // sbyte min - 1 + [InlineData("32768", FocasDataType.Int16)] // short max + 1 + [InlineData("9999999999", FocasDataType.Int32)] // > int max + public void ParseValue_overflow_for_numeric_types_throws_CommandException( + string raw, FocasDataType type) + { + Should.Throw( + () => WriteCommand.ParseValue(raw, type)); + } + + [Fact] + public void ParseValue_CommandException_message_names_the_type_and_value() + { + var ex = Should.Throw( + () => WriteCommand.ParseValue("xyz", FocasDataType.Int16)); + ex.Message.ShouldContain("xyz"); + ex.Message.ShouldContain("Int16"); + } + + [Theory] + [InlineData("R100", FocasDataType.Int16, "R100:Int16")] + [InlineData("X0.0", FocasDataType.Bit, "X0.0:Bit")] + [InlineData("PARAM:1815/0", FocasDataType.Int32, "PARAM:1815/0:Int32")] + [InlineData("MACRO:500", FocasDataType.Float64, "MACRO:500:Float64")] + public void SynthesiseTagName_preserves_FOCAS_address_verbatim( + string address, FocasDataType type, string expected) + { + ReadCommand.SynthesiseTagName(address, type).ShouldBe(expected); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj new file mode 100644 index 0000000..923e2f7 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj @@ -0,0 +1,26 @@ + + + + net10.0 + enable + enable + false + true + ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests + + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + +