dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-s7-cli): resolve Low code-review findings (Driver.S7.Cli-004,005,006,007)

Browse Source 
- Driver.S7.Cli-004: 'await using var driver' is the sole driver
  disposal path; dropped the redundant explicit await ShutdownAsync from
  each command's finally.
- Driver.S7.Cli-005: deleted the stale empty
  tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ directory (the real test
  project lives under tests/Drivers/Cli/).
- Driver.S7.Cli-006: S7CommandBaseBuildOptionsTests cover the probe
  toggle, timeout mapping, host/port/CPU/rack/slot wiring, and tag list
  passthrough.
- Driver.S7.Cli-007: re-added the SubscribeCommand handler comment
  explaining the CliFx IConsole.Output usage and that the poll-thread
  raises events.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 08:34:48 -04:00
parent f46e126208
commit 67ef6c4ebc
8 changed files with 217 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
code-reviews/Driver.S7.Cli/findings.md
View File

@@ -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.

7
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs
View File

@@ -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);
}
}
}

16
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ReadCommand.cs
View File

@@ -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]));
}
/// <summary>Tag-name key used internally. Address + type is already unique.</summary>

9
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs
View File

@@ -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);
}
}
}

16
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs
View File

@@ -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]));
}
/// <summary>Parse <c>--value</c> per <see cref="S7DataType"/>, invariant culture throughout.</summary>

61
tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/CommandDisposalConventionsTests.cs Normal file
View File

@@ -0,0 +1,61 @@
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests;
/// <summary>
/// Driver.S7.Cli-004: every S7 CLI command must own one disposal mechanism for the
/// <c>S7Driver</c>, not two. The chosen mechanism is <c>await using var driver = ...</c>
/// — <c>S7Driver.DisposeAsync</c> already calls <c>ShutdownAsync</c>, so an additional
/// explicit <c>driver.ShutdownAsync(...)</c> in a <c>finally</c> block runs shutdown
/// twice (three times on subscribe). These tests guard against that regression by
/// scanning the command source files.
/// </summary>
[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");
}
}

92
tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/S7CommandBaseBuildOptionsTests.cs Normal file
View File

@@ -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;
/// <summary>
/// Covers <see cref="S7CommandBase.BuildOptions"/> — the pure, deterministic mapping
/// from the base's host/port/CPU/rack/slot/timeout flags onto an
/// <c>S7DriverOptions</c>. The CLI is one-shot so the background connectivity probe
/// must be disabled.
/// </summary>
[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<S7TagDefinition> 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);
}
}

32
tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/SubscribeCommandConsoleHandlerCommentTests.cs Normal file
View File

@@ -0,0 +1,32 @@
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests;
/// <summary>
/// Driver.S7.Cli-007: the S7 subscribe command — a near-verbatim copy of the Modbus
/// subscribe command — must keep the comment that explains why <c>OnDataChange</c>
/// uses <c>console.Output.WriteLine</c> (synchronous, on a driver background thread)
/// instead of <c>System.Console</c> or the async <c>WriteLineAsync</c>. The rationale
/// is non-obvious to a reader and the Modbus copy carries it; the S7 copy must too.
/// </summary>
[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");
}
}