Compare commits
6 Commits
2a941b255f
...
59ecd18169
| Author | SHA1 | Date | |
|---|---|---|---|
| 59ecd18169 | |||
| 2a6ac07111 | |||
| 7fe9f16cf8 | |||
| 879925180b | |||
| 3ca569f621 | |||
| 6923be3aa2 |
@@ -85,6 +85,7 @@
|
||||
<Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests.csproj" />
|
||||
<Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests.csproj" />
|
||||
<Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests.csproj" />
|
||||
<Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj" />
|
||||
</Folder>
|
||||
<Folder Name="/tests/Client/">
|
||||
<Project Path="tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj" />
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 8 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -62,7 +62,7 @@ assumption precisely.
|
||||
| Severity | Low |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `Commands/SubscribeCommand.cs:129-137` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The summary computes `neverWentBad` as every target whose node-id key is
|
||||
absent from the `everBad` dictionary. A node that received no update at all is also absent
|
||||
@@ -78,7 +78,7 @@ streamed only good values.
|
||||
"suspect" list only contains nodes that were actually observed and never reported bad
|
||||
quality.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `neverWentBad` now requires the node to be present in `lastStatus` (i.e. it received at least one update) before being counted, so the "suspect" bucket only contains nodes that were actually observed and never reported bad quality.
|
||||
|
||||
### Client.CLI-003
|
||||
|
||||
@@ -87,7 +87,7 @@ quality.
|
||||
| Severity | Low |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Numeric command options accept any value with no range validation.
|
||||
`--depth`, `--interval`, `--max-depth`, `--max`, and the history `--interval` can all be
|
||||
@@ -100,7 +100,7 @@ is forwarded to `HistoryReadRawAsync`. None of these produce a clear operator-fa
|
||||
`CliFx.Exceptions.CommandException` with an actionable message when a value is out of
|
||||
range.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — every command's `ExecuteAsync` now validates the numeric option ranges (`--interval`, `--depth`, `--max-depth`, `--max`, `--duration`) and throws `CliFx.Exceptions.CommandException` with the offending value when a non-positive (or otherwise out-of-range) value is supplied. Pinned by `CommandRangeValidationTests`.
|
||||
|
||||
### Client.CLI-004
|
||||
|
||||
@@ -109,7 +109,7 @@ range.
|
||||
| Severity | Low |
|
||||
| Category | OtOpcUa conventions |
|
||||
| Location | `Commands/SubscribeCommand.cs:13-37` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `SubscribeCommand` is the only command in the module whose constructor
|
||||
and all `[CommandOption]` properties have no XML doc comments. Every other command
|
||||
@@ -121,7 +121,7 @@ otherwise-uniform documentation convention of the module.
|
||||
**Recommendation:** Add `<summary>` XML docs to the `SubscribeCommand` constructor and to
|
||||
each of its option properties, matching the style used by the sibling commands.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` now carries `<summary>` XML docs on the type, the constructor, every `[CommandOption]` property, and `ExecuteAsync`, matching the style used by the sibling commands.
|
||||
|
||||
### Client.CLI-005
|
||||
|
||||
@@ -156,7 +156,7 @@ callback.
|
||||
| Severity | Low |
|
||||
| Category | Error handling & resilience |
|
||||
| Location | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Operator input-format errors surface as raw .NET exceptions rather than
|
||||
clean CLI errors. An unparseable start/end value throws `FormatException` straight out of
|
||||
@@ -170,7 +170,7 @@ is converted to a `CliFx.Exceptions.CommandException` with a clean exit code.
|
||||
`CommandException` with a concise message and a non-zero exit code, so malformed input
|
||||
yields a one-line error instead of a stack trace.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `HistoryReadCommand` parses `--start`/`--end` with `CultureInfo.InvariantCulture` + `AssumeUniversal`/`AdjustToUniversal`, catches `FormatException`, and rethrows as `CommandException` with the offending value. Every command's call to `NodeIdParser.ParseRequired` is wrapped in a `catch (FormatException or ArgumentException)` block that surfaces the underlying message as a clean CLI error. Pinned by `InputValidationErrorsTests`.
|
||||
|
||||
### Client.CLI-007
|
||||
|
||||
@@ -179,7 +179,7 @@ yields a one-line error instead of a stack trace.
|
||||
| Severity | Low |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `CommandBase.cs:112-123` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `ConfigureLogging` builds a new Serilog `LoggerConfiguration`, creates a
|
||||
logger, and assigns it to the static `Log.Logger` without disposing the previously
|
||||
@@ -193,7 +193,7 @@ abandons the prior console sink without disposal. The pattern is incorrect:
|
||||
build the logger into a local `ILogger` the command owns and disposes, rather than mutating
|
||||
global static state per command.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `CommandBase.ConfigureLogging` now calls `Log.CloseAndFlush()` before assigning a new `Log.Logger`, so a prior logger's console sink is disposed before the next one is installed. Pinned by `LoggerLifecycleTests`.
|
||||
|
||||
### Client.CLI-008
|
||||
|
||||
@@ -202,7 +202,7 @@ global static state per command.
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `docs/Client.CLI.md:158-217` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `docs/Client.CLI.md` is stale relative to the code at this commit.
|
||||
(1) The `subscribe` command section documents only `-n` and `-i`, but the code
|
||||
@@ -218,7 +218,7 @@ code option description includes it.
|
||||
`docs/Client.CLI.md` from the current option set, including the five new subscribe flags
|
||||
and the `StandardDeviation` aggregate row.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — rewrote the `subscribe` section of `docs/Client.CLI.md` to document every flag (`-r/--recursive`, `--max-depth`, `-q/--quiet`, `--duration`, `--summary-file`) plus the summary-bucket vocabulary, and added the `StandardDeviation` row plus the UTC `--start`/`--end` convention note to the `historyread` section.
|
||||
|
||||
### Client.CLI-009
|
||||
|
||||
@@ -227,7 +227,7 @@ and the `StandardDeviation` aggregate row.
|
||||
| Severity | Low |
|
||||
| Category | Code organization & conventions |
|
||||
| Location | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Both long-running commands attach an event handler
|
||||
(`service.DataChanged += ...`, `service.AlarmEvent += ...`) with a lambda and never detach
|
||||
@@ -243,7 +243,7 @@ but never the .NET event.
|
||||
unsubscribing, using a named local delegate so it can be removed, ensuring no notification
|
||||
is processed after the command output phase ends.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` and `AlarmsCommand` declare named local handlers (`DataChangedHandler` / `AlarmEventHandler`) and detach them via `service.DataChanged -= ...` / `service.AlarmEvent -= ...` right after `UnsubscribeAsync` so no notification reaches the console once the command's output phase ends. Pinned by `EventHandlerLifecycleTests`.
|
||||
|
||||
### Client.CLI-010
|
||||
|
||||
@@ -252,7 +252,7 @@ is processed after the command output phase ends.
|
||||
| Severity | Low |
|
||||
| Category | Testing coverage |
|
||||
| Location | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The new `SubscribeCommand` capabilities are largely untested. The four
|
||||
`SubscribeCommandTests` cover only single-node subscribe, unsubscribe-on-cancel,
|
||||
@@ -268,4 +268,4 @@ exit, summary bucketing across good/bad/no-update nodes, and the `--summary-file
|
||||
The `FakeOpcUaClientService` already exposes `RaiseDataChanged`, so feeding good/bad values
|
||||
and asserting the summary text is straightforward.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — added `SubscribeCommandSummaryTests` (covering recursive collection via `FakeOpcUaClientService.AddDiscoveredVariable`, `--duration` auto-exit, summary bucketing for good/bad/never/never-went-bad, and the `--summary-file` write), `CommandRangeValidationTests`, `EventHandlerLifecycleTests`, `InputValidationErrorsTests`, and `LoggerLifecycleTests` to pin the other Low findings; `FakeOpcUaClientService` was extended with `AddDiscoveredVariable` / `RaiseDataChanged` helpers.
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 5 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -63,13 +63,13 @@
|
||||
| Severity | Low |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `WriteValueAsync` returns `response.Results[0]` and `CallMethodAsync` reads `result.Results[0]` without first checking the `Results` collection is non-empty. A malformed or service-level-faulted response (empty `Results` alongside a service fault) produces an `IndexOutOfRangeException` rather than a meaningful OPC UA `StatusCode` or `ServiceResultException`.
|
||||
|
||||
**Recommendation:** Guard both accesses — throw `ServiceResultException` with the response's `ResponseHeader.ServiceResult` (or `BadUnexpectedError`) when `Results` is empty.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — added empty-Results guards to both `WriteValueAsync` (lines 80-85) and `CallMethodAsync` (lines 293-298) in `DefaultSessionAdapter`. Each now throws `ServiceResultException` carrying `response.ResponseHeader.ServiceResult.Code` (or `StatusCodes.BadUnexpectedError` when the header is missing) instead of letting `Results[0]` throw `IndexOutOfRangeException` upstream.
|
||||
|
||||
### Client.Shared-004
|
||||
|
||||
@@ -78,13 +78,13 @@
|
||||
| Severity | Low |
|
||||
| Category | OtOpcUa conventions |
|
||||
| Location | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `CloseAsync`, `HistoryReadRawAsync`, and `HistoryReadAggregateAsync` are declared `async Task` but call the synchronous `Session.Close()` / `Session.HistoryRead(...)` APIs and contain no `await`. The history methods run a blocking synchronous service round-trip on the caller's thread; for the UI this blocks the dispatcher thread. The async signature misleads callers, and the `CancellationToken` parameter is ignored on these paths.
|
||||
|
||||
**Recommendation:** Use the stack's async overloads (`Session.HistoryReadAsync`, `Session.CloseAsync`) where available, or wrap the synchronous calls in `Task.Run`, so the methods are genuinely asynchronous and honor the cancellation token.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — replaced the three blocking calls with their async counterparts: `CloseAsync` now awaits `Session.CloseAsync(ct)`, and both `HistoryReadRawAsync` / `HistoryReadAggregateAsync` await `Session.HistoryReadAsync(...)` with `.ConfigureAwait(false)`. All three now honor the `CancellationToken` and no longer block the caller's dispatcher.
|
||||
|
||||
### Client.Shared-005
|
||||
|
||||
@@ -153,13 +153,13 @@
|
||||
| Severity | Low |
|
||||
| Category | Error handling & resilience / Documentation & comments |
|
||||
| Location | `OpcUaClientService.cs:302-322` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `AcknowledgeAlarmAsync` is typed `Task<StatusCode>` and its XML doc implies the returned code reports the ack outcome, but the method unconditionally `return StatusCodes.Good`. The actual failure path is `DefaultSessionAdapter.CallMethodAsync`, which throws `ServiceResultException` on a bad call result. A failed acknowledgment therefore never returns a bad `StatusCode` — it throws — and the `StatusCode` return value is dead. Callers writing `if (StatusCode.IsBad(result))` will never see a bad result and will not catch the exception.
|
||||
|
||||
**Recommendation:** Either change the return type to `Task` (and let exceptions signal failure), or catch `ServiceResultException` in `AcknowledgeAlarmAsync` and return its `StatusCode`. Update the XML doc to match whichever is chosen.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `AcknowledgeAlarmAsync` now wraps the `CallMethodAsync` invocation in a try/catch for `ServiceResultException`, logging the failure and returning `ex.StatusCode` so callers using `if (StatusCode.IsBad(result))` see the bad status. The `IOpcUaClientService.AcknowledgeAlarmAsync` XML doc now documents both the Good-on-success and bad-StatusCode-from-ServiceResultException contract. Regression tests `AcknowledgeAlarmAsync_OnSuccess_ReturnsGood` and `AcknowledgeAlarmAsync_OnServiceResultException_ReturnsBadStatusCode` cover both paths.
|
||||
|
||||
### Client.Shared-010
|
||||
|
||||
@@ -168,13 +168,13 @@
|
||||
| Severity | Low |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `ConnectionSettings.CertificateStorePath` is initialized to `ClientStoragePaths.GetPkiPath()` as a property initializer, so every `ConnectionSettings` instantiation runs `Environment.GetFolderPath` + `Path.Combine` and, on the first call per process, the legacy-folder migration with `Directory.Exists`/`Directory.Move` filesystem IO. `ConnectToEndpointAsync` constructs a fresh `ConnectionSettings` per endpoint on every connect and every failover attempt, so a failover loop across N endpoints does N redundant path resolutions. The `_migrationChecked` fast-path caps the cost, but doing filesystem work in a property initializer is a surprising side effect — constructing a settings object should not touch disk.
|
||||
|
||||
**Recommendation:** Make `CertificateStorePath` default to `string.Empty` and resolve `ClientStoragePaths.GetPkiPath()` lazily inside `DefaultApplicationConfigurationFactory.CreateAsync` only when the path is unset.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `ConnectionSettings.CertificateStorePath` now defaults to `string.Empty` (no filesystem touched on construction), and `DefaultApplicationConfigurationFactory.CreateAsync` resolves the canonical PKI path via `ClientStoragePaths.GetPkiPath()` only when the supplied path is null/whitespace. The settings-default unit test `Defaults_AreSet` was updated to assert the empty default with a comment pointing at this finding ID.
|
||||
|
||||
### Client.Shared-011
|
||||
|
||||
@@ -183,10 +183,10 @@
|
||||
| Severity | Low |
|
||||
| Category | Testing coverage |
|
||||
| Location | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The test suite is solid for the happy paths, connection lifecycle, and single-failover behavior. Gaps relative to the findings above: (a) no test exercises concurrent `SubscribeAsync`/failover to expose the `_activeDataSubscriptions` race (Client.Shared-005) or re-entrant keep-alive failures (Client.Shared-006); (b) the alarm fallback path in `OnAlarmEventNotification` (the `Task.Run` supplemental read) is not covered — no test drives an alarm event with missing Acked/Active fields and a non-null ConditionNodeId; (c) `WriteValueAsync` string coercion against an unwritten/`Bad`-status node (Client.Shared-008) is untested; (d) the production adapters (`DefaultSessionAdapter`, `DefaultEndpointDiscovery`) have no unit coverage — understandable since they wrap the SDK, but the `Results[0]` guard gap (Client.Shared-003) and the security-mode endpoint-selection logic are untested.
|
||||
|
||||
**Recommendation:** Add tests for re-entrant/concurrent failover, the alarm fallback path with truncated event fields, and string-write coercion against a typeless node. Extract `DefaultEndpointDiscovery` best-endpoint selection into a pure function so it can be unit tested.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — added the previously-missing unit coverage: (a) `OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent` drives the supplemental-read fallback path with null AckedState/ActiveState fields and a non-null SourceNode and asserts the Galaxy attribute reads populate the delivered event; (b) `WriteValueAsync` typeless-node coverage is exercised via the Client.Shared-008 fix that throws a descriptive `InvalidOperationException` on bad/null current reads; (c) `EndpointSelector` was extracted from `DefaultEndpointDiscovery` as a pure static and a new `EndpointSelectorTests` suite (7 tests) covers security-mode selection, the Basic256Sha256 preference, the hostname rewrite, and the null/empty argument guards; (d) acknowledge happy-path and bad-status paths are covered by the two new `AcknowledgeAlarmAsync_*` tests recorded under Client.Shared-009. Concurrent/re-entrant failover coverage already exists via the resolved Client.Shared-005/-006 tests in the suite.
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 2 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -130,7 +130,7 @@ dispose the previous logger if reconfiguration is genuinely intended.
|
||||
| Severity | Low |
|
||||
| Category | Error handling & resilience |
|
||||
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the
|
||||
value and status columns) without guarding against empty input. When `tagNames` and
|
||||
@@ -143,7 +143,13 @@ instead of producing an empty (header-only) table.
|
||||
separator, or an explicit "no rows" line), or use `DefaultIfEmpty(0).Max(...)` for the
|
||||
width computations.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `FormatTable` guards each `rows.Max(...)` width
|
||||
computation with a `rows.Length == 0 ? "<HEADER>".Length : Math.Max(...)` ternary, so
|
||||
an empty batch read returns the header + separator rows (no data rows) instead of
|
||||
throwing `InvalidOperationException`. The fix was landed in commit `1433a1c` alongside
|
||||
the -002 work, and the regression test
|
||||
`SnapshotFormatterTests.FormatTable_with_empty_input_returns_header_only` (added under
|
||||
-005) exercises it.
|
||||
|
||||
### Driver.Cli.Common-005
|
||||
|
||||
@@ -178,7 +184,7 @@ empty-input and `DriverCommandBase` level-selection tests.
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71`
|
||||
states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement
|
||||
@@ -194,4 +200,8 @@ library. The XML doc is stale relative to the shipped driver-CLI set.
|
||||
right-most and intentionally unpadded rather than claiming fixed width. Add FOCAS to the
|
||||
`DriverCommandBase` class-summary driver list.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — (1) `SnapshotFormatter.cs:71` comment reworded
|
||||
to state the source-time column is the right-most one and intentionally not
|
||||
measured/padded, calling out the null-timestamp `"-"` case explicitly. (2) FOCAS was
|
||||
added to the `DriverCommandBase` class-summary driver enumeration in commit `7ff356b`
|
||||
(landed alongside the -003 work).
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 5 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -92,7 +92,7 @@ dead-lettered. Until then, document explicitly that this writer never produces
|
||||
| Severity | Low |
|
||||
| Category | Concurrency & thread safety |
|
||||
| Location | `WonderwareHistorianClient.cs:207`, `WonderwareHistorianClient.cs:132-150` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `_totalQueries` is mutated with `Interlocked.Increment` in `Invoke`, but
|
||||
read inside `GetHealthSnapshot` under `_healthLock`, and every other counter
|
||||
@@ -106,7 +106,7 @@ and the counters are advisory, but the mixed model is a latent hazard.
|
||||
`_healthLock` block (a new `RecordQuery()` helper, or fold it into `RecordSuccess`/
|
||||
`RecordFailure`) so all six health fields share a single lock.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — replaced the mixed `Interlocked.Increment(ref _totalQueries)` + `_healthLock`-protected outcome counters with a single `RecordOutcome(bool success, string? error)` helper that increments `_totalQueries` and exactly one of `_totalSuccesses` / `_totalFailures` under one `_healthLock` acquisition; `GetHealthSnapshot` documents the invariant that `TotalSuccesses + TotalFailures == TotalQueries` at every observed snapshot. Added the regression test `GetHealthSnapshot_ConcurrentCallsAndReads_CountersAreInternallyConsistent` that runs a polling reader concurrently with 50 calls and asserts the invariant never breaks (fails red against the previous code, passes green now).
|
||||
|
||||
### Driver.Historian.Wonderware.Client-004
|
||||
|
||||
@@ -115,7 +115,7 @@ and the counters are advisory, but the mixed model is a latent hazard.
|
||||
| Severity | Low |
|
||||
| Category | Concurrency & thread safety |
|
||||
| Location | `WonderwareHistorianClient.cs:203-267` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** A sidecar-reported failure is recorded in two non-atomic steps under
|
||||
separate lock acquisitions: `Invoke` calls `RecordSuccess()` (line 211) and then the
|
||||
@@ -132,7 +132,7 @@ sidecar-level `Success` flag has been checked, or pass the reply success/error i
|
||||
single `RecordOutcome(bool transportOk, bool sidecarOk, string? error)` that updates all
|
||||
counters under one lock acquisition.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — eliminated the `RecordSuccess` → `ReclassifySuccessAsFailure` undo dance. `InvokeAsync` now takes a `Func<TReply, (bool ok, string? error)>` evaluator, evaluates it once when the transport reply lands, and calls `RecordOutcome(bool success, string? error)` exactly once per call under a single `_healthLock` acquisition. A sidecar-reported failure is now classified as a failure on its first and only counter update — no transient "success then undo" state is observable. The read-side `InvokeAndClassifyAsync` wrapper preserves the prior `InvalidOperationException` throw on sidecar failure. Added regression test `GetHealthSnapshot_SidecarFailure_NeverInflatesSuccessCounter` pinning `TotalSuccesses=0`/`TotalFailures=1` after a sidecar-error call.
|
||||
|
||||
### Driver.Historian.Wonderware.Client-005
|
||||
|
||||
@@ -167,7 +167,7 @@ the reader.
|
||||
| Severity | Low |
|
||||
| Category | Error handling & resilience |
|
||||
| Location | `Internal/PipeChannel.cs:96-107`, `WonderwareHistorianClientOptions.cs:11-12` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `PipeChannel.InvokeAsync` retries exactly once on transport failure and
|
||||
otherwise propagates. The options expose `ReconnectInitialBackoff` and
|
||||
@@ -182,7 +182,7 @@ or the options are dead config that misleads operators.
|
||||
path, or remove the two unused option fields and their XML docs and state plainly that
|
||||
retry/backoff is owned by the caller (the alarm drain worker / history router).
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — removed the dead `ReconnectInitialBackoff`/`ReconnectMaxBackoff` fields (and their `Effective*` accessors) from `WonderwareHistorianClientOptions` and added a `<remarks>` block stating that retry/backoff is owned by the caller (the alarm drain worker and the read-side history router) and that the channel itself performs exactly one in-place reconnect with no delay. Confirmed no consumer referenced the removed fields (only `code-reviews/` references remain). Solution-level build clean — Server picks up the new options shape without change.
|
||||
|
||||
### Driver.Historian.Wonderware.Client-007
|
||||
|
||||
@@ -218,7 +218,7 @@ deserializing.
|
||||
| Severity | Low |
|
||||
| Category | Security |
|
||||
| Location | `ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj:29-32` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The csproj suppresses two NuGet audit advisories
|
||||
(`GHSA-37gx-xxp4-5rgx`, `GHSA-w3x6-4m5h-cxqf`) for the `MessagePack` 2.5.187 dependency
|
||||
@@ -232,7 +232,7 @@ advisory title, why it does not apply to this module usage, and a revisit trigge
|
||||
follow-up to upgrade `MessagePack` once a patched version is available so the suppressions
|
||||
can be dropped.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — the suppression block in the csproj (already added under finding 007) records each advisory title (GHSA-37gx-xxp4-5rgx unsafe-dynamic-codegen, GHSA-w3x6-4m5h-cxqf typeless-resolver gadget chain), why neither applies to this module (default `StandardResolver` only, no `TypelessContractlessStandardResolver` / `DynamicUnion` / `DynamicGenericResolver`, plus the 64 KiB per-sample ValueBytes cap in `DeserializeSampleValue` from finding 007), and the revisit trigger ("Revisit once MessagePack 3.x is available and drop these suppressions at that time"). All three pieces the recommendation asked for are present; the single comment block above both `NuGetAuditSuppress` entries was confirmed to satisfy the audit-trail gap.
|
||||
|
||||
### Driver.Historian.Wonderware.Client-009
|
||||
|
||||
@@ -272,7 +272,7 @@ silent `[Key]` drift between the two duplicated contract sets is caught at build
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `WonderwareHistorianClient.cs:355-361`, `WonderwareHistorianClient.cs:132-150` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Two doc/behaviour mismatches.
|
||||
(1) The `Dispose()` XML comment asserts the underlying channel async cleanup is
|
||||
@@ -291,4 +291,4 @@ node concept. The collapse is reasonable but undocumented.
|
||||
short remark on `GetHealthSnapshot` explaining that the single-channel client maps both
|
||||
connection flags to one transport and does not track per-node health.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — (1) reworded the `Dispose()` XML comment to drop the "non-blocking" claim and instead state that the bridge is **deadlock-safe** because the cleanup never awaits a captured `SynchronizationContext` nor takes any lock the caller could hold, while acknowledging that `NamedPipeClientStream` teardown can block briefly on OS handle release. (2) Added a full `<summary>` + `<remarks>` block to `GetHealthSnapshot` explaining the single-channel collapse — both `ProcessConnectionOpen` and `EventConnectionOpen` report the same channel state, and `ActiveProcessNode`/`ActiveEventNode`/`Nodes` are intentionally null/empty because the client has no per-node telemetry. The remarks also pin the finding-003/004 invariant `TotalSuccesses + TotalFailures == TotalQueries`.
|
||||
|
||||
+30
-30
@@ -12,8 +12,8 @@ Each module's `findings.md` is the source of truth; this file is generated from
|
||||
|---|---|---|---|---|---|---|
|
||||
| [Admin](Admin/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
|
||||
| [Analyzers](Analyzers/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
|
||||
| [Client.CLI](Client.CLI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 8 | 10 |
|
||||
| [Client.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 11 |
|
||||
| [Client.CLI](Client.CLI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 10 |
|
||||
| [Client.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
|
||||
| [Client.UI](Client.UI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 11 |
|
||||
| [Configuration](Configuration/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
|
||||
| [Core](Core/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
|
||||
@@ -26,12 +26,12 @@ Each module's `findings.md` is the source of truth; this file is generated from
|
||||
| [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 |
|
||||
| [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
|
||||
| [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
|
||||
| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 6 |
|
||||
| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 6 |
|
||||
| [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
|
||||
| [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 5 |
|
||||
| [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 5 |
|
||||
| [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 14 |
|
||||
| [Driver.Historian.Wonderware](Driver.Historian.Wonderware/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
|
||||
| [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 10 |
|
||||
| [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 10 |
|
||||
| [Driver.Modbus](Driver.Modbus/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
|
||||
| [Driver.Modbus.Addressing](Driver.Modbus.Addressing/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 9 |
|
||||
| [Driver.Modbus.Cli](Driver.Modbus.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 |
|
||||
@@ -48,37 +48,12 @@ Findings with status `Open` or `In Progress`, ordered by severity.
|
||||
|
||||
| ID | Severity | Category | Location | Description |
|
||||
|---|---|---|---|---|
|
||||
| Client.CLI-002 | Low | Correctness & logic bugs | `Commands/SubscribeCommand.cs:129-137` | The summary computes `neverWentBad` as every target whose node-id key is absent from the `everBad` dictionary. A node that received no update at all is also absent from `everBad`, so it is counted in `neverWentBad` and printed under the he… |
|
||||
| Client.CLI-003 | Low | Correctness & logic bugs | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` | Numeric command options accept any value with no range validation. `--depth`, `--interval`, `--max-depth`, `--max`, and the history `--interval` can all be supplied as `0` or a negative number. A negative `--depth`/`--max-depth` silently d… |
|
||||
| Client.CLI-004 | Low | OtOpcUa conventions | `Commands/SubscribeCommand.cs:13-37` | `SubscribeCommand` is the only command in the module whose constructor and all `[CommandOption]` properties have no XML doc comments. Every other command (`ConnectCommand`, `ReadCommand`, `WriteCommand`, `BrowseCommand`, `AlarmsCommand`, `… |
|
||||
| Client.CLI-006 | Low | Error handling & resilience | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` | Operator input-format errors surface as raw .NET exceptions rather than clean CLI errors. An unparseable start/end value throws `FormatException` straight out of `DateTime.Parse`; an invalid node id throws `FormatException`/`ArgumentExcept… |
|
||||
| Client.CLI-007 | Low | Performance & resource management | `CommandBase.cs:112-123` | `ConfigureLogging` builds a new Serilog `LoggerConfiguration`, creates a logger, and assigns it to the static `Log.Logger` without disposing the previously assigned logger. For a single CLI invocation this leaks at most one logger and the… |
|
||||
| Client.CLI-008 | Low | Documentation & comments | `docs/Client.CLI.md:158-217` | `docs/Client.CLI.md` is stale relative to the code at this commit. (1) The `subscribe` command section documents only `-n` and `-i`, but the code (`SubscribeCommand`) also exposes `-r/--recursive`, `--max-depth`, `-q/--quiet`, `--duration`… |
|
||||
| Client.CLI-009 | Low | Code organization & conventions | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` | Both long-running commands attach an event handler (`service.DataChanged += ...`, `service.AlarmEvent += ...`) with a lambda and never detach it. Because the handler closes over `console`, the captured console and the closure remain refere… |
|
||||
| Client.CLI-010 | Low | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` | The new `SubscribeCommand` capabilities are largely untested. The four `SubscribeCommandTests` cover only single-node subscribe, unsubscribe-on-cancel, disconnect-in-finally, and the subscription message. There is no test for the `--recurs… |
|
||||
| Client.Shared-003 | Low | Correctness & logic bugs | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` | `WriteValueAsync` returns `response.Results[0]` and `CallMethodAsync` reads `result.Results[0]` without first checking the `Results` collection is non-empty. A malformed or service-level-faulted response (empty `Results` alongside a servic… |
|
||||
| Client.Shared-004 | Low | OtOpcUa conventions | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` | `CloseAsync`, `HistoryReadRawAsync`, and `HistoryReadAggregateAsync` are declared `async Task` but call the synchronous `Session.Close()` / `Session.HistoryRead(...)` APIs and contain no `await`. The history methods run a blocking synchron… |
|
||||
| Client.Shared-009 | Low | Error handling & resilience / Documentation & comments | `OpcUaClientService.cs:302-322` | `AcknowledgeAlarmAsync` is typed `Task<StatusCode>` and its XML doc implies the returned code reports the ack outcome, but the method unconditionally `return StatusCodes.Good`. The actual failure path is `DefaultSessionAdapter.CallMethodAs… |
|
||||
| Client.Shared-010 | Low | Performance & resource management | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` | `ConnectionSettings.CertificateStorePath` is initialized to `ClientStoragePaths.GetPkiPath()` as a property initializer, so every `ConnectionSettings` instantiation runs `Environment.GetFolderPath` + `Path.Combine` and, on the first call p… |
|
||||
| Client.Shared-011 | Low | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` | The test suite is solid for the happy paths, connection lifecycle, and single-failover behavior. Gaps relative to the findings above: (a) no test exercises concurrent `SubscribeAsync`/failover to expose the `_activeDataSubscriptions` race… |
|
||||
| Client.UI-003 | Low | OtOpcUa conventions | `ZB.MOM.WW.OtOpcUa.Client.UI.csproj:20-21`, `Program.cs:14-20` | The csproj references `Serilog` and `Serilog.Sinks.Console`, and `docs/Client.UI.md` lists Serilog as the logging technology, but no source file in the module uses Serilog. `Program.BuildAvaloniaApp()` uses Avalonia's `LogToTrace()` and th… |
|
||||
| Client.UI-004 | Low | OtOpcUa conventions | `Views/MainWindow.axaml.cs:125-138` | `OnBrowseCertPathClicked` uses `OpenFolderDialog`, which is obsolete in Avalonia 11.x (the version pinned in the csproj). The supported replacement is the `StorageProvider` API (`StorageProvider.OpenFolderPickerAsync`). Using the obsolete… |
|
||||
| Client.UI-006 | Low | Error handling & resilience | `ViewModels/MainWindowViewModel.cs:244-252`, `ViewModels/AlarmsViewModel.cs:88-112`, `ViewModels/SubscriptionsViewModel.cs:79-94` | Many catch blocks swallow exceptions silently with an empty body and only a comment (`// Redundancy info not available`, `// Subscribe failed`, `// Subscription failed; no item added`, and others). When a subscribe, alarm-subscribe, or red… |
|
||||
| Client.UI-009 | Low | Design-document adherence | `ViewModels/HistoryViewModel.cs:44-54` | `HistoryViewModel.AggregateTypes` exposes eight entries: `null` (Raw) plus Average, Minimum, Maximum, Count, Start, End, and `StandardDeviation`. `docs/Client.UI.md` ("Query Options" table) lists only "Raw (default), Average, Minimum, Maxi… |
|
||||
| Client.UI-010 | Low | Code organization & conventions | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` | `DateTimeRangePicker` declares `MinDateTimeProperty` / `MaxDateTimeProperty` styled properties with public CLR accessors, but neither is read anywhere in the control. `TryParseDateTime`, `OnStartLostFocus`, and `OnEndLostFocus` never clamp… |
|
||||
| Client.UI-011 | Low | Documentation & comments | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` | The certificate-store-path `TextBox` watermark reads `(default: AppData/LmxOpcUaClient/pki)`, referencing the legacy pre-task-#208 folder name. Per `CLAUDE.md` / `docs/Client.UI.md` the canonical path is now `{LocalAppData}/OtOpcUaClient/`… |
|
||||
| Driver.Cli.Common-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` | `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the value and status columns) without guarding against empty input. When `tagNames` and `snapshots` are both empty (equal length, so the mismatch check at line 56 passes),… |
|
||||
| Driver.Cli.Common-006 | Low | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` | Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71` states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement needed" — true only when every snapshot has a non-null `SourceTimestampUtc`. `For… |
|
||||
| Driver.FOCAS.Cli-001 | Low | Error handling & resilience | `Commands/WriteCommand.cs:58-68` | `WriteCommand.ParseValue` parses the numeric `--value` types (`Byte`/`Int16`/`Int32`/`Float32`/`Float64`) with `sbyte.Parse` / `short.Parse` / etc. These throw raw `FormatException` or `OverflowException` for malformed or out-of-range inpu… |
|
||||
| Driver.FOCAS.Cli-002 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` | The `subscribe` command attaches an `OnDataChange` handler that calls the synchronous `console.Output.WriteLine`. `OnDataChange` is raised from the driver's `PollGroupEngine` tick thread, while the command's main flow writes the "Subscribe… |
|
||||
| Driver.FOCAS.Cli-003 | Low | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) | The numeric command options `--cnc-port`, `--timeout-ms`, and `--interval-ms` are accepted without range validation. A zero or negative `--cnc-port` produces an invalid `focas://host:<n>` string; `--timeout-ms 0` yields a zero `TimeSpan` o… |
|
||||
| Driver.FOCAS.Cli-004 | Low | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` | Every command declares `await using var driver = new FocasDriver(...)` |
|
||||
| Driver.FOCAS.Cli-005 | Low | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and `BadCommunicationError` as the key diagnostic signals an operator reads off `probe` / `write` output ("A `BadCommunicationError` means ... `BadDeviceFailure` after a successful co… |
|
||||
| Driver.Historian.Wonderware.Client-003 | Low | Concurrency & thread safety | `WonderwareHistorianClient.cs:207`, `WonderwareHistorianClient.cs:132-150` | `_totalQueries` is mutated with `Interlocked.Increment` in `Invoke`, but read inside `GetHealthSnapshot` under `_healthLock`, and every other counter (`_totalSuccesses`, `_totalFailures`, `_consecutiveFailures`) is mutated only under `_hea… |
|
||||
| Driver.Historian.Wonderware.Client-004 | Low | Concurrency & thread safety | `WonderwareHistorianClient.cs:203-267` | A sidecar-reported failure is recorded in two non-atomic steps under separate lock acquisitions: `Invoke` calls `RecordSuccess()` (line 211) and then the caller calls `ThrowIfFailed` which calls `ReclassifySuccessAsFailure()` (line 256), d… |
|
||||
| Driver.Historian.Wonderware.Client-006 | Low | Error handling & resilience | `Internal/PipeChannel.cs:96-107`, `WonderwareHistorianClientOptions.cs:11-12` | `PipeChannel.InvokeAsync` retries exactly once on transport failure and otherwise propagates. The options expose `ReconnectInitialBackoff` and `ReconnectMaxBackoff` and `WonderwareHistorianClientOptions` documents them as exponential backo… |
|
||||
| Driver.Historian.Wonderware.Client-008 | Low | Security | `ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj:29-32` | The csproj suppresses two NuGet audit advisories (`GHSA-37gx-xxp4-5rgx`, `GHSA-w3x6-4m5h-cxqf`) for the `MessagePack` 2.5.187 dependency with no inline comment recording why the suppression is safe, who reviewed it, or when it should be re… |
|
||||
| Driver.Historian.Wonderware.Client-010 | Low | Documentation & comments | `WonderwareHistorianClient.cs:355-361`, `WonderwareHistorianClient.cs:132-150` | Two doc/behaviour mismatches. (1) The `Dispose()` XML comment asserts the underlying channel async cleanup is non-blocking so the `GetAwaiter()/GetResult()` bridge is safe. `PipeChannel.DisposeAsync` calls `ResetTransport()`, which invokes… |
|
||||
|
||||
## Closed findings
|
||||
|
||||
@@ -273,6 +248,19 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
|
||||
| Analyzers-004 | Low | Resolved | Performance & resource management | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:95-112` |
|
||||
| Analyzers-005 | Low | Resolved | Design-document adherence | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:33-43` |
|
||||
| Analyzers-007 | Low | Resolved | Documentation & comments | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:21-26` |
|
||||
| Client.CLI-002 | Low | Resolved | Correctness & logic bugs | `Commands/SubscribeCommand.cs:129-137` |
|
||||
| Client.CLI-003 | Low | Resolved | Correctness & logic bugs | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` |
|
||||
| Client.CLI-004 | Low | Resolved | OtOpcUa conventions | `Commands/SubscribeCommand.cs:13-37` |
|
||||
| Client.CLI-006 | Low | Resolved | Error handling & resilience | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` |
|
||||
| Client.CLI-007 | Low | Resolved | Performance & resource management | `CommandBase.cs:112-123` |
|
||||
| Client.CLI-008 | Low | Resolved | Documentation & comments | `docs/Client.CLI.md:158-217` |
|
||||
| Client.CLI-009 | Low | Resolved | Code organization & conventions | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` |
|
||||
| Client.CLI-010 | Low | Resolved | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` |
|
||||
| Client.Shared-003 | Low | Resolved | Correctness & logic bugs | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` |
|
||||
| Client.Shared-004 | Low | Resolved | OtOpcUa conventions | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` |
|
||||
| Client.Shared-009 | Low | Resolved | Error handling & resilience / Documentation & comments | `OpcUaClientService.cs:302-322` |
|
||||
| Client.Shared-010 | Low | Resolved | Performance & resource management | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` |
|
||||
| Client.Shared-011 | Low | Resolved | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` |
|
||||
| Configuration-004 | Low | Resolved | OtOpcUa conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs:8`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/OtOpcUaConfigDbContext.cs:417` |
|
||||
| Configuration-005 | Low | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs:50` |
|
||||
| Configuration-007 | Low | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs:44` |
|
||||
@@ -329,11 +317,18 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
|
||||
| Driver.AbLegacy.Cli-005 | Low | Resolved | Design-document adherence | `Commands/SubscribeCommand.cs:23-25`, `docs/Driver.AbLegacy.Cli.md:94-96` |
|
||||
| Driver.AbLegacy.Cli-006 | Low | Resolved | Code organization & conventions | `Commands/ProbeCommand.cs:20-22` |
|
||||
| Driver.AbLegacy.Cli-007 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` |
|
||||
| Driver.Cli.Common-004 | Low | Resolved | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` |
|
||||
| Driver.Cli.Common-006 | Low | Resolved | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` |
|
||||
| Driver.FOCAS-007 | Low | Resolved | Error handling & resilience | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` |
|
||||
| Driver.FOCAS-008 | Low | Resolved | Performance & resource management | `FocasDriver.cs:201`, `FocasDriver.cs:253` |
|
||||
| Driver.FOCAS-009 | Low | Resolved | Design-document adherence | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` |
|
||||
| Driver.FOCAS-010 | Low | Resolved | Code organization & conventions | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) |
|
||||
| Driver.FOCAS-011 | Low | Resolved | Code organization & conventions | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` |
|
||||
| Driver.FOCAS.Cli-001 | Low | Resolved | Error handling & resilience | `Commands/WriteCommand.cs:58-68` |
|
||||
| Driver.FOCAS.Cli-002 | Low | Resolved | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` |
|
||||
| Driver.FOCAS.Cli-003 | Low | Resolved | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) |
|
||||
| Driver.FOCAS.Cli-004 | Low | Resolved | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` |
|
||||
| Driver.FOCAS.Cli-005 | Low | Deferred | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) |
|
||||
| Driver.Galaxy-005 | Low | Resolved | OtOpcUa conventions | `Runtime/EventPump.cs:81-88` |
|
||||
| Driver.Galaxy-010 | Low | Resolved | Security | `GalaxyDriver.cs:311-341` |
|
||||
| Driver.Galaxy-012 | Low | Resolved | Performance & resource management | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` |
|
||||
@@ -345,6 +340,11 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
|
||||
| Driver.Historian.Wonderware-010 | Low | Resolved | Performance and resource management | `Backend/HistorianConfiguration.cs:32-36`, `Backend/HistorianDataSource.cs` (all read methods) |
|
||||
| Driver.Historian.Wonderware-011 | Low | Resolved | Design-document adherence | `Backend/HistorianDataSource.cs:9-12`, `Backend/IHistorianDataSource.cs:9-11`, `Backend/HistorianSample.cs:7-9`, `Backend/HistorianConfiguration.cs:7-9` |
|
||||
| Driver.Historian.Wonderware-012 | Low | Resolved | Testing coverage | `Backend/HistorianDataSource.cs`, `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Tests/` |
|
||||
| Driver.Historian.Wonderware.Client-003 | Low | Resolved | Concurrency & thread safety | `WonderwareHistorianClient.cs:207`, `WonderwareHistorianClient.cs:132-150` |
|
||||
| Driver.Historian.Wonderware.Client-004 | Low | Resolved | Concurrency & thread safety | `WonderwareHistorianClient.cs:203-267` |
|
||||
| Driver.Historian.Wonderware.Client-006 | Low | Resolved | Error handling & resilience | `Internal/PipeChannel.cs:96-107`, `WonderwareHistorianClientOptions.cs:11-12` |
|
||||
| Driver.Historian.Wonderware.Client-008 | Low | Resolved | Security | `ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj:29-32` |
|
||||
| Driver.Historian.Wonderware.Client-010 | Low | Resolved | Documentation & comments | `WonderwareHistorianClient.cs:355-361`, `WonderwareHistorianClient.cs:132-150` |
|
||||
| Driver.Modbus-003 | Low | Resolved | Concurrency & thread safety | `ModbusDriver.cs:59,188,241,259,266,726,745,759` |
|
||||
| Driver.Modbus-007 | Low | Resolved | Design-document adherence | `ModbusDriver.cs:1392`, `ModbusDriverOptions.cs:74-80` |
|
||||
| Driver.Modbus-008 | Low | Resolved | Documentation & comments | `ModbusDriver.cs:411-417,700-703,737-744` |
|
||||
|
||||
+40
-16
@@ -149,53 +149,77 @@ otopcua-cli browse -u opc.tcp://localhost:4840/OtOpcUa -U admin -P admin123 -r -
|
||||
|
||||
### subscribe
|
||||
|
||||
Monitors a node for value changes using OPC UA subscriptions. Prints each data change notification with timestamp, value, and status code. Runs until Ctrl+C, then unsubscribes and disconnects cleanly.
|
||||
Monitors a node (or every Variable in its subtree) for value changes using OPC UA subscriptions.
|
||||
Prints each data-change notification with timestamp, value, and status code, then prints a
|
||||
summary on exit. Exits on Ctrl+C, or automatically after `--duration` seconds.
|
||||
|
||||
```bash
|
||||
# Subscribe to a single node
|
||||
otopcua-cli subscribe -u opc.tcp://localhost:4840 -n "ns=2;s=MyNode" -i 500
|
||||
|
||||
# Browse a subtree and subscribe to every Variable, run for 60 seconds, write the summary to disk
|
||||
otopcua-cli subscribe -u opc.tcp://localhost:4840 -n "ns=3;s=ZB" -r --max-depth 4 \
|
||||
--duration 60 --quiet --summary-file C:\Temp\subscribe-summary.txt
|
||||
```
|
||||
|
||||
| Flag | Description |
|
||||
|------|-------------|
|
||||
| `-n` / `--node` | Node ID to monitor (required) |
|
||||
| `-i` / `--interval` | Sampling/publishing interval in milliseconds (default: 1000) |
|
||||
| `-n` / `--node` | Node ID to monitor (required). When `--recursive` is set, this is the browse root. |
|
||||
| `-i` / `--interval` | Sampling interval in milliseconds (default: 1000) |
|
||||
| `-r` / `--recursive` | Browse recursively from `--node` and subscribe to every Variable found |
|
||||
| `--max-depth` | Maximum recursion depth when `--recursive` is set (default: 10) |
|
||||
| `-q` / `--quiet` | Suppress per-update output; only print the final summary |
|
||||
| `--duration` | Auto-exit after N seconds and print the summary (0 = run until Ctrl+C, default: 0) |
|
||||
| `--summary-file` | Also write the summary to this file path on exit |
|
||||
|
||||
#### Summary buckets
|
||||
|
||||
The summary prints per-node counts across these buckets:
|
||||
|
||||
- **Ever went BAD during window** — node received at least one notification whose status was not Good.
|
||||
- **NEVER went bad (suspect)** — node received at least one notification and every one was Good.
|
||||
- **Last status GOOD / NOT-GOOD** — final observed status across nodes that received any update.
|
||||
- **No update received at all** — node was subscribed but no notification arrived during the window.
|
||||
|
||||
### historyread
|
||||
|
||||
Reads historical data from a node. Supports raw history reads and aggregate (processed) history reads.
|
||||
`--start` and `--end` are parsed with `CultureInfo.InvariantCulture` and treated as UTC; supply
|
||||
them in ISO 8601 UTC form (`YYYY-MM-DDTHH:MM:SSZ`) for unambiguous behaviour across hosts.
|
||||
|
||||
```bash
|
||||
# Raw history
|
||||
otopcua-cli historyread -u opc.tcp://localhost:4840/OtOpcUa \
|
||||
-n "ns=1;s=TestMachine_001.TestHistoryValue" \
|
||||
--start "2026-03-25" --end "2026-03-30"
|
||||
--start "2026-03-25T00:00:00Z" --end "2026-03-30T00:00:00Z"
|
||||
|
||||
# Aggregate: 1-hour average
|
||||
otopcua-cli historyread -u opc.tcp://localhost:4840/OtOpcUa \
|
||||
-n "ns=1;s=TestMachine_001.TestHistoryValue" \
|
||||
--start "2026-03-25" --end "2026-03-30" \
|
||||
--start "2026-03-25T00:00:00Z" --end "2026-03-30T00:00:00Z" \
|
||||
--aggregate Average --interval 3600000
|
||||
```
|
||||
|
||||
| Flag | Description |
|
||||
|------|-------------|
|
||||
| `-n` / `--node` | Node ID to read history for (required) |
|
||||
| `--start` | Start time, ISO 8601 or date string (default: 24 hours ago) |
|
||||
| `--end` | End time, ISO 8601 or date string (default: now) |
|
||||
| `--start` | Start time in ISO 8601 UTC format, e.g. `2026-01-15T08:00:00Z` (default: 24 hours ago) |
|
||||
| `--end` | End time in ISO 8601 UTC format, e.g. `2026-01-15T09:00:00Z` (default: now) |
|
||||
| `--max` | Maximum number of values (default: 1000) |
|
||||
| `--aggregate` | Aggregate function: Average, Minimum, Maximum, Count, Start, End |
|
||||
| `--aggregate` | Aggregate function: Average, Minimum, Maximum, Count, Start, End, StandardDeviation |
|
||||
| `--interval` | Processing interval in milliseconds for aggregates (default: 3600000) |
|
||||
|
||||
#### Aggregate mapping
|
||||
|
||||
| Name | OPC UA Node ID |
|
||||
|------|---------------|
|
||||
| `Average` | `AggregateFunction_Average` |
|
||||
| `Minimum` | `AggregateFunction_Minimum` |
|
||||
| `Maximum` | `AggregateFunction_Maximum` |
|
||||
| `Count` | `AggregateFunction_Count` |
|
||||
| `Start` | `AggregateFunction_Start` |
|
||||
| `End` | `AggregateFunction_End` |
|
||||
| Name | Aliases | OPC UA Node ID |
|
||||
|------|---------|---------------|
|
||||
| `Average` | `avg` | `AggregateFunction_Average` |
|
||||
| `Minimum` | `min` | `AggregateFunction_Minimum` |
|
||||
| `Maximum` | `max` | `AggregateFunction_Maximum` |
|
||||
| `Count` | | `AggregateFunction_Count` |
|
||||
| `Start` | `first` | `AggregateFunction_Start` |
|
||||
| `End` | `last` | `AggregateFunction_End` |
|
||||
| `StandardDeviation` | `stddev`, `stdev` | `AggregateFunction_StandardDeviationSample` |
|
||||
|
||||
### alarms
|
||||
|
||||
|
||||
@@ -109,8 +109,16 @@ public abstract class CommandBase : ICommand
|
||||
/// <summary>
|
||||
/// Configures Serilog based on the verbose flag.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Disposes the previously assigned <see cref="Log.Logger" /> via <see cref="Log.CloseAndFlush" />
|
||||
/// before installing the new one, so repeated CLI invocations (e.g. in the test suite) do not
|
||||
/// leak the prior logger's console sink.
|
||||
/// </remarks>
|
||||
protected void ConfigureLogging()
|
||||
{
|
||||
// Dispose any previously installed logger before swapping in a new one.
|
||||
Log.CloseAndFlush();
|
||||
|
||||
var config = new LoggerConfiguration();
|
||||
if (Verbose)
|
||||
config.MinimumLevel.Debug()
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
using System.Threading.Channels;
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Exceptions;
|
||||
using CliFx.Infrastructure;
|
||||
using Opc.Ua;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.Shared;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.Shared.Models;
|
||||
@@ -43,14 +45,25 @@ public class AlarmsCommand : CommandBase
|
||||
public override async ValueTask ExecuteAsync(IConsole console)
|
||||
{
|
||||
ConfigureLogging();
|
||||
if (Interval <= 0)
|
||||
throw new CommandException($"--interval must be greater than 0 (was {Interval}).");
|
||||
|
||||
NodeId? sourceNodeId;
|
||||
try
|
||||
{
|
||||
sourceNodeId = NodeIdParser.Parse(NodeId);
|
||||
}
|
||||
catch (Exception ex) when (ex is FormatException or ArgumentException)
|
||||
{
|
||||
throw new CommandException($"Invalid --node value: {ex.Message}");
|
||||
}
|
||||
|
||||
IOpcUaClientService? service = null;
|
||||
try
|
||||
{
|
||||
var ct = console.RegisterCancellationHandler();
|
||||
(service, _) = await CreateServiceAndConnectAsync(ct);
|
||||
|
||||
var sourceNodeId = NodeIdParser.Parse(NodeId);
|
||||
|
||||
// Channel serialises SDK notification-thread writes to the main async loop so
|
||||
// that concurrent alarm callbacks never interleave on the shared TextWriter.
|
||||
var outputChannel = Channel.CreateUnbounded<string>(
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Exceptions;
|
||||
using CliFx.Infrastructure;
|
||||
using Opc.Ua;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
|
||||
@@ -42,13 +43,25 @@ public class BrowseCommand : CommandBase
|
||||
public override async ValueTask ExecuteAsync(IConsole console)
|
||||
{
|
||||
ConfigureLogging();
|
||||
if (Depth <= 0)
|
||||
throw new CommandException($"--depth must be greater than 0 (was {Depth}).");
|
||||
|
||||
NodeId? startNode;
|
||||
try
|
||||
{
|
||||
startNode = NodeIdParser.Parse(NodeId);
|
||||
}
|
||||
catch (Exception ex) when (ex is FormatException or ArgumentException)
|
||||
{
|
||||
throw new CommandException($"Invalid --node value: {ex.Message}");
|
||||
}
|
||||
|
||||
IOpcUaClientService? service = null;
|
||||
try
|
||||
{
|
||||
var ct = console.RegisterCancellationHandler();
|
||||
(service, _) = await CreateServiceAndConnectAsync(ct);
|
||||
|
||||
var startNode = NodeIdParser.Parse(NodeId);
|
||||
var maxDepth = Recursive ? Depth : 1;
|
||||
|
||||
await BrowseNodeAsync(service, console, startNode, maxDepth, 0, ct);
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
using System.Globalization;
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Exceptions;
|
||||
using CliFx.Infrastructure;
|
||||
using Opc.Ua;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
|
||||
@@ -62,22 +63,65 @@ public class HistoryReadCommand : CommandBase
|
||||
public override async ValueTask ExecuteAsync(IConsole console)
|
||||
{
|
||||
ConfigureLogging();
|
||||
if (MaxValues <= 0)
|
||||
throw new CommandException($"--max must be greater than 0 (was {MaxValues}).");
|
||||
if (!string.IsNullOrEmpty(Aggregate) && IntervalMs <= 0)
|
||||
throw new CommandException($"--interval must be greater than 0 (was {IntervalMs}).");
|
||||
|
||||
NodeId nodeId;
|
||||
try
|
||||
{
|
||||
nodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
}
|
||||
catch (Exception ex) when (ex is FormatException or ArgumentException)
|
||||
{
|
||||
throw new CommandException($"Invalid --node value: {ex.Message}");
|
||||
}
|
||||
|
||||
DateTime start, end;
|
||||
try
|
||||
{
|
||||
start = string.IsNullOrEmpty(StartTime)
|
||||
? DateTime.UtcNow.AddHours(-24)
|
||||
: DateTime.Parse(StartTime, CultureInfo.InvariantCulture,
|
||||
DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
|
||||
}
|
||||
catch (FormatException ex)
|
||||
{
|
||||
throw new CommandException($"Invalid --start value '{StartTime}': {ex.Message}. Expected ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z.");
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
end = string.IsNullOrEmpty(EndTime)
|
||||
? DateTime.UtcNow
|
||||
: DateTime.Parse(EndTime, CultureInfo.InvariantCulture,
|
||||
DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
|
||||
}
|
||||
catch (FormatException ex)
|
||||
{
|
||||
throw new CommandException($"Invalid --end value '{EndTime}': {ex.Message}. Expected ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z.");
|
||||
}
|
||||
|
||||
AggregateType aggregateType = default;
|
||||
if (!string.IsNullOrEmpty(Aggregate))
|
||||
{
|
||||
try
|
||||
{
|
||||
aggregateType = ParseAggregateType(Aggregate);
|
||||
}
|
||||
catch (ArgumentException ex)
|
||||
{
|
||||
throw new CommandException($"Invalid --aggregate value: {ex.Message}");
|
||||
}
|
||||
}
|
||||
|
||||
IOpcUaClientService? service = null;
|
||||
try
|
||||
{
|
||||
var ct = console.RegisterCancellationHandler();
|
||||
(service, _) = await CreateServiceAndConnectAsync(ct);
|
||||
|
||||
var nodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
var start = string.IsNullOrEmpty(StartTime)
|
||||
? DateTime.UtcNow.AddHours(-24)
|
||||
: DateTime.Parse(StartTime, CultureInfo.InvariantCulture,
|
||||
DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
|
||||
var end = string.IsNullOrEmpty(EndTime)
|
||||
? DateTime.UtcNow
|
||||
: DateTime.Parse(EndTime, CultureInfo.InvariantCulture,
|
||||
DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
|
||||
|
||||
IReadOnlyList<DataValue> values;
|
||||
|
||||
if (string.IsNullOrEmpty(Aggregate))
|
||||
@@ -88,7 +132,6 @@ public class HistoryReadCommand : CommandBase
|
||||
}
|
||||
else
|
||||
{
|
||||
var aggregateType = ParseAggregateType(Aggregate);
|
||||
await console.Output.WriteLineAsync(
|
||||
$"History for {NodeId} ({Aggregate}, interval={IntervalMs}ms)");
|
||||
values = await service.HistoryReadAggregateAsync(
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Exceptions;
|
||||
using CliFx.Infrastructure;
|
||||
using Opc.Ua;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.Shared;
|
||||
|
||||
@@ -29,13 +31,23 @@ public class ReadCommand : CommandBase
|
||||
public override async ValueTask ExecuteAsync(IConsole console)
|
||||
{
|
||||
ConfigureLogging();
|
||||
|
||||
NodeId nodeId;
|
||||
try
|
||||
{
|
||||
nodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
}
|
||||
catch (Exception ex) when (ex is FormatException or ArgumentException)
|
||||
{
|
||||
throw new CommandException($"Invalid --node value: {ex.Message}");
|
||||
}
|
||||
|
||||
IOpcUaClientService? service = null;
|
||||
try
|
||||
{
|
||||
var ct = console.RegisterCancellationHandler();
|
||||
(service, _) = await CreateServiceAndConnectAsync(ct);
|
||||
|
||||
var nodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
var value = await service.ReadValueAsync(nodeId, ct);
|
||||
|
||||
await console.Output.WriteLineAsync($"Node: {NodeId}");
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
using System.Collections.Concurrent;
|
||||
using System.Threading.Channels;
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Exceptions;
|
||||
using CliFx.Infrastructure;
|
||||
using Opc.Ua;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
|
||||
@@ -12,42 +13,92 @@ namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
|
||||
[Command("subscribe", Description = "Monitor a node for value changes")]
|
||||
public class SubscribeCommand : CommandBase
|
||||
{
|
||||
/// <summary>
|
||||
/// Creates the subscribe command used to monitor a node (or a subtree of nodes) for data-change
|
||||
/// notifications.
|
||||
/// </summary>
|
||||
/// <param name="factory">The factory that creates the shared client service for the command run.</param>
|
||||
public SubscribeCommand(IOpcUaClientServiceFactory factory) : base(factory)
|
||||
{
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Gets the node ID to monitor. When <see cref="Recursive" /> is set, this node is the browse root
|
||||
/// and every <c>Variable</c> child it reaches is subscribed.
|
||||
/// </summary>
|
||||
[CommandOption("node", 'n', Description = "Node ID to monitor", IsRequired = true)]
|
||||
public string NodeId { get; init; } = default!;
|
||||
|
||||
/// <summary>
|
||||
/// Gets the sampling interval, in milliseconds, requested for every monitored item.
|
||||
/// </summary>
|
||||
[CommandOption("interval", 'i', Description = "Sampling interval in milliseconds")]
|
||||
public int Interval { get; init; } = 1000;
|
||||
|
||||
/// <summary>
|
||||
/// Gets a value indicating whether the command should browse from <see cref="NodeId" />
|
||||
/// and subscribe to every <c>Variable</c> in the subtree.
|
||||
/// </summary>
|
||||
[CommandOption("recursive", 'r', Description = "Browse recursively from --node and subscribe to every Variable found")]
|
||||
public bool Recursive { get; init; }
|
||||
|
||||
/// <summary>
|
||||
/// Gets the maximum recursion depth applied while collecting variables when <see cref="Recursive" /> is set.
|
||||
/// </summary>
|
||||
[CommandOption("max-depth", Description = "Maximum recursion depth when --recursive is set")]
|
||||
public int MaxDepth { get; init; } = 10;
|
||||
|
||||
/// <summary>
|
||||
/// Gets a value indicating whether per-update lines should be suppressed in favour of the final summary only.
|
||||
/// </summary>
|
||||
[CommandOption("quiet", 'q', Description = "Suppress per-update output; only print a final summary on Ctrl+C")]
|
||||
public bool Quiet { get; init; }
|
||||
|
||||
/// <summary>
|
||||
/// Gets the duration, in seconds, before the command auto-exits and prints its summary.
|
||||
/// A value of <c>0</c> means the command runs until Ctrl+C.
|
||||
/// </summary>
|
||||
[CommandOption("duration", Description = "Auto-exit after N seconds and print summary (0 = run until Ctrl+C)")]
|
||||
public int DurationSeconds { get; init; } = 0;
|
||||
|
||||
/// <summary>
|
||||
/// Gets the optional path that the command should write the final summary to on exit, in addition to stdout.
|
||||
/// </summary>
|
||||
[CommandOption("summary-file", Description = "Write summary to this file path on exit (in addition to stdout)")]
|
||||
public string? SummaryFile { get; init; }
|
||||
|
||||
/// <summary>
|
||||
/// Connects to the server, subscribes to <see cref="NodeId" /> (or its subtree when recursive),
|
||||
/// streams data-change notifications to the console, and prints a summary when the command exits.
|
||||
/// </summary>
|
||||
/// <param name="console">The CLI console used for output and cancellation handling.</param>
|
||||
public override async ValueTask ExecuteAsync(IConsole console)
|
||||
{
|
||||
ConfigureLogging();
|
||||
|
||||
if (Interval <= 0)
|
||||
throw new CommandException($"--interval must be greater than 0 (was {Interval}).");
|
||||
if (Recursive && MaxDepth <= 0)
|
||||
throw new CommandException($"--max-depth must be greater than 0 (was {MaxDepth}).");
|
||||
if (DurationSeconds < 0)
|
||||
throw new CommandException($"--duration must be 0 or a positive number (was {DurationSeconds}).");
|
||||
|
||||
NodeId rootNodeId;
|
||||
try
|
||||
{
|
||||
rootNodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
}
|
||||
catch (Exception ex) when (ex is FormatException or ArgumentException)
|
||||
{
|
||||
throw new CommandException($"Invalid --node value: {ex.Message}");
|
||||
}
|
||||
|
||||
IOpcUaClientService? service = null;
|
||||
try
|
||||
{
|
||||
var ct = console.RegisterCancellationHandler();
|
||||
(service, _) = await CreateServiceAndConnectAsync(ct);
|
||||
|
||||
var rootNodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
|
||||
var targets = new List<(NodeId nodeId, string displayPath)>();
|
||||
if (Recursive)
|
||||
{
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Exceptions;
|
||||
using CliFx.Infrastructure;
|
||||
using Opc.Ua;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
|
||||
@@ -37,14 +38,23 @@ public class WriteCommand : CommandBase
|
||||
public override async ValueTask ExecuteAsync(IConsole console)
|
||||
{
|
||||
ConfigureLogging();
|
||||
|
||||
NodeId nodeId;
|
||||
try
|
||||
{
|
||||
nodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
}
|
||||
catch (Exception ex) when (ex is FormatException or ArgumentException)
|
||||
{
|
||||
throw new CommandException($"Invalid --node value: {ex.Message}");
|
||||
}
|
||||
|
||||
IOpcUaClientService? service = null;
|
||||
try
|
||||
{
|
||||
var ct = console.RegisterCancellationHandler();
|
||||
(service, _) = await CreateServiceAndConnectAsync(ct);
|
||||
|
||||
var nodeId = NodeIdParser.ParseRequired(NodeId);
|
||||
|
||||
// Read current value to determine type for conversion
|
||||
var currentValue = await service.ReadValueAsync(nodeId, ct);
|
||||
var typedValue = ValueConverter.ConvertValue(Value, currentValue.Value);
|
||||
|
||||
+7
-1
@@ -14,7 +14,13 @@ internal sealed class DefaultApplicationConfigurationFactory : IApplicationConfi
|
||||
|
||||
public async Task<ApplicationConfiguration> CreateAsync(ConnectionSettings settings, CancellationToken ct)
|
||||
{
|
||||
var storePath = settings.CertificateStorePath;
|
||||
// Resolve the canonical PKI path lazily on first use so constructing a
|
||||
// ConnectionSettings instance — including the throwaway copies the client
|
||||
// service builds per failover attempt — does not touch the filesystem.
|
||||
// Callers that supply an explicit path override the default.
|
||||
var storePath = string.IsNullOrWhiteSpace(settings.CertificateStorePath)
|
||||
? ClientStoragePaths.GetPkiPath()
|
||||
: settings.CertificateStorePath;
|
||||
|
||||
var config = new ApplicationConfiguration
|
||||
{
|
||||
|
||||
@@ -24,9 +24,47 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery
|
||||
using var client = DiscoveryClient.Create(new Uri(endpointUrl));
|
||||
var allEndpoints = client.GetEndpoints(null);
|
||||
|
||||
return EndpointSelector.SelectBest(allEndpoints, endpointUrl, requestedMode);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Pure best-endpoint selection logic, extracted from <see cref="DefaultEndpointDiscovery"/>
|
||||
/// so it can be unit tested without standing up a real <see cref="DiscoveryClient"/>.
|
||||
/// </summary>
|
||||
internal static class EndpointSelector
|
||||
{
|
||||
private static readonly ILogger Logger = Log.ForContext(typeof(EndpointSelector));
|
||||
|
||||
/// <summary>
|
||||
/// Picks the best endpoint from the discovery response that matches the requested
|
||||
/// security mode, preferring <c>Basic256Sha256</c>, and rewrites the endpoint URL
|
||||
/// host to match the user-supplied URL when the discovery response advertises a
|
||||
/// different hostname.
|
||||
/// </summary>
|
||||
/// <param name="allEndpoints">Endpoints returned by the discovery query, in any order.</param>
|
||||
/// <param name="endpointUrl">The endpoint URL the operator supplied; supplies the hostname rewrite target.</param>
|
||||
/// <param name="requestedMode">The requested OPC UA message security mode.</param>
|
||||
/// <exception cref="InvalidOperationException">
|
||||
/// Thrown when no endpoint matches <paramref name="requestedMode"/>; the message lists the
|
||||
/// security mode + policy combinations the server returned so operators can diagnose mismatches.
|
||||
/// </exception>
|
||||
public static EndpointDescription SelectBest(
|
||||
IEnumerable<EndpointDescription> allEndpoints,
|
||||
string endpointUrl,
|
||||
MessageSecurityMode requestedMode)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(allEndpoints);
|
||||
if (string.IsNullOrWhiteSpace(endpointUrl))
|
||||
throw new ArgumentException("Endpoint URL must not be null or empty.", nameof(endpointUrl));
|
||||
|
||||
// Materialise once so we can both iterate and produce a diagnostic message
|
||||
// without re-running the underlying discovery enumeration.
|
||||
var endpoints = allEndpoints.ToList();
|
||||
|
||||
EndpointDescription? best = null;
|
||||
|
||||
foreach (var ep in allEndpoints)
|
||||
foreach (var ep in endpoints)
|
||||
{
|
||||
if (ep.SecurityMode != requestedMode)
|
||||
continue;
|
||||
@@ -37,18 +75,21 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery
|
||||
continue;
|
||||
}
|
||||
|
||||
// Prefer Basic256Sha256 when multiple endpoints match the requested mode.
|
||||
if (ep.SecurityPolicyUri == SecurityPolicies.Basic256Sha256)
|
||||
best = ep;
|
||||
}
|
||||
|
||||
if (best == null)
|
||||
{
|
||||
var available = string.Join(", ", allEndpoints.Select(e => $"{e.SecurityMode}/{e.SecurityPolicyUri}"));
|
||||
var available = string.Join(", ", endpoints.Select(e => $"{e.SecurityMode}/{e.SecurityPolicyUri}"));
|
||||
throw new InvalidOperationException(
|
||||
$"No endpoint found with security mode '{requestedMode}'. Available endpoints: {available}");
|
||||
}
|
||||
|
||||
// Rewrite endpoint URL hostname to match user-supplied hostname
|
||||
// Rewrite endpoint URL hostname to match user-supplied hostname. Necessary
|
||||
// when the OPC UA server returns a discovery URL using a different hostname
|
||||
// (e.g. internal DNS name) than the one the operator routed to.
|
||||
var serverUri = new Uri(best.EndpointUrl);
|
||||
var requestedUri = new Uri(endpointUrl);
|
||||
if (serverUri.Host != requestedUri.Host)
|
||||
|
||||
@@ -73,6 +73,17 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
|
||||
|
||||
var writeCollection = new WriteValueCollection { writeValue };
|
||||
var response = await _session.WriteAsync(null, writeCollection, ct);
|
||||
// A malformed or service-level-faulted response can come back with an empty
|
||||
// Results collection alongside a service fault. Surface the service result
|
||||
// (or BadUnexpectedError) rather than letting Results[0] throw
|
||||
// IndexOutOfRangeException upstream.
|
||||
if (response.Results == null || response.Results.Count == 0)
|
||||
{
|
||||
var serviceResult = response.ResponseHeader?.ServiceResult.Code ?? StatusCodes.BadUnexpectedError;
|
||||
throw new ServiceResultException(serviceResult,
|
||||
$"Write response contained no results for node {nodeId}.");
|
||||
}
|
||||
|
||||
return response.Results[0];
|
||||
}
|
||||
|
||||
@@ -143,15 +154,18 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
|
||||
if (continuationPoint != null)
|
||||
nodesToRead[0].ContinuationPoint = continuationPoint;
|
||||
|
||||
_session.HistoryRead(
|
||||
// Use the async overload so this method is genuinely asynchronous,
|
||||
// honors the cancellation token, and does not block the caller's thread
|
||||
// (which would block the UI dispatcher for client.ui consumers).
|
||||
var response = await _session.HistoryReadAsync(
|
||||
null,
|
||||
new ExtensionObject(details),
|
||||
TimestampsToReturn.Source,
|
||||
continuationPoint != null,
|
||||
nodesToRead,
|
||||
out var results,
|
||||
out _);
|
||||
ct).ConfigureAwait(false);
|
||||
|
||||
var results = response.Results;
|
||||
if (results == null || results.Count == 0)
|
||||
break;
|
||||
|
||||
@@ -186,15 +200,17 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
|
||||
new HistoryReadValueId { NodeId = nodeId }
|
||||
};
|
||||
|
||||
_session.HistoryRead(
|
||||
// Use the async overload so the method honors the cancellation token and
|
||||
// does not block on a synchronous service round-trip.
|
||||
var response = await _session.HistoryReadAsync(
|
||||
null,
|
||||
new ExtensionObject(details),
|
||||
TimestampsToReturn.Source,
|
||||
false,
|
||||
nodesToRead,
|
||||
out var results,
|
||||
out _);
|
||||
ct).ConfigureAwait(false);
|
||||
|
||||
var results = response.Results;
|
||||
var allValues = new List<DataValue>();
|
||||
|
||||
if (results != null && results.Count > 0)
|
||||
@@ -229,7 +245,9 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
|
||||
{
|
||||
try
|
||||
{
|
||||
if (_session.Connected) _session.Close();
|
||||
// Use the async overload so the caller does not block on the close
|
||||
// service round-trip and the cancellation token is honored.
|
||||
if (_session.Connected) await _session.CloseAsync(ct).ConfigureAwait(false);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
@@ -270,6 +288,15 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
|
||||
},
|
||||
ct);
|
||||
|
||||
// An empty Results collection paired with a service fault must surface as
|
||||
// a ServiceResultException, not an IndexOutOfRangeException from Results[0].
|
||||
if (result.Results == null || result.Results.Count == 0)
|
||||
{
|
||||
var serviceResult = result.ResponseHeader?.ServiceResult.Code ?? StatusCodes.BadUnexpectedError;
|
||||
throw new ServiceResultException(serviceResult,
|
||||
$"Call response contained no results for method {methodId} on {objectId}.");
|
||||
}
|
||||
|
||||
var callResult = result.Results[0];
|
||||
if (StatusCode.IsBad(callResult.StatusCode))
|
||||
throw new ServiceResultException(callResult.StatusCode);
|
||||
|
||||
@@ -96,6 +96,11 @@ public interface IOpcUaClientService : IDisposable
|
||||
/// <param name="eventId">The event identifier returned by the OPC UA server for the alarm event.</param>
|
||||
/// <param name="comment">The operator acknowledgment comment to write with the method call.</param>
|
||||
/// <param name="ct">The cancellation token that aborts the acknowledgment request.</param>
|
||||
/// <returns>
|
||||
/// <see cref="StatusCodes.Good"/> on success, or the server's bad <see cref="StatusCode"/>
|
||||
/// (from the underlying <see cref="ServiceResultException"/>) when the acknowledge call
|
||||
/// returns a bad result. Other transport-level failures still surface as exceptions.
|
||||
/// </returns>
|
||||
Task<StatusCode> AcknowledgeAlarmAsync(string conditionNodeId, byte[] eventId, string comment, CancellationToken ct = default);
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -41,11 +41,13 @@ public sealed class ConnectionSettings
|
||||
public bool AutoAcceptCertificates { get; set; } = true;
|
||||
|
||||
/// <summary>
|
||||
/// Path to the certificate store. Defaults to a subdirectory under LocalApplicationData
|
||||
/// resolved via <see cref="ClientStoragePaths"/> so the one-shot legacy-folder migration
|
||||
/// runs before the path is returned.
|
||||
/// Path to the certificate store. Defaults to <see cref="string.Empty"/>; the
|
||||
/// consuming application configuration factory resolves the canonical path via
|
||||
/// <see cref="ClientStoragePaths.GetPkiPath"/> lazily on first connect, so
|
||||
/// constructing settings — including the throwaway copies built per failover
|
||||
/// attempt — does not touch disk or run the legacy-folder migration probe.
|
||||
/// </summary>
|
||||
public string CertificateStorePath { get; set; } = ClientStoragePaths.GetPkiPath();
|
||||
public string CertificateStorePath { get; set; } = string.Empty;
|
||||
|
||||
/// <summary>
|
||||
/// Validates the settings and throws if any required values are missing or invalid.
|
||||
|
||||
@@ -353,11 +353,24 @@ public sealed class OpcUaClientService : IOpcUaClientService
|
||||
: NodeId.Parse(conditionNodeId + ".Condition");
|
||||
var acknowledgeMethodId = MethodIds.AcknowledgeableConditionType_Acknowledge;
|
||||
|
||||
await _session!.CallMethodAsync(
|
||||
conditionObjId,
|
||||
acknowledgeMethodId,
|
||||
[eventId, new LocalizedText(comment)],
|
||||
ct);
|
||||
// CallMethodAsync throws ServiceResultException on a bad call result;
|
||||
// surface that as the returned StatusCode so callers using the documented
|
||||
// `Task<StatusCode>` contract (e.g. `if (StatusCode.IsBad(result))`) see
|
||||
// the failure instead of an uncaught exception they did not anticipate.
|
||||
try
|
||||
{
|
||||
await _session!.CallMethodAsync(
|
||||
conditionObjId,
|
||||
acknowledgeMethodId,
|
||||
[eventId, new LocalizedText(comment)],
|
||||
ct);
|
||||
}
|
||||
catch (ServiceResultException ex)
|
||||
{
|
||||
Logger.Warning(ex, "Failed to acknowledge alarm on {ConditionId} (status {Status})",
|
||||
conditionNodeId, ex.StatusCode);
|
||||
return ex.StatusCode;
|
||||
}
|
||||
|
||||
Logger.Debug("Acknowledged alarm on {ConditionId}", conditionNodeId);
|
||||
return StatusCodes.Good;
|
||||
|
||||
@@ -68,7 +68,10 @@ public static class SnapshotFormatter
|
||||
int tagW = rows.Length == 0 ? "TAG".Length : Math.Max("TAG".Length, rows.Max(r => r.Tag.Length));
|
||||
int valW = rows.Length == 0 ? "VALUE".Length : Math.Max("VALUE".Length, rows.Max(r => r.Value.Length));
|
||||
int statW = rows.Length == 0 ? "STATUS".Length : Math.Max("STATUS".Length, rows.Max(r => r.Status.Length));
|
||||
// source-time column is fixed-width (ISO-8601 to ms) so no max-measurement needed.
|
||||
// source-time is the right-most column, so it is intentionally not measured or padded;
|
||||
// when a snapshot has a non-null SourceTimestampUtc the cell is 24 chars (ISO-8601 to ms),
|
||||
// and when the timestamp is null FormatTimestamp emits "-" — the resulting unalignment is
|
||||
// harmless because nothing is appended after this column.
|
||||
|
||||
var sb = new System.Text.StringBuilder();
|
||||
sb.Append("TAG".PadRight(tagW)).Append(" ")
|
||||
|
||||
@@ -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]));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
/// <summary>Parse <c>--value</c> per <see cref="FocasDataType"/>, invariant culture throughout.</summary>
|
||||
/// <remarks>
|
||||
/// Driver.FOCAS.Cli-001: numeric parses are wrapped so that malformed input
|
||||
/// (<see cref="FormatException"/> / <see cref="OverflowException"/>) surfaces
|
||||
/// as a clean <see cref="CliFx.Exceptions.CommandException"/> rather than a raw
|
||||
/// .NET stack trace — matching the friendly message the Bit path already produces.
|
||||
/// </remarks>
|
||||
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
|
||||
{
|
||||
|
||||
@@ -54,4 +54,26 @@ public abstract class FocasCommandBase : DriverCommandBase
|
||||
};
|
||||
|
||||
protected string DriverInstanceId => $"focas-cli-{CncHost}:{CncPort}";
|
||||
|
||||
/// <summary>
|
||||
/// Driver.FOCAS.Cli-003: validate numeric option ranges at the CLI boundary so a
|
||||
/// zero/negative <c>--cnc-port</c>, <c>--timeout-ms</c>, or <c>--interval-ms</c>
|
||||
/// surfaces as a clean <see cref="CliFx.Exceptions.CommandException"/> rather than
|
||||
/// either an opaque downstream exception (invalid <c>focas://host:<n></c> /
|
||||
/// zero <c>TimeSpan</c>) or a tight-spinning poll loop. The <c>--interval-ms</c>
|
||||
/// option is subscribe-only — pass <c>null</c> for probe/read/write so this
|
||||
/// helper can be a single shared validator.
|
||||
/// </summary>
|
||||
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}).");
|
||||
}
|
||||
}
|
||||
|
||||
+4
@@ -22,6 +22,10 @@
|
||||
<ProjectReference Include="..\..\ZB.MOM.WW.OtOpcUa.Driver.FOCAS\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.csproj"/>
|
||||
</ItemGroup>
|
||||
|
||||
<ItemGroup>
|
||||
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests"/>
|
||||
</ItemGroup>
|
||||
|
||||
<!-- CLI runs the managed WireFocasClient and talks to the CNC over TCP:8193
|
||||
directly — no Fwlib64.dll copy step needed. -->
|
||||
|
||||
|
||||
+93
-53
@@ -72,8 +72,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
MaxValues = (int)Math.Min(maxValuesPerNode, int.MaxValue),
|
||||
CorrelationId = Guid.NewGuid().ToString("N"),
|
||||
};
|
||||
var reply = await Invoke<ReadRawRequest, ReadRawReply>(MessageKind.ReadRawRequest, MessageKind.ReadRawReply, req, cancellationToken).ConfigureAwait(false);
|
||||
ThrowIfFailed(reply.Success, reply.Error, "ReadRaw");
|
||||
var reply = await InvokeAndClassifyAsync<ReadRawRequest, ReadRawReply>(
|
||||
MessageKind.ReadRawRequest, MessageKind.ReadRawReply, req,
|
||||
r => (r.Success, r.Error), "ReadRaw", cancellationToken).ConfigureAwait(false);
|
||||
return new HistoryReadResult(ToSnapshots(reply.Samples), ContinuationPoint: null);
|
||||
}
|
||||
|
||||
@@ -90,8 +91,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
AggregateColumn = MapAggregate(aggregate),
|
||||
CorrelationId = Guid.NewGuid().ToString("N"),
|
||||
};
|
||||
var reply = await Invoke<ReadProcessedRequest, ReadProcessedReply>(MessageKind.ReadProcessedRequest, MessageKind.ReadProcessedReply, req, cancellationToken).ConfigureAwait(false);
|
||||
ThrowIfFailed(reply.Success, reply.Error, "ReadProcessed");
|
||||
var reply = await InvokeAndClassifyAsync<ReadProcessedRequest, ReadProcessedReply>(
|
||||
MessageKind.ReadProcessedRequest, MessageKind.ReadProcessedReply, req,
|
||||
r => (r.Success, r.Error), "ReadProcessed", cancellationToken).ConfigureAwait(false);
|
||||
return new HistoryReadResult(ToAggregateSnapshots(reply.Buckets), ContinuationPoint: null);
|
||||
}
|
||||
|
||||
@@ -107,8 +109,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
TimestampsUtcTicks = ticks,
|
||||
CorrelationId = Guid.NewGuid().ToString("N"),
|
||||
};
|
||||
var reply = await Invoke<ReadAtTimeRequest, ReadAtTimeReply>(MessageKind.ReadAtTimeRequest, MessageKind.ReadAtTimeReply, req, cancellationToken).ConfigureAwait(false);
|
||||
ThrowIfFailed(reply.Success, reply.Error, "ReadAtTime");
|
||||
var reply = await InvokeAndClassifyAsync<ReadAtTimeRequest, ReadAtTimeReply>(
|
||||
MessageKind.ReadAtTimeRequest, MessageKind.ReadAtTimeReply, req,
|
||||
r => (r.Success, r.Error), "ReadAtTime", cancellationToken).ConfigureAwait(false);
|
||||
return new HistoryReadResult(AlignAtTimeSnapshots(timestampsUtc, reply.Samples), ContinuationPoint: null);
|
||||
}
|
||||
|
||||
@@ -167,11 +170,34 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
MaxEvents = maxEvents,
|
||||
CorrelationId = Guid.NewGuid().ToString("N"),
|
||||
};
|
||||
var reply = await Invoke<ReadEventsRequest, ReadEventsReply>(MessageKind.ReadEventsRequest, MessageKind.ReadEventsReply, req, cancellationToken).ConfigureAwait(false);
|
||||
ThrowIfFailed(reply.Success, reply.Error, "ReadEvents");
|
||||
var reply = await InvokeAndClassifyAsync<ReadEventsRequest, ReadEventsReply>(
|
||||
MessageKind.ReadEventsRequest, MessageKind.ReadEventsReply, req,
|
||||
r => (r.Success, r.Error), "ReadEvents", cancellationToken).ConfigureAwait(false);
|
||||
return new HistoricalEventsResult(ToHistoricalEvents(reply.Events), ContinuationPoint: null);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Returns a snapshot of operation counters and the single pipe channel's connection
|
||||
/// state.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// This client owns one duplex named-pipe channel to the sidecar — it has no notion of
|
||||
/// separate process / event connections and no per-node telemetry. The single channel's
|
||||
/// connected state is reported for both <see cref="HistorianHealthSnapshot.ProcessConnectionOpen"/>
|
||||
/// and <see cref="HistorianHealthSnapshot.EventConnectionOpen"/>, and
|
||||
/// <see cref="HistorianHealthSnapshot.ActiveProcessNode"/> /
|
||||
/// <see cref="HistorianHealthSnapshot.ActiveEventNode"/> /
|
||||
/// <see cref="HistorianHealthSnapshot.Nodes"/> are intentionally null/empty. Consumers
|
||||
/// that need to distinguish two connections should read another driver. (Finding 010.)
|
||||
/// <para>
|
||||
/// All six counter fields (TotalQueries, TotalSuccesses, TotalFailures,
|
||||
/// ConsecutiveFailures, LastSuccessTime, LastFailureTime, LastError) are mutated
|
||||
/// exclusively under <c>_healthLock</c>, so the snapshot is internally consistent —
|
||||
/// in particular <c>TotalSuccesses + TotalFailures == TotalQueries</c> at every
|
||||
/// observed snapshot (a call that has started but not yet completed has not
|
||||
/// incremented any counter). (Finding 003 / 004.)
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public HistorianHealthSnapshot GetHealthSnapshot()
|
||||
{
|
||||
lock (_healthLock)
|
||||
@@ -233,8 +259,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
|
||||
try
|
||||
{
|
||||
var reply = await Invoke<WriteAlarmEventsRequest, WriteAlarmEventsReply>(
|
||||
MessageKind.WriteAlarmEventsRequest, MessageKind.WriteAlarmEventsReply, req, cancellationToken).ConfigureAwait(false);
|
||||
var reply = await InvokeAsync<WriteAlarmEventsRequest, WriteAlarmEventsReply>(
|
||||
MessageKind.WriteAlarmEventsRequest, MessageKind.WriteAlarmEventsReply, req,
|
||||
r => (r.Success, r.Error), cancellationToken).ConfigureAwait(false);
|
||||
|
||||
// Whole-call failure → transient retry for every event in the batch.
|
||||
if (!reply.Success)
|
||||
@@ -279,69 +306,79 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
|
||||
// ===== Helpers =====
|
||||
|
||||
private async Task<TReply> Invoke<TRequest, TReply>(
|
||||
MessageKind requestKind, MessageKind expectedReplyKind, TRequest request, CancellationToken ct)
|
||||
/// <summary>
|
||||
/// Sends one request through the channel and records the outcome (transport success or
|
||||
/// transport failure) under a single <c>_healthLock</c> acquisition that also bumps
|
||||
/// <c>_totalQueries</c>. Sidecar-level success / failure is NOT classified here — the
|
||||
/// caller passes that through <see cref="InvokeAndClassifyAsync"/> instead. (Finding
|
||||
/// 003 / 004: all six counter fields share one synchronization mechanism so a snapshot
|
||||
/// can never observe a torn state.)
|
||||
/// </summary>
|
||||
private async Task<TReply> InvokeAsync<TRequest, TReply>(
|
||||
MessageKind requestKind, MessageKind expectedReplyKind, TRequest request,
|
||||
Func<TReply, (bool ok, string? error)> evaluate, CancellationToken ct)
|
||||
where TReply : class
|
||||
{
|
||||
Interlocked.Increment(ref _totalQueries);
|
||||
try
|
||||
{
|
||||
var reply = await _channel.InvokeAsync<TRequest, TReply>(requestKind, expectedReplyKind, request, ct).ConfigureAwait(false);
|
||||
RecordSuccess();
|
||||
// Classify transport+sidecar in one lock so TotalQueries/TotalSuccesses/
|
||||
// TotalFailures move together and no intermediate "success-then-undo" state is
|
||||
// visible to a concurrent GetHealthSnapshot.
|
||||
var (ok, error) = evaluate(reply);
|
||||
RecordOutcome(ok, error);
|
||||
return reply;
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
RecordFailure(ex.Message);
|
||||
RecordOutcome(success: false, ex.Message);
|
||||
throw;
|
||||
}
|
||||
}
|
||||
|
||||
private void RecordSuccess()
|
||||
/// <summary>
|
||||
/// Convenience wrapper around <see cref="InvokeAsync"/> that throws
|
||||
/// <see cref="InvalidOperationException"/> on a sidecar-reported failure. Used by the
|
||||
/// <see cref="IHistorianDataSource"/> read methods.
|
||||
/// </summary>
|
||||
private async Task<TReply> InvokeAndClassifyAsync<TRequest, TReply>(
|
||||
MessageKind requestKind, MessageKind expectedReplyKind, TRequest request,
|
||||
Func<TReply, (bool ok, string? error)> evaluate, string op, CancellationToken ct)
|
||||
where TReply : class
|
||||
{
|
||||
lock (_healthLock)
|
||||
var reply = await InvokeAsync<TRequest, TReply>(requestKind, expectedReplyKind, request, evaluate, ct).ConfigureAwait(false);
|
||||
var (ok, error) = evaluate(reply);
|
||||
if (!ok)
|
||||
{
|
||||
_totalSuccesses++;
|
||||
_consecutiveFailures = 0;
|
||||
_lastSuccessUtc = DateTime.UtcNow;
|
||||
}
|
||||
}
|
||||
|
||||
private void RecordFailure(string message)
|
||||
{
|
||||
lock (_healthLock)
|
||||
{
|
||||
_totalFailures++;
|
||||
_consecutiveFailures++;
|
||||
_lastFailureUtc = DateTime.UtcNow;
|
||||
_lastError = message;
|
||||
}
|
||||
}
|
||||
|
||||
private void ThrowIfFailed(bool success, string? error, string op)
|
||||
{
|
||||
if (!success)
|
||||
{
|
||||
// Sidecar-reported failure counts as an operation failure even though the
|
||||
// transport delivered a reply. The Invoke wrapper already recorded transport
|
||||
// success — undo that and record the failure so the health snapshot reflects
|
||||
// operation-level success rates rather than just connectivity.
|
||||
ReclassifySuccessAsFailure(error);
|
||||
throw new InvalidOperationException(
|
||||
$"Sidecar {op} failed: {error ?? "<no message>"}.");
|
||||
}
|
||||
return reply;
|
||||
}
|
||||
|
||||
private void ReclassifySuccessAsFailure(string? message)
|
||||
/// <summary>
|
||||
/// Records the outcome of a single call — increments <c>_totalQueries</c> and exactly
|
||||
/// one of <c>_totalSuccesses</c> / <c>_totalFailures</c> under a single
|
||||
/// <c>_healthLock</c> acquisition. (Findings 003 + 004.)
|
||||
/// </summary>
|
||||
private void RecordOutcome(bool success, string? error)
|
||||
{
|
||||
lock (_healthLock)
|
||||
{
|
||||
// Transport-level RecordSuccess happened a moment ago; reverse it.
|
||||
_totalSuccesses--;
|
||||
_totalFailures++;
|
||||
_consecutiveFailures++;
|
||||
_lastFailureUtc = DateTime.UtcNow;
|
||||
_lastError = message;
|
||||
_totalQueries++;
|
||||
if (success)
|
||||
{
|
||||
_totalSuccesses++;
|
||||
_consecutiveFailures = 0;
|
||||
_lastSuccessUtc = DateTime.UtcNow;
|
||||
}
|
||||
else
|
||||
{
|
||||
_totalFailures++;
|
||||
_consecutiveFailures++;
|
||||
_lastFailureUtc = DateTime.UtcNow;
|
||||
_lastError = error;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -452,9 +489,12 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
|
||||
/// <summary>
|
||||
/// Synchronous dispose required by <see cref="IDisposable"/> on
|
||||
/// <see cref="IHistorianDataSource"/>. The underlying channel's async cleanup is
|
||||
/// non-blocking (just resets transport state + disposes streams), so the
|
||||
/// GetAwaiter()/GetResult() bridge is safe.
|
||||
/// <see cref="IHistorianDataSource"/>. The underlying channel's async cleanup runs
|
||||
/// <see cref="System.IO.Pipes.NamedPipeClientStream"/> teardown, which can block briefly
|
||||
/// on OS handle release — strictly speaking it is not non-blocking — but the
|
||||
/// <c>GetAwaiter()/GetResult()</c> bridge is deadlock-safe because the cleanup never
|
||||
/// awaits a captured <see cref="System.Threading.SynchronizationContext"/> nor takes any
|
||||
/// lock that the caller could hold. (Finding 010.)
|
||||
/// </summary>
|
||||
public void Dispose() => _channel.DisposeAsync().AsTask().GetAwaiter().GetResult();
|
||||
}
|
||||
|
||||
+11
-7
@@ -3,24 +3,28 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client;
|
||||
/// <summary>
|
||||
/// Connection options for <see cref="WonderwareHistorianClient"/>.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>Retry / backoff ownership (finding 006):</b> this module performs exactly one
|
||||
/// in-place transport reconnect inside <c>PipeChannel.InvokeAsync</c> with no delay,
|
||||
/// and does NOT implement exponential reconnect backoff. Broader retry/backoff is the
|
||||
/// caller's responsibility — the alarm drain worker
|
||||
/// (<c>Core.AlarmHistorian.SqliteStoreAndForwardSink</c>) and the read-side
|
||||
/// history router are expected to layer their own backoff on top.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="PipeName">Named-pipe name the sidecar listens on (matches the sidecar's <c>OTOPCUA_HISTORIAN_PIPE</c>).</param>
|
||||
/// <param name="SharedSecret">Per-process shared secret the sidecar will verify in the Hello frame.</param>
|
||||
/// <param name="PeerName">Diagnostic peer identifier sent in Hello — typically the OtOpcUa instance id.</param>
|
||||
/// <param name="ConnectTimeout">Cap on the named-pipe connect + Hello round trip on each (re)connect.</param>
|
||||
/// <param name="CallTimeout">Cap on a single read/write call once connected.</param>
|
||||
/// <param name="ReconnectInitialBackoff">Backoff between the first failed reconnect attempts.</param>
|
||||
/// <param name="ReconnectMaxBackoff">Upper bound on the exponential backoff between reconnects.</param>
|
||||
public sealed record WonderwareHistorianClientOptions(
|
||||
string PipeName,
|
||||
string SharedSecret,
|
||||
string PeerName = "OtOpcUa",
|
||||
TimeSpan? ConnectTimeout = null,
|
||||
TimeSpan? CallTimeout = null,
|
||||
TimeSpan? ReconnectInitialBackoff = null,
|
||||
TimeSpan? ReconnectMaxBackoff = null)
|
||||
TimeSpan? CallTimeout = null)
|
||||
{
|
||||
public TimeSpan EffectiveConnectTimeout => ConnectTimeout ?? TimeSpan.FromSeconds(10);
|
||||
public TimeSpan EffectiveCallTimeout => CallTimeout ?? TimeSpan.FromSeconds(30);
|
||||
public TimeSpan EffectiveReconnectInitialBackoff => ReconnectInitialBackoff ?? TimeSpan.FromMilliseconds(500);
|
||||
public TimeSpan EffectiveReconnectMaxBackoff => ReconnectMaxBackoff ?? TimeSpan.FromSeconds(30);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,165 @@
|
||||
using CliFx.Exceptions;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for Client.CLI-003: numeric command options must validate their ranges
|
||||
/// and report a clean operator-facing error instead of silently passing bad values to the SDK.
|
||||
/// </summary>
|
||||
public class CommandRangeValidationTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task BrowseCommand_NegativeDepth_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new BrowseCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
Depth = -1
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--depth");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task BrowseCommand_ZeroDepth_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new BrowseCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
Depth = 0
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--depth");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeCommand_ZeroInterval_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
Interval = 0
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--interval");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeCommand_NegativeInterval_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
Interval = -50
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeCommand_RecursiveZeroMaxDepth_ThrowsCommandException()
|
||||
{
|
||||
// --max-depth only matters when --recursive is set; validation is scoped to that combination.
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
Recursive = true,
|
||||
MaxDepth = 0
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--max-depth");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeCommand_NegativeDuration_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
DurationSeconds = -5
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AlarmsCommand_ZeroInterval_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new AlarmsCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
Interval = 0
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--interval");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HistoryReadCommand_NegativeMax_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new HistoryReadCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
MaxValues = -10
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--max");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HistoryReadCommand_ZeroInterval_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new HistoryReadCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
Aggregate = "Average",
|
||||
IntervalMs = 0
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--interval");
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,59 @@
|
||||
using Opc.Ua;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.Shared.Models;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for Client.CLI-009: long-running commands must detach their event handlers
|
||||
/// from the service before the command finishes, so notifications fired during teardown don't
|
||||
/// land in a disposed console.
|
||||
/// </summary>
|
||||
public class EventHandlerLifecycleTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task SubscribeCommand_AfterExit_DataChangedEventHasNoSubscribers()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=Node",
|
||||
DurationSeconds = 1
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await command.ExecuteAsync(console);
|
||||
|
||||
// The fake's event should have no subscribers after the command completes — every
|
||||
// handler attached by the command must have been detached during teardown.
|
||||
fakeService.HasDataChangedSubscribers.ShouldBeFalse(
|
||||
"SubscribeCommand must detach its DataChanged handler before returning.");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AlarmsCommand_AfterExit_AlarmEventHasNoSubscribers()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new AlarmsCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
|
||||
var task = Task.Run(async () => await command.ExecuteAsync(console));
|
||||
|
||||
await Task.Delay(150);
|
||||
console.RequestCancellation();
|
||||
await task;
|
||||
|
||||
fakeService.HasAlarmEventSubscribers.ShouldBeFalse(
|
||||
"AlarmsCommand must detach its AlarmEvent handler before returning.");
|
||||
}
|
||||
}
|
||||
@@ -55,6 +55,14 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
|
||||
new("ns=2;s=Node2", "Node2", "Variable", false)
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// Optional per-parent-node browse results. When a key matches the requested parent's
|
||||
/// <see cref="NodeId.ToString" />, this dictionary takes precedence over <see cref="BrowseResults" />.
|
||||
/// Tests exercising recursive walks (Client.CLI-010) use it to model a real subtree whose
|
||||
/// child node ids do not collide on descent.
|
||||
/// </summary>
|
||||
public Dictionary<string, IReadOnlyList<BrowseResult>> BrowseResultsByParent { get; } = new();
|
||||
|
||||
public IReadOnlyList<DataValue> HistoryReadResult { get; set; } = new List<DataValue>
|
||||
{
|
||||
new(new Variant(10.0), StatusCodes.Good, DateTime.UtcNow.AddHours(-1), DateTime.UtcNow),
|
||||
@@ -68,6 +76,7 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
|
||||
public Exception? ReadException { get; set; }
|
||||
public Exception? WriteException { get; set; }
|
||||
public Exception? ConditionRefreshException { get; set; }
|
||||
public Exception? SubscribeException { get; set; }
|
||||
|
||||
/// <inheritdoc />
|
||||
public bool IsConnected => ConnectCalled && !DisconnectCalled;
|
||||
@@ -84,6 +93,12 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
|
||||
/// <inheritdoc />
|
||||
public event EventHandler<ConnectionStateChangedEventArgs>? ConnectionStateChanged;
|
||||
|
||||
/// <summary>True when at least one handler is attached to <see cref="DataChanged" />.</summary>
|
||||
public bool HasDataChangedSubscribers => DataChanged != null;
|
||||
|
||||
/// <summary>True when at least one handler is attached to <see cref="AlarmEvent" />.</summary>
|
||||
public bool HasAlarmEventSubscribers => AlarmEvent != null;
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task<ConnectionInfo> ConnectAsync(ConnectionSettings settings, CancellationToken ct = default)
|
||||
{
|
||||
@@ -120,6 +135,9 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
|
||||
public Task<IReadOnlyList<BrowseResult>> BrowseAsync(NodeId? parentNodeId = null, CancellationToken ct = default)
|
||||
{
|
||||
BrowseNodeIds.Add(parentNodeId);
|
||||
var key = parentNodeId?.ToString();
|
||||
if (key != null && BrowseResultsByParent.TryGetValue(key, out var keyed))
|
||||
return Task.FromResult(keyed);
|
||||
return Task.FromResult(BrowseResults);
|
||||
}
|
||||
|
||||
@@ -127,6 +145,7 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
|
||||
public Task SubscribeAsync(NodeId nodeId, int intervalMs = 1000, CancellationToken ct = default)
|
||||
{
|
||||
SubscribeCalls.Add((nodeId, intervalMs));
|
||||
if (SubscribeException != null) throw SubscribeException;
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
|
||||
|
||||
@@ -105,7 +105,7 @@ public class HistoryReadCommandTests
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Execute_InvalidAggregate_ThrowsArgumentException()
|
||||
public async Task Execute_InvalidAggregate_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
@@ -117,7 +117,10 @@ public class HistoryReadCommandTests
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await Should.ThrowAsync<ArgumentException>(async () => await command.ExecuteAsync(console));
|
||||
// Operator-input errors now surface as CliFx CommandException (was ArgumentException)
|
||||
// per Client.CLI-006 so malformed input prints a clean CLI error instead of a stack trace.
|
||||
await Should.ThrowAsync<CliFx.Exceptions.CommandException>(
|
||||
async () => await command.ExecuteAsync(console));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
using CliFx.Exceptions;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for Client.CLI-006: predictable operator-input errors must surface as
|
||||
/// CliFx CommandException with a clean message, not raw FormatException/ArgumentException
|
||||
/// with a stack trace.
|
||||
/// </summary>
|
||||
public class InputValidationErrorsTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task HistoryReadCommand_InvalidStartTime_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new HistoryReadCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
StartTime = "not-a-date"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--start");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HistoryReadCommand_InvalidEndTime_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new HistoryReadCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
EndTime = "garbage"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("--end");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HistoryReadCommand_InvalidAggregate_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new HistoryReadCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=N",
|
||||
Aggregate = "NotARealAggregate"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("aggregate", Case.Insensitive);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ReadCommand_InvalidNodeId_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new ReadCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "not-a-node-id"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("node", Case.Insensitive);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeCommand_InvalidNodeId_ThrowsCommandException()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "not-a-node-id"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
|
||||
ex.Message.ShouldContain("node", Case.Insensitive);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,55 @@
|
||||
using Serilog;
|
||||
using Serilog.Core;
|
||||
using Serilog.Events;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression test for Client.CLI-007: ConfigureLogging must dispose the previously assigned
|
||||
/// Log.Logger before replacing it, so repeated CLI invocations do not leak sinks.
|
||||
/// </summary>
|
||||
public class LoggerLifecycleTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task ConfigureLogging_DisposesPreviousLogger_BeforeReassigning()
|
||||
{
|
||||
// Install a tracker logger before the command runs.
|
||||
var trackerSink = new DisposeTrackingSink();
|
||||
var trackerLogger = new LoggerConfiguration()
|
||||
.WriteTo.Sink(trackerSink)
|
||||
.CreateLogger();
|
||||
Log.Logger = trackerLogger;
|
||||
|
||||
try
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new ConnectCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840"
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await command.ExecuteAsync(console);
|
||||
|
||||
// The command's ConfigureLogging should have disposed the tracker logger we installed.
|
||||
trackerSink.Disposed.ShouldBeTrue(
|
||||
"Previous Log.Logger should be disposed via Log.CloseAndFlush() before ConfigureLogging assigns a new one.");
|
||||
}
|
||||
finally
|
||||
{
|
||||
Log.CloseAndFlush();
|
||||
}
|
||||
}
|
||||
|
||||
private sealed class DisposeTrackingSink : ILogEventSink, IDisposable
|
||||
{
|
||||
public bool Disposed { get; private set; }
|
||||
public void Emit(LogEvent logEvent) { }
|
||||
public void Dispose() => Disposed = true;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,258 @@
|
||||
using Opc.Ua;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
|
||||
using BrowseResult = ZB.MOM.WW.OtOpcUa.Client.Shared.Models.BrowseResult;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for SubscribeCommand summary bucketing, --duration, --quiet, --summary-file,
|
||||
/// and recursive collection. Covers Client.CLI-002, -009, and -010.
|
||||
/// </summary>
|
||||
public class SubscribeCommandSummaryTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task Summary_NodeWithNoUpdate_IsCountedAsNeverNotAsNeverWentBad()
|
||||
{
|
||||
// Client.CLI-002: A node that received no updates at all is "never received an update",
|
||||
// NOT a "suspect that never went bad".
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=SilentNode",
|
||||
DurationSeconds = 1
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await command.ExecuteAsync(console);
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
output.ShouldContain("No update received at all: 1");
|
||||
output.ShouldContain("NEVER went bad (suspect): 0");
|
||||
// The "suspect" detail header should not appear when the suspect list is empty.
|
||||
output.ShouldNotContain("--- Nodes that NEVER received a bad-quality update (suspect) ---");
|
||||
// The "never received update" detail header should appear.
|
||||
output.ShouldContain("--- Nodes that never received an update at all ---");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Summary_NodeReceivedOnlyGoodValues_IsCountedAsNeverWentBad()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=GoodNode",
|
||||
// Use --duration so the command auto-exits.
|
||||
DurationSeconds = 2
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var runTask = Task.Run(async () => await command.ExecuteAsync(console));
|
||||
|
||||
// Wait until the subscription is registered by the fake.
|
||||
await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1);
|
||||
|
||||
fakeService.RaiseDataChanged(
|
||||
"ns=2;s=GoodNode",
|
||||
new DataValue(new Variant(42), StatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow));
|
||||
|
||||
await runTask;
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
output.ShouldContain("NEVER went bad (suspect): 1");
|
||||
output.ShouldContain("Last status GOOD: 1");
|
||||
output.ShouldContain("No update received at all: 0");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Summary_NodeReceivedBadValue_IsCountedAsEverWentBad()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=BadNode",
|
||||
DurationSeconds = 2
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var runTask = Task.Run(async () => await command.ExecuteAsync(console));
|
||||
|
||||
await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1);
|
||||
|
||||
fakeService.RaiseDataChanged(
|
||||
"ns=2;s=BadNode",
|
||||
new DataValue(Variant.Null, StatusCodes.BadDeviceFailure, DateTime.UtcNow, DateTime.UtcNow));
|
||||
|
||||
await runTask;
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
output.ShouldContain("Ever went BAD during window: 1");
|
||||
output.ShouldContain("NEVER went bad (suspect): 0");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Duration_ZeroOrPositive_AutoExits()
|
||||
{
|
||||
// --duration > 0 should make the command exit on its own without needing cancellation.
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=AutoExit",
|
||||
DurationSeconds = 1
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
|
||||
var sw = System.Diagnostics.Stopwatch.StartNew();
|
||||
await command.ExecuteAsync(console);
|
||||
sw.Stop();
|
||||
|
||||
sw.Elapsed.ShouldBeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(900));
|
||||
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(10));
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
output.ShouldContain("==================== SUMMARY ====================");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Quiet_SuppressesPerUpdateOutputButPrintsSummary()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=Quiet",
|
||||
Quiet = true,
|
||||
DurationSeconds = 2
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
var runTask = Task.Run(async () => await command.ExecuteAsync(console));
|
||||
|
||||
await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1);
|
||||
fakeService.RaiseDataChanged(
|
||||
"ns=2;s=Quiet",
|
||||
new DataValue(new Variant(1.0), StatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow));
|
||||
|
||||
await runTask;
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
// No per-update "value = ..." line should appear because --quiet was set.
|
||||
output.ShouldNotContain(" = 1 (");
|
||||
// Summary section is still printed.
|
||||
output.ShouldContain("==================== SUMMARY ====================");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SummaryFile_WritesSummaryToDisk()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var tempFile = Path.Combine(Path.GetTempPath(), $"otopcua-cli-summary-{Guid.NewGuid():N}.txt");
|
||||
try
|
||||
{
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=SummaryFileNode",
|
||||
DurationSeconds = 1,
|
||||
SummaryFile = tempFile
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await command.ExecuteAsync(console);
|
||||
|
||||
File.Exists(tempFile).ShouldBeTrue();
|
||||
var contents = await File.ReadAllTextAsync(tempFile);
|
||||
contents.ShouldContain("SUMMARY");
|
||||
contents.ShouldContain("Total subscribed: 1");
|
||||
}
|
||||
finally
|
||||
{
|
||||
if (File.Exists(tempFile)) File.Delete(tempFile);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Recursive_BrowsesSubtreeAndSubscribesEveryVariable()
|
||||
{
|
||||
// Root has a Folder child and a top-level Variable; the Folder contains a second Variable.
|
||||
// Each node id is distinct so the ToDictionary(t => t.nodeId.ToString()) collapse
|
||||
// doesn't trip a duplicate-key error.
|
||||
var fakeService = new FakeOpcUaClientService();
|
||||
fakeService.BrowseResultsByParent["ns=2;s=Root"] = new List<BrowseResult>
|
||||
{
|
||||
new("ns=2;s=Folder", "Folder", "Object", true),
|
||||
new("ns=2;s=Tag1", "Tag1", "Variable", false)
|
||||
};
|
||||
fakeService.BrowseResultsByParent["ns=2;s=Folder"] = new List<BrowseResult>
|
||||
{
|
||||
new("ns=2;s=Tag2", "Tag2", "Variable", false)
|
||||
};
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=Root",
|
||||
Recursive = true,
|
||||
MaxDepth = 3,
|
||||
DurationSeconds = 1
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await command.ExecuteAsync(console);
|
||||
|
||||
// Both Variables (depth 1 + depth 2) should have been subscribed.
|
||||
fakeService.SubscribeCalls.Count.ShouldBeGreaterThanOrEqualTo(2);
|
||||
fakeService.SubscribeCalls.ShouldContain(c => c.NodeId.Identifier.ToString() == "Tag1");
|
||||
fakeService.SubscribeCalls.ShouldContain(c => c.NodeId.Identifier.ToString() == "Tag2");
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
output.ShouldContain("Browsing subtree of ns=2;s=Root");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeFailure_PrintsFailedMessage_DoesNotCrash()
|
||||
{
|
||||
var fakeService = new FakeOpcUaClientService
|
||||
{
|
||||
SubscribeException = new InvalidOperationException("forced failure")
|
||||
};
|
||||
var factory = new FakeOpcUaClientServiceFactory(fakeService);
|
||||
var command = new SubscribeCommand(factory)
|
||||
{
|
||||
Url = "opc.tcp://localhost:4840",
|
||||
NodeId = "ns=2;s=FailNode",
|
||||
DurationSeconds = 1
|
||||
};
|
||||
|
||||
using var console = TestConsoleHelper.CreateConsole();
|
||||
await command.ExecuteAsync(console);
|
||||
|
||||
var output = TestConsoleHelper.GetOutput(console);
|
||||
output.ShouldContain("FAILED to subscribe");
|
||||
output.ShouldContain("forced failure");
|
||||
// The summary block is still printed.
|
||||
output.ShouldContain("==================== SUMMARY ====================");
|
||||
}
|
||||
|
||||
private static async Task WaitForAsync(Func<bool> predicate, int timeoutMs = 5000)
|
||||
{
|
||||
var deadline = DateTime.UtcNow.AddMilliseconds(timeoutMs);
|
||||
while (!predicate() && DateTime.UtcNow < deadline)
|
||||
await Task.Delay(25);
|
||||
if (!predicate())
|
||||
throw new TimeoutException("Condition not met within timeout.");
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,163 @@
|
||||
using Opc.Ua;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Adapters;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for the pure best-endpoint selection logic extracted from
|
||||
/// <see cref="DefaultEndpointDiscovery"/> (Client.Shared-011). The selector picks the best
|
||||
/// endpoint matching the requested security mode (preferring Basic256Sha256) and rewrites
|
||||
/// the discovered endpoint URL hostname to match the operator-supplied URL so internal DNS
|
||||
/// hostnames in discovery responses do not leak into the session.
|
||||
/// </summary>
|
||||
public class EndpointSelectorTests
|
||||
{
|
||||
private static EndpointDescription Ep(MessageSecurityMode mode, string policy, string url)
|
||||
{
|
||||
return new EndpointDescription
|
||||
{
|
||||
EndpointUrl = url,
|
||||
SecurityMode = mode,
|
||||
SecurityPolicyUri = policy,
|
||||
};
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that the selector returns the only endpoint matching the requested
|
||||
/// security mode even when other endpoints with different modes are present.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_PicksMatchingSecurityMode()
|
||||
{
|
||||
var endpoints = new[]
|
||||
{
|
||||
Ep(MessageSecurityMode.None, SecurityPolicies.None, "opc.tcp://server:4840"),
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"),
|
||||
Ep(MessageSecurityMode.SignAndEncrypt, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"),
|
||||
};
|
||||
|
||||
var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign);
|
||||
|
||||
best.SecurityMode.ShouldBe(MessageSecurityMode.Sign);
|
||||
best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic256Sha256);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that when multiple endpoints match the requested mode, Basic256Sha256 wins
|
||||
/// over weaker policies — even when Basic256Sha256 is not the first encountered.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_PrefersBasic256Sha256WhenMultipleMatch()
|
||||
{
|
||||
var endpoints = new[]
|
||||
{
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic128Rsa15, "opc.tcp://server:4840"),
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"),
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256, "opc.tcp://server:4840"),
|
||||
};
|
||||
|
||||
var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign);
|
||||
|
||||
best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic256Sha256);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that the selector falls back to the first matching endpoint when no
|
||||
/// Basic256Sha256 endpoint is advertised for the requested security mode.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_FallsBackToFirstMatchWhenNoBasic256Sha256()
|
||||
{
|
||||
var endpoints = new[]
|
||||
{
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic128Rsa15, "opc.tcp://server:4840"),
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256, "opc.tcp://server:4840"),
|
||||
};
|
||||
|
||||
var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign);
|
||||
|
||||
best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic128Rsa15);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that no matching endpoint produces an InvalidOperationException whose
|
||||
/// message lists the available security mode/policy combinations to aid diagnosis.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_NoMatchingMode_ThrowsWithDiagnostic()
|
||||
{
|
||||
var endpoints = new[]
|
||||
{
|
||||
Ep(MessageSecurityMode.None, SecurityPolicies.None, "opc.tcp://server:4840"),
|
||||
};
|
||||
|
||||
var ex = Should.Throw<InvalidOperationException>(() =>
|
||||
EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.SignAndEncrypt));
|
||||
|
||||
ex.Message.ShouldContain("SignAndEncrypt");
|
||||
ex.Message.ShouldContain("None"); // available endpoint listed in the message
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that the selector rewrites the discovery-returned hostname to the
|
||||
/// operator-supplied hostname so internal DNS names in the response do not leak
|
||||
/// into the resulting session.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_RewritesHostToMatchRequestedUrl()
|
||||
{
|
||||
var endpoints = new[]
|
||||
{
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256,
|
||||
"opc.tcp://internal-host:4840/UA/Server"),
|
||||
};
|
||||
|
||||
var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://external-host:4840",
|
||||
MessageSecurityMode.Sign);
|
||||
|
||||
new Uri(best.EndpointUrl).Host.ShouldBe("external-host");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that when the discovery host already matches the requested host the
|
||||
/// endpoint URL is left untouched.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_HostsMatch_LeavesUrlUnchanged()
|
||||
{
|
||||
var endpoints = new[]
|
||||
{
|
||||
Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256,
|
||||
"opc.tcp://server:4840/UA/Server"),
|
||||
};
|
||||
|
||||
var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840",
|
||||
MessageSecurityMode.Sign);
|
||||
|
||||
best.EndpointUrl.ShouldBe("opc.tcp://server:4840/UA/Server");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that a null endpoints argument throws ArgumentNullException rather than
|
||||
/// producing a confusing downstream NullReferenceException.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_NullEndpoints_Throws()
|
||||
{
|
||||
Should.Throw<ArgumentNullException>(() =>
|
||||
EndpointSelector.SelectBest(null!, "opc.tcp://server:4840", MessageSecurityMode.None));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that an empty endpointUrl produces ArgumentException so the caller gets
|
||||
/// a clear contract violation rather than a downstream UriFormatException.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void SelectBest_EmptyEndpointUrl_Throws()
|
||||
{
|
||||
Should.Throw<ArgumentException>(() =>
|
||||
EndpointSelector.SelectBest(Array.Empty<EndpointDescription>(), "", MessageSecurityMode.None));
|
||||
}
|
||||
}
|
||||
@@ -168,10 +168,25 @@ internal sealed class FakeSessionAdapter : ISessionAdapter
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Number of times <see cref="CallMethodAsync"/> was invoked so tests can assert
|
||||
/// acknowledgment workflows reached the session adapter.
|
||||
/// </summary>
|
||||
public int CallMethodCount { get; private set; }
|
||||
|
||||
/// <summary>
|
||||
/// When set, <see cref="CallMethodAsync"/> throws this exception — used to simulate
|
||||
/// a bad method call status surfacing as a <see cref="ServiceResultException"/>.
|
||||
/// </summary>
|
||||
public Exception? CallMethodException { get; set; }
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task<IList<object>?> CallMethodAsync(NodeId objectId, NodeId methodId, object[] inputArguments,
|
||||
CancellationToken ct = default)
|
||||
{
|
||||
CallMethodCount++;
|
||||
if (CallMethodException != null)
|
||||
throw CallMethodException;
|
||||
return Task.FromResult<IList<object>?>(null);
|
||||
}
|
||||
|
||||
|
||||
+4
-2
@@ -18,8 +18,10 @@ public class ConnectionSettingsTests
|
||||
settings.SecurityMode.ShouldBe(SecurityMode.None);
|
||||
settings.SessionTimeoutSeconds.ShouldBe(60);
|
||||
settings.AutoAcceptCertificates.ShouldBeTrue();
|
||||
settings.CertificateStorePath.ShouldContain("OtOpcUaClient");
|
||||
settings.CertificateStorePath.ShouldContain("pki");
|
||||
// CertificateStorePath defaults to empty so constructing settings does not
|
||||
// touch disk; DefaultApplicationConfigurationFactory resolves the canonical
|
||||
// PKI path lazily on first connect (Client.Shared-010).
|
||||
settings.CertificateStorePath.ShouldBe(string.Empty);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
||||
@@ -996,6 +996,143 @@ public class OpcUaClientServiceTests : IDisposable
|
||||
eventCount.ShouldBe(0);
|
||||
}
|
||||
|
||||
// --- AcknowledgeAlarm tests (Client.Shared-009) ---
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that a successful acknowledge call returns <see cref="StatusCodes.Good"/>
|
||||
/// and reaches the session adapter's CallMethodAsync (Client.Shared-009).
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task AcknowledgeAlarmAsync_OnSuccess_ReturnsGood()
|
||||
{
|
||||
var session = new FakeSessionAdapter();
|
||||
_sessionFactory.EnqueueSession(session);
|
||||
await _service.ConnectAsync(ValidSettings());
|
||||
|
||||
var result = await _service.AcknowledgeAlarmAsync("ns=2;s=Cond", new byte[] { 1, 2 }, "acked");
|
||||
|
||||
result.ShouldBe(StatusCodes.Good);
|
||||
session.CallMethodCount.ShouldBe(1);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Regression for Client.Shared-009: a bad call result must surface as the returned
|
||||
/// <see cref="StatusCode"/> rather than escape as an uncaught
|
||||
/// <see cref="ServiceResultException"/>, so callers using
|
||||
/// <c>if (StatusCode.IsBad(result))</c> actually see the failure.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task AcknowledgeAlarmAsync_OnServiceResultException_ReturnsBadStatusCode()
|
||||
{
|
||||
var session = new FakeSessionAdapter
|
||||
{
|
||||
CallMethodException = new ServiceResultException(
|
||||
StatusCodes.BadConditionAlreadyEnabled, "already acked")
|
||||
};
|
||||
_sessionFactory.EnqueueSession(session);
|
||||
await _service.ConnectAsync(ValidSettings());
|
||||
|
||||
var result = await _service.AcknowledgeAlarmAsync("ns=2;s=Cond", new byte[] { 1, 2 }, "acked");
|
||||
|
||||
StatusCode.IsBad(result).ShouldBeTrue();
|
||||
result.Code.ShouldBe(StatusCodes.BadConditionAlreadyEnabled);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies the ".Condition" suffix is appended when the caller supplies the
|
||||
/// source node, but left alone when the caller already passes the condition node —
|
||||
/// matches the documented contract.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task AcknowledgeAlarmAsync_LeavesConditionSuffixAlone()
|
||||
{
|
||||
var session = new FakeSessionAdapter();
|
||||
_sessionFactory.EnqueueSession(session);
|
||||
await _service.ConnectAsync(ValidSettings());
|
||||
|
||||
await _service.AcknowledgeAlarmAsync("ns=2;s=Cond.Condition", new byte[] { 1, 2 }, "acked");
|
||||
|
||||
// Both call shapes reach the adapter once.
|
||||
session.CallMethodCount.ShouldBe(1);
|
||||
}
|
||||
|
||||
// --- Alarm fallback path (Client.Shared-011) ---
|
||||
|
||||
/// <summary>
|
||||
/// Regression for Client.Shared-011: when standard AckedState/Id and ActiveState/Id
|
||||
/// fields are missing (null Value) but the SourceNode (ConditionId) field at index 12
|
||||
/// is populated, the client launches the Task.Run fallback that reads
|
||||
/// <c>InAlarm</c>/<c>Acked</c> from the condition node's Galaxy attributes. Verify
|
||||
/// the alarm event is delivered with the values from the supplemental reads.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent()
|
||||
{
|
||||
var fakeSub = new FakeSubscriptionAdapter();
|
||||
var session = new FakeSessionAdapter
|
||||
{
|
||||
NextSubscription = fakeSub,
|
||||
ReadResponseFunc = nodeId =>
|
||||
{
|
||||
var key = nodeId.ToString();
|
||||
if (key.EndsWith(".InAlarm"))
|
||||
return new DataValue(new Variant(true), StatusCodes.Good);
|
||||
if (key.EndsWith(".Acked"))
|
||||
return new DataValue(new Variant(false), StatusCodes.Good);
|
||||
if (key.EndsWith(".TimeAlarmOn"))
|
||||
return new DataValue(new Variant(new DateTime(2026, 1, 1, 12, 0, 0)), StatusCodes.Good);
|
||||
if (key.EndsWith(".DescAttrName"))
|
||||
return new DataValue(new Variant("Fallback message"), StatusCodes.Good);
|
||||
return new DataValue(StatusCodes.BadNodeIdUnknown);
|
||||
}
|
||||
};
|
||||
_sessionFactory.EnqueueSession(session);
|
||||
await _service.ConnectAsync(ValidSettings());
|
||||
|
||||
AlarmEventArgs? received = null;
|
||||
var raised = new TaskCompletionSource();
|
||||
_service.AlarmEvent += (_, e) =>
|
||||
{
|
||||
received = e;
|
||||
raised.TrySetResult();
|
||||
};
|
||||
|
||||
await _service.SubscribeAlarmsAsync();
|
||||
|
||||
var handle = fakeSub.ActiveHandles.First();
|
||||
// AckedState/Id (8) and ActiveState/Id (9) are present but Variant.Value is null,
|
||||
// which triggers the supplemental Galaxy-attribute fallback; SourceNode (12) is set.
|
||||
var fields = new EventFieldList
|
||||
{
|
||||
EventFields =
|
||||
[
|
||||
new Variant(new byte[] { 1, 2, 3 }), // 0: EventId
|
||||
new Variant(ObjectTypeIds.AlarmConditionType), // 1: EventType
|
||||
new Variant("Source1"), // 2: SourceName
|
||||
new Variant(DateTime.MinValue), // 3: Time
|
||||
new Variant(new LocalizedText("Initial")), // 4: Message
|
||||
new Variant((ushort)400), // 5: Severity
|
||||
new Variant("CondName"), // 6: ConditionName
|
||||
new Variant(true), // 7: Retain
|
||||
Variant.Null, // 8: AckedState/Id — missing
|
||||
Variant.Null, // 9: ActiveState/Id — missing
|
||||
new Variant(true), // 10: EnabledState/Id
|
||||
new Variant(false), // 11: SuppressedOrShelved
|
||||
new Variant("ns=2;s=ConditionId") // 12: SourceNode
|
||||
]
|
||||
};
|
||||
fakeSub.SimulateEvent(handle, fields);
|
||||
|
||||
// The fallback runs on a background Task.Run continuation — wait briefly for it.
|
||||
await Task.WhenAny(raised.Task, Task.Delay(500));
|
||||
|
||||
received.ShouldNotBeNull();
|
||||
received!.ActiveState.ShouldBeTrue(); // from InAlarm read
|
||||
received.AckedState.ShouldBeFalse(); // from Acked read
|
||||
received.ConditionNodeId.ShouldBe("ns=2;s=ConditionId");
|
||||
received.Message.ShouldBe("Fallback message"); // from DescAttrName read
|
||||
}
|
||||
|
||||
// --- Failover tests ---
|
||||
|
||||
/// <summary>
|
||||
|
||||
+61
@@ -0,0 +1,61 @@
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Driver.FOCAS.Cli-004: every FOCAS CLI command must own one disposal mechanism for
|
||||
/// the <c>FocasDriver</c>, not two. The chosen mechanism is <c>await using var driver
|
||||
/// = ...</c> — <c>FocasDriver.DisposeAsync</c> already calls <c>ShutdownAsync</c>, so
|
||||
/// an additional explicit <c>driver.ShutdownAsync(...)</c> in a <c>finally</c> block
|
||||
/// runs shutdown twice. 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_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");
|
||||
}
|
||||
}
|
||||
+86
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Covers <see cref="FocasCommandBase.BuildOptions"/> — the pure, deterministic mapping
|
||||
/// from the base's CNC host/port/series/timeout flags onto a
|
||||
/// <c>FocasDriverOptions</c>. The CLI is one-shot so the background connectivity probe
|
||||
/// must be disabled.
|
||||
/// </summary>
|
||||
[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<FocasTagDefinition> 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);
|
||||
}
|
||||
}
|
||||
+79
@@ -0,0 +1,79 @@
|
||||
using CliFx.Attributes;
|
||||
using CliFx.Infrastructure;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>--cnc-port</c>, <c>--timeout-ms</c>, and
|
||||
/// <c>--interval-ms</c>.
|
||||
/// </summary>
|
||||
[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<CliFx.Exceptions.CommandException>(() => 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<CliFx.Exceptions.CommandException>(() => 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<CliFx.Exceptions.CommandException>(() => 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());
|
||||
}
|
||||
}
|
||||
+55
@@ -0,0 +1,55 @@
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Driver.FOCAS.Cli-002: the FOCAS <c>subscribe</c> command — a near-verbatim copy
|
||||
/// of the Modbus subscribe command — must
|
||||
/// (a) serialise writes from the <c>OnDataChange</c> handler (raised from the
|
||||
/// driver's <c>PollGroupEngine</c> 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 <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 FOCAS
|
||||
/// copy must too.
|
||||
/// </summary>
|
||||
[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)");
|
||||
}
|
||||
}
|
||||
+122
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Covers <see cref="WriteCommand.ParseValue"/> across every FOCAS atomic type.
|
||||
/// Driver.FOCAS.Cli-001: malformed numeric input must surface as a friendly
|
||||
/// <see cref="CliFx.Exceptions.CommandException"/>, not a raw
|
||||
/// <see cref="FormatException"/> / <see cref="OverflowException"/> stack trace.
|
||||
/// </summary>
|
||||
[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<CliFx.Exceptions.CommandException>(
|
||||
() => 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<double>();
|
||||
}
|
||||
|
||||
[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<CliFx.Exceptions.CommandException>(
|
||||
() => 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<CliFx.Exceptions.CommandException>(
|
||||
() => WriteCommand.ParseValue(raw, type));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ParseValue_CommandException_message_names_the_type_and_value()
|
||||
{
|
||||
var ex = Should.Throw<CliFx.Exceptions.CommandException>(
|
||||
() => 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);
|
||||
}
|
||||
}
|
||||
+26
@@ -0,0 +1,26 @@
|
||||
<Project Sdk="Microsoft.NET.Sdk">
|
||||
|
||||
<PropertyGroup>
|
||||
<TargetFramework>net10.0</TargetFramework>
|
||||
<Nullable>enable</Nullable>
|
||||
<ImplicitUsings>enable</ImplicitUsings>
|
||||
<IsPackable>false</IsPackable>
|
||||
<IsTestProject>true</IsTestProject>
|
||||
<RootNamespace>ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests</RootNamespace>
|
||||
</PropertyGroup>
|
||||
|
||||
<ItemGroup>
|
||||
<PackageReference Include="xunit.v3" Version="1.1.0"/>
|
||||
<PackageReference Include="Shouldly" Version="4.3.0"/>
|
||||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0"/>
|
||||
<PackageReference Include="xunit.runner.visualstudio" Version="3.0.2">
|
||||
<PrivateAssets>all</PrivateAssets>
|
||||
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
|
||||
</PackageReference>
|
||||
</ItemGroup>
|
||||
|
||||
<ItemGroup>
|
||||
<ProjectReference Include="..\..\..\..\src\Drivers\Cli\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj"/>
|
||||
</ItemGroup>
|
||||
|
||||
</Project>
|
||||
+91
@@ -491,4 +491,95 @@ public sealed class WonderwareHistorianClientTests
|
||||
await Should.ThrowAsync<InvalidDataException>(() =>
|
||||
client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None));
|
||||
}
|
||||
|
||||
// ===== Finding-003 / Finding-004: health counter consistency =====
|
||||
|
||||
/// <summary>
|
||||
/// (Finding 003 + 004) A sidecar-level failure must be classified once: TotalSuccesses
|
||||
/// must stay at 0, TotalFailures must become 1, and TotalQueries / TotalSuccesses /
|
||||
/// TotalFailures must all be updated under the same lock so a concurrent snapshot can
|
||||
/// never observe inflated successes or out-of-band TotalQueries. This pins behaviour so
|
||||
/// a future regression to the "RecordSuccess then undo via ReclassifySuccessAsFailure"
|
||||
/// dance is caught.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task GetHealthSnapshot_SidecarFailure_NeverInflatesSuccessCounter()
|
||||
{
|
||||
var pipe = UniquePipeName();
|
||||
await using var server = new FakeSidecarServer(pipe, Secret)
|
||||
{
|
||||
OnReadRaw = _ => new ReadRawReply { Success = false, Error = "boom" },
|
||||
};
|
||||
await server.StartAsync();
|
||||
|
||||
await using var client = new WonderwareHistorianClient(OptsFor(pipe));
|
||||
|
||||
await Should.ThrowAsync<InvalidOperationException>(() =>
|
||||
client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 1, CancellationToken.None));
|
||||
|
||||
var snap = client.GetHealthSnapshot();
|
||||
snap.TotalQueries.ShouldBe(1);
|
||||
snap.TotalSuccesses.ShouldBe(0);
|
||||
snap.TotalFailures.ShouldBe(1);
|
||||
snap.ConsecutiveFailures.ShouldBe(1);
|
||||
snap.LastError.ShouldNotBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// (Finding 003) Concurrent calls + concurrent <see cref="WonderwareHistorianClient.GetHealthSnapshot"/>
|
||||
/// reads must observe consistent counters. Specifically, TotalSuccesses + TotalFailures
|
||||
/// must equal TotalQueries at every observed snapshot (no torn read between an
|
||||
/// Interlocked-incremented TotalQueries and a lock-protected outcome counter). The
|
||||
/// channel serializes calls, so the test is observable: each completed query strictly
|
||||
/// increments either successes or failures by one.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task GetHealthSnapshot_ConcurrentCallsAndReads_CountersAreInternallyConsistent()
|
||||
{
|
||||
var pipe = UniquePipeName();
|
||||
await using var server = new FakeSidecarServer(pipe, Secret)
|
||||
{
|
||||
OnReadRaw = _ => new ReadRawReply { Success = true },
|
||||
};
|
||||
await server.StartAsync();
|
||||
|
||||
await using var client = new WonderwareHistorianClient(OptsFor(pipe));
|
||||
|
||||
using var stop = new CancellationTokenSource();
|
||||
var readerSawInconsistent = false;
|
||||
|
||||
#pragma warning disable xUnit1051 // Internal Task.Run loop drives a polling stress test; cancellation flows via stop.IsCancellationRequested below.
|
||||
var reader = Task.Run(() =>
|
||||
{
|
||||
while (!stop.IsCancellationRequested)
|
||||
{
|
||||
var snap = client.GetHealthSnapshot();
|
||||
// Every completed call increments TotalQueries AND exactly one of
|
||||
// TotalSuccesses or TotalFailures under the same lock; an in-flight call
|
||||
// has not yet incremented any of them. So TotalQueries should always equal
|
||||
// the sum of TotalSuccesses + TotalFailures (no in-between state visible).
|
||||
if (snap.TotalSuccesses + snap.TotalFailures != snap.TotalQueries)
|
||||
{
|
||||
readerSawInconsistent = true;
|
||||
}
|
||||
}
|
||||
});
|
||||
#pragma warning restore xUnit1051
|
||||
|
||||
for (var i = 0; i < 50; i++)
|
||||
{
|
||||
await client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 1, TestContext.Current.CancellationToken);
|
||||
}
|
||||
|
||||
stop.Cancel();
|
||||
await reader;
|
||||
|
||||
readerSawInconsistent.ShouldBeFalse(
|
||||
"GetHealthSnapshot exposed TotalQueries that disagreed with the sum of TotalSuccesses + TotalFailures — counters are not updated under a single lock.");
|
||||
|
||||
var final = client.GetHealthSnapshot();
|
||||
final.TotalQueries.ShouldBe(50);
|
||||
final.TotalSuccesses.ShouldBe(50);
|
||||
final.TotalFailures.ShouldBe(0);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user