diff --git a/code-reviews/Driver.S7.Cli/findings.md b/code-reviews/Driver.S7.Cli/findings.md index 48b14c9..15f7eb5 100644 --- a/code-reviews/Driver.S7.Cli/findings.md +++ b/code-reviews/Driver.S7.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Checklist coverage @@ -120,7 +120,7 @@ unreachable device, not crash on it. | Severity | Low | | Category | Performance & resource management | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:36,53`, `Commands/ReadCommand.cs:45,54`, `Commands/WriteCommand.cs:51,60`, `Commands/SubscribeCommand.cs:39,73` | -| Status | Open | +| Status | Resolved | **Description:** Every command declares the driver with `await using var driver = new S7Driver(...)` and *also* calls `await driver.ShutdownAsync(...)` in a `finally` block. @@ -136,7 +136,7 @@ not actually disposing. `subscribe` command `UnsubscribeAsync`. Alternatively drop `await using` and keep the explicit `finally`. Pick one disposal mechanism per command. -**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 (DisposeAsync itself runs ShutdownAsync), and 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.S7.Cli-005 @@ -145,7 +145,7 @@ and keep the explicit `finally`. Pick one disposal mechanism per command. | Severity | Low | | Category | Code organization & conventions | | Location | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** A stale directory `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` exists containing only an `obj/` folder — no `.csproj`, no source. The real test @@ -158,7 +158,7 @@ grepping the tree for the S7 CLI test project. directory (including its `obj/`). This is outside the module `src/` tree but is the S7 CLI own orphaned test folder, so it belongs to this module cleanup. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — deleted the stale `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` directory (only contained a leftover `obj/` from before the move into `tests/Drivers/Cli/`; no tracked files). The real test project at `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` is untouched. ### Driver.S7.Cli-006 @@ -167,7 +167,7 @@ S7 CLI own orphaned test folder, so it belongs to this module cleanup. | Severity | Low | | Category | Testing coverage | | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. `S7CommandBase.BuildOptions` — which maps the @@ -184,7 +184,7 @@ not be caught. `ParseValue` is also missing an explicit overflow-edge test (e.g. overflow case to the `ParseValue` numeric tests once Driver.S7.Cli-001 is resolved so the test asserts the wrapped `CommandException`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added `S7CommandBaseBuildOptionsTests` covering Probe.Enabled=false (one-shot CLI guarantee), TimeoutMs→Timeout TimeSpan mapping, host/port/CPU/rack/slot flowing through, and tag-list passthrough. The overflow-edge `ParseValue` test was already added under Driver.S7.Cli-001 (`ParseValue_overflow_for_numeric_types_throws_CommandException`). ### Driver.S7.Cli-007 @@ -193,7 +193,7 @@ the test asserts the wrapped `CommandException`. | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:45-51` | -| Status | Open | +| Status | Resolved | **Description:** The Modbus CLI `SubscribeCommand` carries an explanatory comment on the `OnDataChange` handler ("Route every data-change event to the CliFx console (not @@ -206,4 +206,4 @@ Minor, but the rationale is worth keeping consistent across the CLI family. **Recommendation:** Re-add the one-line comment from the Modbus `SubscribeCommand` so the S7 copy explains why the event handler writes via `console.Output` synchronously. -**Resolution:** _(open)_ +**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. 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 154f184..668f895 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 @@ -33,6 +33,9 @@ public sealed class ProbeCommand : S7CommandBase Writable: false); var options = BuildOptions([probeTag]); + // Driver.S7.Cli-004: `await using` is the sole disposal mechanism — S7Driver.DisposeAsync + // already invokes ShutdownAsync, so the previous explicit ShutdownAsync(CancellationToken.None) + // call in a finally block ran shutdown twice. The await-using on the next line is enough. await using var driver = new S7Driver(options, DriverInstanceId); // Driver.S7.Cli-003: wrap the entire probe sequence so that a refused/unreachable TCP // connect still prints the structured Host/CPU/Health lines instead of crashing with a @@ -66,9 +69,5 @@ public sealed class ProbeCommand : S7CommandBase if (health.LastError is { } err) await console.Output.WriteLineAsync($"Last error: {err}"); } - finally - { - await driver.ShutdownAsync(CancellationToken.None); - } } } 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 0286df2..8c8cc41 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 @@ -45,17 +45,13 @@ public sealed class ReadCommand : S7CommandBase StringLength: StringLength); var options = BuildOptions([tag]); + // Driver.S7.Cli-004: `await using` is the sole disposal mechanism — S7Driver.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 S7Driver(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])); } /// 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 dbe5983..65555e1 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 @@ -37,12 +37,20 @@ public sealed class SubscribeCommand : S7CommandBase Writable: false); var options = BuildOptions([tag]); + // Driver.S7.Cli-004: `await using` is the sole driver-disposal mechanism — S7Driver.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 S7Driver(options, DriverInstanceId); ISubscriptionHandle? handle = null; try { await driver.InitializeAsync("{}", ct); + // Driver.S7.Cli-007: 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 thread-safe for line writes. driver.OnDataChange += (_, e) => { var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + @@ -71,7 +79,6 @@ public sealed class SubscribeCommand : S7CommandBase 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.S7.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs index 3e2256b..b13c2ae 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 @@ -52,17 +52,13 @@ public sealed class WriteCommand : S7CommandBase var parsed = ParseValue(Value, DataType); + // Driver.S7.Cli-004: `await using` is the sole disposal mechanism — S7Driver.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 S7Driver(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])); } /// Parse --value per , invariant culture throughout. diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CommandDisposalConventionsTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CommandDisposalConventionsTests.cs new file mode 100644 index 0000000..77ecc02 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CommandDisposalConventionsTests.cs @@ -0,0 +1,61 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Driver.S7.Cli-004: every S7 CLI command must own one disposal mechanism for the +/// S7Driver, not two. The chosen mechanism is await using var driver = ... +/// — S7Driver.DisposeAsync already calls ShutdownAsync, so an additional +/// explicit driver.ShutdownAsync(...) in a finally block runs shutdown +/// twice (three times on subscribe). 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_S7Driver(string commandFile) + { + var path = Path.Combine(CommandsDir, commandFile); + var source = File.ReadAllText(path); + + source.ShouldContain("await using var driver = new S7Driver("); + } + + 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.S7.Cli", "Commands"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7CommandBaseBuildOptionsTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7CommandBaseBuildOptionsTests.cs new file mode 100644 index 0000000..1e5b616 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7CommandBaseBuildOptionsTests.cs @@ -0,0 +1,92 @@ +using CliFx.Attributes; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.S7.Cli; +using S7NetCpuType = global::S7.Net.CpuType; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Covers — the pure, deterministic mapping +/// from the base's host/port/CPU/rack/slot/timeout flags onto an +/// S7DriverOptions. The CLI is one-shot so the background connectivity probe +/// must be disabled. +/// +[Trait("Category", "Unit")] +public sealed class S7CommandBaseBuildOptionsTests +{ + // Test-only S7CommandBase concrete subclass that exposes the protected BuildOptions + // 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 S7CommandBase.BuildOptions.")] + private sealed class ProbeOnly : S7CommandBase + { + public override ValueTask ExecuteAsync(IConsole console) => default; + public S7DriverOptions Invoke(IReadOnlyList tags) => BuildOptions(tags); + } + + [Fact] + public void BuildOptions_disables_probe_for_one_shot_cli_runs() + { + var sut = new ProbeOnly + { + Host = "10.0.0.5", + Port = 102, + CpuType = S7NetCpuType.S71500, + Rack = 0, + Slot = 0, + 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 { Host = "h", TimeoutMs = 7500 }; + + var options = sut.Invoke([]); + + options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500)); + } + + [Fact] + public void BuildOptions_flows_host_port_cpu_rack_slot_through() + { + var sut = new ProbeOnly + { + Host = "plc.shop.local", + Port = 4102, + CpuType = S7NetCpuType.S7300, + Rack = 1, + Slot = 2, + TimeoutMs = 3000, + }; + + var options = sut.Invoke([]); + + options.Host.ShouldBe("plc.shop.local"); + options.Port.ShouldBe(4102); + options.CpuType.ShouldBe(S7NetCpuType.S7300); + options.Rack.ShouldBe((short)1); + options.Slot.ShouldBe((short)2); + } + + [Fact] + public void BuildOptions_forwards_tag_list_verbatim() + { + var sut = new ProbeOnly { Host = "h" }; + var tag = new S7TagDefinition("t", "MW0", S7DataType.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.S7.Cli.Tests/SubscribeCommandConsoleHandlerCommentTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/SubscribeCommandConsoleHandlerCommentTests.cs new file mode 100644 index 0000000..3749e3e --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/SubscribeCommandConsoleHandlerCommentTests.cs @@ -0,0 +1,32 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests; + +/// +/// Driver.S7.Cli-007: the S7 subscribe command — a near-verbatim copy of the Modbus +/// subscribe command — must keep the comment that explains 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 S7 copy must too. +/// +[Trait("Category", "Unit")] +public sealed class SubscribeCommandConsoleHandlerCommentTests +{ + [Fact] + public void SubscribeCommand_explains_why_OnDataChange_uses_console_Output_synchronously() + { + 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(); + var source = File.ReadAllText(Path.Combine( + dir!.FullName, "src", "Drivers", "Cli", "ZB.MOM.WW.OtOpcUa.Driver.S7.Cli", + "Commands", "SubscribeCommand.cs")); + + // The comment must reference the CliFx console abstraction so future copy-pastes + // do not lose the rationale. + source.ShouldContain("CliFx console"); + source.ShouldContain("IConsole"); + } +}