diff --git a/code-reviews/Driver.S7.Cli/findings.md b/code-reviews/Driver.S7.Cli/findings.md index 15f7eb5c..3a1691a9 100644 --- a/code-reviews/Driver.S7.Cli/findings.md +++ b/code-reviews/Driver.S7.Cli/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `111d6983` | | Status | Reviewed | | Open findings | 0 | @@ -207,3 +207,95 @@ Minor, but the rationale is worth keeping consistent across the CLI family. the S7 copy explains why the event handler writes via `console.Output` synchronously. **Resolution:** Resolved 2026-05-23 — re-added the explanatory comment above the `OnDataChange` handler in the S7 `SubscribeCommand`, mirroring the Modbus copy: it explains the use of the CliFx `IConsole.Output` abstraction (rather than `System.Console`) and notes that the handler runs synchronously because it's raised from a driver background thread. Added `SubscribeCommandConsoleHandlerCommentTests` to guard the rationale against future copy-paste regressions. + +## Re-review 2026-06-19 (commit 111d6983) + +#### Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.S7.Cli-009, Driver.S7.Cli-010 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | Driver.S7.Cli-011 | +| 4 | Error handling & resilience | Driver.S7.Cli-008, Driver.S7.Cli-009 | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | Driver.S7.Cli-012 | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No issues found | +| 10 | Documentation & comments | No issues found | + +### Driver.S7.Cli-008 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Error handling & resilience | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/S7CommandBase.cs` | +| Status | Resolved | + +**Description:** `S7CommandBase` had no `ValidateEndpoint()` method, so an operator passing `--port 0`, `--port 65536`, or `--timeout-ms -1` got an opaque socket or argument exception thrown deep inside the S7.Net library stack rather than a clean `CommandException`. The analogous `ModbusCommandBase.ValidateEndpoint()` already had this guard. + +**Recommendation:** Add a `ValidateEndpoint()` method to `S7CommandBase` that validates `Port` (1..65535) and `TimeoutMs` (strictly positive) before the driver is created, and call it at the top of each command's `ExecuteAsync`. Add tests covering invalid port values, non-positive timeout, boundary port values (1 and 65535), and the happy path. + +**Resolution:** Resolved 2026-06-19 — added `ValidateEndpoint()` to `S7CommandBase` (port 1..65535, TimeoutMs strictly positive, both throw `CommandException` with the flag name); called it in `ProbeCommand`, `ReadCommand`, `WriteCommand`, and `SubscribeCommand`; added `S7EndpointValidationTests` (8 cases). + +### Driver.S7.Cli-009 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:61`, `Commands/ReadCommand.cs`, `Commands/WriteCommand.cs` | +| Status | Resolved | + +**Description:** Two related cancellation-handling gaps: (1) `ProbeCommand` caught `OperationCanceledException` without the `when (ct.IsCancellationRequested)` filter — a driver-internal timeout raised with a different CancellationToken would be caught and re-thrown from the wrong handler branch, masking the real error. (2) `ReadCommand` and `WriteCommand` had no `OperationCanceledException` handler at all — when the operator presses Ctrl+C during a connect or read, CliFx handles the exception silently, but the Modbus/AbCip CLI pattern of printing "Cancelled." and letting the caller see a clean one-line acknowledgement was missing. + +**Recommendation:** (1) Add `when (ct.IsCancellationRequested)` to the `ProbeCommand` catch. (2) Wrap `ReadCommand` and `WriteCommand`'s driver calls in a `try/catch (OperationCanceledException) when (ct.IsCancellationRequested)` that prints "Cancelled." and returns, matching the Modbus pattern. Add source-level tests to guard the patterns. + +**Resolution:** Resolved 2026-06-19 — added `when (ct.IsCancellationRequested)` filter to `ProbeCommand`; wrapped `ReadCommand` and `WriteCommand` driver calls in `try/catch (OperationCanceledException) when (ct.IsCancellationRequested)` printing "Cancelled."; added `CancellationHandlingTests` (3 cases). + +### Driver.S7.Cli-010 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs` | +| Status | Resolved | + +**Description:** `SubscribeCommand.ExecuteAsync` did not call `FlushLogging()` in its `finally` block. `DriverCommandBase.ConfigureLogging()` documents that `FlushLogging()` must be called in a `finally` to prevent buffered Serilog output from being discarded on process exit. For the long-running `subscribe` command (which can run for minutes before Ctrl+C), reconnect warnings and other log lines emitted just before termination would be silently lost. The `AbCip` CLI subscribe command already had this call. + +**Recommendation:** Add `FlushLogging()` as the last statement in `SubscribeCommand`'s outer `finally` block. Guard with a source-level test to prevent regression. + +**Resolution:** Resolved 2026-06-19 — added `FlushLogging()` as the final statement in `SubscribeCommand`'s `finally` block; added `FlushLoggingConventionTests` (1 case). + +### Driver.S7.Cli-011 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Concurrency & thread safety | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:61-67` | +| Status | Resolved | + +**Description:** The `OnDataChange` handler in `SubscribeCommand` called `console.Output.WriteLine` without serialisation. `OnDataChange` is raised on the `PollGroupEngine` background timer thread; if two poll ticks complete close together (e.g. with a very short `--interval-ms`), concurrent handler invocations can interleave partial lines on the output. The Modbus CLI subscribe command guards against this with a `lock(writeLock)` wrapper; the S7 command was a copy-paste that dropped the lock. + +**Recommendation:** Add a `var writeLock = new object();` beside the driver declaration and wrap the `console.Output.WriteLine` call in `lock (writeLock)`. Guard with a source-level test. + +**Resolution:** Resolved 2026-06-19 — added `var writeLock = new object()` and wrapped `console.Output.WriteLine` in `lock (writeLock)` inside `SubscribeCommand.OnDataChange`; added `SubscribeCommandWriteLockTests` (1 case). + +### Driver.S7.Cli-012 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Design-document adherence | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs` | +| Status | Deferred | + +**Description:** The Modbus CLI `ProbeCommand` was enhanced (Driver.Modbus.Cli-006) with a `ComputeVerdict` method that derives a single `OK / DEGRADED / FAIL` headline from both the driver state and the probe-read snapshot StatusCode. This prevents the contradictory output `Health: Healthy` when the probe snapshot carries a Bad quality code. The S7 `ProbeCommand` has the same contradiction risk — it prints `Health: {health.State}` without cross-checking the snapshot quality. + +**Recommendation:** Port the `ComputeVerdict` logic from the Modbus CLI to the S7 CLI: derive a verdict from `health.State` + `snapshot[0].StatusCode` and print it as the headline `Verdict:` line, keeping `Health:` for the raw driver state. + +**Resolution:** Deferred — enhancement rather than bug; requires mirroring the Modbus CLI's `ComputeVerdict` pattern which is a design change not a correctness fix. No test regression risk from deferring. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs index 4e1aaf23..491102b3 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs @@ -27,6 +27,7 @@ public sealed class ProbeCommand : S7CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); var probeTag = new S7TagDefinition( @@ -58,9 +59,12 @@ public sealed class ProbeCommand : S7CommandBase await console.Output.WriteLineAsync(); await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (ct.IsCancellationRequested) { - throw; // Ctrl+C — let CliFx handle it normally. + // Ctrl+C — re-throw so CliFx exits cleanly. The when filter ensures a + // driver-internal timeout (different CancellationToken) is not mis-classified + // as user cancellation and falls through to the structured-report catch below. + throw; } catch { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ReadCommand.cs index 57a26c72..c98c9e54 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ReadCommand.cs @@ -38,6 +38,7 @@ public sealed class ReadCommand : S7CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); var tagName = SynthesiseTagName(Address, DataType); @@ -53,9 +54,18 @@ public sealed class ReadCommand : S7CommandBase // 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 S7Driver(options, DriverInstanceId); - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync([tagName], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); + try + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync([tagName], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0])); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Driver.S7.Cli-009: 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."); + } } /// Tag-name key used internally. Address + type is already unique. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs index dbbf68a3..611ed913 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs @@ -33,6 +33,7 @@ public sealed class SubscribeCommand : S7CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Address, DataType); @@ -48,6 +49,9 @@ public sealed class SubscribeCommand : S7CommandBase // 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 S7Driver(options, DriverInstanceId); + // Driver.S7.Cli-011: serialize console writes from the PollGroupEngine background + // thread so overlapping poll ticks cannot interleave partial lines on the output. + var writeLock = new object(); ISubscriptionHandle? handle = null; try { @@ -62,7 +66,10 @@ public sealed class SubscribeCommand : S7CommandBase var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; - console.Output.WriteLine(line); + lock (writeLock) + { + console.Output.WriteLine(line); + } }; handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); @@ -85,6 +92,11 @@ public sealed class SubscribeCommand : S7CommandBase try { await driver.UnsubscribeAsync(handle, CancellationToken.None); } catch { /* teardown best-effort */ } } + // Driver.S7.Cli-010: flush Serilog before process exit so buffered log lines + // emitted just before Ctrl+C (e.g. reconnect warnings from the PollGroupEngine) + // are not lost on abrupt termination. DriverCommandBase.ConfigureLogging() docs + // require this call in a finally block. + FlushLogging(); } } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs index 90287597..d8deab3a 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs @@ -43,6 +43,7 @@ public sealed class WriteCommand : S7CommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Address, DataType); @@ -60,9 +61,18 @@ public sealed class WriteCommand : S7CommandBase // 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 S7Driver(options, DriverInstanceId); - await driver.InitializeAsync("{}", ct); - var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0])); + try + { + await driver.InitializeAsync("{}", ct); + var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0])); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Driver.S7.Cli-009: 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."); + } } /// Parse --value per , invariant culture throughout. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/S7CommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/S7CommandBase.cs index ecf725f5..68bc68af 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/S7CommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/S7CommandBase.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.S7.Cli; @@ -65,4 +66,25 @@ public abstract class S7CommandBase : DriverCommandBase /// Gets the driver instance ID for this CLI session. protected string DriverInstanceId => $"s7-cli-{Host}:{Port}"; + + /// + /// Driver.S7.Cli-008: validate the endpoint flags at parse time so the operator + /// gets a clear instead of an opaque socket or + /// argument exception thrown deep inside the S7.Net stack. + /// + /// --port: 1..65535 (IANA TCP port space). + /// --timeout-ms: strictly positive — a non-positive value would + /// propagate as an immediate-timeout into S7.Net and surface as a confusing + /// . + /// + /// + 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})."); + } } diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CancellationHandlingTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CancellationHandlingTests.cs new file mode 100644 index 00000000..ecf9d86a --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CancellationHandlingTests.cs @@ -0,0 +1,71 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Driver.S7.Cli-009: every S7 CLI command that catches +/// must use the +/// when (ct.IsCancellationRequested) exception filter so that a +/// driver-internal timeout (thrown with a different cancellation token) does not get +/// mis-handled as a user Ctrl+C. Source-level check mirrors +/// . +/// +[Trait("Category", "Unit")] +public sealed class CancellationHandlingTests +{ + private static readonly string CommandsDir = LocateCommandsDir(); + + /// + /// Verifies that ProbeCommand uses the ct.IsCancellationRequested guard on its + /// OperationCanceledException catch block so a driver-internal timeout is not + /// swallowed as a user-cancelled operation. + /// + [Fact] + public void ProbeCommand_uses_ct_IsCancellationRequested_guard() + { + // Driver.S7.Cli-009: the bare `catch (OperationCanceledException)` without a + // `when` filter would mis-classify a driver-internal timeout as a Ctrl+C and + // either re-throw from the wrong catch block or swallow the error. + var source = File.ReadAllText(Path.Combine(CommandsDir, "ProbeCommand.cs")); + source.ShouldContain("when (ct.IsCancellationRequested)"); + } + + /// + /// Verifies that ReadCommand provides quiet cancellation handling for Ctrl+C. + /// + [Fact] + public void ReadCommand_handles_OperationCanceledException_quietly() + { + // Driver.S7.Cli-009: ReadCommand must catch OperationCanceledException and + // print "Cancelled." (matching the Modbus CLI pattern) rather than letting + // CliFx render an unhandled exception on Ctrl+C. + var source = File.ReadAllText(Path.Combine(CommandsDir, "ReadCommand.cs")); + source.ShouldContain("OperationCanceledException"); + source.ShouldContain("Cancelled."); + } + + /// + /// Verifies that WriteCommand provides quiet cancellation handling for Ctrl+C. + /// + [Fact] + public void WriteCommand_handles_OperationCanceledException_quietly() + { + // Driver.S7.Cli-009: WriteCommand must catch OperationCanceledException and + // print "Cancelled." (matching the Modbus CLI pattern) rather than letting + // CliFx render an unhandled exception on Ctrl+C. + var source = File.ReadAllText(Path.Combine(CommandsDir, "WriteCommand.cs")); + source.ShouldContain("OperationCanceledException"); + source.ShouldContain("Cancelled."); + } + + private static string LocateCommandsDir() + { + 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.S7.Cli", "Commands"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/FlushLoggingConventionTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/FlushLoggingConventionTests.cs new file mode 100644 index 00000000..ff1f534d --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/FlushLoggingConventionTests.cs @@ -0,0 +1,45 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Driver.S7.Cli-010: the long-running subscribe command must call +/// FlushLogging() in its finally block so buffered Serilog log lines +/// emitted just before Ctrl+C are not discarded on process exit. +/// +/// DriverCommandBase.ConfigureLogging() instructs every command to call +/// FlushLogging() in a finally. The short-lived read, +/// write, and probe commands are less exposed (their log volume is +/// small and the process exits immediately after), but subscribe can run for +/// minutes and then Ctrl+C — making the flush critical. +/// +/// +[Trait("Category", "Unit")] +public sealed class FlushLoggingConventionTests +{ + private static readonly string CommandsDir = LocateCommandsDir(); + + /// + /// Verifies that SubscribeCommand calls FlushLogging() to prevent buffered + /// Serilog output from being lost on Ctrl+C. + /// + [Fact] + public void SubscribeCommand_calls_FlushLogging() + { + // Driver.S7.Cli-010: FlushLogging() must appear in the command so that + // DriverCommandBase.ConfigureLogging()'s documented contract is honoured. + var source = File.ReadAllText(Path.Combine(CommandsDir, "SubscribeCommand.cs")); + source.ShouldContain("FlushLogging()"); + } + + private static string LocateCommandsDir() + { + 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.S7.Cli", "Commands"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7EndpointValidationTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7EndpointValidationTests.cs new file mode 100644 index 00000000..2731e302 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7EndpointValidationTests.cs @@ -0,0 +1,73 @@ +using CliFx.Attributes; +using CliFx.Exceptions; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.S7; +using ZB.MOM.WW.OtOpcUa.Driver.S7.Cli; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Driver.S7.Cli-008: S7CommandBase must reject out-of-range port and non-positive +/// timeout values before the command body opens a driver — matching the validation pattern +/// established by ModbusCommandBase.ValidateEndpoint. +/// +[Trait("Category", "Unit")] +public sealed class S7EndpointValidationTests +{ + // Test-only subclass that exposes ValidateEndpoint and delegates the abstract ExecuteAsync. + [Command("noop-validate", Description = "Test-only stub for S7CommandBase.ValidateEndpoint.")] + private sealed class ValidateStub : S7CommandBase + { + /// + public override ValueTask ExecuteAsync(IConsole console) => default; + + /// Exposes the protected ValidateEndpoint method for tests. + public void InvokeValidate() => ValidateEndpoint(); + } + + /// Verifies that a valid port and timeout passes validation without throwing. + [Fact] + public void ValidateEndpoint_valid_inputs_pass() + { + // Driver.S7.Cli-008: verify the happy path does not throw. + var sut = new ValidateStub { Host = "plc.local", Port = 102, TimeoutMs = 5000 }; + Should.NotThrow(() => sut.InvokeValidate()); + } + + /// Verifies that port 0 is rejected as out of range. + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(65536)] + public void ValidateEndpoint_invalid_port_throws_CommandException(int port) + { + // Driver.S7.Cli-008: port must be 1..65535. + var sut = new ValidateStub { Host = "h", Port = port, TimeoutMs = 5000 }; + Should.Throw(() => sut.InvokeValidate()) + .Message.ShouldContain("--port"); + } + + /// Verifies that a non-positive timeout is rejected. + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void ValidateEndpoint_non_positive_timeout_throws_CommandException(int timeoutMs) + { + // Driver.S7.Cli-008: timeout must be strictly positive. + var sut = new ValidateStub { Host = "h", Port = 102, TimeoutMs = timeoutMs }; + Should.Throw(() => sut.InvokeValidate()) + .Message.ShouldContain("--timeout-ms"); + } + + /// Verifies that the boundary port values 1 and 65535 pass validation. + [Theory] + [InlineData(1)] + [InlineData(65535)] + public void ValidateEndpoint_boundary_ports_pass(int port) + { + var sut = new ValidateStub { Host = "h", Port = port, TimeoutMs = 1000 }; + Should.NotThrow(() => sut.InvokeValidate()); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/SubscribeCommandWriteLockTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/SubscribeCommandWriteLockTests.cs new file mode 100644 index 00000000..04516133 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/SubscribeCommandWriteLockTests.cs @@ -0,0 +1,40 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Driver.S7.Cli-011: the subscribe command's OnDataChange handler must +/// serialize its console writes through a lock so that overlapping poll ticks (which +/// arrive on a background thread) cannot interleave partial lines on the output stream. +/// Mirrors the pattern from the Modbus CLI subscribe command. +/// +[Trait("Category", "Unit")] +public sealed class SubscribeCommandWriteLockTests +{ + private static readonly string CommandsDir = LocateCommandsDir(); + + /// + /// Verifies that SubscribeCommand uses a lock in the OnDataChange handler to + /// prevent interleaved console writes from overlapping poll ticks. + /// + [Fact] + public void SubscribeCommand_OnDataChange_uses_write_lock() + { + // Driver.S7.Cli-011: the handler writes via console.Output.WriteLine on a + // background poll thread; concurrent ticks must be serialised with a lock + // to prevent partial-line interleaving. + var source = File.ReadAllText(Path.Combine(CommandsDir, "SubscribeCommand.cs")); + source.ShouldContain("lock (writeLock)"); + } + + private static string LocateCommandsDir() + { + 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.S7.Cli", "Commands"); + } +}